1079065 - (CVE-2014-8630) [SECURITY] Always use the 3 arguments form for open() to prevent shell code injection

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[John Lightsey]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; Trident/7.0; Touch; .NET4.0E; .NET4.0C; Tablet PC 2.0; .NET CLR 3.5.30729; .NET CLR 2.0.50727; .NET CLR 3.0.30729; rv:11.0) like Gecko

Steps to reproduce:

I noticed this during a cursory look through 4.4.6 to examine the other security issues that were fixed.

The filtering of the product name before the 2 arg open in reports.cgi generate_chart() only sanitizes against path traversal attacks. 2 arg open in perl is generally exploitable for shell code injection if the attacker can control the first or last character of the filename. As far as I can tell, an attacker could leverage this input point IF they have enough permissions to control a project name.

The code that validates project names doesn't appear to prevent using a name like:

xyzzy||perl -e 'use URI::Escape;system(uri_unescape(q{touch%20%2Ftmp%2FI_RAN_SHELLCODE}))'|

I don't have a test server with enough permissions to rename a project or time to set one up before this weekend. I'm expecting that you guys will be able to test the injection readily tough and would prefer to have this reported sooner rather than later.

To verify:
- rename a project to the injection listed above
- run reports.cgi for this project to hit the generate_chart() function
- verify that /tmp/I_RAN_SHELLCODE was created

If you have any trouble verifying this, let me know and I'll check it this weekend.

Comment 1

[Gervase Markham [:gerv]]

Hi John,

Thanks for the tip - I'll try this out now.

Gerv

Comment 2

[Gervase Markham [:gerv]]

There are two open()s in this file.

The first line of code is line 124:

    open(DATA, '<', "$dir/0")

$dir is passed in to the get_data function. The call site is on line 55:

    my @data = get_data($dir);

Line 39 says:

my $dir       = bz_locations()->{'datadir'} . "/mining";

So this is not attacker-controlled, and not vulnerable.

