Closed Bug 770429 (CVE-2012-3978) Opened 8 years ago Closed 7 years ago

Bypassing security checks for nsLocation::CheckURL

Categories

(Core :: Security, defect)

x86
Windows XP
defect
Not set

Tracking

()

VERIFIED FIXED
Tracking Status
firefox14 --- wontfix
firefox15 + verified
firefox16 + verified
firefox17 + verified
firefox-esr10 15+ verified

People

(Reporter: moz_bug_r_a4, Assigned: smaug)

Details

(Keywords: sec-high, Whiteboard: [advisory-tracking+])

Attachments

(2 files)

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.
Attached file testcase
This works on fx10-16.
Blake, jst, why bug 307983 was ok? I don't quite understand the
if (nsJSUtils::GetDynamicScriptContext(*aContext)) { 
part.
Does this really need message manager? Couldn't some JS component do the same thing.
And MM's JSContext is a system cx.
Assignee: nobody → bugs
Attached patch WIPSplinter Review
But I'm not quite happy with this approach, since setting href may still not
get the right base uri.
Or, hmm, perhaps the we don't want the base uri handling after all...
Attached patch WIP2Splinter Review
Perhaps we actually want this kind of super simple approach, at least
for ESR. Changing GetContextFromStack handling could be regression risky.
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)
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+
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?
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+
https://hg.mozilla.org/mozilla-central/rev/6d8456a77e57
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Olli, this still needs to get landed on branches. :)
Indeed.
Whiteboard: [advisory-tracking+]
Does this need a test in test-suite?
Whiteboard: [advisory-tracking+] → [advisory-tracking+][qa?]
I think yes, this does.
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?
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
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.