Changeset 1161

Timestamp:

12/04/08 07:48:32 (3 years ago)

Author:

KennethLavrsen

Message:

Item375: First part of addressing URLPARAM XSS issue
Additionally some docs will need some additional work.
Please test this carefully so we know which apps break from this.

Location:

trunk

Files:

• UnitTestContrib/test/unit/Fn_URLPARAM.pm (modified) (3 diffs)
• core/data/System/VarURLPARAM.txt (modified) (2 diffs)
• core/lib/Foswiki.pm (modified) (2 diffs)

trunk/UnitTestContrib/test/unit/Fn_URLPARAM.pm

@@ -55 +55 @@
         '%URLPARAM{"foo" default="bar"}%', $this->{test_web}, $this->{test_topic});
     $this->assert_str_equals('', "$str");
+    
+    $this->{request}->param( -name=>'foo', -value=>'<evil script>\'\"%');
+    $str = $this->{twiki}->handleCommonTags(
+        '%URLPARAM{"foo" default="bar"}%', $this->{test_web}, $this->{test_topic});
+    $this->assert_str_equals('&#60;evil script&#62;&#39;\&#34;&#37;', "$str");
 }
 
@@ -62 +67 @@
     my $str;
 
-    $this->{request}->param( -name=>'foo', -value=>'&?*!"');
+    $this->{request}->param( -name=>'foo', -value=>'<>\'%&?*!"');
     $str = $this->{twiki}->handleCommonTags(
         '%URLPARAM{"foo" encode="entity"}%', $this->{test_web}, $this->{test_topic});
-    $this->assert_str_equals('&#38;?&#42;!&#34;', "$str");
+    $this->assert_str_equals('&#60;&#62;&#39;&#37;&#38;?&#42;!&#34;', "$str");
 
     $this->{request}->param( -name=>'foo', -value=>'&?*!" ');
@@ -76 +81 @@
         '%URLPARAM{"foo" encode="quote"}%', $this->{test_web}, $this->{test_topic});
     $this->assert_str_equals('&?*!\" ', "$str");
+    
+    $this->{request}->param( -name=>'foo', -value=>'<evil script>\'\"%');
+    $str = $this->{twiki}->handleCommonTags(
+        '%URLPARAM{"foo" default="bar" encode="safe"}%', $this->{test_web}, $this->{test_topic});
+    $this->assert_str_equals('&#60;evil script&#62;&#39;\&#34;&#37;', "$str");
+    
+    $this->{request}->param( -name=>'foo', -value=>'<evil script>\'\"%');
+    $str = $this->{twiki}->handleCommonTags(
+        '%URLPARAM{"foo" default="bar" encode="off"}%', $this->{test_web}, $this->{test_topic});
+    $this->assert_str_equals('<evil script>\'\"%', "$str");
+    
+    $this->{request}->param( -name=>'foo', -value=>'<evil script>\'\"%');
+    $str = $this->{twiki}->handleCommonTags(
+        '%URLPARAM{"foo" default="bar" encode="none"}%', $this->{test_web}, $this->{test_topic});
+    $this->assert_str_equals('<evil script>\'\"%', "$str");
 }
 

trunk/core/data/System/VarURLPARAM.txt

@@ -10 +10 @@
      | =default="..."= | Default value in case parameter is empty or missing | empty string |
      | =newline="<br />"= | Convert newlines in textarea to other delimiters | no conversion |