The second open is on line 141:

    if (! open FILE, $data_file) {

    my $data_file = $dir . '/' . $product->id;

$product->id is a Bugzilla-assigned numeric value, and is not attacker-controlled. (The product name is $product->name.) $dir is passed in, but is the same value as above:

my $dir       = bz_locations()->{'datadir'} . "/mining";

So also, as far as I can see, not vulnerable.

Please let me know if you think I've misanalysed this, and thanks very much for auditing our code and reporting what you find :-)

Gerv

Comment 3

[John Lightsey]

I think you're looking at 4.5.6. I was looking at 4.4.6.

sub generate_chart {
    my ($dir, $image_file, $product, $datasets) = @_;
    $product = $product ? $product->name : '-All-';
    my $data_file = $product;
    $data_file =~ s/\//-/gs;
    $data_file = $dir . '/' . $data_file;

    if (! open FILE, $data_file) {

Comment 4

[Simon Green]

I don't believe that there is a security issue here because the files in data/mining are not executable, and thus the following

  1 #!/usr/bin/perl
  2 
  3 my $a = q#/home/simon/bugzilla/fourfour/data/mining/x ||perl -e 'use URI::Escape;system(uri_unescape(q{touch%20%2Ftmp%2FI_RAN_SHELLCODE}))'|#;
  4 open FILE, $a;

returns sh: 1: /home/simon/bugzilla/fourfour/data/mining/x: not found

Having said that, Windows or other Linux operating systems might be affected. I don't use Windows so cannot test that. If it is an issue, it would only be in 4.4 and earlier.

However, I think all two params open() calls should be changed to three parameters. Not doing so is just asking for trouble at some point. It's kinda like not updating bash because you are not affected by shellshock now.

  -- simon

Comment 5

[Gervase Markham [:gerv]]

Ah, yes - looks like this got changed in bug 604388 so that data wasn't lost when products were renamed. I thought this code looked unfamiliar.

Simon: your error says "file not found"; when I turn the starting filename to one which actually exists, I get "permission denied", which is perhaps the error you meant. However, I also get a file /tmp/I_RAN_SHELLCODE, so I think perhaps this is a potential risk.

However, there are a number of migitations. Firstly, only a limited number of people (those with the 'editcomponents' privilege) can edit products. Secondly, the length of a product name is limited to 64 characters both client and server-side. This might not be enough of a mitigation, except that because the code converts "/" to "-", you can't use "/" - hence Dylan's Perl-based URI-unescaping trick. However, that blows your 64-character budget if you want a command with "/" in. (Product names absolutely can contain spaces.)

I've tried creating a product called "x || gerv |", where gerv is a command on my system which does something noticeable, and I can't exploit this. Even pasting Dylan's exploit code directly into the variable doesn't work - is there a permissions issue?

However, I am by no means willing to bet that it's unexploitable. We should certainly stop using 2-arg open().

Gerv

Comment 6

[John Lightsey]

The editcomponents privilege seems like the only significant limitation to me. If the total limitation on the product name is 64 characters, you can use 61 for the injection. That is plenty of space to download code or switch to a different payload that allows more space. For example, I could put the payload in the user agent string and have the shellcode execute it:

HTTP_USER_AGENT='touch /tmp/I_RAN_AGAIN' perl -e 'my $fn = q{x;sh -c '\''$HTTP_USER_AGENT'\''|}; print length($fn) . "\n"; open FH, "/tmp/$fn";'

Thanks for confirming this so quickly.

Comment 7

[Gervase Markham [:gerv]]

Hi John,

I am going to look at producing patches to eliminate 2-arg open entirely. (Remembering, of course, this bug is not present on the latest Bugzilla.) However, I haven't managed to exploit this, although I really can't see why it doesn't work. If you were able to spend a fun afternoon confirming it works with an actual Bugzilla, that would be great :-)

Gerv

Comment 8

[John Lightsey]

No problem. I'll spin up a VM with Bugzilla this weekend to work up a proper proof of concept. There might be something I'm missing in reading through the code that mitigates this further.

Comment 9

[Dylan Hardison [:dylan] (he/him)]

(In reply to Gervase Markham [:gerv] from comment #7)
> Hi John,
> 
> I am going to look at producing patches to eliminate 2-arg open entirely.
> (Remembering, of course, this bug is not present on the latest Bugzilla.)
> However, I haven't managed to exploit this, although I really can't see why
> it doesn't work. If you were able to spend a fun afternoon confirming it
> works with an actual Bugzilla, that would be great :-)
> 
> Gerv

Do you mean entirely from the codebase? If so, allow me to aid you in
your quest:

these are the occurrences in master:

search_plugin.cgi: Two-argument "open" used at line 30, column 5.
showdependencygraph.cgi: Two-argument "open" used at line 52, column 5.
showdependencygraph.cgi: Two-argument "open" used at line 261, column 5.
showdependencygraph.cgi: Two-argument "open" used at line 290, column 5.
Bugzilla/Attachment.pm: Two-argument "open" used at line 331, column 13.
Bugzilla/Attachment.pm: Two-argument "open" used at line 377, column 13.
Bugzilla/Error.pm: Two-argument "open" used at line 77, column 9.
Bugzilla/Config/Common.pm: Two-argument "open" used at line 237, column 13.
Bugzilla/Install/CPAN.pm: Two-argument "open" used at line 200, column 5.
Bugzilla/Install/Filesystem.pm: Two-argument "open" used at line 636, column 9.
Bugzilla/Send/Sendmail.pm: Two-argument "open" used at line 34, column 5.

If you end up not being able to slay these dragons, I would be most happy to oblige.

Comment 10

[Gervase Markham [:gerv]]

Attachment: Patch v.1 for 4.0

Here's a patch for 4.0 (to start with) that eliminates all uses of 2-argument open. I found all the references using this grep:

ack "open[( ]" *.cgi *.pl Bugzilla

I then removed false positives and fixed the rest. 

I found one other instance which is sort-of exploitable; if you are an admin and can configure the "webdotbase" parameter, which is the path to an executable, you can (rather unsurprisingly) cause the execution of any local executable (although you can't control the parameters). This hardly seems like a hole, given that this is the point, and my patch doesn't change it, but I mention it for completeness. One possible fix is that suggested in bug 950486.

Gerv

Comment 11

[Daniel Veditz [:dveditz]]

Should we call the security impact "moderate" because Product renaming is generally limited to trusted admins or near-admins? Although if that trust is misplaced that one moderate data-administration privilege could be leveraged into much more (assuming this is confirmed as a workable attack) so I could also be sold on "high".

Gerv, Simon-- please change the sec- keyword if you think I'm low-balling the impact.

Comment 12

[Gervase Markham [:gerv]]

(In reply to Dylan William Hardison [:dylan] from comment #9)
> Do you mean entirely from the codebase? If so, allow me to aid you in
> your quest:

Sorry I wasn't clear; I meant that I've not been able to actually exploit Bugzilla by creating a product with an appropriate name, and that if someone were actually to confirm it would be done, that would be great. Yes, I can use grep - see attached patch :-)

Gerv

Comment 13

[Gervase Markham [:gerv]]

dveditz: not sure what the criteria are, but on BMO, 18 people are admins, and 25 have editcomponents. So for us, "near-admin" is probably right. We don't let just anyone create new products in our Bugzilla. But it could be different elsewhere.

Gerv

Comment 14

[Gervase Markham [:gerv]]

Attachment: Patch v.2 for 4.0

Oops, missed one. (I planned to come back to it and forgot.)

Gerv

Comment 15

[Frédéric Buclin]

I think that what mitigates the problem with 2 args open() calls are:

1) command would be executed as the 'apache' user, which has limited privileges;
2) admins are trusted users and I can hardly imagine that they want to shoot themselves in the foot. On test installations such as landfill where mostly everybody can ask to be an admin, that's another story. But 1) still stands.

So to me, this bug looks more like a security enhancement than a security bug. But I fully agree that 2 args is bad and should be forbidden from our codebase. 3 args is the way to go.

Comment 16

[John Lightsey]

I had some free time tonight to configure a test server and work up a proper proof of concept.

Rename a project to:

x;eval $HTTP_USER_AGENT|

The payload goes into your user agent string. It needs to be shell code and you start off with an empty PATH variable. That's probably the hurdle that was making it difficult to reproduce. It's simple to work around.

root@tiger:/srv/www# curl -o /dev/null -A 'export PATH=/usr/bin:/bin;echo "I am running code as "`whoami` > /tmp/I_RAN_FROM_THE_USERAGENT' 'http://localhost/reports.cgi?product=x%3Beval%20%24HTTP_USER_AGENT%7C&datasets=x'
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   303  100   303    0     0   1506      0 --:--:-- --:--:-- --:--:--  1515
root@tiger:/srv/www# cat /tmp/I_RAN_FROM_THE_USERAGENT
I am running code as www-data
root@tiger:/srv/www#

Whether or not you guys treat this as a vulnerability is your call. If it's a vulnerability, it seems minor to me since the internal permission requirements are high. #1 above doesn't sound like a realistic argument though. Most remote code execution flaws execute as the web server user.

I could only see this flaw being a problem in environments where Bugzilla and some other web application are hosted under the same system user. Nothing in Bugzilla appears to suggest that admins, limited to the editcomponent privilege or not, are intended to run arbitrary code on the webserver. Flaws requiring highly privileged users are always a bit fuzzy to handle though.

Comment 17

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8501084 [details] [diff] [review]
Patch v.2 for 4.0

Review of attachment 8501084 [details] [diff] [review]:
-----------------------------------------------------------------

Using the revised example, I am able to confirm this fixes the bug.

r=dylan

we'll need this for bmo as well.

Comment 18

[Gervase Markham [:gerv]]

Hi John,

Thanks for taking the time to reproduce this. We appreciate you helping us out in this way. I'll produce patches for all supported versions and for the bugzilla.mozilla.org branch.

Dylan: can I confirm that in your review, you checked that I hadn't screwed up any of the conversions?

Gerv

Comment 19

[Gervase Markham [:gerv]]

Attachment: Patch v.1 for 4.2

Comment 20

[Gervase Markham [:gerv]]

Attachment: Patch v.1 for 4.4

Comment 21

[Gervase Markham [:gerv]]

Attachment: Patch v.1 for tip

Comment 22

[Gervase Markham [:gerv]]

Attachment: Patch v.1 for BMO

BMO has some additional open() calls not present elsewhere. I suspect this one from Bugzilla/PatchReader/AddCVSContext.pm:

open my $fh, "<", $this->{FILENAME} or die "Could not open $this->{FILENAME}";

would be exploitable by someone able to control the name of a file in our CVS repo... if we were still using CVS, which we aren't.

This file is a local copy of the PatchReader.pm module, which still contains the same construction here:
http://cpansearch.perl.org/src/TMANNERM/PatchReader-0.9.6/lib/PatchReader/AddCVSContext.pm
PatchReader is an optional module for Bugzilla. Do we need to notify the maintainer to make a security release?

Gerv

Comment 23

[Gervase Markham [:gerv]]

wicked: see comment 22. CPAN suggests you are the maintainer of PatchReader.

I haven't followed it through to be certain that it's exploitable, but it's strongly suspect.

Gerv

Comment 24

[John Lightsey]

One tip when you're looking to get rid of all two argument opens.  IO::File->new($filename) and FileHandle->new($filename) are additional ways of writing them.

perl -e 'my $user_input = "whoami|"; my $fh = IO::File->new($user_input); print while (readline($fh));'

Comment 25

[John Lightsey]

I would also agree that PatchReader::AddCVSContext is exploitable. You need the checkout command to succeed to get to the  open() call though. Probably the simplest way to attack this code is with the same kind of payload that defeats file existence checks.

IE: code that does
my $untrusted_filename = ...

if (-e $untrusted_filename) {
   open my $fh, $untrusted_filename;
   ...

You get past this with a null before the pipe character. It'll end up executing the target file rather than opening it for reading. In the case of AddCVSContext, the null will strip the pipe character from the system() call that runs cvs and into the open() call.

Something like "directory/filename\0|"

Nice find.

Comment 26

[Gervase Markham [:gerv]]

Interesting :-) John: are you at all interested in doing any more security code-reviewing on Bugzilla? :-)

