919021 - Eliminate all use of Array<jsval> in ctypes code

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[Jeff Walden [:Waldo]]

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

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

FunctionType::ArgTypesGetter is the only truly buggy thing here, as the rest properly root the free-standing array.

Comment 2

[Terrence Cole [: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!

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Ready to land

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

Comment 4

[Jeff Walden [:Waldo]]

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.

Comment 5

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 6

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/636a8b68894d

Comment 7

[Carsten Book [:Tomcat]]

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

Comment 8

[Daniel Veditz [:dveditz]]

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.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

and esr24

Comment 11

[Ryan VanderMeulen [:RyanVM]]

If this gets beta approval, it will be merged to b2g26 automatically.

Comment 12

[Jeff Walden [:Waldo]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/949dbbda2cfc
https://hg.mozilla.org/releases/mozilla-esr24/rev/772a61603ce9

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/949dbbda2cfc

Comment 16

[Jeff Walden [:Waldo]]

esr24 had one other use of Array<jsval> that needed fixing in order to land.

https://hg.mozilla.org/releases/mozilla-esr24/rev/4e7d3877547d