Skip to content

Commit 96598aa

Browse files
adangeladangel
committed
[core] Fix stored XSS in VBHTMLRenderer and YAHTMLRenderer
Refs GHSA-8rr6-2qw5-pc7r
1 parent 0f84b4d commit 96598aa

9 files changed

Lines changed: 134 additions & 50 deletions

File tree

‎docs/pages/release_notes.md‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,16 @@ This is a {{ site.pmd.release_type }} release.
2424

2525
### 🚀️ New and noteworthy
2626

27+
#### Security fixes
28+
* This release fixes a stored XSS vulnerability in VBHTMLRenderer and YAHTMLRenderer via unescaped violation messages.
29+
Affects CI/CD pipelines that run PMD with `--format vbhtml` or `--format yahtml` on untrusted source code
30+
(e.g. pull requests from external contributors) and expose the HTML report as a build artifact.
31+
JavaScript executes in the browser context of anyone who opens the report.
32+
Note: The default `html` format is **not affected** by unescaped violation messages, but a similar problem
33+
existed with suppressed violation markers.
34+
If you use these reports, it is recommended to upgrade PMD.
35+
Reported by [Smaran Chand](https://github.com/smaranchand) (@smaranchand).
36+
2737
### 🌟️ New and Changed Rules
2838
#### New Rules
2939
* The new Java rule {% rule java/codestyle/UnnecessaryInterfaceDeclaration %} detects classes that
@@ -43,6 +53,7 @@ This is a {{ site.pmd.release_type }} release.
4353
### 🐛️ Fixed Issues
4454
* core
4555
* [#6471](https://github.com/pmd/pmd/issues/6471): \[core] BaseAntlrTerminalNode should return type instead of index for getTokenKind()
56+
* [#6475](https://github.com/pmd/pmd/issues/6475): \[core] Fix stored XSS in VBHTMLRenderer and YAHTMLRenderer
4657
* doc
4758
* [#6396](https://github.com/pmd/pmd/pull/6396): \[doc] Mention test-pmd-tool as alternative for testing
4859
* java-bestpractices

‎pmd-core/src/main/java/net/sourceforge/pmd/renderers/HTMLRenderer.java‎

Lines changed: 26 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import java.io.IOException;
88
import java.io.PrintWriter;
99
import java.io.Writer;
10+
import java.net.URLEncoder;
1011
import java.util.Iterator;
1112
import java.util.List;
1213
import java.util.Optional;
@@ -68,6 +69,10 @@ public String defaultFileExtension() {
6869
return "html";
6970
}
7071

72+
private static String escape(String s) {
73+
return StringEscapeUtils.escapeHtml4(s);
74+
}
75+
7176
/**
7277
* Write the body of the main body of the HTML content.
7378
*/
@@ -135,16 +140,16 @@ private void glomRuleViolations(Writer writer, Iterator<RuleViolation> violation
135140
buf.append("> ").append(System.lineSeparator());
136141
buf.append("<td align=\"center\">").append(violationCount).append("</td>").append(System.lineSeparator());
137142
buf.append("<td width=\"*%\">")
138-
.append(renderFileName(rv.getFileId(), rv.getBeginLine()))
143+
.append(renderFileNameEscaped(rv.getFileId(), rv.getBeginLine()))
139144
.append("</td>")
140145
.append(System.lineSeparator());
141146
buf.append("<td align=\"center\" width=\"5%\">").append(rv.getBeginLine()).append("</td>").append(System.lineSeparator());
142147

143-
String d = StringEscapeUtils.escapeHtml4(rv.getDescription());
148+
String d = escape(rv.getDescription());
144149

145150
String infoUrl = rv.getRule().getExternalInfoUrl();
146151
if (StringUtils.isNotBlank(infoUrl)) {
147-
d = "<a href=\"" + infoUrl + "\">" + d + "</a>";
152+
d = "<a href=\"" + URLEncoder.encode(infoUrl, "UTF-8") + "\">" + d + "</a>";
148153
}
149154
buf.append("<td width=\"*\">")
150155
.append(d)
@@ -157,13 +162,13 @@ private void glomRuleViolations(Writer writer, Iterator<RuleViolation> violation
157162
}
158163
}
159164

160-
private String renderFileName(FileId fileId, int beginLine) {
161-
return maybeWrap(StringEscapeUtils.escapeHtml4(determineFileName(fileId)),
165+
private String renderFileNameEscaped(FileId fileId, int beginLine) {
166+
return maybeWrap(escape(determineFileName(fileId)),
162167
linePrefix == null || beginLine < 0 ? "" : linePrefix + beginLine);
163168
}
164169

165-
private String renderRuleName(Rule rule) {
166-
String name = rule.getName();
170+
private String renderRuleNameEscaped(Rule rule) {
171+
String name = escape(rule.getName());
167172
String infoUrl = rule.getExternalInfoUrl();
168173
if (StringUtils.isNotBlank(infoUrl)) {
169174
return "<a href=\"" + infoUrl + "\">" + name + "</a>";
@@ -192,8 +197,8 @@ private void glomProcessingErrors(PrintWriter writer, List<Report.ProcessingErro
192197
}
193198
colorize = !colorize;
194199
buf.append("> ").append(System.lineSeparator());
195-
buf.append("<td>").append(renderFileName(pe.getFileId(), -1)).append("</td>").append(System.lineSeparator());
196-
buf.append("<td><pre>").append(pe.getDetail()).append("</pre></td>").append(System.lineSeparator());
200+
buf.append("<td>").append(renderFileNameEscaped(pe.getFileId(), -1)).append("</td>").append(System.lineSeparator());
201+
buf.append("<td><pre>").append(escape(pe.getDetail())).append("</pre></td>").append(System.lineSeparator());
197202
buf.append("</tr>").append(System.lineSeparator());
198203
writer.write(buf.toString());
199204
}
@@ -221,11 +226,12 @@ private void glomSuppressions(PrintWriter writer, List<Report.SuppressedViolatio
221226
colorize = !colorize;
222227
buf.append("> ").append(System.lineSeparator());
223228
RuleViolation rv = sv.getRuleViolation();
224-
buf.append("<td align=\"left\">").append(renderFileName(rv.getFileId(), rv.getBeginLine())).append("</td>").append(System.lineSeparator());
229+
String userMessage = Optional.ofNullable(sv.getUserMessage()).orElse("");
230+
buf.append("<td align=\"left\">").append(renderFileNameEscaped(rv.getFileId(), rv.getBeginLine())).append("</td>").append(System.lineSeparator());
225231
buf.append("<td align=\"center\">").append(rv.getBeginLine()).append("</td>").append(System.lineSeparator());
226-
buf.append("<td align=\"center\">").append(renderRuleName(rv.getRule())).append("</td>").append(System.lineSeparator());
227-
buf.append("<td align=\"center\">").append(sv.getSuppressor().getId()).append("</td>").append(System.lineSeparator());
228-
buf.append("<td align=\"center\">").append(sv.getUserMessage() == null ? "" : sv.getUserMessage()).append("</td>").append(System.lineSeparator());
232+
buf.append("<td align=\"center\">").append(renderRuleNameEscaped(rv.getRule())).append("</td>").append(System.lineSeparator());
233+
buf.append("<td align=\"center\">").append(escape(sv.getSuppressor().getId())).append("</td>").append(System.lineSeparator());
234+
buf.append("<td align=\"center\">").append(escape(userMessage)).append("</td>").append(System.lineSeparator());
229235
buf.append("</tr>").append(System.lineSeparator());
230236
writer.write(buf.toString());
231237
}
@@ -252,24 +258,24 @@ private void glomConfigurationErrors(final PrintWriter writer, final List<Config
252258
}
253259
colorize = !colorize;
254260
buf.append("> ").append(System.lineSeparator());
255-
buf.append("<td>").append(renderRuleName(ce.rule())).append("</td>").append(System.lineSeparator());
256-
buf.append("<td>").append(ce.issue()).append("</td>").append(System.lineSeparator());
261+
buf.append("<td>").append(renderRuleNameEscaped(ce.rule())).append("</td>").append(System.lineSeparator());
262+
buf.append("<td>").append(escape(ce.issue())).append("</td>").append(System.lineSeparator());
257263
buf.append("</tr>").append(System.lineSeparator());
258264
writer.write(buf.toString());
259265
}
260266
writer.write("</table>");
261267
}
262268

263-
private String maybeWrap(String filename, String line) {
269+
private String maybeWrap(String filenameEscaped, String line) {
264270
if (StringUtils.isBlank(linkPrefix)) {
265-
return filename;
271+
return filenameEscaped;
266272
}
267-
String newFileName = filename.replace('\\', '/');
273+
String newFileName = filenameEscaped.replace('\\', '/');
268274

269275
if (replaceHtmlExtension) {
270-
int index = filename.lastIndexOf('.');
276+
int index = filenameEscaped.lastIndexOf('.');
271277
if (index >= 0) {
272-
newFileName = filename.substring(0, index);
278+
newFileName = filenameEscaped.substring(0, index);
273279
}
274280
}
275281

‎pmd-core/src/main/java/net/sourceforge/pmd/renderers/VBHTMLRenderer.java‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
import java.io.IOException;
88
import java.util.Iterator;
99

10+
import org.apache.commons.lang3.StringEscapeUtils;
11+
1012
import net.sourceforge.pmd.reporting.Report;
1113
import net.sourceforge.pmd.reporting.RuleViolation;
1214

@@ -28,6 +30,10 @@ public String defaultFileExtension() {
2830
return "vb.html";
2931
}
3032

33+
private static String escape(String s) {
34+
return StringEscapeUtils.escapeHtml4(s);
35+
}
36+
3137
@Override
3238
public void start() throws IOException {
3339
getWriter().write(header());
@@ -55,7 +61,7 @@ public void renderFileViolations(Iterator<RuleViolation> violations) throws IOEx
5561
}
5662
filename = nextFilename;
5763
sb.append("<table border=\"0\" width=\"80%\">");
58-
sb.append("<tr id=TableHeader><td colspan=\"2\"><font class=title>&nbsp;").append(filename)
64+
sb.append("<tr id=TableHeader><td colspan=\"2\"><font class=title>&nbsp;").append(escape(filename))
5965
.append("</font></tr>");
6066
sb.append(lineSep);
6167
}
@@ -68,7 +74,7 @@ public void renderFileViolations(Iterator<RuleViolation> violations) throws IOEx
6874

6975
colorize = !colorize;
7076
sb.append("<td width=\"50\" align=\"right\"><font class=body>").append(rv.getBeginLine()).append("&nbsp;&nbsp;&nbsp;</font></td>");
71-
sb.append("<td><font class=body>").append(rv.getDescription()).append("</font></td>");
77+
sb.append("<td><font class=body>").append(escape(rv.getDescription())).append("</font></td>");
7278
sb.append("</tr>");
7379
sb.append(lineSep);
7480
writer.write(sb.toString());
@@ -97,8 +103,8 @@ public void end() throws IOException {
97103
sb.append("<tr id=RowColor2>");
98104
}
99105
colorize = !colorize;
100-
sb.append("<td><font class=body>").append(determineFileName(error.getFileId())).append("</font></td>");
101-
sb.append("<td><font class=body><pre>").append(error.getDetail()).append("</pre></font></td></tr>");
106+
sb.append("<td><font class=body>").append(escape(determineFileName(error.getFileId()))).append("</font></td>");
107+
sb.append("<td><font class=body><pre>").append(escape(error.getDetail())).append("</pre></font></td></tr>");
102108
}
103109
sb.append("</table>");
104110
writer.write(sb.toString());
@@ -116,8 +122,8 @@ public void end() throws IOException {
116122
sb.append("<tr id=RowColor2>");
117123
}
118124
colorize = !colorize;
119-
sb.append("<td><font class=body>").append(error.rule().getName()).append("</font></td>");
120-
sb.append("<td><font class=body>").append(error.issue()).append("</font></td></tr>");
125+
sb.append("<td><font class=body>").append(escape(error.rule().getName())).append("</font></td>");
126+
sb.append("<td><font class=body>").append(escape(error.issue())).append("</font></td></tr>");
121127
}
122128
sb.append("</table>");
123129
writer.write(sb.toString());

‎pmd-core/src/main/java/net/sourceforge/pmd/renderers/YAHTMLRenderer.java‎

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,15 @@
77
import java.io.File;
88
import java.io.IOException;
99
import java.io.PrintWriter;
10+
import java.net.URLEncoder;
1011
import java.nio.charset.StandardCharsets;
1112
import java.nio.file.Files;
1213
import java.util.LinkedList;
1314
import java.util.List;
1415
import java.util.SortedMap;
1516
import java.util.TreeMap;
1617

18+
import org.apache.commons.lang3.StringEscapeUtils;
1719
import org.apache.commons.lang3.StringUtils;
1820

1921
import net.sourceforge.pmd.properties.PropertyDescriptor;
@@ -47,6 +49,10 @@ public String defaultFileExtension() {
4749
return "html";
4850
}
4951

52+
private static String escape(String s) {
53+
return StringEscapeUtils.escapeHtml4(s);
54+
}
55+
5056
private void addViolation(RuleViolation violation) {
5157
String packageName = violation.getAdditionalInfo().getOrDefault(RuleViolation.PACKAGE_NAME, "");
5258
String className = violation.getAdditionalInfo().getOrDefault(RuleViolation.CLASS_NAME, "");
@@ -118,17 +124,17 @@ private void renderIndex(String outputDir) throws IOException {
118124

119125
for (ReportNode node : reportNodesByPackage.values()) {
120126
out.print(" <tr><td><b>");
121-
out.print(node.getPackageName());
127+
out.print(escape(node.getPackageName()));
122128
out.print("</b></td> <td>");
123129
if (node.hasViolations()) {
124130
out.print("<a href=\"");
125-
out.print(node.getClassName());
131+
out.print(URLEncoder.encode(node.getClassName(), "UTF-8"));
126132
out.print(".html");
127133
out.print("\">");
128-
out.print(node.getClassName());
134+
out.print(escape(node.getClassName()));
129135
out.print("</a>");
130136
} else {
131-
out.print(node.getClassName());
137+
out.print(escape(node.getClassName()));
132138
}
133139
out.print("</td> <td>");
134140
out.print(node.getViolationCount());
@@ -151,32 +157,32 @@ private void renderClasses(String outputDir) throws IOException {
151157
out.println(" <head>");
152158
out.println(" <meta charset=\"UTF-8\">");
153159
out.print(" <title>PMD - ");
154-
out.print(node.getClassName());
160+
out.print(escape(node.getClassName()));
155161
out.println("</title>");
156162
out.println(" </head>");
157163
out.println(" <body>");
158164
out.println(" <h2>Class View</h2>");
159165
out.print(" <h3 align=\"center\">Class: ");
160-
out.print(node.getClassName());
166+
out.print(escape(node.getClassName()));
161167
out.println("</h3>");
162168
out.println(" <table border=\"\" align=\"center\" cellspacing=\"0\" cellpadding=\"3\">");
163169
out.println(" <tr><th>Method</th><th>Violation</th></tr>");
164170
for (RuleViolation violation : node.getViolations()) {
165171
out.print(" <tr><td>");
166172
String methodName = violation.getAdditionalInfo().get(RuleViolation.METHOD_NAME);
167-
out.print(StringUtil.nullToEmpty(methodName));
173+
out.print(escape(StringUtil.nullToEmpty(methodName)));
168174
out.print("</td><td>");
169175
out.print("<table border=\"0\">");
170176

171-
out.print(renderViolationRow("Rule:", violation.getRule().getName()));
172-
out.print(renderViolationRow("Description:", violation.getDescription()));
177+
out.print(renderViolationRowEscaped("Rule:", violation.getRule().getName()));
178+
out.print(renderViolationRowEscaped("Description:", violation.getDescription()));
173179

174180
String variableName = violation.getAdditionalInfo().get(RuleViolation.VARIABLE_NAME);
175181
if (StringUtils.isNotBlank(variableName)) {
176-
out.print(renderViolationRow("Variable:", variableName));
182+
out.print(renderViolationRowEscaped("Variable:", variableName));
177183
}
178184

179-
out.print(renderViolationRow("Line:", violation.getEndLine() > 0
185+
out.print(renderViolationRowEscaped("Line:", violation.getEndLine() > 0
180186
? violation.getBeginLine() + " and " + violation.getEndLine()
181187
: String.valueOf(violation.getBeginLine())));
182188

@@ -193,12 +199,12 @@ private void renderClasses(String outputDir) throws IOException {
193199
}
194200
}
195201

196-
private String renderViolationRow(String name, String value) {
202+
private String renderViolationRowEscaped(String name, String value) {
197203
return "<tr><td><b>"
198-
+ name
204+
+ escape(name)
199205
+ "</b></td>"
200206
+ "<td>"
201-
+ value
207+
+ escape(value)
202208
+ "</td></tr>";
203209
}
204210

0 commit comments

Comments
 (0)