Closed
Bug 1447668
Opened 6 years ago
Closed 6 years ago
ctypes: Out-of-range float to int conversions are undefined behavior
Categories
(Core :: js-ctypes, enhancement, P1)
Core
js-ctypes
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: jorendorff, Assigned: Waldo)
Details
(Keywords: sec-audit, Whiteboard: [adv-main61-][post-critsmash-triage][adv-main70-])
Attachments
(2 files, 2 obsolete files)
16.59 KB,
patch
|
Waldo
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
3.77 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
https://searchfox.org/mozilla-central/source/js/src/ctypes/CTypes.cpp#3099 In jsvalToIntegerExplicit<IntegerType>, the cast `IntegerType(d)` on that line is undefined behavior when d is out of range for the target type. Same thing everywhere `Convert<size_t>` and `Convert<intptr_t>` are used. This is bad because the undefined values are pretty much immediately fed to array accesses. LLVM is smart enough to make this exploitable, I expect with 50% confidence, so marking this bug security-sensitive for now.
Reporter | ||
Comment 1•6 years ago
|
||
ni?Waldo because if we already have safe conversion functions in mfbt that we can just snap in here, he'll know.
Flags: needinfo?(jwalden+bmo)
Reporter | ||
Updated•6 years ago
|
status-firefox61:
--- → affected
Priority: -- → P1
Reporter | ||
Comment 2•6 years ago
|
||
What triggered this is RyanVM wanting to delete the ancient MSVC-specific code at https://searchfox.org/mozilla-central/source/js/src/ctypes/CTypes.cpp#2576-2587 I imagine we can do that as part of any fix here.
Reporter | ||
Comment 3•6 years ago
|
||
UndefinedBehaviorSanitizer agrees that this is undefined: $ CFLAGS=-fsanitize=undefined CXXFLAGS=-fsanitize=undefined ../configure --enable-ctypes --enable-debug --enable-nspr-build ... $ make -j8 ... $ ./dist/bin/js js> ctypes.int32_t(1e200) /Users/jorendorff/work/gecko/js/src/ctypes/CTypes.cpp:2572:23: runtime error: 1e+200 is outside the range of representable values of type 'int' /Users/jorendorff/work/gecko/js/src/ctypes/CTypes.cpp:3099:50: runtime error: 1e+200 is outside the range of representable values of type 'int' ctypes.int32_t(-2147483648)
Assignee | ||
Comment 4•6 years ago
|
||
mfbt doesn't have conversion/detection functions that offer correct/safe conversion semantics on floating-point values with fractional components. It has Number{Is,Equals}SignedInteger32. (64-bit versions are trivial to add -- literally all the work should be complete except calling some function templates with different template parameters and adding tests.) It should be easy to add Number{Is,Equals}UnsignedInteger{32,64} by copying the signed-integer versions and making a few relatively simple tweaks. But it doesn't have anything that considers fractional components, either to round or discard them or something else. Such could be added by users with a preflight std::trunc, or it perhaps is foldable into a broader bitwise algorithm at negligible cost. But does this really want an mfbt algorithm at all? Seems like JS::To{Ui,I}nt{8,16,32,64} -- or a template version of those, that means we can write one function call instead of 8 different possible calls -- is the right approach. This patch passes the modified jsapi-test, but I haven't built a browser to know that the ctypes change works and doesn't require additional test-fixing.
Attachment #8961120 -
Flags: review?(jorendorff)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(jwalden+bmo)
Comment 5•6 years ago
|
||
> LLVM is smart enough to make this exploitable, I expect with 50% confidence, so marking this bug security-sensitive for now.
Jason, can you validate that we're actually in danger from this and it isn't just a potential issue? That will affect whether we backport and how we rate this for security.
Flags: needinfo?(jorendorff)
Reporter | ||
Comment 6•6 years ago
|
||
After thinking some more, this is just a potential issue. As of 2015 it was possible to duck a bound-check in LLVM using this kind of UB: https://github.com/rust-lang/rust/issues/10184#issuecomment-139858153 But I think it's unlikely users can control values passed to ctypes code and used as indices into ctypes arrays. sec-high at worst.
Flags: needinfo?(jorendorff)
Reporter | ||
Comment 7•6 years ago
|
||
(In reply to Jeff Walden [:Waldo] from comment #4) > But does this really want an mfbt algorithm at all? Seems like > JS::To{Ui,I}nt{8,16,32,64} -- or a template version of those, that means we > can write one function call instead of 8 different possible calls -- is the > right approach. Hey, good point.
Reporter | ||
Comment 8•6 years ago
|
||
Comment on attachment 8961120 [details] [diff] [review] Patch? Review of attachment 8961120 [details] [diff] [review]: ----------------------------------------------------------------- Great, r=me, but please also patch (or delete) the Convert function template in CTypes.cpp. Second patch, maybe?
Attachment #8961120 -
Flags: review?(jorendorff) → review+
Reporter | ||
Comment 9•6 years ago
|
||
A spot-check in js/src/jit-test/tests/ctypes would be nice, especially since the behavior you chose actually makes sense. :D (Configuring the SM shell with --enable-ctypes --enable-nspr-build works for me.)
Assignee | ||
Comment 10•6 years ago
|
||
Bah, didn't realize there were two different conversion failures in the one code path. This should fix both of them now, and with tests. Maybe you understand all this implicit/explicit/exact convert goo, but it is almost entirely opaque to me, and stupid for the int32/double type distinction apparently latent. It all could be a lot clearer/simpler about intent/implementation -- with actually understandable/consistent semantics -- but as mentioned on IRC, I'm trying to waste as little time on this as I can.
Attachment #8962486 -
Flags: review?(jorendorff)
Reporter | ||
Comment 11•6 years ago
|
||
Any distinction ctypes makes between Int32Values and DoubleValues is a bug. ---- ImplicitConvert/ExplicitConvert explained: It's API design. CTypes is constantly converting JS::Values into C/C++ objects. In fact this is one of approximately 3 core things CTypes does, and it accounts for much of the code. f(256) // (where f is a C function) convert 256 to the argument type buf[0] = 256; // (where buf is a CData array) convert 256 to element type In my judgment, neither line of JS code here makes it visually obvious that a conversion is happening. So CTypes uses ImplicitConvert for these JS-to-C++ conversions. ImplicitConvert is fairly picky. If the target type is uint8_t, and you try to pass JS::Int32Value(256), ImplicitConvert will throw a TypeError. The point of ImplicitConvert is to support passing reasonable JS values to C/C++ without ceremony. (But "Errors should never pass silently", especially around these super-sharp tools.) When you call a CType, as in `ctypes.int8_t(256)`, ExplicitConvert is used instead. The point of ExplicitConvert is to support lossy/unsafe conversions and any other C hackery you need to do, with a visible yet readable opt-in syntax. ("Unless explicitly silenced.")
Reporter | ||
Comment 12•6 years ago
|
||
Of course C++ itself has both implicit conversions and explicit casts. This is the same thing (except with more reasonable choices around what can be left implicit, at the margin).
Reporter | ||
Updated•6 years ago
|
Attachment #8962486 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 13•6 years ago
|
||
[Security approval request comment] > How easily could an exploit be constructed based on the patch? We don't know that one *can* be constructed. It isn't practical to audit every single ctypes user to determine whether one can be -- that is, whether fully untrusted numbers ever flow into ctypes-using code in such a way as to trigger this undefined behavior of conversion, *and* for the resulting actual behavior to be used in untoward array accesses that are also exploitable. I would be inclined to take this with approximately the usual caution -- don't land it too early, don't try to stick it in late in a cycle, don't have any obviously incriminating comments or tests in the patch. > Do comments in the patch, the check-in comment, or tests included in > the patch paint a bulls-eye on the security problem? No. *This* version of the patch extricates the new ctypes test -- which to be fair tests certain ctypes input behavior, but doesn't show anything like feeding into array inputs (or other cases that we know compilers *will* definitely optimize into exploitable patterns). Also: there's a lot of change going on in this patch, that to some degree obfuscates precisely what the salient code change is. That helps with bulls-eyes. If we remain concerned, it wouldn't be hard to file a cover bug about "Clean up ctypes conversion routines" and get a review there and land such that no one external even realizes that it's fixing something potentially security-related. I don't think we *must* go to that effort for this right now, with what we know now. > Which older supported branches are affected by this flaw? > If not all supported branches, which bug introduced the flaw? Probably goes back all the way. > Do you have backports for the affected branches? If not, how different, > hard to create, and risky will they be? I don't expect much of this code has significantly changed recently, so I'd expect backporting to be pretty easy. Haven't done it yet. > How likely is this patch to cause regressions; how much testing does it need? The risk seems relatively low to me. Something like the normal amount of testing of browser functionality not breaking seems adequate, or at least the best we can do.
Attachment #8961120 -
Attachment is obsolete: true
Attachment #8962486 -
Attachment is obsolete: true
Attachment #8966773 -
Flags: sec-approval?
Attachment #8966773 -
Flags: review+
Assignee | ||
Comment 14•6 years ago
|
||
This is just the test parts of the prior patches, extricated. Won't land til the prior patch has landed and sat for awhile, tho tbh I'm not sure this test really reveals all that much, if anything, about this bug's security concerns.
Attachment #8966776 -
Flags: review+
Comment 15•6 years ago
|
||
Comment on attachment 8966773 [details] [diff] [review] Final, combined patch We're mid-cycle right now so let's get this in. sec-approval+ (though technically a sec-audit doesn't need it). I appreciate the caution.
Attachment #8966773 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 16•6 years ago
|
||
Yeah, I wasn't sure what sec-audit was supposed to imply here -- I figured it was orthogonal to the sec-{low,high,critical} categorization which left me adrift.
Comment 17•6 years ago
|
||
(In reply to Jeff Walden [:Waldo] from comment #16) > Yeah, I wasn't sure what sec-audit was supposed to imply here -- I figured > it was orthogonal to the sec-{low,high,critical} categorization which left > me adrift. It usually means a potential problem found by code inspection or other dev work but not something that needs a sec-high or sec-critical, for example.
Comment 18•6 years ago
|
||
Backed out for build bustages at /builds/worker/workspace/build/src/js/src/ctypes/CTypes.cpp:2607: https://hg.mozilla.org/integration/mozilla-inbound/rev/e233cb06379dfefa544fe2dbd4c82945b627cc70 Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e0f8f325c2ce3c0e39587f117d63d0872bb9826a Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=173596345&repo=mozilla-inbound&lineNumber=3532 z:/build/build/src/js/src/ctypes/CTypes.cpp(2608): error C2912: explicit specialization 'char16_t js::ctypes::ConvertUnsignedTargetTo<char16_t>(char16_t)' is not a specialization of a function template Also fails SM fuzzing: https://treeherder.mozilla.org/logviewer.html#?job_id=173596364&repo=mozilla-inbound [task 2018-04-13T20:48:39.856Z] TEST-PASS | js/src/jit-test/tests/ctypes/conversion-finalizer.js | Success (code 0, args "") [0.1 s] [task 2018-04-13T20:48:39.857Z] {"action": "test_start", "pid": 3465, "source": "jittests", "test": "ctypes/conversion-finalizer.js", "thread": "main", "time": 1523652519.760834} [task 2018-04-13T20:48:39.857Z] {"action": "test_end", "extra": {"jitflags": ""}, "message": "Success", "pid": 3465, "source": "jittests", "status": "PASS", "test": "ctypes/conversion-finalizer.js", "thread": "main", "time": 1523652519.856832} [task 2018-04-13T20:48:39.888Z] /builds/worker/workspace/build/src/js/src/jit-test/lib/asserts.js:75:23 Error: Assertion failed: expected exception TypeError, got RangeError: the string "foo" does not fit in the type int32_t [task 2018-04-13T20:48:39.888Z] Stack: [task 2018-04-13T20:48:39.889Z] assertErrorMessage@/builds/worker/workspace/build/src/js/src/jit-test/lib/asserts.js:75:23 [task 2018-04-13T20:48:39.889Z] assertTypeErrorMessage@/builds/worker/workspace/build/src/js/src/jit-test/lib/asserts.js:91:7 [task 2018-04-13T20:48:39.889Z] test@/builds/worker/workspace/build/src/js/src/jit-test/tests/ctypes/conversion-primitive.js:7:3 [task 2018-04-13T20:48:39.889Z] @/builds/worker/workspace/build/src/js/src/jit-test/tests/ctypes/conversion-primitive.js:44:3
Flags: needinfo?(jwalden+bmo)
Comment 19•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5be9aada3037ef8bce52a8dae0995581af757c71 https://hg.mozilla.org/mozilla-central/rev/5be9aada3037
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 20•6 years ago
|
||
Is there a good reason to consider backporting this to 60 or can it ride the 61 train?
status-firefox59:
--- → wontfix
status-firefox60:
--- → fix-optional
status-firefox-esr52:
--- → wontfix
Assignee | ||
Comment 21•6 years ago
|
||
There are a number of patches landed on trunk recently that this seems to depend on, so the backport isn't entirely trivial. Given I was fence-sitting on this as to whether to backport, I think we probably can just wait it out.
Flags: needinfo?(jwalden+bmo)
Updated•6 years ago
|
Updated•6 years ago
|
Group: javascript-core-security → core-security-release
Updated•6 years ago
|
Whiteboard: [adv-main61-]
Updated•6 years ago
|
Flags: qe-verify-
Whiteboard: [adv-main61-] → [adv-main61-][post-critsmash-triage]
Updated•5 years ago
|
Group: core-security-release
Comment 22•5 years ago
|
||
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/2ce54fbea7dc Add a test. r=jorendorff
Comment 23•5 years ago
|
||
bugherder |
Comment 24•5 years ago
|
||
bugherder uplift |
status-firefox70:
--- → fixed
Updated•5 years ago
|
Whiteboard: [adv-main61-][post-critsmash-triage] → [adv-main61-][post-critsmash-triage][adv-main70-]
You need to log in
before you can comment on or make changes to this bug.
Description
•