Tags:
archive_me1Add my vote for this tag create new tag
, view all tags
See patch below to manually fix the TWiki 01 May 2000 release. -- PeterThoeny - 13 Jun 2000

My heart sank when I read the code for %INCLUDE file processing:

 my( $attributes ) = @_;
    my $incfile = extractNameValuePair( $attributes );
    my $fileName = "$dataDir/$webName/$incfile";
    if( -e $fileName ) {
        return &readFile( $fileName );
    }
    return &readFile( "$dataDir/$incfile" );
I couldn't find any real checking of the value of '$incfile', so the usual question arose - what does INCLUDE{"../../../file"} do?

Lets try:
<pre>
    %INCLUDE{"../../../../../../etc/passwd"}%
</pre>

No longer works - 14 Jun.





    Warning: Can't find topic ////////////etc.passwd 

What about:
<pre>
    %INCLUDE{"../.htpasswd"}%
</pre>

No longer works - 14 Jun.




    Warning: Can't find topic //.htpasswd 

For includes to work sensibly, we need to very very strictly define what is the allowed target of an included file. I suspect that the intention is to only allow includes under the data dir, in which case, you should only allow [a-zA-Z0-9] and '/' in include names (if you even need to allow subdirs?)

Here's my 60 second patch:

*** wiki-orig.pm        Tue Jun 13 11:13:04 2000
--- wiki.pm     Tue Jun 13 11:16:28 2000
***************
*** 802,807 ****
--- 802,811 ----
  {
      my( $attributes ) = @_;
      my $incfile = extractNameValuePair( $attributes );
+ 
+     $incfile =~ s!\.+!\.!g;               # collapse all '..' to '.'
+     $incfile =~ tr![A-Za-z0-9\./]!!cd;    # zap anything suspicious
+ 
      my $fileName = "$dataDir/$webName/$incfile";
      if( -e $fileName ) {
          return &readFile( $fileName );

-- CrisBailiff - 12 Jun 2000


Now don't go and spoil all of my fun, Cris! :-)

Suggestion: strip out the file name, untaint the path, and paste it back together. I can't see any reason to limit the characters in topic names, sometimes a topic like `++++' is useful.

Actually, limiting to //twiki/data/ may be overkill too. One of my favorites is for looking at the code (search, for example):
<pre>
    %INCLUDE{"../../cgi-bin/search"}%
</pre>

No longer works - 14 Jun.





    Warning: Can't find topic ////cgi-bin.search 

-- KevinKinnell - 12 Jun 2000


Hmm, yes, exactly my point as to why it should be restricted to the 'data' dir!

I always prefer to strip out anything I don't expect - its ok to add stuff back into the pattern (like +++, or '-') but if you do it the other way round (just removing '..') you run into all sorts of unexpected security problems with characters you don't know about.

For example, UTF-8 character encoding offers lots of ways of coding '..' which won't match /../ but will be auto-magically converted back into '..' by perl5.6, glibc or the operating system on many 'modern' or internationalised operating systems that know about unicode and UTF-8 (there's even a warning pasted on the end of the UTF-8 RFC, which most UTF-8 implementors seem to have ignored).

UTF-8 is just my pet CGI security example - there are lots of other tricks, like the difference between perl and C's handling of nulls/%00 in pattern matches/file opens etc. etc.

(Just untainting a string against '.*' defeats the whole point of '-T'...)

I've already added '-' back into the regex in my test install, because I suddenly wanted topics and webs with '-' in them - it would be best if there was a specific regex in the config file to declare 'once and for all' the allowed syntax...

-- CrisBailiff - 12 Jun 2000


I fixed it by using the existing $securityFilter variable in wikicfg.pm :

  {
      my( $attributes ) = @_;
      my $incfile = extractNameValuePair( $attributes );
+ 
+     # CrisBailiff, PeterThoeny 12 Jun 2000: Add security
+     $incfile =~ s/$securityFilter//go;    # zap anything suspicious
+     $incfile =~ s/passwd//goi;    # filter out passwd filename
+     # Filter out ".." from filename, this is to
+     # prevent includes of "../../file"
+     $incfile =~ s/\.+/\./g;
+
      my $fileName = "$dataDir/$webName/$incfile";
      if( -e $fileName ) {
          return &readFile( $fileName );

Some comments:

  • Filtering out '..' should be made optional (enabled by default). It could be disabled in a trusted intranet environment.
  • The current $securityFilter variable is used to filter out dangerous characters. For better security we should consider switching over to allowing only secure characters. But this will break locale characters, like filenames having WikiNamesWithUmlauts or KanjiCharacterSet.
  • Sorry KevinKinnell, the fix prevents including the TWiki source code!
    Oh, well. I can always edit in another %<local tag>% in wikicfg.pm ... %EXTERNALDIRLIST%, for example. :-)
  • The cvs repository has been updated. Also this main TWiki installation's wiki.pm .

-- PeterThoeny - 13 Jun 2000


Sorry, had to re-open this one - found another in readTemplate - the file name passed in (at least to oops) is not checked...

Here's my patch:

*** wiki-orig.pm        Tue Jun 13 11:13:04 2000
--- wiki.pm     Wed Jun 14 10:44:21 2000
***************
*** 503,508 ****
--- 503,512 ----
  sub readTemplate
  {
      my( $name ) = @_;
+ 
+     $name =~ s!\.+!\.!g;
+     $name =~ tr![A-Za-z0-9\./-]!!cd;
+ 
      my $webtmpl = "$templateDir/$webName/$name.tmpl";
      if( -e $webtmpl ) {
          return &readFile( $webtmpl );

Peter, you certainly could use $securityFilter here too. My point remains, though, that it probably isn't strong enough to do what you need. You should certainly include the 'null byte' \0 in the list of dangerous characters - perl considers '/home/httpd/html/../../../etc/passwd\0.html' to be a filename ending in '.html', so many scripts go ahead and open it. open(2) sees things a little differently smile

Certainly the i18n stuff needs working through - its comes down to where the utf-8 conversion is performed, and whether that convertor is 'incautious' in the sense of RFC2279:

6.  Security Considerations

   Implementors of UTF-8 need to consider the security aspects of how
   they handle illegal UTF-8 sequences.  It is conceivable that in some
   circumstances an attacker would be able to exploit an incautious
   UTF-8 parser by sending it an octet sequence that is not permitted by
   the UTF-8 syntax.

   A particularly subtle form of this attack could be carried out
   against a parser which performs security-critical validity checks
   against the UTF-8 encoded form of its input, but interprets certain
   illegal octet sequences as characters.  For example, a parser might
   prohibit the NUL character when encoded as the single-octet sequence
   00, but allow the illegal two-octet sequence C0 80 and interpret it
   as a NUL character.  Another example might be a parser which
   prohibits the octet sequence 2F 2E 2E 2F ("/../"), yet permits the
   illegal octet sequence 2F C0 AE 2E 2F.

See what I mean? You probably can't 'do the right thing' by just removing 'dangerous' characters unless you know what your webserver, perl and operating system are really doing. Sorry.

Anyway, thanks for a great tool - I have to keep gnawing on the internals, as I plan to use twiki on our intranet, but we are a security company, so we have to have a healthy dose of paranoia...

-- CrisBailiff - 13 Jun 2000


Cris, it is nice to have a security specialist on board! We certainly appreciate your feedback!

For now I will keep "filter out dangerous chars" (vs. "allow only valid chars").

Regarding the 'null byte' \0, the $securityFilter filters out the backslash character, so this one should be covered.

Please let us know if there are other security issues.

I changed wiki.pm and updated the cvs repository:

  sub readTemplate
  {
      my( $name, $topic ) = @_;
+ 
+     # CrisBailiff, PeterThoeny 13 Jun 2000: Add security
+     $name =~ s/$securityFilter//go;    # zap anything suspicious
+     $name =~ s/\.+/\./g;               # Filter out ".." from filename
+ 
      my $webtmpl = "$templateDir/$webName/$name.tmpl";
      if( -e $webtmpl ) {
          return &readFile( $webtmpl );

Please note that wikicfg.pm also changed, I added a flag:

#                   Remove ".." from 



    Warning: Can't find topic ""."" 
 filename, to
#                   prevent includes of "../../file". Default "1"
$doSecureInclude    = "1";

-- PeterThoeny - 13 Jun 2000


Peter, Sorry for the confusion - the \0 was just my representation of the character when it's in a perl string - it really is just a 'NUL' (byte with value 0x00), there's no actual backslash, so there is no backslash to strip out...

The null gets there from simple urlencoding - bin/oops?template../../../etc/passwd%00= for example. So you really do need to strip it. I suggest there is no valid use for anything below ASCII 32 (i.e. why do you need escape characters in a file name?)

-- CrisBailiff - 13 Jun 2000


Oops, I should have looked more carefully. Of corse you are right. This is the new $securityFilter (defined in wikicfg.pm ) that "filters out dangerous chars", including all chars below the space char. (Please edit if needed)

$securityFilter     = "[\\\*\?\~\^\$\@\%\`\"\'\&\;\|\<\>\x00-\x1F]";

Security patch for TWiki 01 May 2000

Here is the patch you can apply to the TWiki 01 May 2000 release to fix the above security issues.

Changes in file wiki.pm : (changes are in red)

sub readTemplate
{
    my( $name ) = @_;

    $name =~ s/$securityFilter//go;    # zap anything suspicious
    $name =~ s/\.+/\./g;               # Filter out ".." from filename

    $webtmpl = "$templateDir/$webName/$name.tmpl";


sub handleIncludeFile
{
    my( $attributes ) = @_;
    my $incfile = extractNameValuePair( $attributes );

    $incfile =~ s/$securityFilter//go;    # zap anything suspicious
    $incfile =~ s/passwd//goi;    # filter out passwd filename
    # Filter out ".." from filename, this is to
    # prevent includes of "../../file"
    $incfile =~ s/\.+/\./g;

    my $fileName = "$dataDir/$webName/$incfile";

Changes in file wikicfg.pm :

$securityFilter     = "[\\\*\?\~\^\$\@\%\`\"\'\&\;\|\<\>\x00-\x1F]";

-- PeterThoeny - 13 Jun 2000

Re the comments about security holes due to overlong UTF-8 encoding - in the EncodeURLsWithUTF8 feature, the code is quite careful to exclude overlong UTF-8 encodings in URLs, so should be OK there.

However, filtering-in remains critical, so I have re-opened this bug until that's done. Now that we have InternationalisationEnhancements, it's quite easy to filter-in only alphabetic characters, including accented characters. Filtering-in of East Asian languages may be harder, but is still not too hard when using Unicode regexes in Perl 5.8 - and UTF-8 is virtually required for Chinese support anyway due to GB2312 and Big5 character encoding issues with TWiki (see JapaneseAndChineseSupport).

See also TWikiSecurity and TaintChecking.

-- RichardDonkin - 26 Nov 2004

Edit | Attach | Watch | Print version | History: r11 < r10 < r9 < r8 < r7 | Backlinks | Raw View | Raw edit | More topic actions
Topic revision: r11 - 2008-09-02 - TWikiJanitor
 
  • Learn about TWiki  
  • Download TWiki
This site is powered by the TWiki collaboration platform Powered by Perl Hosted by OICcam.com Ideas, requests, problems regarding TWiki? Send feedback. Ask community in the support forum.
Copyright © 1999-2015 by the contributing authors. All material on this collaboration platform is the property of the contributing authors.