We did a Bugzilla security release today, to fix some holes responsibly disclosed to us by Check Point Vulnerability Research, to whom we are very grateful. The most serious of them would allow someone to create and control an account for an arbitrary email address they don’t own. If your Bugzilla gives group permissions based on someone’s email domain, as some do, this could be a privilege escalation.
(Update 2014-10-07 05:42 BST: to be clear, this pattern is most commonly used to add “all people in a particular company” to a group, using an email address regexp like .*@mozilla.com$. It is used this way on bugzilla.mozilla.org to allow Mozilla Corporation employees access to e.g. Human Resources bugs. Membership of the Mozilla security group, which has access to unfixed vulnerabilities, is done on an individual basis and could not be obtained using this bug. The same is true of BMO admin privileges.)
These bugs are actually quite interesting, because they seem to represent a new Perl-specific security problem. (At least, as far as I’m aware it’s new, but perhaps we are about to find that everyone knows about it but us. Update 2014-10-08 09:20 BST: everything old is new again; but the level of response, including changes to CGI.pm, suggest that this had mostly faded from collective memory.) This is how it works. I’m using the most serious bug as my example. The somewhat less serious bugs caused by this pattern were XSS holes. (Check Point are going to be presenting on this vulnerability at the 31st Chaos Communications Congress in December in Hamburg, so check their analysis out too.)
Here’s the vulnerable code:
my $otheruser = Bugzilla::User->create({
login_name => $login_name,
realname => $cgi->param('realname'),
cryptpassword => $password});
This code creates a new Bugzilla user in the database when someone signs up. $cgi is an object representing the HTTP request made to the page.
The issue is a combination of two things. Firstly, the $cgi->param() call is context-sensitive – it can return a scalar or an array, depending on the context in which you call it – i.e. the type of the variable you assign the return value to. The ability for functions to do this is a Perl “do what I mean” feature.
Let’s say you called a page as follows, with 3 instances of the same parameter:
index.cgi?foo=bar&foo=baz&foo=quux
If you call param() in an array context (the @ sigil represents a variable which is an array), you get an array of values:
@values = $cgi->param('foo');
-->
['bar', 'baz', 'quux']
If you call it in a scalar context (the $ sigil represents a variable which is a scalar), you get a single value, probably the first one:
$value = $cgi->param('foo');
-->
'bar'
So what context is it being called in, in the code under suspicion? Well, that’s exactly the problem. It turns out that functions called during hash value assignment are evaluated in a list context. However, when the result comes back, that value or those values are assigned to be part of uthe hash as if they were a set of individual, comma-separated scalars. I suspect this behaviour exists because of the close relationship of lists and hashes; it allows you to do stuff like:
my @array = ("foo", 3, "bar", 6);
my %hash = @array;
-->
{ "foo" => 3, "bar" => 6 }
Therefore, when assigning the result of a function call as a hash value, if the return value is a single scalar, all goes as you would expect, but if it’s an array, the second and subsequent values end up being added as key/value pairs in the hash as well. This allows an attacker to override values already in the hash (specified earlier), which may have already been validated, with values controlled by them. In our case, real_name can be any string, so doesn’t need validation, but login_name most definitely does, and it already has been by the time this code is called.
So, in the case of the problematic code above, something like:
index.cgi?realname=JRandomUser&realname=login_name&realname=admin@mozilla.com
would end up overriding the already-validated login_name variable, giving the attacker control of the value used in the call to Bugzilla::User->create(). Oops.
We found 15 instances of this pattern in our code, four of which were exploitable to some degree. If you maintain a Perl web application, you may want to audit it for this pattern. Clearly, CGI.pm param() calls are the first thing to look for, but it’s possible that this pattern could occur with other modules which use the same context-sensitive return feature. The generic fix is to require the function call to be evaluated in scalar context:
my $otheruser = Bugzilla::User->create({
login_name => $login_name,
realname => scalar $cgi->param('realname'),
cryptpassword => $password});
I’d say it might be wise to not ever allow hash values to be assigned directly from functions without a call to scalar.
Pingback: ADnjus | Bugzilla 0-day can reveal 0-day bugs in OSS giants like Mozilla, Red Hat
List context is a Perl feature that has sadly been misused in this particular case, but doesn’t qualify as a vulnerability. As you said, the proper use when assigning a context sensitive value to a hash is supposed to be together with the “scalar” statement. Better yet: sanitize your parameters upfront and only pass on variables which you control, never something like user input.
This is all textbook web security, as you know, and language agnostic. What happened was just an unfortunate slip from the developers, which lead to this serious vulnerability.
This looks like a variant of HPP (https://www.owasp.org/index.php/Testing_for_HTTP_Parameter_pollution_(OTG-INPVAL-004). Pretty cool bug!
“Supplying multiple HTTP parameters with the same name may cause an application to interpret values in unanticipated ways. By exploiting these effects, an attacker may be able to bypass input validation, trigger application errors or modify internal variables values. As HTTP Parameter Pollution (in short HPP) affects a building block of all web technologies, server and client side attacks exist.”
Breno: I wouldn’t call this “an unfortunatey slip from the developers”, I’d say that this particular feature (or rather, the combination of two features) is a footgun. And its use in CGI.pm is very unfortunate.
For a start, “param” is a singular word. If it were called “params”, it might be a bit more obvious that it can return a list.
Yep, I think you are right. The general attack idea is known, but the particular vulnerability caused by the list context of hash assignments is (to my knowledge) new.
Pingback: Bugzilla 0-day can reveal 0-day bugs in OSS giants like Mozilla, Red Hat - Binary Reveux
This is very old hat: http://seclists.org/vulnwatch/2006/q4/6
And among several similar reasons, it’s why Hash::MultiValue was invented for parameter handling in Plack: http://bulknews.typepad.com/blog/2009/12/perl-why-parameters-sucks-and-what-we-can-do.html
The method is called “param” because it returns all of the values of a particular (single) parameter. CGI.pm already has a “params” method which returns a list containing the names of all of the parameters in the HTTP request.
Well, OK, naming is hard :-) But I think my point stands – it’s not all that obvious that $cgi->param(‘foo’) can return a list, and it’s easy to write code which assumes it doesn’t, and that code works fine right up until the time someone attacks you.
If you think I’m wrong about “easy”, let’s see how many CVEs there are over the next few months as other Perl codebases get audited for this, and what Check Point say at CCC in December.
Primary maintainer of CGI.pm here – there’s a lot of history in CGI.pm that makes it difficult to patch these kind of issues without massively breaking back compat, but of course critical vulnerabilities need looking at. I’ve made some progress with the existing issues queue and aim to really gut the module with v5 releases, probably post perl 5.22 when it should (hopefully) be clearer that CGI.pm is no longer core.
There’s an important bit of information missing here. The Bugzilla code actually subclasses CGI.pm with Bugzilla::CGI, so the link to the CGI.pm param method is misleading as Bugzilla::CGI has its own param method that overrides the CGI.pm param method and does not call SUPER. So really you need to be pointing at the Bugzilla source code: https://bzr.mozilla.org/bugzilla/4.4/view/head:/Bugzilla/CGI.pm#L345
I’ve been working my way through the Bugzilla::CGI code to get any useful changes back upstream into CGI.pm, some of these i can and some i can’t at present due to aforementioned back compat issues.
As always – patches welcome :)
Ooops! sorry, clarification that Bugzilla::CGI *does* call SUPER in the param method. Nonetheless, previous link still applies.
It’s always easy to write code with bugs in when you don’t read (or, perhaps more importantly, understand) the documentation :-)
I’m happy to admit that I’ve written code with this bug in. I don’t blame Perl, I blame my own incompetance.
Part of the confusion is that => looks like a hash construction operator, but in fact it’s just a list separator that magically quotes its LHS. (Similar confusions exist in other languages, e.g. in C++ T t(); is a function declaration and not a default constructed variable, while in JavaScript ({x : y}) is an object initialised with a property but {x : y} is a labelled expression in a block.)
Hi Lee,
Thanks for dropping in :-) Glad to hear you are on the case. As you note, we do call SUPER, hence we inherit the wantarray behaviour.
I completely understand that back compat means it’s hard to fix these issues. I’m not blaming you at all. If I were you, I think I’d add explicit scalarparam() and arrayparam() calls (naming is hard; do think of better ones) and document param() as being deprecated. That way, people have to make an explicit choice about what they want, and it’s not possible to introduce a bug through unawareness of the return type.
Pingback: В BugZilla устранена опасная уязвимость, позволяющая повысить свои привилегии | AllUNIX.ru — Всероссийский портал о UNIX-системах
Yes, unfortunately CGI.pm has a lot of history that makes this kind of stuff “fun”.
My first thought is to add a warning so any call to ->param in list context will raise loud noises and point the user at (to be updated) documentation. Of course, this warning would be configurable so can be switched off.
I think it’s important that any users of the module can see clearly that they might be doing something wrong and many don’t ever see blog posts like this, or follow CVE announcements and the like; so having them upgrade CGI and suddenly see scary warnings might get their attention.
Actually splitting ->param into two distinct methods would be a good idea for the v5 roadmap. Deprecating ->param in the v4 releases is probably going to upset far more people than simply raising a warning. As i said, fun :)
Not a new issue (using list return values when building a hash) and no “Perl bug” at all. What about “realname” being “%00” or “do_something_very_bad()” – would you allow that to enter your database? Great fun if the realname is shown somewhere with admin priviledges…
Always check your arguments, expecially when they come from (web) users. Perl’s taint mode helps for remembering the checks.
You put “Perl bug” in quotes – who are you quoting? I never said it’s a bug in Perl. It’s a footgun, but it’s not a bug.
I’d be entirely fine with a realname of %00 or do_something_very_bad(), because we don’t pass the string unfiltered to the database.
Well, it’s a CGI bug, but still, Perl could offer an optional, stricter hash-initialization mode where, a duplicate key in {}, or a duplicate key in list assignment to a hash would automatically error.
Something along the lines of:
use strict ‘hash-init’;
my %foo = ( key => $param, key => $param ); # dies
but re-assignment won’t die:
my %foo = ( key => $param);
$foo{key} = $param2; # does not die
In my experience, doubled keys within the same assignment are accidental bugs in 99% of cases – an optional strict mode would not only solve security bugs like that but also avoid accidental bugs.
There are Tie-modules which implement similar behavior and of course the option to lock hashes, but this is not quite the same – often changing the the values is desired, the bugs are usually, where this happens within the same statement.
Oops, clarification: not a bug in CGI.pm itself of course, just an unfortunate design decision which easily causes bugs. During re-read I found my statement easy to misunderstand.
This wouldn’t necessarily cover all bases, because an attacker could still add extra optional members to the hash which were not always present on some codepaths but which were checked for later, and that would avoid this “die” saving the developer. I agree it would do a lot, but it wouldn’t be foolproof.
Pingback: Critical Bugzilla vulnerability could give hackers access to undisclosed software flaws - Computer Stuff
Again, validating user input before doing anything with it is textbook security and, if you’re doing anything other than that, your code is likely susceptible to attacks – no matter what language you pick.
That said, I agree with you that frameworks exist to assist developers in not shooting themselves on the foot, and any measures that can be taken to prevent security issues and encourage best practices should be taken. This is one of the reasons CGI.pm was removed from core in modern perl releases :)
Modern Perl frameworks such as Dancer (perldancer.org) turn lists of parameters in param(“foo”) into a list reference (which is a single value), not a common list. If Bugzilla had been written in Dancer, for example, the bug would likely have been just a bug, not a vulnerability, as it would turn “realname” into an array reference and not expand into the hash itself.
Other popular Perl web frameworks such as Catalyst (catalystframework.org) and Mojolicious (mojolicio.us) have recently updated their code and documentation as well. Catalyst included a bigger “hey, don’t do that” note next to param()’s documentation pointing to this post, and Mojolicious made “param()” only return one value, providing “multi_param()” if you want the entire list – making the code more robust against this kind of mishap.
I’m not sure what you would have had us do differently; the real name doesn’t need validating because it can be any string. We’re not coding in C here.
The Dancer solution is bogus, because it means to call the param() function reliably, you either need to do:
my $p = $cgi->param('foo');
my $scalar = ref $p eq 'ARRAY' ? $p->[0] : $p;
or
my @array = ref $p eq 'ARRAY' ? @$p : ($p);
And that’s just ugly. See http://bulknews.typepad.com/blog/2009/12/perl-why-parameters-sucks-and-what-we-can-do.html for more on this (with Catalyst as the example).
Pingback: Bugzilla zero-day can reveal zero-day vulnerabilities in top open-source projects | eKanpSack
I never realized that about javascript! So all I was missing was the parentheses to make it work.
FWIW – v4.05 of CGI.pm has been pushed to CPAN, which addresses this issue (amongst others, please see Changes).
Didn’t mean it was foolproof, but it would help in avoiding many of bugs, just like the other “strict” rules.
The suggested strict mode could also force the “=>” operator to evaluate its arguments as scalars or references, of course, as putting list-type things next to it makes very little sense.
Pingback: В BugZilla устранена опасная уязвимость, открывшая новый вид атак на web-приложения | AllUNIX.ru — Всероссийский портал о UNIX-системах
Pingback: Bugzilla 0-day can reveal 0-day bugs in OSS giants like Mozilla, Red Hat | VIGALUM
Pingback: Bugzilla 0-day can reveal 0-day bugs in OSS giants like Mozilla, Red Hat | infopunk.org
Pingback: SB14-293: Vulnerability Summary for the Week of October 13, 2014 « CyberSafe NV
Pingback: SB14-293: Vulnerability Summary for the Week of October 13, 2014 | 007 Software
Pingback: Bulletin (SB14-293) Vulnerability Summary for the Week of October 13, 2014 | ECI Networks
Pingback: SB14-293: Vulnerability Summary for the Week of October 13, 2014 | Blogg'n @ ECI
Pingback: SB14-293: Vulnerability Summary for the Week of October 13, 2014 « ECI Blog @WordPress
Pingback: A Bug in Bug Tracker "Bugzilla" exposes Private Bugs | Hacker News
Hi to all, as I am really eager of reading this blog’s post to be updated on a regular basis.
It includes nice stuff.
Hi, I am new here I found this blog very interesting. As it is an old blog but very much engaging and informative thank you for the post.
Pingback: Is array injection a vulnerability and what is the proper term for it? – Config9.com