Closed Bug 1315905 Opened 3 years ago Closed 3 years ago

Cleanup Necko http security check

Categories

(Core :: Networking, defect)

50 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 + fixed
firefox52 + fixed
firefox53 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

(Whiteboard: [necko-active][OA])

Attachments

(3 files, 11 obsolete files)

m-b
13.65 KB, patch
Details | Diff | Splinter Review
2.66 KB, patch
valentin
: review+
Details | Diff | Splinter Review
m-a
50.07 KB, patch
Details | Diff | Splinter Review
Attached patch clean_necko.patch (obsolete) — Splinter Review
Instead comparing and setting each attribute of OA, this check seems cleaner.
I kept appId check separately because this is going to be removed soon in other bugs.
Attachment #8808540 - Flags: review?(valentin.gosu)
Attachment #8808540 - Flags: review?(valentin.gosu) → review+
Whiteboard: [necko-active]
Attached patch part 1 - necko check (obsolete) — Splinter Review
Basically you've already reviewed this patch.
Attachment #8808540 - Attachment is obsolete: true
Attachment #8810753 - Flags: review?(valentin.gosu)
Attached patch part 2 - remote mozBrowser (obsolete) — Splinter Review
In our tests, and just for b2g, we create mozbrowser in the child process. These iframe has isolateMozBrowser flag set to true, but the tabContext, in the parent, doesn't. This because there is this scenario:

[ parent: necko ] -> [ child: normal tabContext -> mozBrowser tab ]
Attachment #8810754 - Flags: review?(valentin.gosu)
Attachment #8810753 - Flags: review?(valentin.gosu)
Attachment #8810754 - Flags: review?(valentin.gosu)
Attached patch part 1 - necko check (obsolete) — Splinter Review
Attachment #8810753 - Attachment is obsolete: true
Attachment #8811171 - Flags: review?(valentin.gosu)
Attached patch part 2 - fix tests (obsolete) — Splinter Review
Attachment #8810754 - Attachment is obsolete: true
Attachment #8811172 - Flags: review?(valentin.gosu)
Comment on attachment 8811172 [details] [diff] [review]
part 2 - fix tests

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

Did these tests pass before? If so, please explain why.

::: dom/events/test/test_bug1096146.html
@@ +153,5 @@
>        { "set": [["dom.beforeAfterKeyboardEvent.enabled", true],
>                  ["dom.mozBrowserFramesEnabled", true],
> +                ["network.disable.ipc.security", true],
> +                ["dom.ipc.tabs.disabled", false],
> +                ["network.disable.ipc.security", true]] },

Any reason for doing this twice?
Attachment #8811172 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8811171 [details] [diff] [review]
part 1 - necko check

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

::: netwerk/ipc/NeckoParent.cpp
@@ +197,5 @@
>        debugString.Append("(");
> +
> +      nsAutoCString serializedSuffix;
> +      aSerialized.mOriginAttributes.CreateSuffix(serializedSuffix);
> +      debugString.Append(serializedSuffix);

Wouldn't this lead to appending the firstPartyDomain to the crash reason?
> Wouldn't this lead to appending the firstPartyDomain to the crash reason?

Yes, it does. Wondering if firstPartDomain should be an hash (sha1, md5, whatever) instead being the real domain as string.
Let's just skip adding it to the crash reason. (maybe clone the OA without firstPartyDomain?) Otherwise the patch looks good.
Attached patch part 1 - necko check (obsolete) — Splinter Review
anonymized first part domain
Attachment #8811171 - Attachment is obsolete: true
Attachment #8811171 - Flags: review?(valentin.gosu)
Attachment #8811225 - Flags: review?(valentin.gosu)
Attached patch part 1 - necko check (obsolete) — Splinter Review
Wrong patch, sorry for the spam.
Attachment #8811225 - Attachment is obsolete: true
Attachment #8811225 - Flags: review?(valentin.gosu)
Attachment #8811226 - Flags: review?(valentin.gosu)
Attached patch part 1 - necko check (obsolete) — Splinter Review
Attachment #8811226 - Attachment is obsolete: true
Attachment #8811226 - Flags: review?(valentin.gosu)
Attachment #8811236 - Flags: review?(valentin.gosu)
Comment on attachment 8811226 [details] [diff] [review]
part 1 - necko check

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

