1292534 - (CVE-2017-5469) flex: buffer overflow in generated code

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P1)

Description

[Petr Cerny [:hrosik]]

Attachment: mozilla-flex_buffer_overrun.patch

http://seclists.org/oss-sec/2016/q3/163

CVE-2016-6354

Some versions of The Fast Lexical Analyzer contain a bug which causes it to produce code potentially vulnerable to a buffer overrun.

While the fix should be made upstream (CMU Sphinx and ANGLE) in the first place, it might be a good idea to apply this hotfix for now.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

making sure this is on milan's radar

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Petr, do you know if this has been filed against ANGLE itself?

Comment 3

[Petr Cerny [:hrosik]]

(In reply to Milan Sreckovic [:milan] from comment #2)
> Petr, do you know if this has been filed against ANGLE itself?

I have no idea, although it doesn't seem to be public in their issue racking system.

I have reported the issue to CMU Sphinx via the SourceForge issue tracking system.

Comment 4

[Daniel Veditz [:dveditz]]

rating sec-high assuming this bug can be triggered from web content.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Jeff, how should we land this?

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8778238 [details] [diff] [review]
mozilla-flex_buffer_overrun.patch

Review of attachment 8778238 [details] [diff] [review]:
-----------------------------------------------------------------

We only accept strings up to our JS max string size, which is still less than INT32_MAX, so `int` should be fine, but only for now!
Use int64_t.

::: gfx/angle/src/compiler/preprocessor/Tokenizer.cpp
@@ +1379,5 @@
>  		YY_CURRENT_BUFFER_LVALUE->yy_n_chars = yyg->yy_n_chars = 0;
>  
>  	else
>  		{
> +			int num_to_read =

Use int64_t.

Technically safest:
yy_size_t num_to_read = YY_CURRENT_BUFFER_LVALUE->yy_buf_size;
num_to_read -= min(num_to_read, number_to_move);
num_to_read -= min(num_to_read, 1);

Comment 7

[Kelsey Gilbert [:jgilbert]]

(In reply to Milan Sreckovic [:milan] from comment #5)
> Jeff, how should we land this?

Request sec-approval on the fixed patch, and land when the sec folks tell us to.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Petr, are you still working on the fix? We are going to release 49 & ES45.4 pretty soon.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

The 49 RC build will be on Monday; this could still potentially land early Monday and make it in if we need to ship this in 49. Milan can you request sec-approval/uplift?

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Too late for 49 & esr 45.4

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> (In reply to Milan Sreckovic [:milan] from comment #5)
> > Jeff, how should we land this?
> 
> Request sec-approval on the fixed patch, and land when the sec folks tell us
> to.

I know that part :)  This is an ANGLE change - do we land in Gecko code, then request an upstream change, or do we wait for the upstream change, what do we do with our github branch of ANGLE code in the meantime, there is a few extra moving parts.

Comment 12

[Petr Cerny [:hrosik]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #8)
> Petr, are you still working on the fix? We are going to release 49 & ES45.4
> pretty soon.

Sorry this went a off my radar - we just do carry the attached patch.

(In reply to Jeff Gilbert [:jgilbert] from comment #6)
> Comment on attachment 8778238 [details] [diff] [review]
> mozilla-flex_buffer_overrun.patch
> 
> We only accept strings up to our JS max string size, which is still less
> than INT32_MAX, so `int` should be fine, but only for now!

Um, I'm clearly missing a lot here - where do JS strings enter this (I'm not saying they are not, just being curious).

> Use int64_t.

Point taken, yet as this is automatically generated code, I would tend to think it should be addressed either in Flex itself or its input data (parser definition).

In addition, changing type from 32 bit to 64 might (I think) have unexpected side-effects once there is an assignment from <int64_t> to <int32_t>. IIRC the result (if the former doesn't fit the latter) is implementation defined. I'm not sure off the top of my head whether this really is "implementation defined" or "undefined behaviour", but even the first one doesn't sound like something desirable.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Seems it won't be fixed for 50.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Jeff and/or Milan, what's the status here?

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

For the most part, the code hasn't changed.  Tokenizer.cpp was updated to match what this patch has, the other two files have been left untouched.  Either way, what we have in the repo still doesn't look safe.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Sounds like this still affects 52/53.

Comment 17

[Stephanie Ouillon [:arroway] (needinfo me)]

Hi, what's new there since December? What are our options to move forward on this?

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Milan Sreckovic [:milan] from comment #11)
> (In reply to Jeff Gilbert [:jgilbert] from comment #7)
> > (In reply to Milan Sreckovic [:milan] from comment #5)
> > > Jeff, how should we land this?
> > 
> > Request sec-approval on the fixed patch, and land when the sec folks tell us
> > to.
> 
> I know that part :)  This is an ANGLE change - do we land in Gecko code,
> then request an upstream change, or do we wait for the upstream change, what
> do we do with our github branch of ANGLE code in the meantime, there is a
> few extra moving parts.

re-needinfo-ing jgilbert with Milan's question from comment 11

My take would be land the darn thing, and deal with the aftermath as needed....

Comment 19

[Julien Cristau [:jcristau]]

Looks like a wontfix for 52 again.  Milan, Jeff, can we get this landed before 53?

Comment 20

[Kelsey Gilbert [:jgilbert]]

I'll drive this through.

Comment 21

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8778238 [details] [diff] [review]
mozilla-flex_buffer_overrun.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
medium-hard

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
no

Which older supported branches are affected by this flaw?
all

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Shouldn't need separate backports. Trivial to write if needed.

How likely is this patch to cause regressions; how much testing does it need?
very low; has testing upstream.

Comment 22

[Al Billings [:abillings - ex-MoCo]]

Sec-approval+ for checkin on 3/21, two weeks into the new cycle. We're shipping Firefox 52 on 3/7.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2f5bc87d8cd

The Tokenizer.cpp hunk was unnecessary on trunk as that change appears to already be there. We'll need to take care to double-check other branches when doing uplifts rather than simply grafting what landed on m-c. Speaking of which, please request Aurora/Beta/ESR52/ESR45 approval on this as soon as possible :)

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c2f5bc87d8cd

Comment 25

[Gerry Chang [:gchang]]

Hi :jgilbert,
Since this bug also affects 53/54, do you think it's worth uplifting to 53/54 if this patch is not too risky?

Comment 26

[Kelsey Gilbert [:jgilbert]]

Fixed upstream:
https://github.com/google/angle/commit/93561c3a4c01a4014cfffb5cd6c917cc460ea0e8

Comment 27

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8778238 [details] [diff] [review]
mozilla-flex_buffer_overrun.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: sec-high
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: -
[User impact if declined]: sec bug
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: dev54, beta53, esr45, esr52
[Is the change risky?]: no
[Why is the change risky/not risky?]: well-understood
[String changes made/needed]: none

Comment 28

[Gerry Chang [:gchang]]

Comment on attachment 8778238 [details] [diff] [review]
mozilla-flex_buffer_overrun.patch

Fix a sec-high. Aurora54+ & Beta53+.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c1fe1aa002fe

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cf5e46f441ce

Comment 31

[Andrei Vaida [:avaida]]

I don't see anything actionable for manual QA. Setting qe-verify-.

Comment 32

[Julien Cristau [:jcristau]]

Comment on attachment 8778238 [details] [diff] [review]
mozilla-flex_buffer_overrun.patch

fix buffer overruns in flex-generated files, esr45+, esr52+

Note to whoever does the uplift to not just graft the changeset from m-c, see comment 23 (especially for esr45, I think).

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Hey Lee, I just wanted to give you a heads-up that I noticed there's still an unpatched version of this issue in Skia code:
https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/sksl/lex.sksl.c#1522

Talking to jgilbert about it, he didn't think it was likely to be dangerous to us, but I figured you'd want to know about it just in case.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/3dcc5f5c2df4
https://hg.mozilla.org/releases/mozilla-esr45/rev/c709d4b36145

Comment 35

[Lee Salzman [:lsalzman]]

Though this bug shouldn't currently affect the Skia milestone we use (m51), it might affect us in the future with newer Skia milestones. As such, I filed an upstream Skia security bug about it: https://bugs.chromium.org/p/skia/issues/detail?id=6447