Last Comment Bug 437169 - [SECURITY] Local files on the server can be attached to a bug (making them publicly visible) when importing bugs with -attach_path
: [SECURITY] Local files on the server can be attached to a bug (making them pu...
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Bug Import/Export & Moving (show other bugs)
: 2.22.1
: All All
: P1 minor (vote)
: Bugzilla 2.22
Assigned To: Greg Hendricks
: default-qa
Mentors:
Depends on: 344298
Blocks: 443044
  Show dependency treegraph
 
Reported: 2008-06-03 20:02 PDT by ilja
Modified: 2008-09-30 23:18 PDT (History)
7 users (show)
mkanat: approval+
mkanat: approval3.2+
LpSolit: blocking3.2+
mkanat: approval3.0+
LpSolit: blocking3.0.5+
mkanat: approval2.22+
LpSolit: blocking2.22.5+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
First Crack (1.04 KB, patch)
2008-06-04 09:44 PDT, Greg Hendricks
LpSolit: review+
Details | Diff | Review
Final (1.05 KB, patch)
2008-06-04 11:59 PDT, Greg Hendricks
LpSolit: review+
Details | Diff | Review

Description ilja 2008-06-03 20:02:22 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.14) Gecko/20080404 Firefox/2.0.0.14
Build Identifier: 3.0.4