Looks good. r=me with one minor nit.

::: netwerk/ipc/NeckoParent.cpp
@@ +197,5 @@
>        debugString.Append("(");
> +
> +      nsAutoCString serializedSuffix;
> +      aSerialized.mOriginAttributes.CreateAnonymizedSuffix(serializedSuffix);
> +      debugString.Append(serializedSuffix);

We don't really need to append aSerialized more than once. You can move this outside the loop.
Attachment #8811226 - Attachment is obsolete: false
Comment on attachment 8811236 [details] [diff] [review]
part 1 - necko check

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

r+ with comment 12 and comment 6 addressed.
Attachment #8811236 - Flags: review?(valentin.gosu) → review+
(In reply to Valentin Gosu [:valentin] from comment #13)
> Review of attachment 8811236 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comment 12 and comment 6 addressed.

I meant comment 12 and comment 5.
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa99928b7bb8
Cleanup Necko http security check - part 1, r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c8cc3af1870
Cleanup Necko http security check - part 2 - tests, r=valentin
Whiteboard: [necko-active] → [necko-active][OA]
https://hg.mozilla.org/mozilla-central/rev/aa99928b7bb8
https://hg.mozilla.org/mozilla-central/rev/9c8cc3af1870
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Duplicate of this bug: 1317611
[Tracking Requested - why for this release]: Fixes crashes with the signatures in bug 1317611.
Track 51+/52+ as this can fix crash in bug 1317611.

Hi :baku,
Can you help nominate this uplift request to Beta51 and Aurora52?
Flags: needinfo?(amarchesini)
Comment on attachment 8811236 [details] [diff] [review]
part 1 - necko check

Approval Request Comment
[Feature/regressing bug #]: Necko security check and the introduction of OriginAttributes
[User impact if declined]: A crash can happen: race condition.
[Describe test coverage new/current, TreeHerder]: almost impossible to reproduce with a test.
[Risks and why]: This patch improves the security check in necko about what is allowed and what is not to be send via HttpChannel by a child process. The patch is not too complex. I don't think there are risks in having in in beta. 
[String/UUID change made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8811236 - Flags: approval-mozilla-beta?
Attachment #8811236 - Flags: approval-mozilla-aurora?
Comment on attachment 8811236 [details] [diff] [review]
part 1 - necko check

Fix a crash. Beta51+ and Aurora52+. Should be in 51 beta 3.
Attachment #8811236 - Flags: approval-mozilla-beta?
Attachment #8811236 - Flags: approval-mozilla-beta+
Attachment #8811236 - Flags: approval-mozilla-aurora?
Attachment #8811236 - Flags: approval-mozilla-aurora+
needs rebasing for uplift 

grafting 375554:aa99928b7bb8 "Bug 1315905 - Cleanup Necko http security check - part 1, r=valentin"
merging netwerk/ipc/NeckoParent.cpp
merging netwerk/ipc/NeckoParent.h
warning: conflicts while merging netwerk/ipc/NeckoParent.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(amarchesini)
Attached patch part 1 - m-a (obsolete) — Splinter Review
Flags: needinfo?(amarchesini)
Attachment #8814044 - Attachment is patch: true
also needs to rebased for beta it seems

grafting 376922:8342b7cca4bc "Bug 1315905 - Cleanup Necko http security check - part 1, r=valentin, a=gchang"
merging caps/BasePrincipal.cpp
merging caps/BasePrincipal.h
merging dom/base/ChromeUtils.cpp
merging netwerk/ipc/NeckoParent.cpp
merging netwerk/ipc/NeckoParent.h
warning: conflicts while merging netwerk/ipc/NeckoParent.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(amarchesini)
Attached patch m-bSplinter Review
Flags: needinfo?(amarchesini)
Attached patch m-a fix (obsolete) — Splinter Review
Attached patch m-a fix 2 (obsolete) — Splinter Review
Attachment #8814061 - Attachment is patch: true
Attachment #8814100 - Attachment is patch: true
Backed out of aurora because the assertions were still happening after the followups:

https://hg.mozilla.org/releases/mozilla-aurora/rev/55b599ca3530c8286f6ad15405d99d2bcaecfbd1

Beta seems fine.
Flags: needinfo?(amarchesini)
Flags: needinfo?(amarchesini)
Attachment #8814312 - Flags: review?(valentin.gosu)
Attachment #8814312 - Flags: review?(valentin.gosu) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/369bbb5eb097
Cleanup Necko http security check, r=valentin
Comment on attachment 8814312 [details] [diff] [review]
patch for m-a (but also for m-b and m-c)

This patch should land in m-a and m-b. It fixes the intermittent failures.
Attachment #8814312 - Flags: approval-mozilla-beta?
Attachment #8814312 - Flags: approval-mozilla-aurora?
Comment on attachment 8814312 [details] [diff] [review]
patch for m-a (but also for m-b and m-c)

Fix an intermittent failures. Beta51+ and Aurora52+. Should be in 51 beta 4.
Attachment #8814312 - Flags: approval-mozilla-beta?
Attachment #8814312 - Flags: approval-mozilla-beta+
Attachment #8814312 - Flags: approval-mozilla-aurora?
Attachment #8814312 - Flags: approval-mozilla-aurora+
I'm hitting conflicts uplifting this to aurora.
Flags: needinfo?(amarchesini)
Attached patch m-aSplinter Review
This patch contains is the only patch to land to m-a.
Attachment #8811172 - Attachment is obsolete: true
Attachment #8811226 - Attachment is obsolete: true
Attachment #8811236 - Attachment is obsolete: true
Attachment #8814044 - Attachment is obsolete: true
Attachment #8814100 - Attachment is obsolete: true
Attachment #8814106 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Attachment #8816087 - Attachment is patch: true
needs rebasing for beta

grafting 378431:e1a57d1dcfdb "Bug 1315905 - Cleanup Necko http security check. r=valentin, a=gchang"
merging caps/BasePrincipal.cpp
merging caps/BasePrincipal.h
merging dom/base/ChromeUtils.cpp
merging dom/browser-element/mochitest/browserElementTestHelpers.js
merging dom/devicestorage/test/test_app_permissions.html
merging dom/devicestorage/test/test_fs_app_permissions.html
merging dom/events/test/test_bug1096146.html
merging dom/presentation/tests/mochitest/test_presentation_1ua_sender_and_receiver.js
merging dom/presentation/tests/mochitest/test_presentation_dc_receiver.html
merging dom/presentation/tests/mochitest/test_presentation_dc_receiver_oop.html
merging dom/presentation/tests/mochitest/test_presentation_tcp_receiver.html
merging dom/presentation/tests/mochitest/test_presentation_tcp_receiver_oop.html
merging dom/presentation/tests/mochitest/test_presentation_terminate.js
merging dom/presentation/tests/mochitest/test_presentation_terminate_establish_connection_error.js
merging netwerk/ipc/NeckoParent.cpp
merging netwerk/ipc/NeckoParent.h
warning: conflicts while merging dom/events/test/test_bug1096146.html! (edit, then use 'hg resolve --mark')
warning: conflicts while merging netwerk/ipc/NeckoParent.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(amarchesini)
Do we still need to uplift? Is that beta the new release?
Flags: needinfo?(amarchesini) → needinfo?(cbook)
i think we still need this in beta gchang ?
Flags: needinfo?(cbook) → needinfo?(gchang)
Hi Tomcat, 
Yes, please help to uplift this to beta.
Flags: needinfo?(gchang) → needinfo?(cbook)
baku: ok seems we still need a rebased patch for beta
Flags: needinfo?(cbook) → needinfo?(amarchesini)
This works fine on beta: https://bugzilla.mozilla.org/attachment.cgi?id=8814312
Flags: needinfo?(amarchesini)
You need to log in before you can comment on or make changes to this bug.