Bug 770429 (CVE-2012-3978)

Bypassing security checks for nsLocation::CheckURL

VERIFIED FIXED

Status

()

Core
Security
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: moz_bug_r_a4, Assigned: smaug)

Tracking

({sec-high})

unspecified
x86
Windows XP
sec-high
Points:
---

Firefox Tracking Flags

(firefox14 wontfix, firefox15+ verified, firefox16+ verified, firefox17+ verified, firefox-esr1015+ verified)

Details

(Whiteboard: [advisory-tracking+])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
GetContextFromStack ignores JSContext for ContentFrameMessageManager.  If GetContextFromStack ends up not getting a JSContext, nsLocation::CheckURL does not do security checks.  This could allow content to load restricted URLs.
(Reporter)

Comment 1

5 years ago
Created attachment 638605 [details]
testcase

This works on fx10-16.
(Assignee)

Comment 2

5 years ago
Blake, jst, why bug 307983 was ok? I don't quite understand the
if (nsJSUtils::GetDynamicScriptContext(*aContext)) { 
part.
(Assignee)

Comment 3

5 years ago
Does this really need message manager? Couldn't some JS component do the same thing.
(Assignee)

Comment 4

5 years ago
And MM's JSContext is a system cx.
Assignee: nobody → bugs
(Assignee)

Comment 5

5 years ago
Created attachment 638808 [details] [diff] [review]
WIP

But I'm not quite happy with this approach, since setting href may still not
get the right base uri.
(Assignee)

Comment 6

5 years ago
Or, hmm, perhaps the we don't want the base uri handling after all...
(Assignee)

Comment 7

5 years ago
Created attachment 638840 [details] [diff] [review]
WIP2

Perhaps we actually want this kind of super simple approach, at least
for ESR. Changing GetContextFromStack handling could be regression risky.
(Assignee)

Comment 8

5 years ago
Comment on attachment 638840 [details] [diff] [review]
WIP2

Blake, what do you think of this?
I really want the safest possible option for ESR. We can
clean up the JSContext stack handling on trunk
(GetContextFromStack is odd).
Attachment #638840 - Flags: review?(mrbkap)
status-firefox-esr10: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox15: --- → +
tracking-firefox16: --- → +
See bug 770431 which combines this with another bug to get remote code execution
Keywords: sec-high
Comment on attachment 638840 [details] [diff] [review]
WIP2

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

Yeah, this is probably the best we can do here. Hopefully this will get cleaner once we go to one context per thread... It seems like the split here is that we want to know both "can the currently running code load this URI" and then separately do stuff based on the "entry script".
Attachment #638840 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 11

5 years ago
Comment on attachment 638840 [details] [diff] [review]
WIP2

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  part of an sg:crit problem
User impact if declined: security problem
Fix Landed on Version: NA
Risk to taking this patch (and alternatives if risky): Should be low-risk
String or UUID changes made by this patch: N/A
Attachment #638840 - Flags: approval-mozilla-esr10?
Attachment #638840 - Flags: approval-mozilla-beta?
Attachment #638840 - Flags: approval-mozilla-aurora?
status-firefox14: affected → wontfix
status-firefox17: --- → affected
tracking-firefox-esr10: ? → 15+
tracking-firefox17: --- → +
Comment on attachment 638840 [details] [diff] [review]
WIP2

[Triage Comment]
Low-risk, sg:high fix. Approving for all branches.
Attachment #638840 - Flags: approval-mozilla-esr10?
Attachment #638840 - Flags: approval-mozilla-esr10+
Attachment #638840 - Flags: approval-mozilla-beta?
Attachment #638840 - Flags: approval-mozilla-beta+
Attachment #638840 - Flags: approval-mozilla-aurora?
Attachment #638840 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/mozilla-central/rev/6d8456a77e57
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Olli, this still needs to get landed on branches. :)
(Assignee)

Comment 15

5 years ago
Indeed.
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/6f118ea66b73
status-firefox16: affected → fixed
status-firefox17: affected → fixed
(Assignee)

Comment 17

5 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/c7db519aae8c
status-firefox15: affected → fixed
(Assignee)

Comment 18

5 years ago
https://hg.mozilla.org/releases/mozilla-esr10/rev/ead222b58bd5
status-firefox-esr10: affected → fixed
Whiteboard: [advisory-tracking+]
Does this need a test in test-suite?
Whiteboard: [advisory-tracking+] → [advisory-tracking+][qa?]
(Assignee)

Comment 20

5 years ago
I think yes, this does.
(Assignee)

Comment 21

5 years ago
Though making the testcase automatic can be a bit tricky.
Okay, we'll have to verify this manually before release then. What is the expected results with the attached testcase?
(Assignee)

Comment 23

5 years ago
The testcase has good instructions in it.
(In reply to Olli Pettay [:smaug] from comment #23)
> The testcase has good instructions in it.

Heh, I should have opened it before asking. Thanks.
Whiteboard: [advisory-tracking+][qa?] → [advisory-tracking+][qa+]
Alias: CVE-2012-3978
Keywords: verifyme
Whiteboard: [advisory-tracking+][qa+] → [advisory-tracking+]
Confirmed reproducible with Firefox 14.0.1.

Verified fixed with:
 * 2012-08-27 Firefox 17.0a1
 * 2012-08-27 Firefox 16.0a2
 * Firefox 15.0 build1
 * Firefox 10.0.7esr build1
Status: RESOLVED → VERIFIED
status-firefox-esr10: fixed → verified
status-firefox15: fixed → verified
status-firefox16: fixed → verified
status-firefox17: fixed → verified
Keywords: verifyme
QA Contact: anthony.s.hughes
Group: core-security
You need to log in before you can comment on or make changes to this bug.