1215723 - Make IsSecureURI available in the child process

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[(no longer active)]

For what I'm planning to do in bug 1214305, I'm trying to make IsSecureURI available in the child process so that we can avoid the trip to the parent process every time we want to decide whether a channel needs to be intercepted.

This doesn't mean that we will make any security decision in the child process, since we will only use this information to decide what URL to use when dispatching the FetchEvent to the service worker, but I just discovered bug 948574 where it seems we very intentionally decided to not have this information mirrored in the child process.  I agree that we should not use the information in the child for displaying the certerror, but want to double check whether making IsSecureURI available in the child process is OK.

David, what do you think?

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I imagine this could work. What would the approach be? Have the HSTS implementation emit events to the child process when any of that information changes?

Comment 2

[(no longer active)]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #1)
> I imagine this could work. What would the approach be? Have the HSTS
> implementation emit events to the child process when any of that information
> changes?

Yes.

Comment 3

[(no longer active)]

Attachment: Part 1: Make DataStorage a singleton for each file name

This is needed so that we'd be able to identify a DataStorage instance
based on its file name.

Comment 4

[(no longer active)]

Attachment: Part 2: Initialize DataStorage items in the content process from the data in the parent

Comment 5

[(no longer active)]

Attachment: Part 3: Propagate updates to DataStorage from the parent process to the content processes

Comment 6

[(no longer active)]

Attachment: Part 4: Make isSecureHost and isSecureURI usable from the content process

Comment 7

[(no longer active)]

Attachment: Part 5: Add an automated test

Comment 8

[:Cykesiopka]

Comment on attachment 8681538 [details] [diff] [review]
Part 5: Add an automated test

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

Minor drive by: It would be nice if the Assert.jsm equivalents of the do_check_* methods could be used, but no big deal.

Comment 9

[(no longer active)]

