Closed
Bug 1348433
Opened 7 years ago
Closed 7 years ago
Latent incorrect static_assert in jsstr.cpp
Categories
(Core :: JavaScript Engine, enhancement)
Tracking
()
RESOLVED
INVALID
People
(Reporter: q1, Unassigned)
Details
(Keywords: csectype-intoverflow, sec-audit)
On x64 builds, the static_assert on lines 2879-80 of str_fromCodePoint() (js \src\jsstr.cpp) (below) is too permissive, because args.length() on line 2881 isa |unsigned int|. Therefore, if ARGS_LENGTH_MAX were >= 0x80000000, the pod_malloc() argument in line 2881 could overflow, causing a buffer overrun. This bug appears to be latent because ARGS_LENGTH_MAX == 500000 (see js\src\vm\ArgumentsObject.h), but it should be fixed. (args.length() is checked against ARGS_LENGTH_MAX in js::SpreadCallOperation() and GenericArgsBase::Init(), but because I don't know much about the JS implementation, there could be some other path that should, but does not, check this value. Hence I have made this bug private). 2859: static bool 2860: str_fromCodePoint(JSContext* cx, unsigned argc, Value* vp) 2861: { 2862: CallArgs args = CallArgsFromVp(argc, vp); 2863: 2864: // Optimize the single code-point case. 2865: if (args.length() == 1) ... 2873: if (args.length() <= JSFatInlineString::MAX_LENGTH_TWO_BYTE / 2) ... 2875: 2876: // Steps 1-2 (omitted). 2877: 2878: // Step 3. 2879: static_assert(ARGS_LENGTH_MAX < std::numeric_limits<size_t>::max() / 2, 2880: "|args.length() * 2 + 1| does not overflow"); 2881: char16_t* elements = cx->pod_malloc<char16_t>(args.length() * 2 + 1);
Updated•7 years ago
|
Group: core-security → javascript-core-security
Updated•7 years ago
|
Keywords: csectype-intoverflow,
sec-other
Updated•7 years ago
|
Comment 2•7 years ago
|
||
To the best of my knowledge, by intended design. (I think blame for the GenericArgsBase hit would point at one of the relevant bugs that introduced that design. We were trying to not-check in certain places at one point, when we thought we could optimize them out based on other latent limits imposed in context, but those were fragile enough in practice that we killed them off.) Every place that initializes a CallArgs passes through those two signatures. There are (or maybe were -- I've been trying to kill them off when I can) JSAPI functions that take Value* plus size_t, but those are in jsapi.cpp, and the raw pointer/length are funneled into creating a CallArgs -- and so would hit the GenericArgsBase check. The interface to perform a function call, internally, only accepts a CallArgs that implies the GenericArgsBase check, more generally. I don't believe there are any other ways to create a function call frame's arguments, that doesn't pass by one of those two checks, by dint of the internal API's signature. So I believe this can be safely unhidden/closed.
Group: javascript-core-security
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
Comment 3•7 years ago
|
||
...by intended design, those two checks chokepoint the system, I mean. (And really even just GenericArgsBase does -- the check in SpreadCallOperation, if you look at it, notes that it's doing double duty with GenericArgsBase's check.)
Flags: needinfo?(jwalden+bmo)
You need to log in
before you can comment on or make changes to this bug.
Description
•