1315905 - Cleanup Necko http security check

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Andrea Marchesini [:baku]]

Attachment: clean_necko.patch

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.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - necko check

Basically you've already reviewed this patch.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - remote mozBrowser

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 ]

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 1 - necko check

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 2 - fix tests

Comment 5

[Valentin Gosu [:valentin] (he/him)]

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?

Comment 6

[Valentin Gosu [:valentin] (he/him)]

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?

Comment 7

[Andrea Marchesini [:baku]]

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

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Let's just skip adding it to the crash reason. (maybe clone the OA without firstPartyDomain?) Otherwise the patch looks good.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 1 - necko check

anonymized first part domain

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 1 - necko check

Wrong patch, sorry for the spam.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 1 - necko check

Comment 12

[Valentin Gosu [:valentin] (he/him)]

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.

Comment 13

[Valentin Gosu [:valentin] (he/him)]

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.

Comment 14

[Valentin Gosu [:valentin] (he/him)]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/aa99928b7bb8
https://hg.mozilla.org/mozilla-central/rev/9c8cc3af1870

Comment 17

[areya]

https://hg.mozilla.org/mozilla-central/rev/aa99928b7bb8
https://hg.mozilla.org/mozilla-central/rev/9c8cc3af1870

Comment 19

[Marco Castelluccio [:marco]]

[Tracking Requested - why for this release]: Fixes crashes with the signatures in bug 1317611.

Comment 20

[Gerry Chang [:gchang]]

Track 51+/52+ as this can fix crash in bug 1317611.

Hi :baku,
Can you help nominate this uplift request to Beta51 and Aurora52?

Comment 21

[Andrea Marchesini [:baku]]

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

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8811236 [details] [diff] [review]
part 1 - necko check

Fix a crash. Beta51+ and Aurora52+. Should be in 51 beta 3.

Comment 23

[Carsten Book [:Tomcat]]

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')

Comment 24

[Andrea Marchesini [:baku]]

Attachment: part 1 - m-a

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8342b7cca4bc
https://hg.mozilla.org/releases/mozilla-aurora/rev/cd6fe5a803f3

Comment 26

[Carsten Book [:Tomcat]]

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')

Comment 27

[Andrea Marchesini [:baku]]

Attachment: m-b

Comment 28

[Andrea Marchesini [:baku]]

Attachment: m-a fix

Comment 29

[Andrea Marchesini [:baku]]

Attachment: m-a fix 2

Comment 30

[Carsten Book [:Tomcat]]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/3722df768f32768071ab28c8d0880194cf80551b
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/faf338a9891a7af3a9c128c634f0a9e958bfb2da

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a5cf41cc6a0d
https://hg.mozilla.org/releases/mozilla-beta/rev/ed3bae0b42fb
https://hg.mozilla.org/releases/mozilla-beta/rev/823df64d0f50
https://hg.mozilla.org/releases/mozilla-beta/rev/9f89f9027a35

Comment 32

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

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.

Comment 33

[Andrea Marchesini [:baku]]

Attachment: patch for m-a (but also for m-b and m-c)

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/369bbb5eb097
Cleanup Necko http security check, r=valentin

Comment 35

[Andrea Marchesini [:baku]]

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.

Comment 36

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/369bbb5eb097

Comment 37

[Gerry Chang [:gchang]]

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.

Comment 38

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

I'm hitting conflicts uplifting this to aurora.

Comment 39

[Andrea Marchesini [:baku]]

Attachment: m-a

This patch contains is the only patch to land to m-a.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

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

Comment 41

[Carsten Book [:Tomcat]]

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')

Comment 42

[Andrea Marchesini [:baku]]

Do we still need to uplift? Is that beta the new release?

Comment 43

[Carsten Book [:Tomcat]]

i think we still need this in beta gchang ?

Comment 44

[Gerry Chang [:gchang]]

Hi Tomcat, 
Yes, please help to uplift this to beta.

Comment 45

[Carsten Book [:Tomcat]]

baku: ok seems we still need a rebased patch for beta

Comment 46

[Andrea Marchesini [:baku]]

This works fine on beta: https://bugzilla.mozilla.org/attachment.cgi?id=8814312

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/83da1807aafab6fe395af4a71085ec70eb009b96