The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Bugzilla 2.22

Status

()

Bugzilla
Bug Import/Export & Moving
P1
minor
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: ilja, Assigned: Greg Hendricks)

Tracking

2.22.1
Bugzilla 2.22
Dependency tree / graph
Bug Flags:
approval +
approval3.2 +
blocking3.2 +
approval3.0 +
blocking3.0.5 +
approval2.22 +
blocking2.22.5 +

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.05 KB, patch
Frédéric Buclin
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
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.
Flags: blocking3.2?
Flags: blocking3.0.5?
OS: Windows Vista → All
Hardware: PC → All
Version: unspecified → 3.0.4
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking2.22.5?
Priority: -- → P1
Version: 3.0.4 → 2.22.1
Target Milestone: --- → Bugzilla 2.22
Version: 2.22.1 → 3.0.4

Comment 2

9 years ago
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

9 years ago
(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

9 years ago
Changing the bug summary as you cannot run commands. But you can still make any local file public (including localconfig!).
Flags: blocking3.2?
Flags: blocking3.2+
Flags: blocking3.0.5?
Flags: blocking3.0.5+
Flags: blocking2.22.5?
Flags: blocking2.22.5+
Summary: potential command execution when processing an attachment → Local files on the server can be attached to a bug (making them publicly visible) when importing bugs with -attach_path

Updated

9 years ago
Depends on: 344298
(Assignee)

Updated

9 years ago
Assignee: import-export → ghendricks
Flags: blocking3.2?
Flags: blocking3.2+
Flags: blocking3.0.5?
Flags: blocking3.0.5+
Flags: blocking2.22.5?
Flags: blocking2.22.5+

Updated

9 years ago
Flags: blocking3.2?
Flags: blocking3.2+
Flags: blocking3.0.5?
Flags: blocking3.0.5+
Flags: blocking2.22.5?
Flags: blocking2.22.5+
(Assignee)

Comment 5

9 years ago
Created attachment 323727 [details] [diff] [review]
First Crack

This is against the tip.
Attachment #323727 - Flags: review?(LpSolit)

Comment 6

9 years ago
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?
(Assignee)

Comment 7

9 years ago
(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

9 years ago
The regexp is fine as is IMO. This way, it will catch filename.ext/../../../../../etc/passwd.
Status: NEW → ASSIGNED

Comment 9

9 years ago
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.
Attachment #323727 - Flags: review?(LpSolit) → review+

Comment 10

9 years ago
If someone else wants to review it too, please do. Requesting approval for now.
Flags: approval?
Flags: approval3.2?
Flags: approval3.0?
Flags: approval2.22?
(Assignee)

Comment 11

9 years ago
Created attachment 323738 [details] [diff] [review]
Final

With the gs
Attachment #323727 - Attachment is obsolete: true
(Reporter)

Comment 12

9 years ago
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

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

Huh? How do you do that?

Updated

9 years ago
Attachment #323738 - Flags: review+
(Reporter)

Comment 14

9 years ago
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

9 years ago
(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.
(Reporter)

Comment 16

9 years ago
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. 
Group: webtools-security → bugzilla-security
Group: bugzilla-security → webtools-security

Updated

9 years ago
Blocks: 443044
Group: webtools-security → bugzilla-security

Comment 17

9 years ago
As bbaetz said in comment 1, 2.22.1 and higher, and 2.23.3 and higher are affected.
Severity: normal → minor
Summary: 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 publicly visible) when importing bugs with -attach_path
Version: 3.0.4 → 2.22.1

Comment 18

9 years ago
Going to check in this bug now, right before the release.
Flags: approval?
Flags: approval3.2?
Flags: approval3.2+
Flags: approval3.0?
Flags: approval3.0+
Flags: approval2.22?
Flags: approval2.22+
Flags: approval+

Comment 19

9 years ago
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

Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 20

9 years ago
Security advisory sent, unlocking bug.
Group: bugzilla-security
You need to log in before you can comment on or make changes to this bug.