(In reply to Cykesiopka from comment #8)
> Minor drive by: It would be nice if the Assert.jsm equivalents of the
> do_check_* methods could be used, but no big deal.

I copied what the other tests in this directory were using.

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8681534 [details] [diff] [review]
Part 1: Make DataStorage a singleton for each file name

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

Looks good. Just the one nit.

::: security/manager/ssl/DataStorage.h
@@ +15,5 @@
>  #include "nsIThread.h"
>  #include "nsITimer.h"
>  #include "nsString.h"
> +#include "nsRefPtrHashtable.h"
> +#include "mozilla/StaticPtr.h"

nit: let's sort this one with the rest of the mozilla/ includes ^
(and also, I think nsRefPtrHashtable.h would sort earlier than nsString.h)

Comment 11

[(no longer active)]

Attachment: Part 1: Make DataStorage a singleton for each file name

Addressed the review comment, and also fixed a leak discovered on the try server by using ClearOnShutdown() to delete the singleton.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8681535 [details] [diff] [review]
Part 2: Initialize DataStorage items in the content process from the data in the parent

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

Great - r=me with comments addressed.

::: security/manager/ssl/DataStorage.cpp
@@ +18,5 @@
>  #include "nsStreamUtils.h"
>  #include "nsThreadUtils.h"
>  #include "prprf.h"
> +#include "mozilla/dom/PContent.h"
> +#include "mozilla/dom/ContentChild.h"

nit: let's sort these in with the rest

@@ +80,5 @@
>  
>    MutexAutoLock lock(mMutex);
>  
> +  // Ignore attempts to initialize several times.
> +  if (mReady) {

mReady isn't protected by mMutex but by mReadyMonitor (and the two shouldn't be acquired at the same time). Also, Init can finish before mReady is true, so in theory it still could be called more than once. Maybe a better approach would be to add a boolean mInitCalled and check it instead.

@@ +100,5 @@
> +    // In the child process, we ask the parent process for the data.
> +    MOZ_ASSERT(XRE_IsContentProcess());
> +    aDataWillPersist = false;
> +    InfallibleTArray<DataStorageItem> items;
> +    dom::ContentChild::GetSingleton()->SendReadDataStorageArray(mFilename, &items);

nit: looks like this line is a bit long (<80 chars is the goal)

@@ +739,5 @@
>  DataStorage::SetTimer()
>  {
>    MOZ_ASSERT(!NS_IsMainThread());
> +
> +  if (!XRE_IsParentProcess()) {

This should be an assertion, right? (as in, MOZ_ASSERT(XRE_IsParentProcess()))

::: security/manager/ssl/DataStorageIPCUtils.h
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_DataStorageIPCUtils_h
> +#define mozilla_DataStorageIPCUtils_h

nit: if I understand https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style correctly, since this is exported to ipc, wouldn't this be "ipc_DataStorageIPCUtils_h"?

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8681536 [details] [diff] [review]
Part 3: Propagate updates to DataStorage from the parent process to the content processes

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

Cool. Just some minor nits.

::: security/manager/ssl/DataStorage.cpp
@@ +575,5 @@
>      return rv;
>    }
>  
> +  RunOnAllContentParents([&](dom::ContentParent* aParent) {
> +      DataStorageItem item;

nit: two space indentation

@@ +614,5 @@
>      unused << AsyncSetTimer(lock);
>    }
> +
> +  RunOnAllContentParents([&](dom::ContentParent* aParent) {
> +      unused << aParent->SendDataStorageRemove(mFilename, aKey, aType);

nit: indentation

@@ +745,5 @@
>      }
>    }
> +
> +  RunOnAllContentParents([&](dom::ContentParent* aParent) {
> +      unused << aParent->SendDataStorageClear(mFilename);

nit: indentation

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8681538 [details] [diff] [review]
Part 5: Add an automated test

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

This looks good. We can convert all of these tests to use Assert.jsm in a follow-up bug.

::: security/manager/ssl/tests/unit/sss_readstate_child_worker.js
@@ +1,3 @@
> +function run_test() {
> +  var SSService = Cc["@mozilla.org/ssservice;1"]
> +                  .getService(Ci.nsISiteSecurityService);

nit: generally in these tests, the leading '.' is lined up with the '[' on the previous line in this kind of situation

::: security/manager/ssl/tests/unit/test_sss_readstate_child.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// The purpose of this test is to create a site security service state file
> +// and see that the site security service reads it properly.

It would be nice if this comment mentioned that we're checking that the state gets reflected in the child properly.

@@ +33,5 @@
> +  outputStream.close();
> +  Services.obs.addObserver(start_test_in_child, "data-storage-ready", false);
> +  do_test_pending();
> +  var SSService = Cc["@mozilla.org/ssservice;1"]
> +                  .getService(Ci.nsISiteSecurityService);

nit: again with the '.'/'[' alignment

Comment 15

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/228cdfaa82c15c4f2b21c16d618148d702b397a5
Bug 1215723 - Part 1: Make DataStorage a singleton for each file name; r=keeler

https://hg.mozilla.org/integration/mozilla-inbound/rev/b31ac98bb3c8e4f6ea4e9e294eafc9e8ba349e24
Bug 1215723 - Part 2: Initialize DataStorage items in the content process from the data in the parent; r=keeler

https://hg.mozilla.org/integration/mozilla-inbound/rev/62dbb95bd79a40984600fdf3dc050efe4c1f17b8
Bug 1215723 - Part 3: Propagate updates to DataStorage from the parent process to the content processes; r=keeler

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd7f58b60ddcbbf9d6fb2f63359c6ff1fffa64fb
Bug 1215723 - Part 4: Make isSecureHost and isSecureURI usable from the content process; r=keeler

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a945ce1cd40563ab6cd02899d375a58df6e0c14
Bug 1215723 - Part 5: Add an automated test; r=keeler

Comment 16

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

All backed out for android s4 bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=16953602&repo=mozilla-inbound

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

Comment 17

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1205b5fdc13d0301099597f38fe86dbd6f0b24ba
Bug 1215723 - Part 1: Make DataStorage a singleton for each file name; r=keeler

https://hg.mozilla.org/integration/mozilla-inbound/rev/671540abe891b29d758ed75f6bfae1f0f7fca1e6
Bug 1215723 - Part 2: Initialize DataStorage items in the content process from the data in the parent; r=keeler

https://hg.mozilla.org/integration/mozilla-inbound/rev/81a5f140ea34afa1417f6c19324c1ede06cbdea5
Bug 1215723 - Part 3: Propagate updates to DataStorage from the parent process to the content processes; r=keeler

https://hg.mozilla.org/integration/mozilla-inbound/rev/b35d2c16759cd1231c6ddac4900ed9da6a72095a
Bug 1215723 - Part 4: Make isSecureHost and isSecureURI usable from the content process; r=keeler

https://hg.mozilla.org/integration/mozilla-inbound/rev/cec659ec5130280e2ac263ccc6ff822d212c6c1d
Bug 1215723 - Part 5: Add an automated test; r=keeler

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1205b5fdc13d
https://hg.mozilla.org/mozilla-central/rev/671540abe891
https://hg.mozilla.org/mozilla-central/rev/81a5f140ea34
https://hg.mozilla.org/mozilla-central/rev/b35d2c16759c
https://hg.mozilla.org/mozilla-central/rev/cec659ec5130