Closed
Bug 1075578
(CVE-2014-1573)
Opened 10 years ago
Closed 10 years ago
[SECURITY] Improper filtering of CGI arguments
Categories
(Bugzilla :: Bugzilla-General, defect, P1)
Bugzilla
Bugzilla-General
Tracking
()
RESOLVED
FIXED
Bugzilla 4.0
People
(Reporter: LpSolit, Assigned: LpSolit)
References
Details
(Keywords: regression, Whiteboard: EMBARGO until Monday Oct 6, 7am PDT (10am Eastern))
Attachments
(4 files, 5 obsolete files)
11.53 KB,
patch
|
dkl
:
review+
gerv
:
review+
|
Details | Diff | Splinter Review |
11.03 KB,
patch
|
dkl
:
review+
|
Details | Diff | Splinter Review |
12.59 KB,
patch
|
dkl
:
review+
|
Details | Diff | Splinter Review |
11.63 KB,
patch
|
dkl
:
review+
|
Details | Diff | Splinter Review |
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+
Assignee | ||
Updated•10 years ago
|
Summary: [SECURITY] The 'realname' parameter is not correctly filtered on user account creation, leading to user data override → [SECURITY] Improper filtering of CGI arguments
Assignee | ||
Updated•10 years ago
|
Whiteboard: [Same CVE number as the original bug 1074812: CVE-2014-1572]
Assignee | ||
Comment 2•10 years ago
|
||
This patch fixes the vulnerability reported in comment 1 and the original issue about token.cgi from bug 1074812.
Comment 3•10 years ago
|
||
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
Assignee | ||
Comment 4•10 years ago
|
||
(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
Updated•10 years ago
|
Alias: CVE-2014-1572
Whiteboard: [Same CVE number as the original bug 1074812: CVE-2014-1572]
Assignee | ||
Comment 6•10 years ago
|
||
2nd vuln. fixed.
Attachment #8498216 -
Attachment is obsolete: true
Comment 8•10 years ago
|
||
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
Assignee | ||
Comment 9•10 years ago
|
||
3rd vuln. fixed
Attachment #8498285 -
Attachment is obsolete: true
Assignee | ||
Comment 11•10 years ago
|
||
4th vuln. fixed. Two files left and I'm done with this bug.
Attachment #8498376 -
Attachment is obsolete: true
Assignee | ||
Comment 13•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Blocks: CVE-2014-1572
No longer depends on: CVE-2014-1572
Comment 14•10 years ago
|
||
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+
Updated•10 years ago
|
Flags: approval?
Comment 15•10 years ago
|
||
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+
Assignee | ||
Comment 16•10 years ago
|
||
(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).
Comment 17•10 years ago
|
||
(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
Comment 18•10 years ago
|
||
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
Comment 19•10 years ago
|
||
(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
Comment 20•10 years ago
|
||
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
Comment 21•10 years ago
|
||
(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.
Comment 22•10 years ago
|
||
(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
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8499050 -
Flags: review?(dkl)
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8499092 -
Flags: review?(dkl)
Comment 25•10 years ago
|
||
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+
Updated•10 years ago
|
Flags: approval4.4?
Comment 26•10 years ago
|
||
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+
Updated•10 years ago
|
Flags: approval4.2?
Updated•10 years ago
|
Whiteboard: EMBARGO until Monday Oct 6, 7am PDT (10am Eastern)
Comment 27•10 years ago
|
||
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.
Assignee | ||
Comment 28•10 years ago
|
||
(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. :)
Updated•10 years ago
|
Flags: approval?
Flags: approval4.4?
Flags: approval4.4+
Flags: approval4.2?
Flags: approval4.2+
Flags: approval+
Assignee | ||
Comment 29•10 years ago
|
||
Attachment #8499926 -
Flags: review?(dkl)
Assignee | ||
Comment 30•10 years ago
|
||
Attachment #8499926 -
Attachment is obsolete: true
Attachment #8499926 -
Flags: review?(dkl)
Attachment #8499928 -
Flags: review?(dkl)
Comment 31•10 years ago
|
||
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+
Updated•10 years ago
|
Flags: approval4.0?
Comment 32•10 years ago
|
||
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: 10 years ago
Resolution: --- → FIXED
Comment 33•10 years ago
|
||
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
Updated•10 years ago
|
Flags: approval4.0? → approval4.0+
Updated•10 years ago
|
Group: bugzilla-security
You need to log in
before you can comment on or make changes to this bug.
Description
•