Last Comment Bug 770429 - (CVE-2012-3978) Bypassing security checks for nsLocation::CheckURL
: Bypassing security checks for nsLocation::CheckURL
: sec-high
Product: Core
Classification: Components
Component: Security (show other bugs)
: unspecified
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: Anthony Hughes (:ashughes) [GFX][QA][Mentor]
: David Keeler [:keeler] (use needinfo?)
Depends on:
  Show dependency treegraph
Reported: 2012-07-03 00:22 PDT by moz_bug_r_a4
Modified: 2013-01-10 21:22 PST (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

WIP (1.95 KB, patch)
2012-07-03 11:17 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
WIP2 (1.79 KB, patch)
2012-07-03 13:16 PDT, Olli Pettay [:smaug]
mrbkap: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description moz_bug_r_a4 2012-07-03 00:22:28 PDT
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.
Comment 1 moz_bug_r_a4 2012-07-03 00:23:54 PDT
Created attachment 638605 [details]

This works on fx10-16.
Comment 2 Olli Pettay [:smaug] 2012-07-03 01:45:06 PDT
Blake, jst, why bug 307983 was ok? I don't quite understand the
if (nsJSUtils::GetDynamicScriptContext(*aContext)) { 
Comment 3 Olli Pettay [:smaug] 2012-07-03 02:10:08 PDT
Does this really need message manager? Couldn't some JS component do the same thing.
Comment 4 Olli Pettay [:smaug] 2012-07-03 03:39:13 PDT
And MM's JSContext is a system cx.
Comment 5 Olli Pettay [:smaug] 2012-07-03 11:17:07 PDT
Created attachment 638808 [details] [diff] [review]

But I'm not quite happy with this approach, since setting href may still not
get the right base uri.
Comment 6 Olli Pettay [:smaug] 2012-07-03 11:17:56 PDT
Or, hmm, perhaps the we don't want the base uri handling after all...
Comment 7 Olli Pettay [:smaug] 2012-07-03 13:16:40 PDT
Created attachment 638840 [details] [diff] [review]

Perhaps we actually want this kind of super simple approach, at least
for ESR. Changing GetContextFromStack handling could be regression risky.
Comment 8 Olli Pettay [:smaug] 2012-07-03 15:48:54 PDT
Comment on attachment 638840 [details] [diff] [review]

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).
Comment 9 Daniel Veditz [:dveditz] 2012-07-05 12:41:50 PDT
See bug 770431 which combines this with another bug to get remote code execution
Comment 10 Blake Kaplan (:mrbkap) 2012-07-17 18:56:00 PDT
Comment on attachment 638840 [details] [diff] [review]

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".
Comment 11 Olli Pettay [:smaug] 2012-07-18 12:29:08 PDT
Comment on attachment 638840 [details] [diff] [review]

[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
Comment 12 Alex Keybl [:akeybl] 2012-07-18 16:44:11 PDT
Comment on attachment 638840 [details] [diff] [review]

[Triage Comment]
Low-risk, sg:high fix. Approving for all branches.
Comment 13 Olli Pettay [:smaug] 2012-07-19 01:20:09 PDT
Comment 14 Andrew McCreight [:mccr8] 2012-07-31 06:48:56 PDT
Olli, this still needs to get landed on branches. :)
Comment 15 Olli Pettay [:smaug] 2012-07-31 08:06:03 PDT
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-14 15:19:22 PDT
Does this need a test in test-suite?
Comment 20 Olli Pettay [:smaug] 2012-08-14 16:40:32 PDT
I think yes, this does.
Comment 21 Olli Pettay [:smaug] 2012-08-14 16:41:19 PDT
Though making the testcase automatic can be a bit tricky.
Comment 22 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-14 17:05:28 PDT
Okay, we'll have to verify this manually before release then. What is the expected results with the attached testcase?
Comment 23 Olli Pettay [:smaug] 2012-08-14 17:13:37 PDT
The testcase has good instructions in it.
Comment 24 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-14 17:32:02 PDT
(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.
Comment 25 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-27 12:17:08 PDT
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

Note You need to log in before you can comment on or make changes to this bug.