Last Comment Bug 795745 - Conditional jump or move depends on uninitialised value(s) [@ ComputePrecisionInRange]
: Conditional jump or move depends on uninitialised value(s) [@ ComputePrecisio...
Status: VERIFIED FIXED
[qa-]
: csectype-uninitialized, regression, sec-other, testcase, valgrind
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla18
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
: general
Mentors:
Depends on:
Blocks: jsfunfuzz 771744
  Show dependency treegraph
 
Reported: 2012-09-30 13:18 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2012-12-13 17:07 PST (History)
12 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
-
wontfix
+
fixed
+
fixed
unaffected


Attachments
Valgrind stack (15.07 KB, text/plain)
2012-09-30 13:18 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
Patch with tests (2.05 KB, patch)
2012-10-01 17:39 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
lukasblakk+bugs: approval‑mozilla‑beta+
dveditz: sec‑approval+
Details | Diff | Review

Description Gary Kwong [:gkw] [:nth10sd] 2012-09-30 13:18:05 PDT
Created attachment 666368 [details]
Valgrind stack

Number.prototype.toPrecision('')

throws a lot of "Conditional jump or move depends on uninitialised value(s)" messages on js opt shell on m-c changeset cd82278e2bb8 without any CLI arguments with Valgrind.

Not sure if this is truly s-s, feel free to open up if this is not the case.
Comment 1 Julian Seward [:jseward] 2012-10-01 01:37:22 PDT
Gary, for these uninitialised-value error reports, can you also report
the origin(s) of the uninitialised values(s), as produced by --track-origins=yes?
These make it a lot easier to figure out what the problem is.
Comment 2 Gary Kwong [:gkw] [:nth10sd] 2012-10-01 01:58:06 PDT
Is this correct?

==3542== Conditional jump or move depends on uninitialised value(s)
==3542==    at 0x4E647C: ComputePrecisionInRange(JSContext*, int, int, JS::Value const&, int*) [clone .constprop.103] (jsatominlines.h:88)
==3542==    by 0x4E698E: num_toExponential(JSContext*, unsigned int, JS::Value*) (jsnum.cpp:854)
==3542==    by 0x4D2D3A: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:370)
==3542==    by 0x4CDAE5: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2460)
==3542==    by 0x4D2A0A: js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) (jsinterp.cpp:324)
==3542==    by 0x4D3ABA: js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) (jsinterp.cpp:509)
==3542==    by 0x43FBD4: JS_ExecuteScript (jsapi.cpp:5640)
==3542==    by 0x421CB1: Process(JSContext*, JSObject*, char const*, bool) (js.cpp:439)
==3542==    by 0x425B65: Shell(JSContext*, js::cli::OptionParser*, char**) (js.cpp:4688)
==3542==    by 0x41834F: main (js.cpp:4947)
==3542==  Uninitialised value was created by a stack allocation
==3542==    at 0x4E6806: num_toExponential(JSContext*, unsigned int, JS::Value*) (jsnum.cpp:863)
==3542==
Comment 3 Julian Seward [:jseward] 2012-10-01 02:22:02 PDT
Looks plausible.  Do all of the errors in comment #0 have the same
origin point, that is, a stack allocation in num_toExponential?
Comment 4 Jan de Mooij [:jandem] 2012-10-01 02:31:27 PDT
The "int *precision" passed to ComputePrecisionInRange is not initialized and passed to IntToCString:

    ToCStringBuf cbuf;
    char *numStr = IntToCString(&cbuf, *precision);

With a Clang debug build:

js> Number.prototype.toPrecision('')
typein:1:0 RangeError: precision 34685472 out of range
Comment 5 Gary Kwong [:gkw] [:nth10sd] 2012-10-01 16:32:42 PDT
I'm guessing bug 771744 was the regressor based on the line number:

http://hg.mozilla.org/mozilla-central/rev/4483ab804f1e
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2012-10-01 17:39:52 PDT
Created attachment 666786 [details] [diff] [review]
Patch with tests

Sigh, helper-method-extraction fail.

A truly intelligent compiler, that could figure out the dependence on undefined behavior here, could perfectly well compile this to code to nonsense.  All the relevant code is easily visible even to file-at-a-time compilers, so I have no difficulty believing some compilers can figure out this use-of-undefined-value and optimize based upon it.  The question is would the optimization enable black magic to be completed; I'm pretty sure only a compiler expert from each major compiler could say what might be possible here.

Compilers not so smart, that just passed the undefined |*precision| value to IntToCString, would produce a garbage error message (with some indeterminate integer in the output string) but would not be exploitable.

