1170025 - Possible string buffer overflow in cvt_f ?

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Mats Palmgren (inactive)]

(Spawned off from bug 1169326. This is the only JS engine issue from that bug.)

[e-mail] writes:
I have found several unsafe calls to sprintf which will or can cause the (usually stack-based) destination buffer to be overrun, possibly leading to crash problems and/or security issues. Here's a list:

[...]

38.0.1\js\src\jsprf.cpp
   cvt_f (also has a suspect use of js_memcpy on line 303; also can fmt0 > fmt1?)

Comment 1

[Andrew McCreight [:mccr8]]

Could you look into this, Waldo? Thanks.

Comment 2

[Jeff Walden [:Waldo]]

As far as the printf-like thing there goes, someone went and replaced this with a snprintf_literal at some point semi-recently, that being a function that sprintfs into a constant-sized array (size determined by template argument deduction), then caps it off with a null-terminator.  Before this change happened, we were doing |sprintf(fout, fin, d)| where |fout| has size 300, so something like "%500f" would overflow, but format strings are trusted input, and "%500f" is utterly insane, so I think we can ignore it.

Now, as far as the memcpy goes.  cvt_f has two callers.

In one caller, we have |fmt0| as pointing at the '%' in a format specifier, initialized from |fmt - 1|.  The |fmt1| that's passed in is |fmt|, modified *only* by intervening increments.  So in that case, |fmt0 < fmt1| for sure.

In the other caller, we have |fmt0, fmt1| as |pattern, &pattern[i + 1]|.  |i| is |fmt - dolPt|, where |dolPt| is assigned |fmt| after where |fmt0| was computed -- so |dolPt| is the result of one of those |fmt| increments in the previous case.  So |fmt > dolPt|, so |i > 0|, so |pattern < &pattern[i + 1]| (assuming nothing insane like pointer wraparound, to be pedantic).

Now, does the js_memcpy overflow?  Can |fmt1 - fmt0 > 20| or so?  No, because of the |if (amount >= (int)sizeof(fin))| test.

So there's no issue here.  At least, not if the format string isn't utterly insane, which it shouldn't be because this is all trusted code.

Comment 3

[Jeff Walden [:Waldo]]

There may well be issues in jsprf.cpp.  Format string handling code is going to be complex and sadmaking, by its nature.

But given jsprf.cpp consumes only trusted input unless misused (in which case a bug would lie with that caller using it), I don't think it's productive to look for bugs in it, nor (to be unusually blunt) to investigate bugs within it.  If it's important to investigate alleged bugs in it nonetheless, we should pay the price in time replace jsprf.cpp with type-aware, type-safe, non-variadic formatting code.