Bug 1075578 (CVE-2014-1573)

[SECURITY] Improper filtering of CGI arguments

RESOLVED FIXED in Bugzilla 4.0

Status

()

defect
P1
blocker
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: LpSolit, Assigned: LpSolit)

Tracking

({regression})

unspecified
Bugzilla 4.0
Dependency tree / graph
Bug Flags:
approval +
approval4.4 +
blocking4.4.6 +
approval4.2 +
blocking4.2.11 +
approval4.0 +
blocking4.0.15 +
testcase +

Details

(Whiteboard: EMBARGO until Monday Oct 6, 7am PDT (10am Eastern))

Attachments

(4 attachments, 5 obsolete attachments)

I wrote a test in bug 1074980 to catch the |foo => $cgi->param()| syntax, and it found 15 places which use this syntax. The first one I checked is already exploitable (XSS), and I didn't look at the other ones yet. I think things are worse than expected in bug 1074812.

I will comment in this bug when new vulnerabilities are found, and will also update my patch accordingly.
Flags: testcase+
Flags: blocking4.4.6+
Flags: blocking4.2.11+
Flags: blocking4.0.15+
Summary: [SECURITY] The 'realname' parameter is not correctly filtered on user account creation, leading to user data override → [SECURITY] Improper filtering of CGI arguments
Whiteboard: [Same CVE number as the original bug 1074812: CVE-2014-1572]
Posted patch WIP, v0.1 (obsolete) — Splinter Review
This patch fixes the vulnerability reported in comment 1 and the original issue about token.cgi from bug 1074812.
Do we need to check that they are all exploitable? Can't we just patch them all and forbid the pattern? Or do we need more detailed exploit information to e.g. allocate CVEs or write proper release notes?

