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
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