Closed Bug 603333 Opened 9 years ago Closed 9 years ago

[ANGLE] Infinite loops and buffer overflow in byte_scan

Categories

(Core :: Canvas: WebGL, defect, blocker)

defect
Not set
blocker

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: timeless, Assigned: vlad)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, crash, hang, Whiteboard: [sg:critical] upstream r469 has fix)

313 static int byte_scan(InputSrc *in, yystypepp * yylvalpp)
318     int len, ch, ii, ival = 0;

320     for (;;) {

331         len = 0;
332         switch (ch) {
333         default:
334                         return ch; // Single character token
335         case EOF:
336             return -1;
337                 case 'A': case 'B': case 'C': case 'D': case 'E':
366             return CPP_IDENTIFIER;

this is useless:
367             break;

368         case '0':
369             yylvalpp->symbol_name[len++] = ch;
370             ch = cpp->currentInput->getch(cpp->currentInput, yylvalpp);
371             if (ch == 'x' || ch == 'X') {
372                                 yylvalpp->symbol_name[len++] = ch;
373                 ch = cpp->currentInput->getch(cpp->currentInput, yylvalpp);
374                 if ((ch >= '0' && ch <= '9') ||
375                     (ch >= 'A' && ch <= 'F') ||
376                     (ch >= 'a' && ch <= 'f'))
377                 {
380                     do {

nothing here seems to enforce a limit on len v. yylvalpp->symbol_name[]:
381                                                 yylvalpp->symbol_name[len++] = ch;

Is it on the wrong side of do { ?

396                         ch = cpp->currentInput->getch(cpp->currentInput, yylvalpp);
397                     } while ((ch >= '0' && ch <= '9') ||
398                              (ch >= 'A' && ch <= 'F') ||
399                              (ch >= 'a' && ch <= 'f'));
400                 } else {
402                 }
403                 yylvalpp->symbol_name[len] = '\0';
404                                 cpp->currentInput->ungetch(cpp->currentInput, ch, yylvalpp);

406                 return CPP_INTCONSTANT;

407             } else if (ch >= '0' && ch <= '7') { // octal integer constants

410                 do {

nothing here seems to enforce a limit on len v. yylvalpp->symbol_name[]:
411                     yylvalpp->symbol_name[len++] = ch;

Is it on the wrong side of do { ?

420                     ch = cpp->currentInput->getch(cpp->currentInput, yylvalpp);
421                 } while (ch >= '0' && ch <= '7');

424                 yylvalpp->symbol_name[len] = '\0';

427                 return CPP_INTCONSTANT;
428             } else {
429                                 cpp->currentInput->ungetch(cpp->currentInput, ch, yylvalpp);
430                                 ch = '0';
431             }
432             // Fall through...
433         case '1': case '2': case '3': case '4':
434         case '5': case '6': case '7': case '8': case '9':
435             do {

This could result in an infinite loop if there's a string long enough to get len == MAX_SYMBOL_LEN (512). The code should probably break if that happens...
436                 if (len < MAX_SYMBOL_NAME_LEN) {

I'm not sure why this is comparing len against 0, shouldn't that not be needed?
437                     if (len > 0 || ch != '0') {
438                         yylvalpp->symbol_name[len] = ch;
439                    len++;
440                     }
441                     ch = cpp->currentInput->getch(cpp->currentInput, yylvalpp);
442                 }
443             } while (ch >= '0' && ch <= '9');

444             if (ch == '.' || ch == 'e' || ch == 'f' || ch == 'h' || ch == 'x'|| ch == 'E') {
445                 return lFloatConst(yylvalpp->symbol_name, len, ch, yylvalpp);
446             } else {
463                 return CPP_INTCONSTANT;
464             }

this isn't reachable:
465             break;

466         case '-':
476         case '+':
486         case '*':
494         case '%':
504         case ':':
512         case '^':
525         case '=':
533         case '!':
541         case '|':
553         case '&':
565         case '<':
589         case '>':
607         case '.':
617                     return '.';

620         case '/':
621             ch = cpp->currentInput->getch(cpp->currentInput, yylvalpp);
622             if (ch == '/') {
628                 return '\n';

629             } else if (ch == '*') {
631                 ch = cpp->currentInput->getch(cpp->currentInput, yylvalpp);
650                 // Go try it again...
651             } else if (ch == '=') {
652                 return CPP_DIV_ASSIGN;
653             } else {
655                 return '/';
656             }
657             break;

658         case '"':
659             ch = cpp->currentInput->getch(cpp->currentInput, yylvalpp);
660             while (ch != '"' && ch != '\n' && ch != EOF) {
661                 if (ch == '\\') {
663                     return -1;
664                 }

This could result in an infinite loop if there's a string long enough to get len == MAX_SYMBOL_LEN (512). The code should probably break if that happens...
665                 if (len < MAX_STRING_LEN) {
666                     string_val[len] = ch;
667                     len++;
668                     ch = cpp->currentInput->getch(cpp->currentInput, yylvalpp);
669                 }
670             };

674                 return CPP_STRCONSTANT;
681 } // byte_scan
I'm assuming that ReadToken isn't actually at risk as I'm hoping that the input to ReadToken comes from a consumer instead of untrusted data. However, if I'm wrong, this is more or less what Coverity has to say about it:

257 int ReadToken(TokenStream *pTok, yystypepp * yylvalpp)
259     char symbol_name[MAX_SYMBOL_NAME_LEN + 1];
260     char string_val[MAX_STRING_LEN + 1];
261     int ltoken, len;
262     char ch;

265     if (ltoken >= 0) {
268         switch (ltoken) {
270         case CPP_TYPEIDENTIFIER:
271             len = 0;
272             ch = lReadByte(pTok);
273             while ((ch >= 'a' && ch <= 'z') ||
274                      (ch >= 'A' && ch <= 'Z') ||
275                      (ch >= '0' && ch <= '9') ||
276                      ch == '_')
277             {

This could result in an infinite loop if there's a string long enough to get
len == MAX_SYMBOL_NAME_LEN (128?). The code should probably break if that happens...

278                 if (len < MAX_SYMBOL_NAME_LEN) {
279                     symbol_name[len] = ch;
280                     len++;
281                     ch = lReadByte(pTok);
282                 }
283             }
284             symbol_name[len] = '\0';

-- Unrelated, I wonder why there's an assert here:
285             assert(ch == '\0');

287             return CPP_IDENTIFIER;

Useless:
288             break;
289         case CPP_STRCONSTANT:
296             break;

297         case CPP_FLOATCONSTANT:
298             len = 0;
299             ch = lReadByte(pTok);
300             while ((ch >= '0' && ch <= '9')||(ch=='e'||ch=='E'||ch=='.')||(ch=='+'||ch=='-'))
301             {

This could result in an infinite loop if there's a string long enough to get
len == MAX_SYMBOL_NAME_LEN (128?). The code should probably break if that happens...
302                 if (len < MAX_SYMBOL_NAME_LEN) {
303                     symbol_name[len] = ch;
304                     len++;
305                     ch = lReadByte(pTok);
306                 }
307             }
308             symbol_name[len] = '\0';

309             assert(ch == '\0');

312             break;
313         case CPP_INTCONSTANT:
314             len = 0;
315             ch = lReadByte(pTok);
316             while ((ch >= '0' && ch <= '9'))
317             {

This could result in an infinite loop if there's a string long enough to get
len == MAX_SYMBOL_NAME_LEN (128?). The code should probably break if that happens...
318                 if (len < MAX_SYMBOL_NAME_LEN) {
319                     symbol_name[len] = ch;
320                     len++;
321                     ch = lReadByte(pTok);
322                 }
323             }
324             symbol_name[len] = '\0';

325             assert(ch == '\0');

328             break;
Keywords: hang
Vlad, what is the good way of reporting security bugs to ANGLE upstream?
Summary: Infinite loops and buffer overflow in byte_scan → [ANGLE] Infinite loops and buffer overflow in byte_scan
Just file it at http://code.google.com/p/angleproject/ as with all angle bugs -- there's no point in filing them in our bugzilla.
Even if it's security critical?
Whiteboard: [sg:critical]
Yeah, I think it's worth filing an sg:crit just so we can follow up to make sure it's fixed up stream.  Thanks a bunch Benoit!
blocking2.0: --- → final+
Thanks go to Timeless :)
If anyone already has a patch, please send it to me. Otherwise I will look into it.
Assignee: nobody → bjacob
Assignee: bjacob → alokp
Component: Graphics → Canvas: WebGL
QA Contact: thebes → canvas.webgl
(In reply to comment #3)
> Just file it at http://code.google.com/p/angleproject/ as with all angle bugs
> -- there's no point in filing them in our bugzilla.

If they're security bugs exposed in Firefox it'd be nice to have a bug to track our own vulnerability. We can then decide whether to wait for an upstream fix or patch around it.
Did we file a bug at http://code.google.com/p/angleproject/ per Comment 3?  

Alok, are you making any progress here?
I guess we just need to merge in a new ANGLE, then?
Assignee: alokp → vladimir
Assignee: vladimir → alokp
Whiteboard: [sg:critical] → [sg:critical] upstream r469 has fix
Assignee: alokp → vladimir
Vlad, are you prepared to do a new angle library merge? Seems we should do that before the last beta, aiming for beta9 for now.
blocking2.0: final+ → beta9+
For a new ANGLE library merge, we are blocked on 578880. That's why this bug is blocked on it.
Erm, not blocked on bug 578880 -- that's just for linux-x64, where this stuff isn't even being built yet.  Our current angle rev is r445; if this is fixed now, I'll do an update shortly.
or wait, are we blocked on the atof etc. thing?  I'm tempted to basically disable angle on all linux until linux gets its act together.  Would rather do that than ship an old version to windows/osx.
AIUI we disabled ANGLE on all Linux then updated to r498.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)
blocking2.0: beta9+ → betaN+
No longer depends on: 578880
Keywords: coverity, crash, hang
Depends on: 578880
Keywords: coverity, crash, hang
Group: core-security
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.