Gerv
(In reply to Gervase Markham [:gerv] from comment #3)
> Do we need to check that they are all exploitable? Can't we just patch them
> all and forbid the pattern?

The pattern is already forbidden, see bug 1074980. But blindly prepending "scalar" everywhere doesn't make much sense. I like to know which parts of the code are vulnerable as this often discloses more hidden problems.
Blocks: 1074980
Alias: CVE-2014-1572
Whiteboard: [Same CVE number as the original bug 1074812: CVE-2014-1572]
Posted patch WIP, v0.2 (obsolete) — Splinter Review
2nd vuln. fixed.
Attachment #8498216 - Attachment is obsolete: true
Note: the PoC works, but Bugzilla fails to autolinkify the final "}" so clicking the link in comment 7 won't work. Copy and paste instead.

Gerv
Posted patch WIP, v0.3 (obsolete) — Splinter Review
3rd vuln. fixed
Attachment #8498285 - Attachment is obsolete: true
Posted patch WIP, v0.4 (obsolete) — Splinter Review
4th vuln. fixed. Two files left and I'm done with this bug.
Attachment #8498376 - Attachment is obsolete: true
Here is the final patch for trunk. This patch doesn't apply cleanly to 4.x and will need backports once this one is reviewed.
Attachment #8498399 - Attachment is obsolete: true
Attachment #8498492 - Flags: review?(dkl)
No longer depends on: CVE-2014-1572
Comment on attachment 8498492 [details] [diff] [review]
patch for trunk, v1

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

r=dkl
Attachment #8498492 - Flags: review?(dkl) → review+
Flags: approval?
Comment on attachment 8498492 [details] [diff] [review]
patch for trunk, v1

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

It may be a bit late to suggest this, but $cgi is an object we control, right? A Bugzilla/CGI.pm object? So perhaps we should have a new function on that objection, oneparam(), which only returns a scalar value, and make the param() call throw? That would prevent anyone making this mistake again in the future. I know we have a test, but is it possible that the same problem could occur in other contexts?

Alternative idea: should we try and ban assigning the return value of functions to hash members entirely? I.e. you need to extract it to a variable first? Or is that overkill?

Having suggested all that, this patch seems good to me for fixing the problem now.

Gerv
Attachment #8498492 - Flags: review+
(In reply to Gervase Markham [:gerv] from comment #15)
> A Bugzilla/CGI.pm object? So perhaps we should have a new function on
> that objection, oneparam(), which only returns a scalar value

This wouldn't really help. You would still have to think about returning a scalar or an array. If the developer thinks about that, he could as well write "scalar" in front of "$cgi->param(). And the number of places to fix s/param/oneparam/ would be huge and really prone to errors.


> Alternative idea: should we try and ban assigning the return value of
> functions to hash members entirely? I.e. you need to extract it to a
> variable first? Or is that overkill?

IMO overkill to forbid it explicitly (i.e. enforced in a test). In methods called very often, you don't want to store the data in a separate variable (see e.g. bug 797636).
(In reply to Frédéric Buclin from comment #16)
> (In reply to Gervase Markham [:gerv] from comment #15)
> > A Bugzilla/CGI.pm object? So perhaps we should have a new function on
> > that objection, oneparam(), which only returns a scalar value
> 
> This wouldn't really help. You would still have to think about returning a
> scalar or an array. 

No, it would always return a scalar. If we needed a version returning an array (which we probably would), we'd implement multiparam() as well. Or we could call them scalarparam() and arrayparam().

> If the developer thinks about that, he could as well
> write "scalar" in front of "$cgi->param(). 

We would force them to decide what they wanted by not allowing param() at all.

> And the number of places to fix
> s/param/oneparam/ would be huge and really prone to errors.

Well no, because a) grep, and b) we would make param() throw, so instances where we kept using it would become obvious.

> IMO overkill to forbid it explicitly (i.e. enforced in a test). In methods
> called very often, you don't want to store the data in a separate variable
> (see e.g. bug 797636).

But in cases like those, you aren't assigning to a hash. (Good work in that bug, BTW.)

Gerv
Hi guys,
I'm getting no response from security@bugzilla.org as to planned timeframes.
Can you help me understand when the patch and advisory would be made public? I would like to coordinate, as we will be releasing as well.
Thx,
Shahar
(In reply to Shahar Tal from comment #18)
> Hi guys,
> I'm getting no response from security@bugzilla.org as to planned timeframes.
> Can you help me understand when the patch and advisory would be made public?
> I would like to coordinate, as we will be releasing as well.
> Thx,
> Shahar

Sorry. Did you mean security@mozilla.org or security@bugzilla.org. Either way I am sorry I did not see the email sent.

We are working hard to get this released this week but due to the other areas that were discovered due to your report, we want to include as much fixes as we can. This unfortunately needs to go through the standard review/approval process as any major code changes require. We are getting very close. 

That being said, we are getting close to Friday here and I tend to dislike releasing on Fridays as a courtesy to sysadmins. Would this be best to plan for a Monday release and we just have everything ready to go by then?

Thoughts

dkl
Sure, I'll be syncing for a Monday release, can we plan for 10:00AM ET?
Do let me know if we foresee any dramatic changes.
Cheers, and keep up the good work!
Shahar
(In reply to David Lawrence [:dkl] from comment #19)
> That being said, we are getting close to Friday here and I tend to dislike
> releasing on Fridays as a courtesy to sysadmins.

It's _already_ Friday in Australia and East Asia and we have yet to port and test this on other supported branches. Assuming CheckPoint isn't telling people about this, and didn't give details to the reporter(s?) they talked to we may be putting more people at risk shipping now and revealing the flaw than waiting until Monday.
(In reply to Shahar Tal from comment #20)
> Sure, I'll be syncing for a Monday release, can we plan for 10:00AM ET?
> Do let me know if we foresee any dramatic changes.
> Cheers, and keep up the good work!
> Shahar

As long as we can get all of the release preparation stuff done by EOB tomorrow (friday ET) then I do not see an issue with releasing at 10:00AM ET Monday.

dkl
Attachment #8499050 - Flags: review?(dkl)
Attachment #8499092 - Flags: review?(dkl)
Comment on attachment 8499050 [details] [diff] [review]
patch for 4.4, v1

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

r=dkl
Attachment #8499050 - Flags: review?(dkl) → review+
Flags: approval4.4?
Comment on attachment 8499092 [details] [diff] [review]
patch for 4.2, v1

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

r=dkl
Attachment #8499092 - Flags: review?(dkl) → review+
Flags: approval4.2?
Whiteboard: EMBARGO until Monday Oct 6, 7am PDT (10am Eastern)
7am PDT means that sysadmins on the west coast would have to deploy this Friday. So do I interpret "review+" correctly that the corresponding patch has been tested and is recommended to be deployed on affected live systems? Flag meanings and workflows might not be known to everybody, hence some verbosity on recommendations could help.
(In reply to Andre Klapper from comment #27)
> So do I interpret "review+" correctly that the corresponding patch
> has been tested and is recommended to be deployed on affected live systems?

Correct. I still have to backport my patch to the 4.0 branch and we are ready to release. If you use these patches before Monday, please do not make them public (public git repo, public server, etc...). And of course do not talk about these problems before Monday. :)
Flags: approval?
Flags: approval4.4?
Flags: approval4.4+
Flags: approval4.2?
Flags: approval4.2+
Flags: approval+
Posted patch patch for 4.0, v1 (obsolete) — Splinter Review
Attachment #8499926 - Flags: review?(dkl)
Attachment #8499926 - Attachment is obsolete: true
Attachment #8499926 - Flags: review?(dkl)
Attachment #8499928 - Flags: review?(dkl)
Comment on attachment 8499928 [details] [diff] [review]
patch for 4.0, v1.1

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

r=dkl
Attachment #8499928 - Flags: review?(dkl) → review+
Flags: approval4.0?
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   eb8f788..406dea6  4.0 -> 4.0

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   b07267a..ce590bf  4.2 -> 4.2

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   4e5cf94..7b8e0ab  4.4 -> 4.4

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   553568d..9e186bd  master -> master
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Because some installations have applied a patch for bug 1074812 already it's better if this extended issue gets a new CVE -- CVE-2014-1573 for this bug, CVE-2014-1572 will cover only the initial problem reported by Check Point
Alias: CVE-2014-1572 → CVE-2014-1573
Flags: approval4.0? → approval4.0+
Group: bugzilla-security
You need to log in before you can comment on or make changes to this bug.