when importing a bug in xml, there is an insecure call done to open() which could potentially allow command execution. the bug in the code is as follows: 
sub process_attachment() {
    my ( $twig, $attach ) = @_;
...
        my $encoding = $attach->first_child('data')->{'att'}->{'encoding'};
...
        elsif ($encoding =~ /filename/) {
...
            my $attach_filename = $attach_path . "/" . $attach->field('data'); <-- field('data') comes from the xml file
            open(ATTACH_FH, $attach_filename) or <-- here's the insecure open
                Error("cannot open $attach_filename", undef);
...
}

basicly, you could have something like "; <shell commands> |".
a nice solutions would be to use the 3-arg open. 
another issue is that there is no check done for directory traversal. 

Reproducible: Didn't try

Steps to Reproduce:
1.
2.
3.
Comment 1 Bradley Baetz (:bbaetz) 2008-06-03 22:30:52 PDT
why isn't any of this being caught by taint checks? Does the xml parser thing detaint stuff as a side effect, or something?

Broken by bug 344298, v2.22.1 and 2.23.3
Comment 2 Max Kanat-Alexander 2008-06-04 02:31:22 PDT
This is slightly mitigated by the fact that Bugzilla itself would never generate such a path, and the paths we *do* generate are fairly easy to check for (which should make this easier to fix).

However, yes, it's still definitely a security issue for any installation that has generally-available bug importing (which isn't any installation that I know of, but we offer the feature so I'm sure that some installations use it).
Comment 3 Frédéric Buclin 2008-06-04 08:16:48 PDT
(In reply to comment #1)
> why isn't any of this being caught by taint checks?

It's tainted:

Insecure dependency in piped open while running with -T switch at ./importxml.pl line 381, <> chunk 1.

I get this if I have (in the XML file):

<data encoding="filename">../../../../../../../../../../../../../../etc/X11/xorg.conf;kwrite |</data>

But if I have the following, xorg.conf is attached instead of the expected file:

<data encoding="filename">../../../../../../../../../../../../../../etc/X11/xorg.conf</data>

I could use it to attach /etc/passwd, and then everybody could happily view this file as an attachment in the bug.


Now if I add trick_taint($attach_filename); before calling open() in importxml.pl with the first data above, I get (instead of the "Insecure dependency" error):

./importxml.pl -v --attach_path data/ /root/patches/evil_bug2.xml
sh: data//../../../../../../../../../../../../../../etc/X11/xorg.conf: Permission denied
sh: kwrite: No such file or directory


So it seems the tainting protection is working correctly and you cannot execute commands locally. But you can still attach "secret" files, which is annoying.
Comment 4 Frédéric Buclin 2008-06-04 08:24:02 PDT
Changing the bug summary as you cannot run commands. But you can still make any local file public (including localconfig!).
Comment 5 Greg Hendricks 2008-06-04 09:44:57 PDT
Created attachment 323727 [details] [diff] [review]
First Crack

This is against the tip.
Comment 6 Max Kanat-Alexander 2008-06-04 10:04:22 PDT
Comment on attachment 323727 [details] [diff] [review]
First Crack

>+            $filename =~ s/(.*\/|.*\\)//g;

  I think that should be anchored at the beginning of the string, no?
Comment 7 Greg Hendricks 2008-06-04 10:24:02 PDT
(In reply to comment #6)
> (From update of attachment 323727 [details] [diff] [review])
> >+            $filename =~ s/(.*\/|.*\\)//g;
> 
>   I think that should be anchored at the beginning of the string, no?
> 

I don't see how it matters. There should never be any relative path anywhere in the string, and this should catch anything before the filename yes?

I am no REGEXP guru though so I am open to suggestions.
Comment 8 Frédéric Buclin 2008-06-04 10:30:27 PDT
The regexp is fine as is IMO. This way, it will catch filename.ext/../../../../../etc/passwd.
Comment 9 Frédéric Buclin 2008-06-04 10:54:20 PDT
Comment on attachment 323727 [details] [diff] [review]
First Crack

>+            $filename =~ s/(.*\/|.*\\)//g;

I think we should write /gs instead of /g to catch newlines correctly. open() is supposed to throw an error like this:

Unsuccessful open on filename containing newline at ./importxml.pl line 385, <> chunk 1.

But we can do this effort. So r=LpSolit with /gs.
Comment 10 Frédéric Buclin 2008-06-04 10:58:05 PDT
If someone else wants to review it too, please do. Requesting approval for now.
Comment 11 Greg Hendricks 2008-06-04 11:59:32 PDT
Created attachment 323738 [details] [diff] [review]
Final

With the gs
Comment 12 ilja 2008-06-04 12:43:17 PDT
that looks good to me, although, technically, there might still be a small infoleak this way. since .. is still ok, you could end up opening the previous directory. on some unices you can still do that and get to see the dirstructure. 
Comment 13 Frédéric Buclin 2008-06-04 12:46:30 PDT
(In reply to comment #12)
> you could end up opening the previous directory

Huh? How do you do that?
Comment 14 ilja 2008-06-04 13:18:59 PDT
basicly on some unices you can just read() a directory. 
on NetBSD 3.1 for example you can do cat ..; in a dir, and it will print out the dir structure of that directory.
Comment 15 Frédéric Buclin 2008-06-04 13:21:32 PDT
(In reply to comment #14)
> basicly on some unices you can just read() a directory. 

No, this won't work as the input string is tainted. You cannot run commands.
Comment 16 ilja 2008-06-04 13:25:50 PDT
I know that. this was just an example. 
suppose you'd put in nothing but .. as the filename. then it would read the directory data and assume that is the file used. 
Comment 17 Frédéric Buclin 2008-08-04 15:58:38 PDT
As bbaetz said in comment 1, 2.22.1 and higher, and 2.23.3 and higher are affected.
Comment 18 Max Kanat-Alexander 2008-08-12 02:39:00 PDT
Going to check in this bug now, right before the release.
Comment 19 Max Kanat-Alexander 2008-08-12 02:43:05 PDT
tip:

Checking in importxml.pl;
/cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
new revision: 1.85; previous revision: 1.84
done

3.2 branch:

Checking in importxml.pl;
/cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
new revision: 1.82.2.3; previous revision: 1.82.2.2
done

3.0 branch:

Checking in importxml.pl;
/cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
new revision: 1.74.2.2; previous revision: 1.74.2.1
done

2.22 branch:
Checking in importxml.pl;
/cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
new revision: 1.47.2.10; previous revision: 1.47.2.9
done

Comment 20 Max Kanat-Alexander 2008-08-12 04:12:23 PDT
Security advisory sent, unlocking bug.

Note You need to log in before you can comment on or make changes to this bug.