Bug 1292534 (CVE-2017-5469)

flex: buffer overflow in generated code

RESOLVED FIXED in Firefox -esr45

Status

()

defect
P1
major
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: hrosik, Assigned: jgilbert)

Tracking

({csectype-intoverflow, sec-high})

unspecified
mozilla55
All
Unspecified
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox49+ wontfix, firefox-esr4553+ fixed, firefox50+ wontfix, firefox51+ wontfix, firefox52+ wontfix, firefox-esr5253+ fixed, firefox53+ fixed, firefox54+ fixed, firefox55+ fixed)

Details

(Whiteboard: [gfx-noted][adv-main53+][adv-esr52.1+][adv-esr45.9+])

Attachments

(1 attachment)

Reporter

Description

3 years ago
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.
Alias: CVE-2016-6354
Status: UNCONFIRMED → NEW
Component: Security → Canvas: WebGL
Ever confirmed: true
Group: core-security → core-security-release
making sure this is on milan's radar
Flags: needinfo?(milan)
Petr, do you know if this has been filed against ANGLE itself?
Flags: needinfo?(milan) → needinfo?(pcerny)
Reporter

Comment 3

3 years ago
(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.
Flags: needinfo?(pcerny)
rating sec-high assuming this bug can be triggered from web content.
Jeff, how should we land this?
Flags: needinfo?(jgilbert)
Assignee

Comment 6

3 years ago
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);
Attachment #8778238 - Flags: review?(jgilbert)
Attachment #8778238 - Flags: review-
Attachment #8778238 - Flags: review+
Assignee

Comment 7

3 years ago
(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.
Flags: needinfo?(jgilbert)
Petr, are you still working on the fix? We are going to release 49 & ES45.4 pretty soon.
Flags: needinfo?(pcerny)
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?
Flags: needinfo?(milan)
(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.
Flags: needinfo?(milan) → needinfo?(jgilbert)
Reporter

Comment 12

3 years ago
(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.
Flags: needinfo?(pcerny)
Jeff and/or Milan, what's the status here?
Flags: needinfo?(milan)
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.
Sounds like this still affects 52/53.
Flags: needinfo?(milan)
Whiteboard: [gfx-noted]
Hi, what's new there since December? What are our options to move forward on this?
Flags: needinfo?(milan)
Flags: needinfo?(jgilbert)
(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....
Flags: needinfo?(jgilbert)
Looks like a wontfix for 52 again.  Milan, Jeff, can we get this landed before 53?
Assignee

Comment 20

2 years ago
I'll drive this through.
Assignee: nobody → jgilbert
Flags: needinfo?(jgilbert)
Priority: -- → P1
Assignee

Updated

2 years ago
Flags: needinfo?(milan)
Assignee

Comment 21

2 years ago
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.
Attachment #8778238 - Flags: sec-approval?
Sec-approval+ for checkin on 3/21, two weeks into the new cycle. We're shipping Firefox 52 on 3/7.
Whiteboard: [gfx-noted] → [gfx-noted][checkin on 3/21]
Attachment #8778238 - Flags: sec-approval? → sec-approval+
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 :)
Flags: needinfo?(jgilbert)
Whiteboard: [gfx-noted][checkin on 3/21] → [gfx-noted]
https://hg.mozilla.org/mozilla-central/rev/c2f5bc87d8cd
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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?
Assignee

Comment 27

2 years ago
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
Flags: needinfo?(jgilbert)
Attachment #8778238 - Flags: review-
Attachment #8778238 - Flags: approval-mozilla-esr52?
Attachment #8778238 - Flags: approval-mozilla-esr45?
Attachment #8778238 - Flags: approval-mozilla-beta?
Attachment #8778238 - Flags: approval-mozilla-aurora?
Comment on attachment 8778238 [details] [diff] [review]
mozilla-flex_buffer_overrun.patch

Fix a sec-high. Aurora54+ & Beta53+.
Attachment #8778238 - Flags: approval-mozilla-beta?
Attachment #8778238 - Flags: approval-mozilla-beta+
Attachment #8778238 - Flags: approval-mozilla-aurora?
Attachment #8778238 - Flags: approval-mozilla-aurora+
I don't see anything actionable for manual QA. Setting qe-verify-.
Flags: qe-verify-
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).
Attachment #8778238 - Flags: approval-mozilla-esr52?
Attachment #8778238 - Flags: approval-mozilla-esr52+
Attachment #8778238 - Flags: approval-mozilla-esr45?
Attachment #8778238 - Flags: approval-mozilla-esr45+
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.
Flags: needinfo?(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
Flags: needinfo?(lsalzman)
Whiteboard: [gfx-noted] → [gfx-noted][adv-main53+][adv-esr52.1+]
Whiteboard: [gfx-noted][adv-main53+][adv-esr52.1+] → [gfx-noted][adv-main53+][adv-esr52.1+][adv-esr45.9+]
Alias: CVE-2016-6354 → CVE-2017-5469
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.