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)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed
firefox70 --- fixed

People

(Reporter: jorendorff, Assigned: Waldo)

Details

(Keywords: sec-audit, Whiteboard: [adv-main61-][post-critsmash-triage][adv-main70-])

Attachments

(2 files, 2 obsolete files)

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.
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)
Priority: -- → P1
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.
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)
Attached patch Patch? (obsolete) — Splinter Review
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: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(jwalden+bmo)
> 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)
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)
(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.
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+
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.)
Attached patch Part deux (obsolete) — Splinter Review
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)
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.")
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).
Attachment #8962486 - Flags: review?(jorendorff) → review+
[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+
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 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+
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.
(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.
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)
Is there a good reason to consider backporting this to 60 or can it ride the 61 train?
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)
Group: javascript-core-security → core-security-release
Whiteboard: [adv-main61-]
Flags: qe-verify-
Whiteboard: [adv-main61-] → [adv-main61-][post-critsmash-triage]
Group: core-security-release
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.