1221365 - Move "Is origin potentially trustworthy?" logic outside ServiceWorkerManager.cpp and expose to JS

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Matthew N. [:MattN]]

An initial version of the logic at https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy was implemented inside ServiceWorkerManager.cpp as a short-term workaround. Since there are other components (e.g. InsecurePasswordUtils.jsm and WebRTC) that want to use the same logic we should move it to its own method which is also exposed to JS.

Comment 1

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1221365 - Move "Is origin potentially trustworthy?" logic outside ServiceWorkerManager.cpp. r=ckerschb r?bkelly

Bug 1221365 - Move "Is origin potentially trustworthy?" logic outside ServiceWorkerManager.cpp. r=ckerschb,bkelly

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8682933 [details]
MozReview Request: Bug 1221365 - Move "Is origin potentially trustworthy?" logic outside ServiceWorkerManager.cpp. r=ckerschb r?bkelly

https://reviewboard.mozilla.org/r/24177/#review21643

Looks pretty good, just fix up my nits please.

::: dom/security/nsContentSecurityManager.cpp:399
(Diff revision 1)
> +{

Nit: please change aRetVal to aIsTrustWorthy
and make sure that aURI is non null, I think it's:
NS_ENSURE_ARG_POINTER(aURI)

::: dom/security/nsContentSecurityManager.cpp:403
(Diff revision 1)
> +    nsAutoCString scheme;

no need for those if checks, and please use early returns.

::: dom/security/nsContentSecurityManager.cpp:405
(Diff revision 1)
> +    if (NS_WARN_IF(NS_FAILED(rv))) {

just use NS_ENSURE_SUCCESS(rv, rv);
applies to the rest of the patch as well unless you really do need that warning.

::: dom/security/nsContentSecurityManager.cpp:412
(Diff revision 1)
> +      *aRetVal = true;

return NS_OK and then no need for those additional if checks underneath.

Comment 3

[Matthew N. [:MattN]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> Looks pretty good, just fix up my nits please.

Btw. I was leaving the existing style so it could easily be inter-diffed to see that I'm not changing the logic. I agree that the style was a bit unusual.

> ::: dom/security/nsContentSecurityManager.cpp:405
> (Diff revision 1)
> > +    if (NS_WARN_IF(NS_FAILED(rv))) {
> 
> just use NS_ENSURE_SUCCESS(rv, rv);
> applies to the rest of the patch as well unless you really do need that
> warning.

My understanding is that NS_ENSURE_SUCCESS is frowned upon compared to explicitly returning:
if (NS_FAILED(rv)) {
  return rv;
}

I don't feel strongly about the warning though.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Matthew N. [:MattN] from comment #3)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> > Looks pretty good, just fix up my nits please.
> 
> Btw. I was leaving the existing style so it could easily be inter-diffed to
> see that I'm not changing the logic. I agree that the style was a bit
> unusual.

I don't feel strongly to be honest. I just thought since we are already fixing it up we might as well fix up the coding style and use early returns :-)

> My understanding is that NS_ENSURE_SUCCESS is frowned upon compared to
> explicitly returning:
> if (NS_FAILED(rv)) {
>   return rv;
> }

I am pretty sure you can use them interchangeably.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8682933 [details]
MozReview Request: Bug 1221365 - Move "Is origin potentially trustworthy?" logic outside ServiceWorkerManager.cpp. r=ckerschb r?bkelly

https://reviewboard.mozilla.org/r/24177/#review21673

r=me with comments addressed.  Thanks.

::: dom/security/nsContentSecurityManager.cpp:399
(Diff revision 1)
> +{

Also NS_ENSURE_ARG_POINTER(aRetVal);

Also, please assert main thread to clarify when this can be used.

::: dom/security/nsContentSecurityManager.cpp:431
(Diff revision 1)
> +    bool isFile;

Please initialize isFile and isHttps to false.

::: dom/workers/ServiceWorkerManager.cpp:1407
(Diff revision 1)
> +    if (NS_FAILED(rv) || !trustworthyURI) {

NS_WARN_IF() please.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> I don't feel strongly to be honest. I just thought since we are already
> fixing it up we might as well fix up the coding style and use early returns
> :-)

Yea, I would suggest fixing the style to match NS_ENSURE_* usage in nsContentSecurityManager.  I'm pretty sure Jonas strongly prefers that style.

Comment 7

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1221365 - Tests for "Is origin potentially trustworthy?" logic. r=ckerschb,bkelly

Bug 1221365 - Tests for "Is origin potentially trustworthy?" logic. r=ckerschb,bkelly

Comment 8

[:Paolo Amadini]

Matt, I've added a very simple xpcshell test for the function. It's based on your exact MozReview commit so you should be able to pull it from the MozReview repository and fold/rebase/evolve as necessary.

hg pull -r 823385bba1a58d914be2c893a3b0d44aa282ee91 https://reviewboard-hg.mozilla.org/gecko

Comment 9

[Matthew N. [:MattN]]

(In reply to :Paolo Amadini from comment #7)
> Created attachment 8683259 [details]
> MozReview Request: Bug 1221365 - Tests for "Is origin potentially
> trustworthy?" logic. r=ckerschb,bkelly

Thanks for the tests! I was going to write some today.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8683259 [details]
MozReview Request: Bug 1221365 - Tests for "Is origin potentially trustworthy?" logic. r=ckerschb,bkelly

https://reviewboard.mozilla.org/r/24243/#review21727

::: dom/security/test/unit/xpcshell.ini:8
(Diff revision 1)
> +[test_isURIPotentiallyTrustworthy.js]

Thanks, looks good to me

Comment 11

[Tanvi Vyas[:tanvi]]

https://reviewboard.mozilla.org/r/24177/#review21767

::: dom/security/nsContentSecurityManager.cpp:409
(Diff revision 1)
> +    if (scheme.EqualsLiteral("https") ||

Do we need to add wss here?

::: dom/security/nsContentSecurityManager.cpp:438
(Diff revision 1)
> +      rv = aURI->SchemeIs("https", &isHttps);

Why is there a second https check here?

::: dom/workers/ServiceWorkerManager.cpp:1404
(Diff revision 1)
> -    if (!IsFromAuthenticatedOriginInternal(doc)) {
> +    nsCOMPtr<nsIURI> documentURI = doc->GetDocumentURI();

As a followup, someone working on ServiceWorker code should consider whether they really want documentURI here or the document's principal URI.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tanvi Vyas [:tanvi] from comment #11)
> ::: dom/security/nsContentSecurityManager.cpp:409
> (Diff revision 1)
> > +    if (scheme.EqualsLiteral("https") ||
> 
> Do we need to add wss here?

Can we have a document with a wss: URI?  If this is added here, we should definitely exclude it the service worker code.

> As a followup, someone working on ServiceWorker code should consider whether
> they really want documentURI here or the document's principal URI.

Yea, it probably should the document principal URI to handle cases like sandboxed iframes correctly.

Comment 13

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/24177/#review21767

> Do we need to add wss here?

Please file a follow-up bug for that. This bug is just about moving the existing logic, we can make it match the spec in separate bugs. The "chrome" scheme should also probably be added as allowed by the spec.

Comment 14

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/24177/#review21767

> Why is there a second https check here?

Not sure, I removed it with the additional "file" check too

Comment 15

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/24177/#review21643

> just use NS_ENSURE_SUCCESS(rv, rv);
> applies to the rest of the patch as well unless you really do need that warning.

NS_ENSURE_SUCCESS would mean that we could return something other than NS_OK which would cause an exception in JS. I would prefer if this function never threw exceptions.

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8682933 [details]
MozReview Request: Bug 1221365 - Move "Is origin potentially trustworthy?" logic outside ServiceWorkerManager.cpp. r=ckerschb r?bkelly

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24177/diff/1-2/

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8682933 [details]
MozReview Request: Bug 1221365 - Move "Is origin potentially trustworthy?" logic outside ServiceWorkerManager.cpp. r=ckerschb r?bkelly

https://reviewboard.mozilla.org/r/24177/#review21795

::: dom/security/nsContentSecurityManager.cpp:428
(Diff revision 2)
> +}

Thanks - looks way better now :-)

::: dom/workers/ServiceWorkerManager.cpp:1407
(Diff revision 2)
> +    if (NS_WARN_IF(NS_FAILED(rv)) || !trustworthyURI) {

Nit: since the documentURI is definitely not going to go away :-) you could save the additonal addref nonsense by inlining doc->GetDocumentURI.

csm->IsURIPotentiallyTrustworthy(doc->GetDocumentURI(), &trustworthyURI);

::: dom/workers/ServiceWorkerManager.cpp:1408
(Diff revision 2)
>        return false;

Nit: something is off here, either we eliminate the NS_FAILED portion from the callsite or we make IsURIPotentiallyTrustworthy actually return an error if something goes wrong within the function body - up to you.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

I'd like to look at this patch again tomorrow after some of the latest feedback.

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8682933 [details]
MozReview Request: Bug 1221365 - Move "Is origin potentially trustworthy?" logic outside ServiceWorkerManager.cpp. r=ckerschb r?bkelly

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24177/diff/2-3/

Comment 20

[Tanvi Vyas[:tanvi]]

Hi Ben,

(In reply to Ben Kelly [:bkelly] from comment #12)
> (In reply to Tanvi Vyas [:tanvi] from comment #11)
> > ::: dom/security/nsContentSecurityManager.cpp:409
> > (Diff revision 1)
> > > +    if (scheme.EqualsLiteral("https") ||
> > 
> > Do we need to add wss here?
> 
> Can we have a document with a wss: URI?  If this is added here, we should
> definitely exclude it the service worker code.
> 
The method is supposed to be for any URI.  It might be checking if a subresource is potentially trustworthy, hence I think wss makes sense here.  Why would you need to exclude this from the service worker code?  We could add a boolean parameter aCheckWss to the function.  We may also need one for aLocalhost in the future.  These can be handled as followups.

> > As a followup, someone working on ServiceWorker code should consider whether
> > they really want documentURI here or the document's principal URI.
> 
> Yea, it probably should the document principal URI to handle cases like
> sandboxed iframes correctly.

Can you file a followup bug for this?

Thanks!

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

https://reviewboard.mozilla.org/r/24175/#review22049

::: dom/interfaces/security/nsIContentSecurityManager.idl:51
(Diff revision 3)
> +   * This method returns false instead of throwing upon errors.

FWIW, I think this is different style from other security checks.  It seems most of these APIs return NS_OK for secure/good and NS_ERROR_BAD_URI for not-secure/bad.

For example, see nsScriptSecurityManager's CheckSameOriginURI().

If Christian's ok with it, though, its fine with me.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8682933 [details]
MozReview Request: Bug 1221365 - Move "Is origin potentially trustworthy?" logic outside ServiceWorkerManager.cpp. r=ckerschb r?bkelly

https://reviewboard.mozilla.org/r/24177/#review22053

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tanvi Vyas - out 11/6 [:tanvi] from comment #20)
> > Yea, it probably should the document principal URI to handle cases like
> > sandboxed iframes correctly.
> 
> Can you file a followup bug for this?

I looked and ServiceWorkerManager already does a CheckMayLoad() with the principal after the secure origin check.  So the sandboxed iframe case is already handled.  No follow-up needed.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8683259 [details]
MozReview Request: Bug 1221365 - Tests for "Is origin potentially trustworthy?" logic. r=ckerschb,bkelly

https://reviewboard.mozilla.org/r/24243/#review22057

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8d97dd72f3c
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e9852b26be5

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a8d97dd72f3c
https://hg.mozilla.org/mozilla-central/rev/2e9852b26be5

Comment 27

[Tanvi Vyas[:tanvi]]

(In reply to Ben Kelly [:bkelly] from comment #23)
> (In reply to Tanvi Vyas - out 11/6 [:tanvi] from comment #20)
> > > Yea, it probably should the document principal URI to handle cases like
> > > sandboxed iframes correctly.
> > 
> > Can you file a followup bug for this?
> 
> I looked and ServiceWorkerManager already does a CheckMayLoad() with the
> principal after the secure origin check.  So the sandboxed iframe case is
> already handled.  No follow-up needed.

Hi Ben,

You probably still need a followup.  What about an about:blank page with content added by it's opener?  documentURI will give you about:blank and document Principal URI will give you the URI of the opener.  Other examples are data:, javascript:, etc.

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Thanks.  I will work this in bug 1223378.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Matt, shouldn't the IsURIPotentiallyTrustworthy() method take an nsIPrincipal?  The spec talks about checking an "origin", not a URI.  For example, I think a system principal should pass this check, but it does not have a parsable URI.

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8682933 [details]
MozReview Request: Bug 1221365 - Move "Is origin potentially trustworthy?" logic outside ServiceWorkerManager.cpp. r=ckerschb r?bkelly

Uplift request for both patches.

Approval Request Comment
[Feature/regressing bug #]: Service workers
[User impact if declined]: We are trying to ship service workers in 44 if possible.  I would like the extra checks in bug 1223378 included in that release, but they depend on this bug.
[Describe test coverage new/current, TreeHerder]: Existing mochitests for service workers and new test included in this bug.
[Risks and why]: Minimal.  Only affects service workers.  Code is added to nsContentSecurityManager, but its only called by service workers right now.
[String/UUID change made/needed]: None.

Comment 31

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8682933 [details]
MozReview Request: Bug 1221365 - Move "Is origin potentially trustworthy?" logic outside ServiceWorkerManager.cpp. r=ckerschb r?bkelly

Given that service workers is planned for FF44, the sooner we uplift to Aurora, the sooner we can get feedback/find other issues. Let's uplift to Aurora44.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9814dd56b94f
https://hg.mozilla.org/releases/mozilla-aurora/rev/d35eb3ac914b

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/9814dd56b94f
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/d35eb3ac914b

Comment 34

[Matthew N. [:MattN]]

(In reply to Ben Kelly [:bkelly] from comment #29)
> Matt, shouldn't the IsURIPotentiallyTrustworthy() method take an
> nsIPrincipal?  The spec talks about checking an "origin", not a URI.  For
> example, I think a system principal should pass this check, but it does not
> have a parsable URI.

Perhaps but the non-SW consumers that I know of want something like this for URIs so we would still need a way to do that.