Fix XSS injection vulnerabilities in admin help and quicklinks (CVE-2…

…020-12685) Found and reported by Sean Fernandez. Thank you! Also: * Gate these pages so visitor must be logged in. Although a login bounce looks really odd in the tiny quicklinks window, better to do this than have it exposed to the world. * Tighten matching hex character pairs in URL-encoded input to avoid runtime errors. * Remove check for unused scratch window_override. * Clean up HTML.
interchange · May 14, 2020 · 243ab0eea0ae1d8d8f3e333128349f104b7e04bf · 243ab0e
1 parent 168ff44
commit 243ab0eea0ae1d8d8f3e333128349f104b7e04bf
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 33 deletions.
diff --git a/dist/lib/UI/pages/admin/help.html b/dist/lib/UI/pages/admin/help.html
@@ -1,3 +1,7 @@
+[if-mm !logged_in]
+	[bounce page="__UI_BASE__/login"]
+[/if-mm]
+
 [comment]
 	There are 3 special help keys: home, faq, and 404. They have editable
 	database entries like any other, but the keys get special handling here.
@@ -7,27 +11,30 @@
 	three listed above and whatever the current topic is.
 [/comment]
 
-[tmp fontface]Verdana, Arial, Helvetica, sans-serif[/tmp]
+[tmpn fontface]Verdana, Arial, Helvetica, sans-serif[/tmpn]
 
-[comment]
-	This is a grievous hack made necessary by Apache mod_rewrite, which
-	re-urlencodes query strings on a rewrite.
-[/comment]
-[tmp help_key][/tmp]
-[calc]
-    my $topic = $CGI->{topic}; 
-	$topic =~ s/\%(\w\w)/chr( hex($1))/eg;
-	$Scratch->{help_orig} = $Scratch->{help_key}=$topic;
+[calcn]
+	my $topic = $CGI->{topic};
+
+	# This is a grievous hack made necessary by Apache mod_rewrite, which
+	# re-urlencodes query strings on a rewrite.
+	$topic =~ s/\%([0-9a-f]{2})/chr(hex($1))/aieg;
+
+	# Scrub user-supplied help topic once for whole page to prevent XSS (CVE-2020-12685)
+	$topic =~ s/[^\w.]//ag;
+
+	$Scratch->{help_orig} = $topic;
+	$Tag->tmpn(help_key => $topic);
 	return;
-[/calc]
+[/calcn]
 
 
-[if scratch help_key =~ /^\s*$/]
-	[tmp help_key]home[/tmp]
+[if !scratch help_key]
+	[tmpn help_key]home[/tmpn]
 [/if]
 
 [if type=data term="@_UI_HELP_TABLE_@::code::[scratch help_key]" op=eq compare=""]
-	[tmp help_key]404[/tmp]
+	[tmpn help_key]404[/tmpn]
 [/if]
 
 [tmp help_title]
@@ -88,7 +95,7 @@
 	[page href="@@MV_PAGE@@" form="topic=faq"]<font face="[scratch fontface]" size=2>FAQ</font></a>
 [/if]
 	<img src="bg.gif" height=1 width=12>
-[if scratch help_orig =~ /\S/]
+[if scratch help_orig]
 [page href=admin/flex_editor
 	  form="
 	  	mv_data_table=ichelp
@@ -100,7 +107,7 @@
 [/if]
 </td>
 <td align=right valign=center>
-<input name=topic type=hidden value="[cgi topic]">
+<input name=topic type=hidden value="[cgi name=topic filter=entities keep=1]">
 <input name=help_search size=10 class=s3>
 <input type=submit value="[L]Search Help[/L]" class=s3>
 &nbsp;
@@ -123,7 +130,7 @@
 [loop search="
 				fi=ichelp.txt
 				st=text
-				se=[cgi help_search]
+				se=[cgi name=help_search filter=oneline keep=1]
 				rf=code,title
 				ml=10
 			"
@@ -135,7 +142,7 @@
 [/list]
 </ol>
 [no-match]
-  No help found for [cgi name=help_search filter=entities].
+  No help found for [cgi name=help_search filter="oneline entities" keep=1].
 [/no-match]
 [more-list] [more] [/more-list]
 [/loop]

diff --git a/dist/lib/UI/pages/admin/quicklinks.html b/dist/lib/UI/pages/admin/quicklinks.html
@@ -1,4 +1,8 @@
-@_UI_STD_INIT_@<html>
+[if-mm !logged_in]
+	[bounce page="__UI_BASE__/login"]
+[/if-mm]
+@_UI_STD_INIT_@
+<html>
 <head>
 <title>
 [L]Quicklinks[/L]
@@ -19,9 +23,13 @@
 <tr class=rnorm>
 <td class=rnorm nowrap style="font-size: smaller">
 
-[if !scratch window_override]
-[seti win][data session arg][/seti]
-[/if]
+[calcn]
+	# Prevent XSS injection via mv_arg form parameter (CVE-2020-12685)
+	$Session->{arg} =~ s/\W+//ag;
+
+	$Scratch->{win} = $Session->{arg};
+	return;
+[/calcn]
 
 [menu menu-type=simple name=Quicklinks menu_group=mgroup localize=name,description]
 {PAGE:}
@@ -36,40 +44,40 @@
 {/PAGE?}
 [/menu]
 
-</FONT>
-                </TD>
-        </TR>
-</TABLE>
+</td>
+</tr>
+</table>
+
 </td></tr></table>
 </center>
 </div>
+
 <div align="center">
-<a href="javascript:self.window.close()">&#91;[L]close[/L]]</a>
+<a href="javascript:self.window.close()">&#91;[L]close[/L]&#93;</a>
 <br>
 <a  target="mainwindow[scratch win]"
 	href="[area href='__UI_BASE__/menu_editor'
 				form='
 					qmenu_name=Quicklinks
 				'
-			]">&#91;[L]edit[/L]]</a>
+			]">&#91;[L]edit[/L]&#93;</a>
 [if cgi mgroup eq full]
 <a href="[area
 			href='@@MV_PAGE@@'
 			form='
 				mv_arg=[scratch win]
 				'
-		]">&#91;[L]short menu[/L]]</a>
+		]">&#91;[L]short menu[/L]&#93;</a>
 [else]
 <a href="[area
 			href='@@MV_PAGE@@'
 			form='
 				mgroup=full
 				mv_arg=[scratch win]
 				'
-		]">&#91;[L]full menu[/L]]</a>
+		]">&#91;[L]full menu[/L]&#93;</a>
 [/else]
 [/if]
 </div>
-</BODY>
-</HTML>
-
+</body>
+</html>
diff --git a/doc/WHATSNEW-5.12 b/doc/WHATSNEW-5.12
@@ -256,6 +256,8 @@ UserDB
 Admin
 -----
 
+* Fix XSS injection vulnerabilities in help and quicklinks (CVE-2020-12685).
+
 * Respect custom BACKUP_DIRECTORY in admin table downloads. This matches
   behavior expected by [export-database].