-     | =encode="entity"= | Encode special characters into HTML entities. See [[%IF{"'%INCLUDINGTOPIC%'='Macros'" then="#"}%VarENCODE][ENCODE]] for more details. | no encoding |
-     | =encode="url"= | Encode special characters for URL parameter use, like a double quote into =%22= | no encoding |
-     | =encode="quote"= | Escape double quotes with backslashes (=\"=), does not change other characters; required when feeding URL parameters into other macros | no encoding |
+     | =encode="off"= <br /> =encode="entity"= <br /> =encode="safe"= <br /> =encode="url"= <br /> =encode="quote"= | Control how special characters are encoded <hr /> =off=: No encoding. Avoid using this when possible. See the security warning below. <hr /> =entity=: Encode special characters into HTML entities. See [[%IF{"'%INCLUDINGTOPIC%'='Macros'" then="#"}%VarENCODE][ENCODE]] for more details. <hr /> =safe=: Encode characters ='"&lt;&gt;= into HTML entities. <hr /> =url=: Encode special characters for URL parameter use, like a double quote into =%22= <hr /> =quote=: Escape double quotes with backslashes (=\"=), does not change other characters; required when feeding URL parameters into other macros. | "safe" |
      | =multiple="on"= %BR% =multiple="[<nop>[$item]]"= | If set, gets all selected elements of a =&lt;select multiple="multiple"&gt;= tag. A format can be specified, with =$item= indicating the element, e.g. =multiple="Option: $item"= | first element |
      | =separator=", "= | Separator between multiple selections. Only relevant if multiple is specified | ="\n"= (new line) |
@@ -18 +16 @@
    * __%X% Notes:__
       * URL parameters passed into HTML form fields must be entity [[%IF{"'%INCLUDINGTOPIC%'='Macros'" then="#"}%VarENCODE][ENCODEd]].
-      * Double quotes in URL parameters must be escaped when passed into other macros.%BR% Example: =%<nop>SEARCH{ "%<nop>URLPARAM{ "search" encode="quotes" }%" noheader="on" }%=
+      * Double quotes in URL parameters must be escaped when passed into other macros.%BR% Example: =%<nop>SEARCH{ "%<nop>URLPARAM{ "search" encode="quote" }%" noheader="on" }%=
       * When used in a template topic, this macro will be expanded when the template is used to create a new topic. See TemplateTopics#TemplateTopicsVars for details.
       * Watch out for internal parameters, such as =rev=, =skin=, =template=, =topic=, =web=; they have a special meaning in Foswiki. Common parameters and view script specific parameters are documented at CommandAndCGIScripts.
       * If you have =%<nop>URLPARAM{= in the value of a URL parameter, it will be modified to =%&lt;nop&gt;URLPARAM{=. This is to prevent an infinite loop during expansion.
-      * There is a risk that this macro could be misused for cross-site scripting.
+      * Security warning! Using URLPARAM can easily be misused for cross-site scripting unless specific characters are entity encoded. By default URLPARAM encodes the characters ='"&lt;&gt;= into HTML entities (same as encode="safe") which is relatively safe. The safest is to use encode="entity". When passing URLPARAM inside another macro always use double quotes ("") combined with using URLPARAM with encode="quote". For maximum security against cross-site scripting you are adviced to install the Foswiki:Extensions.SafeWikiPlugin.
    * Related: [[%IF{"'%INCLUDINGTOPIC%'='Macros'" then="#"}%VarENCODE][ENCODE]], [[%IF{"'%INCLUDINGTOPIC%'='Macros'" then="#"}%VarSEARCH][SEARCH]], FormattedSearch, [[%IF{"'%INCLUDINGTOPIC%'='Macros'" then="#"}%VarQUERYSTRING][QUERYSTRING]]

trunk/core/lib/Foswiki.pm

@@ -3867 +3867 @@
     my $param     = $params->{_DEFAULT} || '';
     my $newLine   = $params->{newline};
-    my $encode    = $params->{encode};
+    my $encode    = $params->{encode} || 'safe';
     my $multiple  = $params->{multiple};
     my $separator = $params->{separator};
@@ -3897 +3897 @@
     if ( defined $value ) {
         $value =~ s/\r?\n/$newLine/go if ( defined $newLine );
-        if ($encode) {
-            if ( $encode =~ /^entit(y|ies)$/i ) {
-                $value = entityEncode($value);
-            }
-            elsif ( $encode =~ /^quotes?$/i ) {
-                $value =~ s/\"/\\"/go
-                  ;    # escape quotes with backslash (Bugs:Item3383 fix)
-            }
-            else {
-                $value =~ s/\r*\n\r*/<br \/>/;    # Legacy
-                $value = urlEncode($value);
-            }
+        if ( $encode =~ /^entit(y|ies)$/i ) {
+            $value = entityEncode($value);
+        }
+        elsif ( $encode =~ /^quotes?$/i ) {
+            $value =~ s/\"/\\"/go
+              ;    # escape quotes with backslash (Bugs:Item3383 fix)
+        }
+        elsif ( $encode =~ /^(off|none)$/i ) {
+            # no encoding
+        }
+        elsif ( $encode =~ /^url$/i ) {
+            $value =~ s/\r*\n\r*/<br \/>/;    # Legacy
+            $value = urlEncode($value);
+        }
+        else { # safe or default
+            # entity encode ' " < > and %
+            $value =~ s/([<>%'"])/'&#'.ord($1).';'/ge;
         }
     }