Item14235: Sanitize some template fields.

These are findings by Intel performing a dynamic site scan on http://rocketboards.org

• URL Redirector Abuse in maxrev parameter: Test URL 1
• XSS error in edit.tmpl using redirectto, templatetopic and parenttopic paremeters:
  • Test URL 2
  • Test URL 3
• XSS error in attach using filename: Item14069 was NOT fixed
  • Test URL 4

-- MichaelDaum - 28 Nov 2016

This breaks our sanity tests. The encoded %TEMPLATETOPIC% gets it's % encoded, and becomes an illegal template name.

See TestCaseAmISane, specifically the test case:

preview    Should redirect to preview (save the topic - its used in later tests) 

After this fix is applied, the save fails with:

Attention

'%TEMPLATETOPIC%' is not a valid value for the 'templatetopic' parameter
Either the name is invalid, or it refers to a non-existant topic. 

Note that %TEMPLATETOPIC% is not a macro, but a "skin token" and is replaced in edit.pm and preview.pm. It looks like the encoding is happening too early, and $tmpl =~ s/%TEMPLATETOPIC%/$templateTopic/; is unable to replace the token. It looks like the %REDIRECTTO% is handled the same way. Dumping the contents of the template as seen by Preview.pm:

...
<input type="hidden" name="templatetopic" value="&#37;TEMPLATETOPIC&#37;" />
...

We can't build 2.1.3 with this as-is.

-- GeorgeClark - 19 Dec 2016

Maybe encoding needs to be done in UI::Edit and UI::Preview? Or we need to change the order of processing in Edit and Preview, ensuring that token expansion happens before the calls to expand macros and render TML.

-- GeorgeClark - 19 Dec 2016

The following patch modifies the expansion order in Foswiki::UI::Preview to do the token substitutions before macro expansion. Unit tests pass, but I'm unsure of other side effects. As it only impacts preview, I'm checking it in.

diff --git a/core/lib/Foswiki/UI/Preview.pm b/core/lib/Foswiki/UI/Preview.pm
index 7e8319f..e7255a2 100644
--- a/core/lib/Foswiki/UI/Preview.pm
+++ b/core/lib/Foswiki/UI/Preview.pm
@@ -141,22 +141,6 @@ sub preview {
     # note: preventing linkage in rendered form can only happen in templates
     # see formtables.tmpl
 
-    $tmpl = $topicObject->expandMacros($tmpl);
-    $tmpl = $topicObject->renderTML($tmpl);
-    $tmpl =~ s/%TEXT%/$displayText/g;
-
-    # write the hidden form fields
-    $tmpl =~ s/%FORMFIELDS%/$formFields/g;
-
-    # SMELL: this should be done using CGI::hidden
-    $text = Foswiki::entityEncode( $text, "\n" );
-
-    $tmpl =~ s/%HIDDENTEXT%/$text/g;
-
-    $tmpl =~ s/<\/?(nop|noautolink)\/?>//gis;
-
-    # I don't know _where_ these should be done,
-    # so I'll do them as late as possible
     my $originalrev = $query->param('originalrev');    # rev edit started on
          #ASSERT($originalrev ne '%ORIGINALREV%') if DEBUG;
     $tmpl =~ s/%ORIGINALREV%/$originalrev/g if ( defined($originalrev) );
@@ -172,6 +156,23 @@ sub preview {
 
     #ASSERT($newtopic ne '%NEWTOPIC%') if DEBUG;
     $tmpl =~ s/%NEWTOPIC%/$newtopic/g if ( defined($newtopic) );
+
+    # CAUTION: Once expandMacros executes, any template tokens that are expanded
+    # inside a %ENCODE will be corrupted.  So do token substitution before this point.
+    $tmpl = $topicObject->expandMacros($tmpl);
+    $tmpl = $topicObject->renderTML($tmpl);
+    $tmpl =~ s/%TEXT%/$displayText/g;
+
+    # write the hidden form fields
+    $tmpl =~ s/%FORMFIELDS%/$formFields/g;
+
+    # SMELL: this should be done using CGI::hidden
+    $text = Foswiki::entityEncode( $text, "\n" );
+
+    $tmpl =~ s/%HIDDENTEXT%/$text/g;
+
+    $tmpl =~ s/<\/?(nop|noautolink)\/?>//gis;
+
 ###
     $session->writeCompletePage($tmpl);
 }

-- GeorgeClark - 20 Dec 2016

With my recent checkin - brute force removing ['"] from the fields protected by ENCODE, the xss seems to be fixed. But this all needs more review. Anddd, The CommentPlugin is now toast. I'm unable to post comments. So something is rotten here.

-- Main.GeorgeClark - 20 Dec 2016 - 20:00

CommentPlugin appears completely unrelated. The Language selector is broken, and corrupts any forms on the page. Not sure what's going on. I've tried deleting the Language cache, but that didn't help. My local foswiki doesn't have the issue.

-- GeorgeClark - 20 Dec 2016

Fixed the language selector. Nothing to do with this task. Somehow the pattern skin WebTopBarExample was out of date. Git checkout fixed it.

-- GeorgeClark - 21 Dec 2016

Added a POV to exploit filename in the attach templates

-- MichaelDaum - 23 Dec 2016

Commenting is disabled while not logged in