Closed Bug 1251308 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- unaffected
firefox46 + wontfix
firefox47 + wontfix
firefox48 --- fixed

People

(Reporter: bzbarsky, Assigned: bbouvier)

References

Details

(Keywords: regression, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main48+])

Attachments

(1 file, 6 obsolete files)

[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.
Flags: needinfo?(luke)
Flags: needinfo?(bbouvier)
We don't have a sec rating yet but I'll track this anyway, may be a regression from 46
Sounds like we should change the OOM case to return a null filename (which, iiuc, is a valid filename) instead?
Flags: needinfo?(luke)
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.
Flags: needinfo?(bbouvier)
Attached patch fix.patch (obsolete) — Splinter Review
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?
Attachment #8724664 - Flags: review?(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?
Attachment #8724664 - Flags: review?(bzbarsky)
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.
> 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).
(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).
I like that static string approach at least for backporting purposes.

The filename here should not have security meaning in callers.
Benjamin, can you audit the call sites so we can figure out a security rating for this? Thanks.
Flags: needinfo?(bbouvier)
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?
Flags: needinfo?(bbouvier)
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)
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.
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.
(In reply to Benjamin Bouvier [:bbouvier] from comment #14)
Makes sense to me
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.
Attached patch wip.patch (obsolete) — Splinter Review
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?
Attachment #8727518 - Flags: feedback?(luke)
Attachment #8724664 - Attachment is obsolete: true
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 ..."?
Attachment #8727518 - Flags: feedback?(luke)
Attached patch 1251308.patch (obsolete) — Splinter Review
Duh, I should have caught it. The good news is that we can have more control on the API and do something cleaner, imo.
Assignee: nobody → bbouvier
Attachment #8727518 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8727733 - Flags: review?(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?
Attachment #8727733 - Flags: review?(luke) → review+
(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)).
Attached patch 1251308.patch (obsolete) — Splinter Review
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).
Attachment #8727733 - Attachment is obsolete: true
Attachment #8728356 - Flags: review?(luke)
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?
Attachment #8728356 - Flags: sec-approval?
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.
Attached patch 1251308.patch (obsolete) — Splinter Review
[Security approval request comment]
See comment 23.
Attachment #8728356 - Attachment is obsolete: true
Attachment #8728356 - Flags: sec-approval?
Attachment #8728356 - Flags: review?(luke)
Attachment #8728458 - Flags: sec-approval?
Attachment #8728458 - Flags: review?(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
Attachment #8728458 - Flags: review?(luke) → review+
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!
Attachment #8728458 - Flags: sec-approval?
Attached patch 1251308.patch (obsolete) — Splinter Review
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.
Attachment #8728458 - Attachment is obsolete: true
Attachment #8729491 - Flags: review+
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.
Attached patch 1251308.patchSplinter Review
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
Attachment #8729491 - Attachment is obsolete: true
Attachment #8730186 - Flags: review+
Attachment #8730186 - Flags: approval-mozilla-beta?
Attachment #8730186 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/2b83147ead26
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Group: javascript-core-security → core-security-release
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.
Flags: needinfo?(bbouvier)
(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?).
Flags: needinfo?(bbouvier)
Not backporting is probably ok.
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.
Attachment #8730186 - Flags: approval-mozilla-beta?
Attachment #8730186 - Flags: approval-mozilla-beta-
Attachment #8730186 - Flags: approval-mozilla-aurora?
Attachment #8730186 - Flags: approval-mozilla-aurora-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main48+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.