Gerv

Comment 27

[John Lightsey]

Sure, I'll definitely dig into the codebase more when I have time. If there's anything specific you'd like a second set of eyes on, let me know and I'll focus in that direction.

Comment 28

[Gervase Markham [:gerv]]

John: I guess the key risks are constructs which could lead to server compromise, and constructs which could lead to unauthorized information disclosure from Bugzilla itself. (Those might require a little more Bugzilla-specific knowledge.)

Also, I would draw your attention to:
https://www.mozilla.org/security/bug-bounty.html
This bug is only a sec-moderate, but find a sec-high or sec-critical and we will show our gratitude in the appropriate manner :-)

Gerv

Comment 29

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8501598 [details] [diff] [review]
Patch v.1 for 4.2

Review of attachment 8501598 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, despite my horror at the cases where we intentionally shell out.

r=dylan

::: Bugzilla/Attachment/PatchReader.pm
@@ +108,5 @@
>  
>      # Send through interdiff, send output directly to template.
>      # Must hack path so that interdiff will work.
>      $ENV{'PATH'} = $lc->{diffpath};
> +    open my $interdiff_fh, '-|', "$lc->{interdiffbin} $old_filename $new_filename";

definitely will want to invest in shell quoting in the future.

::: showdependencygraph.cgi
@@ +258,5 @@
>          or warn install_string('chmod_failed', { path => $pngfilename,
>                                                   error => $! });
>  
>      binmode $pngfh;
> +    open(DOT, '-|', "\"$webdotbase\" -Tpng $filename");

