Closed
Bug 1455110
Opened 6 years ago
Closed 6 years ago
PolicyHasRegValue passing incorrect size to RegQueryValueExW
Categories
(Firefox :: Enterprise Policies, defect)
Tracking
()
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | + | fixed |
firefox61 | --- | fixed |
People
(Reporter: bugzilla, Assigned: bugzilla)
References
Details
(Keywords: csectype-bounds, sec-moderate)
Attachments
(1 file)
1.38 KB,
patch
|
jimm
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
https://searchfox.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#3174 > DWORD len = sizeof(aValue); aValue is a DWORD*, when we should be passing sizeof(DWORD). On 64-bit, this could be bad if there is malformed data in the registry.
Assignee | ||
Comment 1•6 years ago
|
||
In addition to fixing the size, I think we're better off to use RegGetValue, as it does additional validation on the type of the registry value. It also gives us good cover for landing this.
Updated•6 years ago
|
Keywords: csectype-bounds,
sec-moderate
Updated•6 years ago
|
Comment 2•6 years ago
|
||
Comment on attachment 8969066 [details] [diff] [review] s/RegQueryValueEx/RegGetValue/ Review of attachment 8969066 [details] [diff] [review]: ----------------------------------------------------------------- gah, did I approve this? DWORD len = sizeof(aValue);
Attachment #8969066 -
Flags: review?(jmathies) → review+
Updated•6 years ago
|
status-firefox59:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Assignee | ||
Comment 3•6 years ago
|
||
Comment on attachment 8969066 [details] [diff] [review] s/RegQueryValueEx/RegGetValue/ [Security approval request comment] How easily could an exploit be constructed based on the patch? Pretty difficult I would think, as it would require editing the Windows registry on the host machine before running Firefox. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. The comment refers to replacing the API used and omits the actual security problem. Which older supported branches are affected by this flaw? 60 beta, 61 nightly. If not all supported branches, which bug introduced the flaw? bug 1440573 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This patch should be a trivial, low risk uplift to beta. How likely is this patch to cause regressions; how much testing does it need? Unlikely. It just uses a newer API for accessing registry values.
Attachment #8969066 -
Flags: sec-approval?
Comment 4•6 years ago
|
||
Comment on attachment 8969066 [details] [diff] [review] s/RegQueryValueEx/RegGetValue/ As a sec-moderate bug, this doesn't need sec-approval to go in. You can just check it into trunk. We only require it for sec-critical and sec-high rated security bugs (and we need bugs to get ratings) that affect more than trunk. https://wiki.mozilla.org/Security/Bug_Approval_Process So you're good to go!
Attachment #8969066 -
Flags: sec-approval?
Assignee | ||
Comment 5•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/45c6f888c2acabcc8076ce8220e0993f2aee88df Bug 1455110: Replace RegOpenKeyEx/RegQueryValueEx with RegGetValue; r=jimm
Comment 6•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/45c6f888c2acabcc8076ce8220e0993f2aee88df
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment 7•6 years ago
|
||
Please request beta uplift when you get a chance so we don't ship this.
Flags: needinfo?(aklotz)
Assignee | ||
Comment 8•6 years ago
|
||
Comment on attachment 8969066 [details] [diff] [review] s/RegQueryValueEx/RegGetValue/ Approval Request Comment [Feature/Bug causing the regression]: bug 1440573 [User impact if declined]: Potential security issue [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: Simple change [String changes made/needed]: None
Flags: needinfo?(aklotz)
Attachment #8969066 -
Flags: approval-mozilla-beta?
Comment 9•6 years ago
|
||
Comment on attachment 8969066 [details] [diff] [review] s/RegQueryValueEx/RegGetValue/ Fix for a regression in 60, let's uplift for beta 16.
Attachment #8969066 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 10•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/f2d63307f232
Updated•6 years ago
|
Group: firefox-core-security → core-security-release
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•