1022016 - Infallible nsDependentJSString is a total footgun

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bobby Holley (:bholley)]

nsDependentJSString has a constructor that does the following:

  /**
   * In the case of string ids, getting the string's chars is infallible, so
   * the dependent string can be constructed directly.
   */
  explicit nsDependentJSString(JS::Handle<jsid> id)
    : nsDependentString(JS_GetInternedStringChars(JSID_TO_STRING(id)),
                        JS_GetStringLength(JSID_TO_STRING(id)))
  {
  }

This only works if the id is actually an JSString. If it happens to be an integer (which is trivial to make happen from script), this code will assert in debug builds, and create a string from arbitrary memory on release builds. You can literally pick the memory you want to access, and pass that (from script) as a number to the relevant code site. Yuck.

Comment 1

[Bobby Holley (:bholley)]

This has been around forever.

Here's a trivial exploitable site in the Navigator resolve hook: http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#1922

Comment 2

[Bobby Holley (:bholley)]

Oh, I missed the JSID_IS_STRING checks, both there in and in the caller of GlobalResolve.

So this is actually ok, I think. There's an unsafe usage in XrayWrapper, but it's debug-only, so I don't think it's a security issue.

I'll fix the footgun, but we probably don't need to backport it. Opening this bug up.

Comment 3

[Bobby Holley (:bholley)]

So at this point the only improper usage came from bug 987111, so I'm marking this as a regression from that bug, and tracking given proximity to the branch.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, you have to check that your ID is string before you nsDependendJSString it.

I agree that it's a bit dangerous.  Peopl shouldn't be writing code that involves jsid if they can avoid it.  ;)

Comment 5

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #4)
> I agree that it's a bit dangerous.  Peopl shouldn't be writing code that
> involves jsid if they can avoid it.  ;)

Unfortunately, I can't. ;-)

Patch coming up.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Redesign nsDependentJSString API to be less of a footgun. v1

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

initFromStringId?

Comment 8

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8436235 [details] [diff] [review]
Redesign nsDependentJSString API to be less of a footgun. v1

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

There is only one thing I don't get about this patch... I will not be available for a week and I don't want to hold you up with this so r=me. But please explain to me why is this needed there.

::: dom/base/nsJSUtils.h
@@ +169,5 @@
> +
> +    // Stringify, making sure not to run script.
> +    JS::Rooted<JSString*> str(aContext);
> +    if (v.isObject()) {
> +      str = JS_NewStringCopyZ(aContext, "[Object]");

Why do we need this exactly? Shouldn't wrappers take care of this part if needed?

Comment 9

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] (off the grid until 13th July) from comment #8)
> > +    // Stringify, making sure not to run script.
> > +    JS::Rooted<JSString*> str(aContext);
> > +    if (v.isObject()) {
> > +      str = JS_NewStringCopyZ(aContext, "[Object]");
> 
> Why do we need this exactly? Shouldn't wrappers take care of this part if
> needed?

Yes, possibly by running script. And I don't want want nsDependentJSString to potentially run script. I want the failure mode to be OOM-only.

Comment 10

[Bobby Holley (:bholley)]

Green try run (with some other stuff): https://tbpl.mozilla.org/?tree=Try&rev=12e2c896f82a

https://hg.mozilla.org/integration/mozilla-inbound/rev/c45ab5ebb393

Comment 11

[[e-mail]]

Comment on attachment 8436235 [details] [diff] [review]
Redesign nsDependentJSString API to be less of a footgun. v1

>+    MOZ_ASSERT(IsEmpty(), "init() on initialized string");
>+    nsDependentString* base = this;
>+    new (base) nsDependentString(aChars, aLength);
Or just use Rebind(aChars, aLength); which handles initialised strings.

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c45ab5ebb393

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8436235 [details] [diff] [review]
Redesign nsDependentJSString API to be less of a footgun. v1

This is still needed on aurora to avoid a security bug in debug builds. Given that people dogfood with debug builds, I think this is important.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 987111
User impact if declined: Security bugs
Testing completed (on m-c, etc.): Baked on m-c
Risk to taking this patch (and alternatives if risky): Low risk 
String or IDL/UUID changes made by this patch: None

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8436235 [details] [diff] [review]
Redesign nsDependentJSString API to be less of a footgun. v1

Aurora approval granted.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/042efa075740