1251308 - JS::DescribeScriptedCaller return value is now a lie in some cases

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Could cause security problems; needs careful audit.

The API documentation for JS::DescribeScriptedCaller clearly says the return value indicates whether a scripted caller was found.

The patch for bug 1229642 changed the implementation so that now a false value could mean no scripted caller _or_ could mean the caller asked for a filename and we OOMed trying to create a copy of the filename.

Note that very long filenames are not hard to produce (e.g. inline <script> in a long data: URI), so OOM here doesn't necessarily mean we're really out of memory in any meaningful sense.  Further, since pages get to control the filename, this allows pages to (probabilistically, at least) cause JS::DescribeScriptedCaller to lie about whether there was a scripted caller.

I haven't audited all the codepaths that call this function, but it seems like at least the debugging stuff in nsGlobalWindow::ShowSlowScriptDialog will behave wonkily in this case, though not sure if it handles no filename right now.  Also, it looks like some callers do NOT check the return value (because they assume that it represents what it's documented to represent and they know they have a scripted caller) and then use filename.get().  See for example ReportWrapperDenial.  Will that work correctly (e.g. without crashing) in the OOM case in the new world?

And, again, I did _not_ audit anything close to all the consumers.  I'm pretty worried about what might be lurking in there.

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

We don't have a sec rating yet but I'll track this anyway, may be a regression from 46

Comment 2

[Luke Wagner [:luke]]

Sounds like we should change the OOM case to return a null filename (which, iiuc, is a valid filename) instead?

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Per the previous implementation, AutoFilename::get() could return null:

https://hg.mozilla.org/mozilla-central/rev/5e0769303a5e#l26.25

Assuming the callers already had to handle this case (which is not clear to me), a fix would be to set the outparam to a null filename, as suggested by Luke in comment 2.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fix.patch

Per previous comments, assigning to nullptr if the copy didn't succeed, and carrying on with lineno/column. Boris, does this look like a right fix?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8724664 [details] [diff] [review]
fix.patch

I don't know whether this is correct, sorry.  In the old impl, AutoFilename::get() would return null if either i.scriptSource() was null or ScriptSource::filename() returned null.  Were those possible things that could happen?

Comment 6

[Luke Wagner [:luke]]

They were infallible so they'd only return null if the originally-passed filename was null.  So this would change observable behavior for oom, which I think is fine as long as the filename is not being used to make security judgements.  I'd hope that was all based on the compartment's principle, not filename string.  The "real" fix of course is to make DescribeScriptedCaller fallible like every other JSAPI (making the current bool return some "hasScriptedCaller" outparam); I'd been hoping to avoid that.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> so they'd only return null if the originally-passed filename was null

Did Gecko ever pass null in practice?

> which I think is fine as long as the filename is not being used to make security judgements

Well, or if null would cause a crash when we didn't use to crash...  Maybe we would have crashed on OOM anyway, so maybe it's alright, but if Gecko never uses null filenames, this part should be checked.

> The "real" fix of course is to make DescribeScriptedCaller fallible like every other JSAPI

There are places where it's called that have no sane way to handle failure, afaict.  But they could at least pretend to, I guess (e.g. not log anything on OOM or something).

Comment 8

[Luke Wagner [:luke]]

