1493629 - AddressSanitizer: heap-use-after-free [@ fetch_sub] with WRITE of size 8 in WorkerCSPEventListener

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Christian Holler (:decoder)]

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 64.0a1-20180920220102-https://hg.mozilla.org/mozilla-central/rev/5347c7e4811a1a4d80bcee4119cead9192fdff69.

For detailed crash information, see attachment.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 3

[Christian Holler (:decoder)]

Needinfo for :baku, could you take a look at this since you are the author of WorkerCSPEventListener? Thanks!

Comment 4

[Daniel Veditz [:dveditz]]

Assigning to :baku as a likely victim, but if it's not you please hand it off to a specific someone to look at this sec-high bug rather than dropping it back to "nobody".

Comment 5

[Andrea Marchesini [:baku]]

Attachment: csp.patch

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 9013920 [details] [diff] [review]
csp.patch

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

I guess that works, thanks. r=ckerschb

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 9013920 [details] [diff] [review]
csp.patch

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Not easy at all. It's use-after-free hard to reproduce.

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?: m-b

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

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: Easy

How likely is this patch to cause regressions; how much testing does it need?: None. Note that this could be not enough to fix for the crash: the patch adds security checks in the CSP EventListener creation, but maybe the bug is elsewhere.

Comment 9

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

We only have two betas left. This bug will need release management approval, so we can backport to affected branches, before I can give sec-approval.

Ritu?

We should take this since it only affects mozilla-central and beta.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Yes, we'll take it still.

Comment 11

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

Comment on attachment 9013920 [details] [diff] [review]
csp.patch

sec-approval+. Please nominate branch patches as well.

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 9013920 [details] [diff] [review]
csp.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1472927

User impact if declined: a crash can occur

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: none

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This patch introduces some security checks on how CSP worker event listener is used.
No big changes.

String changes made/needed: none

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dae89dbd2159c0b2a5865389dadab9a8f938e8ee
https://hg.mozilla.org/mozilla-central/rev/dae89dbd2159

Comment 14

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9013920 [details] [diff] [review]
csp.patch

Uplift approved for 63 beta 13, thanks.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/01237eeedfac

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out from beta for wpt failures on ContentSecurityPolicy:

https://hg.mozilla.org/releases/mozilla-beta/rev/9671f6ac6cbeae04e605a35ebda003d1006e2af5

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&resultStatus=usercancel%2Crunnable%2Ctestfailed%2Cbusted%2Cexception%2Cretry&group_state=expanded&revision=01237eeedfac765312a1322b28719782c3a71731

debug failure: https://treeherder.mozilla.org/logviewer.html#?job_id=203677584&repo=mozilla-beta

[task 2018-10-05T15:56:30.194Z] 15:56:30     INFO - TEST-PASS | /content-security-policy/inside-worker/dedicated-inheritance.html | Filesystem and blob. 
[task 2018-10-05T15:56:30.195Z] 15:56:30     INFO - TEST-PASS | /content-security-policy/inside-worker/dedicated-inheritance.html | Same-origin 'fetch()' in http:?pipe=sub|header(Content-Security-Policy,default-src%20*) 
[task 2018-10-05T15:56:30.195Z] 15:56:30     INFO - TEST-UNEXPECTED-TIMEOUT | /content-security-policy/inside-worker/dedicated-inheritance.html | Same-origin 'fetch()' in http:?pipe=sub|header(Content-Security-Policy,connect-src%20%27none%27) - Test timed out
[task 2018-10-05T15:56:30.195Z] 15:56:30     INFO - TEST-INFO | expected FAIL

Please also check the Nightly and DevEdition wpt failures (there are 2 per platform compared to 1 on debug).

Comment 18

[Andrea Marchesini [:baku]]

We need to uplift bug 1490165 as well.

Comment 19

[Andrea Marchesini [:baku]]

Actually, instead of uplifting the 3 patches of bug 1490165, I can create a custom patch for m-b. This is better because we don't need all those 3 patches from bug 1490165.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: m-b

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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