437169 - [SECURITY] Local files on the server can be attached to a bug (making them publicly visible) when importing bugs with -attach_path

Status: RESOLVED FIXED

(Bugzilla :: Bug Import/Export & Moving, defect, P1)

Description

[ilja]

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

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]

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]

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

Changing the bug summary as you cannot run commands. But you can still make any local file public (including localconfig!).

Comment 5

[Greg Hendricks]

Attachment: First Crack

This is against the tip.

Comment 6

[Max Kanat-Alexander]

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]

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

The regexp is fine as is IMO. This way, it will catch filename.ext/../../../../../etc/passwd.

Comment 9

[Frédéric Buclin]

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]

If someone else wants to review it too, please do. Requesting approval for now.

Comment 11

[Greg Hendricks]

Attachment: Final

With the gs

Comment 12

[ilja]

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]

(In reply to comment #12)
> you could end up opening the previous directory

Huh? How do you do that?

Comment 14

[ilja]

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]

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

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]

As bbaetz said in comment 1, 2.22.1 and higher, and 2.23.3 and higher are affected.

Comment 18

[Max Kanat-Alexander]

Going to check in this bug now, right before the release.

Comment 19

[Max Kanat-Alexander]

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]

Security advisory sent, unlocking bug.