(In reply to Boris Zbarsky [:bz] from comment #7)
> Did Gecko ever pass null in practice?

I see crashes when we forget to handle null filenames inside SM, so I had assumed so, but it's possible that's only via shell methods and never from Gecko under normal conditions.  Another alternative, if we knew the filename didn't have security meaning, would be to just return a static string "out of memory" (which would be better than null anyway).

Comment 9

[Boris Zbarsky [:bzbarsky]]

I like that static string approach at least for backporting purposes.

The filename here should not have security meaning in callers.

Comment 10

[Andrew McCreight [:mccr8]]

Benjamin, can you audit the call sites so we can figure out a security rating for this? Thanks.

Comment 11

[Luke Wagner [:luke]]

I tried looking at all the uses but a few go off into CSP and it's hard to convince myself that there isn't a case where we allow something we used to reject before.  Assuming the easy fix above (just return a static string literal "out of memory"), this should be easy to backport.  That being said, since the issue isn't publicly known, maybe it's ok to let it ride the trains?

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

I was looking into that, but is the static string literal such an easy fix? It seems that the API of DescribeScriptedCaller should be changed, as it currently takes a UniqueChars ptr, and it could probably not with a static string... (as we don't want to have to duplicate the static string, of course)

Comment 13

[Luke Wagner [:luke]]

Yeah, what we'd need to do is change 'filename' from a UniqueChars to a UniquePtr<char[], JS::MaybeFreePolicy> for some new JS::MaybeFreePolicy whose constructor took an enum indicating whether to actually call free.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

At this point, wouldn't this just be easier and more pragmatic to add the out parameter and change the external API? It would involve more changes and won't ease backporting, but it should be pretty mechanical changes.

Comment 15

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #14)
Makes sense to me

Comment 16

[Boris Zbarsky [:bzbarsky]]

Note that some of the consumers of this API cannot handle it throwing sanely right now, so you will either need to silently suppress exceptions or change a lot more infrastructure.  Certainly the changes involved are NOT mechanical.

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: wip.patch

So passing the bool outparam indicating success is actually some trouble too, because then we need to add static const "out of memory" strings at more places in the embedder. That's bad.

Reverting to a solution close to MaybeFreePolicy:
- for non-wasm frames, just revert to the sourcescript solution
- for wasm frames, try a copy and put out-of-memory if it failed

That makes things a bit complicated in the custom deleter class...

Can I safely push the patch to try, or should I assume unexpected watchers on try and just all test suites locally?

Comment 18

[Luke Wagner [:luke]]

Comment on attachment 8727518 [details] [diff] [review]
wip.patch

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

Thanks for writing this up!  But Arg! I just noticed, digging through the impl of UniquePtr, that UniquePtr::reset() would be broken with this design since simply updates ptr(), but not del().  We don't really need to support reset() here, but it seems like we shouldn't leave that handgun lying on the table so I'm afraid we need to handroll our own class here (probably back to AutoFilename, since Unique implies it's actually a UniquePtr).  I'm sorry for not realizing this earlier.

::: js/src/jsapi.cpp
@@ +6059,5 @@
> +    if (filename) {
> +        static char outOfMemory[] = "out of memory";
> +        FilenameDeleter& deleter = filename->get_deleter();
> +        if (!i.isWasm()) {
> +            // Non-wasm frames have a script source to read the filename from.

How about reversing the order of then/else (handling special cases first) and then change this comment to "All other frames have a ..."?

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1251308.patch

Duh, I should have caught it. The good news is that we can have more control on the API and do something cleaner, imo.

Comment 20

[Luke Wagner [:luke]]

Comment on attachment 8727733 [details] [diff] [review]
1251308.patch

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

Excellent, thanks!

::: js/src/jsapi.cpp
@@ +6012,5 @@
>  }
>  
>  namespace JS {
>  
> +void AutoFilename::reset() {

{ on newline (here and below)

@@ +6030,5 @@
> +
> +void AutoFilename::setScriptSource(void* p) {
> +    ss_ = p;
> +    if (p)
> +        reinterpret_cast<ScriptSource*>(p)->incref();

How about setting msg_ = ss->filename() here.  With this and the below change, then get() could simply 'return msg_' and be an inline function in jsapi.h.  It would be good to also MOZ_ASSERT(msg_); in get() since, as discussed in comments above, null is probably not considered a valid filename by clients.

@@ +6033,5 @@
> +    if (p)
> +        reinterpret_cast<ScriptSource*>(p)->incref();
> +}
> +
> +void AutoFilename::setOOM() {

Instead of hardcoding to OOM, could you have a setUnowned(const char* msg)?

::: js/src/jsapi.h
@@ +5389,5 @@
> +  private:
> +    // Actually a ScriptSource, not put here to avoid including the world.
> +    void* ss_;
> +
> +    char* msg_;

Can this be "filename_" instead?

Comment 21

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Luke Wagner [:luke] from comment #20)
> @@ +6030,5 @@
> > +
> > +void AutoFilename::setScriptSource(void* p) {
> > +    ss_ = p;
> > +    if (p)
> > +        reinterpret_cast<ScriptSource*>(p)->incref();
> 
> How about setting msg_ = ss->filename() here.  With this and the below
> change, then get() could simply 'return msg_' and be an inline function in
> jsapi.h.  It would be good to also MOZ_ASSERT(msg_); in get() since, as
> discussed in comments above, null is probably not considered a valid
> filename by clients.
> 

ss->filename() returns a const char*. If we want to be able to delete in the owned case, we need char*. I agree that it is nicer to have a simple get() function. Then I've included two char* in AutoFilename, one const and one non-const (alternatives being using const_cast (ugly), tagged union {char*;const char*} (hackish)).

Comment 22

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1251308.patch

Updated patch (Waldo recommended to use a Variant on irc, which does a great job here, even if it's more wordy). Luke, I'd like you to have another quick look, just to make sure. There are cases where filename is nullptr (e.g. calls to EvalInContext return a nullptr filename, as basic/testBug1235874.js showed -- adding an assertion that we have a non-nullptr made it fail).

Comment 23

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8728356 [details] [diff] [review]
1251308.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Good question; see bz's comment 0. In the worst case, people can craft very long names and instantaneously crash Firefox, if my understanding is correct. According to comment 6, security judgments are based on compartments, not filenames, so security issues couldn't be provoked because of this.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not clearly. However bz found the issue just by reading the code, so anybody digging into the code and knowing it well could too.

Which older supported branches are affected by this flaw?
If not all supported branches, which bug introduced the flaw?
Bug 1229642 => mozilla 46+ (so 47 and 48 are affected too).

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Hopefully, this patch could be backported everywhere.

How likely is this patch to cause regressions; how much testing does it need?
It shouldn't cause any regression. Although it's a pretty safe change, it would need full testing; I haven't done a try-build because I am not sure what our policies are with respect to that? Can we safely try-build sec patches? Or do we need to run locally all the test suites?

Comment 24

[Luke Wagner [:luke]]

Comment on attachment 8728356 [details] [diff] [review]
1251308.patch

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

I would've just used a const_cast, but the Variant approach here is even nicer.  Mmm, Variants; I may have to start using these.

::: js/src/jsapi.cpp
@@ +6031,5 @@
> +}
> +
> +void AutoFilename::setScriptSource(void* p)
> +{
> +    ss_ = p;

Can you MOZ_ASSERT(!ss_);?

@@ +6041,5 @@
> +}
> +
> +void AutoFilename::setUnowned(const char* filename)
> +{
> +    filename_.as<const char*>() = filename;

Can you MOZ_ASSERT(!filename_.as<const char*>());?

@@ +6046,5 @@
> +}
> +
> +void AutoFilename::setOwned(char* filename)
> +{
> +    filename_.as<UniqueChars>() = UniqueChars(filename);

I would think this would assert since filename_ will have been constructed with the <const char*> type by this point.  Rather, what I think you're supposed to do is 'filename_ = AsVariant(UniqueChars(filename));'.

The only way I think you can test DescribeScriptedCaller of wasm from within SM is to have a wasm module import 'evalcx' (a shell function) and pass 0 (which will eval the string "0" b/c of the magic of type coercion).  Could you confirm and add such a test?

@@ +6049,5 @@
> +{
> +    filename_.as<UniqueChars>() = UniqueChars(filename);
> +}
> +
> +const char* AutoFilename::get() const {

{ on newline

@@ +6077,5 @@
>          return false;
>  
> +    if (filename) {
> +        if (i.isWasm()) {
> +            static const char* outOfMemory = "out of memory";

I don't think you need a static var to hold this pointer into global memory.  I'd just pass "out of memory" inline in the setUnowned() below.

Comment 25

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1251308.patch

[Security approval request comment]
See comment 23.

Comment 26

[Luke Wagner [:luke]]

Comment on attachment 8728458 [details] [diff] [review]
1251308.patch

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

::: js/src/jsapi.cpp
@@ +6031,5 @@
> +}
> +
> +void AutoFilename::setScriptSource(void* p)
> +{
> +    MOZ_ASSERT(!ss_);

Probably also good to MOZ_ASSERT(!get());

@@ +6048,5 @@
> +}
> +
> +void AutoFilename::setOwned(char* filename)
> +{
> +    filename_ = AsVariant(UniqueChars(filename));

and here too

Comment 27

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

Comment on attachment 8728458 [details] [diff] [review]
1251308.patch

This doesn't need sec-approval+ to land as it is a sec-moderate rated issue. We only require approval for sec-high and sec-critical rated issues. Land away!

Comment 28

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1251308.patch

Carrying forward r+, with suggested changes + have setOwned take a UniqueChars&& param, to avoid the silly sequence:

UniqueChars copy = ...;
filename->setOwned(copy.release());
// in AutoFilename::setOwned(char* filename):
variant = AsVariant<UniqueChars>(UniqueChars(filename));

(this was also triggering a build issue on windows, which thought it had to use the deleted copy ctor of UniqueChars and not the move ctor).

Posting to try and then to inbound.

Comment 29

[Benjamin Bouvier [:bbouvier] (inactive)]

For what it's worth, the build error was due to the fact that the AutoFilename was annotated with JS_PUBLIC_API and the copy assignment operator not explicitly deleted. Updated the patch to reflect that, try running it right now.

Comment 30

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b83147ead26

Comment 31

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1251308.patch

Carrying forward r+. Updated patch with deleted copy assignment operator in AutoFilename to make msvc happy.

Approval Request Comment
[Feature/regressing bug #]: bug 1229642
[User impact if declined]: potential way to OOM / instacrash the browser. No possible exploits a priori.
[Describe test coverage new/current, TreeHerder]: green on try, pushed on inbound today
[Risks and why]: fairly low; this reverts and adapts code to former behavior
[String/UUID change made/needed]: n/a

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2b83147ead26

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Benjamin, this is a pretty big code change. Given that this is a sec-mod and not easily exploitable, can we just let this ride the trains? I am also a bit concerned that we nom'd this for uplift without letting it bake on Nightly.

Comment 34

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Ritu Kothari (:ritu) from comment #33)
> Hi Benjamin, this is a pretty big code change. Given that this is a sec-mod
> and not easily exploitable, can we just let this ride the trains? I am also
> a bit concerned that we nom'd this for uplift without letting it bake on
> Nightly.

This is what Luke suggested in comment 11 too, yeah. If my understanding is correct, the worse that can happen is insta-crashing the browser, but bz's initial comment suggests it could be worse than that. That being said, the code has now landed on Nightly and has passed all tests. I am fine with not uplifting, if other people are fine too (bz, luke?).

Comment 35

[Boris Zbarsky [:bzbarsky]]

Not backporting is probably ok.

Comment 36

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8730186 [details] [diff] [review]
1251308.patch

Everybody agrees that given the size of the code change and possible risk to quality, it's ok to let this ride the trains.