770429 - (CVE-2012-3978) Bypassing security checks for nsLocation::CheckURL

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

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]

Attachment: testcase

This works on fx10-16.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Does this really need message manager? Couldn't some JS component do the same thing.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

And MM's JSContext is a system cx.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP

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][[e-mail]]]

Or, hmm, perhaps the we don't want the base uri handling after all...

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP2

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][[e-mail]]]

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).

Comment 9

[Daniel Veditz [:dveditz]]

See bug 770431 which combines this with another bug to get remote code execution

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

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".

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 638840 [details] [diff] [review]
WIP2

[Triage Comment]
Low-risk, sg:high fix. Approving for all branches.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/6d8456a77e57

Comment 14

[Andrew McCreight [:mccr8]]

Olli, this still needs to get landed on branches. :)

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Indeed.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6f118ea66b73

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c7db519aae8c

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/ead222b58bd5

Comment 19

[u279076]

Does this need a test in test-suite?

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

I think yes, this does.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Though making the testcase automatic can be a bit tricky.

Comment 22

[u279076]

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][[e-mail]]]

The testcase has good instructions in it.

Comment 24

[u279076]

(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

[u279076]

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