yeeesh, this is an improvement, but I would like to see a shell_escape() function around these filenames.

@@ +287,5 @@
>          or warn install_string('chmod_failed', { path => $mapfilename,
>                                                   error => $! });
>  
>      binmode $mapfh;
> +    open(DOT, '-|', "\"$webdotbase\" -Tismap $filename");

see previous comment

Comment 30

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8501600 [details] [diff] [review]
Patch v.1 for 4.4

Review of attachment 8501600 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan

Comment 31

[John Lightsey]

Just so it doesn't get forgotten in the mix, are you folks coordinating to get PatchReader fixed on CPAN or should I contact that author?

Comment 32

[Frédéric Buclin]

(In reply to John Lightsey from comment #31)
> Just so it doesn't get forgotten in the mix, are you folks coordinating to
> get PatchReader fixed on CPAN or should I contact that author?

the author is already aware of the problem

Comment 33

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8501627 [details] [diff] [review]
Patch v.1 for BMO

Review of attachment 8501627 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan

Comment 34

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8501604 [details] [diff] [review]
Patch v.1 for tip

Review of attachment 8501604 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan

Comment 35

[Gervase Markham [:gerv]]

OK, so the situation is:

* The flaw in editproducts.cgi is a medium-severity vulnerability for 4.4 and older.

* The flaw in PatchReader is also not particularly serious, because this code wasn't well-used even when people used CVS (I don't know of users other than Mozilla) and so few people use CVS these days anyway. We should wait until it's fixed on CPAN, and then publish a note in our advisory encouraging people to upgrade. Bug 1068494 removes the use of PatchReader::AddCVSContext anyway; we should make sure that's committed to trunk.
 
teemu: what are the plans for a new version of PatchReader?

dkl/glob: can we apply the patch to BMO straight away?

dkl: will you be coordinating a security release for this? Given the severity level, can we hold it for a while to see if we find more stuff?

Gerv

Comment 36

[Frédéric Buclin]

