Skip to content

Commit 394739f

Browse files
authored
Fix more shapes of log-injection (#485)
This change fixes more shapes of log injection vulnerabilities, and bumps the toolkit version to guarantee safety when passing non-`String` types to `stripAll`.
1 parent 56aba73 commit 394739f

File tree

11 files changed

+153506
-40
lines changed

11 files changed

+153506
-40
lines changed

core-codemods/src/test/java/io/codemodder/codemods/codeql/CodeQLLogInjectionCodemodTest.java

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,29 @@
22

33
import io.codemodder.testutils.CodemodTestMixin;
44
import io.codemodder.testutils.Metadata;
5+
import org.junit.jupiter.api.Nested;
56

6-
@Metadata(
7-
codemodType = CodeQLLogInjectionCodemod.class,
8-
testResourceDir = "codeql-log-injection",
9-
renameTestFile =
10-
"app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/Templates.java",
11-
doRetransformTest = false,
12-
expectingFixesAtLines = {124},
13-
dependencies = {})
14-
final class CodeQLLogInjectionCodemodTest implements CodemodTestMixin {}
7+
final class CodeQLLogInjectionCodemodTest {
8+
9+
@Nested
10+
@Metadata(
11+
codemodType = CodeQLLogInjectionCodemod.class,
12+
testResourceDir = "codeql-log-injection/template",
13+
renameTestFile =
14+
"app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/Templates.java",
15+
doRetransformTest = false,
16+
expectingFixesAtLines = {124},
17+
dependencies = {})
18+
class TemplateTest implements CodemodTestMixin {}
19+
20+
@Nested
21+
@Metadata(
22+
codemodType = CodeQLLogInjectionCodemod.class,
23+
testResourceDir = "codeql-log-injection/templateedit",
24+
renameTestFile =
25+
"app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/TemplateEdit.java",
26+
doRetransformTest = false,
27+
expectingFixesAtLines = {128},
28+
dependencies = {})
29+
class TemplateEditTest implements CodemodTestMixin {}
30+
}

core-codemods/src/test/resources/codeql-log-injection/Templates.java.after renamed to core-codemods/src/test/resources/codeql-log-injection/template/Templates.java.after

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818

1919
package org.apache.roller.weblogger.ui.struts2.editor;
2020

21-
import static io.github.pixee.security.Newlines.stripNewLines;
21+
import static io.github.pixee.security.Newlines.stripAll;
2222
import org.apache.commons.lang3.StringUtils;
2323
import org.apache.commons.logging.Log;
2424
import org.apache.commons.logging.LogFactory;
@@ -123,7 +123,7 @@ public class Templates extends UIAction {
123123

124124
} catch (WebloggerException ex) {
125125
log.error("Error getting templates for weblog - "
126-
+ stripNewLines(getActionWeblog().getHandle()), ex);
126+
+ stripAll(getActionWeblog().getHandle()), ex);
127127
addError("Error getting template list - check Roller logs");
128128
}
129129

@@ -196,7 +196,7 @@ public class Templates extends UIAction {
196196
setNewTmplAction(null);
197197

198198
} catch (WebloggerException ex) {
199-
log.error("Error adding new template for weblog - " + stripNewLines(getActionWeblog().getHandle()), ex);
199+
log.error("Error adding new template for weblog - " + stripAll(getActionWeblog().getHandle()), ex);
200200
addError("Error adding new template - check Roller logs");
201201
}
202202
}
@@ -254,7 +254,7 @@ public class Templates extends UIAction {
254254
}
255255