So whether this is a critical problem, or just one that produces bogus output, depends how aggressively compilers optimize.  Paranoia and uncertainty and lack of knowledge suggest assuming this is a critical issue, although it's very much possible it isn't (or isn't for certain compilers, but is for others).
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-10-02 17:13:42 PDT
Pushed to Try.
https://tbpl.mozilla.org/?tree=Try&rev=70bffe879810
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2012-10-02 17:20:46 PDT
I was thinking of just pushing this to inbound -- the patch is clear enough that I don't think try's worth the resources.  But there is the little matter of security-sensitivity of this.  I guess you want it on trunk before you take it on branches, so maybe pushing is just the right thing and I'm just angsting overmuch (especially given that this is only questionably a vulnerability).  Anyway, I think probably we want this on as many branches as we can, ideally as quickly as possible, to be on the safe side.
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-10-02 17:27:38 PDT
FWIW, it tagged along with some other stuff, so it's not a big drain on Try's resources :)
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2012-10-02 17:39:29 PDT
Oh hey, excellent timing!  Since we haven't shipped this yet, this doesn't need security-approval specifically (although it does need the usual branch approvals).  Let's get 'er in ASAP and avoid that issue entirely.  :-)  And I guess since this isn't shipped yet, so indeed I was angsting overmuch, and we should just fix it everywhere and not overworry about any issue that might or might not temporarily exist in alphas/betas/nightlies.
Comment 11 Jeff Walden [:Waldo] (remove +bmo to email) 2012-10-02 17:57:01 PDT
Comment on attachment 666786 [details] [diff] [review]
Patch with tests

Or no, I misread the sec-approval process mail.  Sigh.

[Security approval request comment]
How easily can the security issue be deduced from the patch?
Pretty easily -- it's easy to track back to the callers and see that |*precision| is uninitialized memory, and that it's a hidden bug should indicate that the patch is worth looking at to find that.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Nothing mentions it as such, just as a bug implementing the relevant APIs (and testing them), but I didn't attempt to obfuscate anything.  The patch lets the cat out of the bag; all the rest here doesn't add much, I think.

Which older supported branches are affected by this flaw?
Aurora, Beta, everything but actual shipped-to-the-public-at-large releases.

If not all supported branches, which bug introduced the flaw?
Bug 771744 introduced this, thinko when making changes requested by the reviewer.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Haven't attempted backporting, likely trivial -- this code hasn't changed much, if at all, since the bug was introduced.

How likely is this patch to cause regressions; how much testing does it need?
Unlikely as basically an obvious thinko, shouldn't need testing beyond what the included test provides.  Although, I'd be a little happier if we were running the added test under valgrind or something on occasion to catch the UMR.  :-\  (Side issue, I know.)
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-10-02 19:25:11 PDT
(In reply to Ryan VanderMeulen from comment #7)
> https://tbpl.mozilla.org/?tree=Try&rev=70bffe879810

Looks green.
Comment 13 Alex Keybl [:akeybl] 2012-10-03 10:44:54 PDT
Already wontfixing for FF16 since this doesn't seem critical enough to take between our final beta and release.
Comment 14 Daniel Veditz [:dveditz] 2012-10-03 17:13:57 PDT
Comment on attachment 666786 [details] [diff] [review]
Patch with tests

This looks like a correctness problem not an exploitable security issue for any value of the undefined int. I wouldn't over-worry about what bogosities compiler optimizations might introduce until we see evidence for it.

sec-approved, and I think we can unhide the bug.
Comment 15 Gary Kwong [:gkw] [:nth10sd] 2012-10-03 17:24:43 PDT
> sec-approved, and I think we can unhide the bug.

Unhiding, and this can be landed.
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-10-03 18:45:46 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6eb4dda69796
Comment 17 Ed Morley [:emorley] 2012-10-04 08:58:17 PDT
https://hg.mozilla.org/mozilla-central/rev/6eb4dda69796
Comment 18 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-16 15:47:40 PDT
This needs mozilla-beta uplift nomination so we can get it into 17.
Comment 19 Jeff Walden [:Waldo] (remove +bmo to email) 2012-10-16 16:07:46 PDT
Comment on attachment 666786 [details] [diff] [review]
Patch with tests

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 771744
User impact if declined: bogus results from some method calls if you call some Number.prototype.to* with invalid arguments.
Testing completed (on m-c, etc.): It's baked on m-c, patch includes a test.
Risk to taking this patch (and alternatives if risky): Not much, pretty clear what the issue is, pretty clear what the fix should be.
String or UUID changes made by this patch: N/A
Comment 20 Scoobidiver (away) 2012-10-17 14:10:05 PDT
Pushed to Beta:
http://hg.mozilla.org/releases/mozilla-beta/rev/7e12261924cd
Comment 21 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-17 15:50:42 PDT
[qa-] for verification since this has a in-testsuite coverage.
Comment 22 Gary Kwong [:gkw] [:nth10sd] 2012-12-13 17:07:10 PST
A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.

Note You need to log in before you can comment on or make changes to this bug.