Closed Bug 1221365 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

(Blocks 3 open bugs, )

Details

Attachments

(2 files)

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.
Bug 1221365 - Move "Is origin potentially trustworthy?" logic outside ServiceWorkerManager.cpp. r=ckerschb,bkelly
Attachment #8682933 - Flags: review?(mozilla)
Attachment #8682933 - Flags: review?(bkelly)
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.
Attachment #8682933 - Flags: review?(mozilla)
(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.
(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 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.
Attachment #8682933 - Flags: review?(bkelly) → review+
(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.
Bug 1221365 - Tests for "Is origin potentially trustworthy?" logic. r=ckerschb,bkelly
Attachment #8683259 - Flags: review?(mozilla)
Attachment #8683259 - Flags: review?(bkelly)
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
(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 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
Attachment #8683259 - Flags: review?(mozilla) → review+
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.
(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.
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.
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
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 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/
Attachment #8682933 - Attachment description: MozReview Request: Bug 1221365 - Move "Is origin potentially trustworthy?" logic outside ServiceWorkerManager.cpp. r=ckerschb,bkelly → MozReview Request: Bug 1221365 - Move "Is origin potentially trustworthy?" logic outside ServiceWorkerManager.cpp. r?ckerschb r=bkelly
Attachment #8682933 - Flags: review?(mozilla)
Attachment #8682933 - Flags: review?(mozilla) → review+
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.
I'd like to look at this patch again tomorrow after some of the latest feedback.
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/
Attachment #8682933 - Attachment description: MozReview Request: Bug 1221365 - Move "Is origin potentially trustworthy?" logic outside ServiceWorkerManager.cpp. r?ckerschb r=bkelly → MozReview Request: Bug 1221365 - Move "Is origin potentially trustworthy?" logic outside ServiceWorkerManager.cpp. r=ckerschb r?bkelly
Attachment #8682933 - Flags: review+ → review?(bkelly)
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!
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.
Attachment #8682933 - Flags: review?(bkelly) → review+
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
(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.
Attachment #8683259 - Flags: review?(bkelly) → review+
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
https://hg.mozilla.org/mozilla-central/rev/a8d97dd72f3c
https://hg.mozilla.org/mozilla-central/rev/2e9852b26be5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(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.
Flags: needinfo?(bkelly)
Thanks.  I will work this in bug 1223378.
Flags: needinfo?(bkelly)
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.
Flags: needinfo?(MattN+bmo)
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.
Attachment #8682933 - Flags: approval-mozilla-aurora?
Blocks: 1223481
Blocks: 1223624
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.
Attachment #8682933 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1226324
(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.
Flags: needinfo?(MattN+bmo)
You need to log in before you can comment on or make changes to this bug.