256256
} catch (Exception ex) {
257-
log.error("Error removing page - " + stripNewLines(getRemoveId()), ex);
257+
log.error("Error removing page - " + stripAll(getRemoveId()), ex);
258258
addError("editPages.remove.error");
259259
}
260260
} else {
Lines changed: 202 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,202 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one or more
3+
* contributor license agreements. The ASF licenses this file to You
4+
* under the Apache License, Version 2.0 (the "License"); you may not
5+
* use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License. For additional information regarding
15+
* copyright in this work, please see the NOTICE file in the top level
16+
* directory of this distribution.
17+
*/
18+
19+
package org.apache.roller.weblogger.ui.struts2.editor;
20+
21+
import static io.github.pixee.security.Newlines.stripAll;
22+
import org.apache.commons.lang3.StringUtils;
23+
import org.apache.commons.logging.Log;
24+
import org.apache.commons.logging.LogFactory;
25+
import org.apache.roller.weblogger.WebloggerException;
26+
import org.apache.roller.weblogger.business.WebloggerFactory;
27+
import org.apache.roller.weblogger.pojos.TemplateRendition.TemplateLanguage;
28+
import org.apache.roller.weblogger.pojos.WeblogTemplate;
29+
import org.apache.roller.weblogger.ui.struts2.util.UIAction;
30+
import org.apache.roller.weblogger.util.cache.CacheManager;
31+
import org.apache.struts2.convention.annotation.AllowedMethods;
32+
import org.apache.struts2.interceptor.validation.SkipValidation;
33+
34+
import java.util.Date;
35+
import java.util.EnumMap;
36+
import java.util.Map;
37+
38+
39+
/**
40+
* Action which handles editing for a single WeblogTemplate.
41+
*/
42+
// TODO: make this work @AllowedMethods({"execute","move"})
43+
public class TemplateEdit extends UIAction {
44+
45+
private static Log log = LogFactory.getLog(TemplateEdit.class);
46+
47+
// form bean for collection all template properties
48+
private TemplateEditBean bean = new TemplateEditBean();
49+
50+
// the template we are working on
51+
private WeblogTemplate template = null;
52+
53+
public TemplateEdit() {
54+
this.actionName = "templateEdit";
55+
this.desiredMenu = "editor";
56+
this.pageTitle = "pagesForm.title";
57+
}
58+
59+
@Override
60+
public void myPrepare() {
61+
try {
62+
setTemplate(WebloggerFactory.getWeblogger().getWeblogManager().getTemplate(getBean().getId()));
63+
} catch (WebloggerException ex) {
64+
log.error("Error looking up template - " + stripAll(getBean().getId()), ex);
65+
}
66+
}
67+
68+
69+
/**
70+
* Show template edit page.
71+
*/
72+
@SkipValidation
73+
@Override
74+
public String execute() {
75+
try {
76+
if (getTemplate() == null) {
77+
addError("Unable to locate specified template");
78+
return LIST;
79+
}
80+
WeblogTemplate page = getTemplate();
81+
getBean().copyFrom(template);
82+
83+
// empty content-type indicates that page uses auto content-type detection
84+
if (StringUtils.isEmpty(page.getOutputContentType())) {
85+
getBean().setAutoContentType(Boolean.TRUE);
86+
} else {
87+
getBean().setAutoContentType(Boolean.FALSE);
88+
getBean().setManualContentType(page.getOutputContentType());
89+
}
90+
91+
} catch (WebloggerException ex) {
92+
log.error("Error updating page - " + stripAll(getBean().getId()), ex);
93+
addError("Error saving template - check Roller logs");
94+
}
95+
96+
return INPUT;
97+
}
98+
99+
/**
100+
* Save an existing template.
101+
*/
102+
public String save() {
103+
log.debug("Entering save()");
104+
105+
if (getTemplate() == null) {
106+
addError("Unable to locate specified template");
107+
return LIST;
108+
}
109+
110+
// validation
111+
myValidate();
112+
113+
if (!hasActionErrors()) {
114+
try {
115+
116+
WeblogTemplate templateToSave = getTemplate();
117+
getBean().copyTo(templateToSave);
118+
templateToSave.setLastModified(new Date());
119+
120+
if (getBean().getAutoContentType() == null || !getBean().getAutoContentType()) {
121+
templateToSave.setOutputContentType(getBean().getManualContentType());
122+
} else {
123+
// empty content-type indicates that template uses auto content-type detection
124+
templateToSave.setOutputContentType(null);
125+
}
126+
127+
// save template
128+
WebloggerFactory.getWeblogger().getWeblogManager().saveTemplate(templateToSave);
129+
log.debug("Saved template: " + stripAll(templateToSave.getId()));
130+
131+
//flush
132+
WebloggerFactory.getWeblogger().flush();
133+
134+
// notify caches
135+
CacheManager.invalidate(templateToSave);
136+
137+
// success message
138+
addMessage("pageForm.save.success", templateToSave.getName());
139+
140+
} catch (Exception ex) {
141+
log.error("Error updating page - " + stripAll(getBean().getId()), ex);
142+
addError("Error updating template - check Roller logs");
143+
}
144+
}
145+
146+
log.debug("Leaving save()");
147+
return INPUT;
148+
}
149+
150+
private void myValidate() {
151+
152+
// if name changed make sure there isn't a conflict
153+
if (!getTemplate().getName().equals(getBean().getName())) {
154+
try {
155+
if (WebloggerFactory.getWeblogger().getWeblogManager()
156+
.getTemplateByName(getActionWeblog(), getBean().getName()) != null) {
157+
addError("pagesForm.error.alreadyExists", getBean().getName());
158+
}
159+
} catch (WebloggerException ex) {
160+
log.error("Error checking page name uniqueness", ex);
161+
}
162+
}
163+
164+
// if link changed make sure there isn't a conflict
165+
if (!StringUtils.isEmpty(getBean().getLink()) &&
166+
!getBean().getLink().equals(getTemplate().getLink())) {
167+
try {
168+
if (WebloggerFactory.getWeblogger().getWeblogManager()
169+
.getTemplateByLink(getActionWeblog(), getBean().getLink()) != null) {
170+
addError("pagesForm.error.alreadyExists", getBean().getLink());
171+
}
172+
} catch (WebloggerException ex) {
173+
log.error("Error checking page link uniqueness", ex);
174+
}
175+
}
176+
}
177+
178+
public Map<TemplateLanguage, String> getTemplateLanguages() {
179+
Map<TemplateLanguage, String> langMap = new EnumMap<>(TemplateLanguage.class);
180+
for (TemplateLanguage lang : TemplateLanguage.values()) {
181+
langMap.put(lang, lang.getReadableName());
182+
}
183+
return langMap;
184+
}
185+
186+
187+
public TemplateEditBean getBean() {
188+
return bean;
189+
}
190+
191+
public void setBean(TemplateEditBean bean) {
192+
this.bean = bean;
193+
}
194+
195+
public WeblogTemplate getTemplate() {
196+
return template;
197+
}
198+
199+
public void setTemplate(WeblogTemplate template) {
200+
this.template = template;
201+
}
202+
}

0 commit comments

Comments
 (0)