Closed Bug 919021 Opened 7 years ago Closed 7 years ago

Eliminate all use of Array<jsval> in ctypes code

Categories

(Core :: js-ctypes, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
blocking-b2g koi+
Tracking Status
firefox24 --- wontfix
firefox25 --- wontfix
firefox26 + fixed
firefox27 + fixed
firefox-esr17 --- wontfix
firefox-esr24 26+ fixed
b2g18 --- wontfix
b2g-v1.1hd --- wontfix
b2g-v1.2 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

Details

(Keywords: sec-other, Whiteboard: [qa-][adv-main26-][adv-esr24.2-])

Attachments

(1 file, 1 obsolete file)

ctypes code uses Array<JS::Value> a few places to create arrays of values.  This can be done safely if you smack a JS::AutoArrayRooter after the array and never modify the length of the array while the rooter's alive.  But this is pretty fragile, and not everyone does it.  (Hence the hiddenness here.)  We should make Array<JS::Value> fail to compile, and we should convert all uses to JS::AutoValueVector (doable as the class is only used on the stack).
Attached patch Patch (obsolete) — Splinter Review
FunctionType::ArgTypesGetter is the only truly buggy thing here, as the rest properly root the free-standing array.
Attachment #807987 - Flags: review?(terrence)
Comment on attachment 807987 [details] [diff] [review]
Patch

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

r=me

::: js/src/ctypes/CTypes.cpp
@@ +4784,5 @@
>    }
> +  JS::AutoValueVector fieldRoots(cx);
> +  if (!fieldRoots.resize(len)) {
> +    return false;
> +  }

I guess leave the braces off this single line if, as they are elided below. Unless there are closer examples that it would look weird near. Whatever.

::: js/src/jsapi.h
@@ +551,4 @@
>      MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
>  };
>  
> +class MOZ_STACK_CLASS AutoValueVector : public AutoVectorRooter<Value>

Thank you!
Attachment #807987 - Flags: review?(terrence) → review+
Attached patch Ready to landSplinter Review
[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Possibly not at all, actually.  FunctionType::ArgTypesGetter doesn't root the values in the array it constructs.  But those values all derive from types stored in function info associated with a HandleObject, so should stay alive through the GC that might happen in JS_NewArrayObject.  As long as our GC doesn't move, this is probably okay -- but sketchy.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not really, except that replacing a random array type with something that looks like it roots harder might raise suspicion.

Which older supported branches are affected by this flaw?
Probably all of them.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be easy to make.

How likely is this patch to cause regressions; how much testing does it need?
Unlikely, doesn't need much testing.

For safety and just-to-be-sureness, seems to me, we should fix this everywhere.  But as the one problematic case here appears not actually to be a problem, I think we don't need to worry about obfuscation, etc. when we land this, and it shouldn't hurt us to land earlier in the cycle instead of later.
Attachment #807987 - Attachment is obsolete: true
Attachment #808028 - Flags: sec-approval?
Attachment #808028 - Flags: review+
I'd kind of like to keep this hidden so that it doesn't cause people to start looking for other places where we have arrays of values that aren't rooted correctly.  But in and of itself, I don't think there's a specific issue to worry about here.
Keywords: sec-other
Comment on attachment 808028 [details] [diff] [review]
Ready to land

sec-approval+. Since this is a "sec-other," it is unlikely to have branch patches approved without need.
Attachment #808028 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/636a8b68894d - don't think you need tests here per comment #3, so leaving the in-testsuite flag untouched
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Might be worth fixing on the ESR24 branch we're going to live with for a while and to uplift to Aurora (26) so it's fixed in the next long-lived b2g(1.2+) branch. Wontfix for near-EOL esr17.
Let's get an uplift nomination here for aurora/beta since FF26 is on Beta now. Also setting koi+ so that this does get onto b2g 26.
blocking-b2g: --- → koi+
Flags: needinfo?(jwalden+bmo)
If this gets beta approval, it will be merged to b2g26 automatically.
Comment on attachment 808028 [details] [diff] [review]
Ready to land

Oops, sorry, slipped off my plate due to a hundred other things.  I'm going to pretend this backports fairly easily to all the branches (it should, might be tree-wide changes that make it not be a super-straight application, but it should be basically good) and request approval in advance of patch creation.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: keeping code consistency for maintainability in the ESR-long-run
User impact if declined: believed to be none, although the code will be kinda sketchy in how it does things
Fix Landed on Version: 27
Risk to taking this patch (and alternatives if risky): basically none, it's a relatively mechanical bit of change
String or UUID changes made by this patch: N/A

[Approval Request Comment]
Bug caused by (feature/regressing bug #): ancient ctypes code, no idea how/when it showed up
User impact if declined: see above
Testing completed (on m-c, etc.): in on trunk/aurora already
Risk to taking this patch (and alternatives if risky): see above
String or IDL/UUID changes made by this patch: N/A
Attachment #808028 - Flags: approval-mozilla-esr24?
Attachment #808028 - Flags: approval-mozilla-beta?
Attachment #808028 - Flags: approval-mozilla-esr24?
Attachment #808028 - Flags: approval-mozilla-esr24+
Attachment #808028 - Flags: approval-mozilla-beta?
Attachment #808028 - Flags: approval-mozilla-beta+
Flags: needinfo?(jwalden+bmo)
Backed out for bustage related to using static_assert. I attempted a s/static_assert/JS_STATIC_ASSERT fix after talking to froydnj, but that had other issues.
https://hg.mozilla.org/releases/mozilla-esr24/rev/0ac7327b4425
Whiteboard: {qa-]
esr24 had one other use of Array<jsval> that needed fixing in order to land.

https://hg.mozilla.org/releases/mozilla-esr24/rev/4e7d3877547d
Flags: needinfo?(jwalden+bmo)
Whiteboard: {qa-] → [qa-]
Whiteboard: [qa-] → [qa-][adv-main26-][adv-esr24.2-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.