603333 - [ANGLE] Infinite loops and buffer overflow in byte_scan

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[timeless]

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

Comment 1

[timeless]

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;

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Vlad, what is the good way of reporting security bugs to ANGLE upstream?

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Even if it's security critical?

Comment 5

[Damon Sicore (:damons)]

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!

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Thanks go to Timeless :)

Comment 7

[Alok Priyadarshi]

If anyone already has a patch, please send it to me. Otherwise I will look into it.

Comment 8

[Daniel Veditz [:dveditz]]

(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.

Comment 9

[Damon Sicore (:damons)]

Did we file a bug at http://code.google.com/p/angleproject/ per Comment 3?  

Alok, are you making any progress here?

Comment 10

[Alok Priyadarshi]

It is fixed in r469.
http://code.google.com/p/angleproject/source/detail?r=469

Comment 11

[Joe Drew (not getting mail)]

I guess we just need to merge in a new ANGLE, then?

Comment 12

[Johnny Stenback (:jst)]

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.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

For a new ANGLE library merge, we are blocked on 578880. That's why this bug is blocked on it.

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 16

[Joe Drew (not getting mail)]

AIUI we disabled ANGLE on all Linux then updated to r498.

Comment 18

[LegNeato]

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)