Latent incorrect static_assert in jsstr.cpp

RESOLVED INVALID

Status

()

Core
JavaScript Engine
RESOLVED INVALID
a year ago
a year ago

People

(Reporter: q1, Unassigned)

Tracking

({csectype-intoverflow, sec-audit})

51 Branch
x86_64
Unspecified
csectype-intoverflow, sec-audit
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

a year ago
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);
(Reporter)

Updated

a year ago
Hardware: Unspecified → x86_64
Group: core-security → javascript-core-security
Jeff, maybe you could take a look?
Flags: needinfo?(jwalden+bmo)
Keywords: csectype-intoverflow, sec-other
Keywords: sec-other → sec-audit
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
Last Resolved: a year ago
Resolution: --- → INVALID
...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.