Again, is any of the vulnerabilities reported here exploitable by someone who has not editcomponents privileges? If not, then I will repeat what I said in comment 15: this is more a security enhancement (and so no security advisory is needed and no security releases are required either) than a security fix.

Comment 37

[David Lawrence [:dkl]]

(In reply to Gervase Markham [:gerv] from comment #35)
> dkl/glob: can we apply the patch to BMO straight away?

Once we commit it upstream we will backport the fix. As for when see below.

> dkl: will you be coordinating a security release for this? Given the
> severity level, can we hold it for a while to see if we find more stuff?

I agree with LpSolit and others that since it requires being an empowered user, it is not 
as a critical issue. Therefore we bundle it with any other issues that come in and release
at a later time.

dkl

Comment 38

[Dave Miller [:justdave]]

(In reply to David Lawrence [:dkl] from comment #37)
> I agree with LpSolit and others that since it requires being an empowered
> user, it is not as a critical issue. Therefore we bundle it with any other issues that come
> in and release at a later time.

Things are never as simple as they seem when you look at the big picture.  While this vulnerability by itself is not very critical, it becomes a non-privileged remote code execution vulnerability when combined with CVE-2014-1572 that we just released a fix for last week, if exploited on a system with lazy group regexp configuration.

Comment 39

[Gervase Markham [:gerv]]

(In reply to David Lawrence [:dkl] from comment #37)
> (In reply to Gervase Markham [:gerv] from comment #35)
> > dkl/glob: can we apply the patch to BMO straight away?
> 
> Once we commit it upstream we will backport the fix. As for when see below.

You know I backported it already, right? Or by backport did you mean "commit"?

Gerv

Comment 40

[David Lawrence [:dkl]]

(In reply to Gervase Markham [:gerv] from comment #39)
> (In reply to David Lawrence [:dkl] from comment #37)
> > (In reply to Gervase Markham [:gerv] from comment #35)
> > > dkl/glob: can we apply the patch to BMO straight away?
> > 
> > Once we commit it upstream we will backport the fix. As for when see below.
> 
> You know I backported it already, right? Or by backport did you mean
> "commit"?
> 
> Gerv

Admittedly I see that now, but I did actually mean commit. If this was a critical vulnerability I would have WebOps hotpatch (which of course we would have to keep doing each push) but since it is not, we can commit when the upstream version is committed as it will be public then.

dkl

Comment 41

[Teemu Mannermaa (:wicked)]

(In reply to Gervase Markham [:gerv] from comment #35)
> teemu: what are the plans for a new version of PatchReader?

It's long overdue for one for sure. I guess I should sync that release with release of this in Bugzilla?

Looks like attachment #8501627 [details] [diff] [review] contains the two lines I need to change in PatchReader, right? In case anybody wants to help me get this released, you could make a patch against https://github.com/tmannerm/PatchReader (attaching here or emailing it to me is fine) with any changelog text that's appropriate for the issue. Thanks!

Comment 42

[Gervase Markham [:gerv]]

(In reply to Teemu Mannermaa (:wicked) from comment #41)
> Looks like attachment #8501627 [details] [diff] [review] [diff] [review] contains the two
> lines I need to change in PatchReader, right? 

Well, I would advise you to do your own audit of all open() calls in PatchReader. But you will certainly need those two lines.

Gerv

Comment 43

[Gervase Markham [:gerv]]

Is the status here that we are holding until a security release comes along that we can piggyback on?

wicked: have you managed to ship an updated PatchReader yet?

Gerv

Comment 44

[Daniel Veditz [:dveditz]]

call this one CVE-2014-8630

Comment 45

[Frédéric Buclin]

Marking the bug as blocker to have it in our radar.

Comment 46

[David Lawrence [:dkl]]

Attachment: Patch v.2 for tip

Updated patch for tip.

Comment 47

[David Lawrence [:dkl]]

Attachment: Patch v.1 for 5.0

Comment 48

[David Lawrence [:dkl]]

To ssh://[e-mail]/bugzilla/bugzilla.git
   367d9c2..4dabf1a  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   272b0b6..19117cc  5.0 -> 5.0

To ssh://[e-mail]/bugzilla/bugzilla.git
   d98f8cd..f5b9cba  4.4 -> 4.4

To ssh://[e-mail]/bugzilla/bugzilla.git
   f78bebb..7e1b7a5  4.2 -> 4.2

To ssh://[e-mail]/bugzilla/bugzilla.git
   9162eab..5c7b317  4.0 -> 4.0

Comment 49

[David Lawrence [:dkl]]

Released