Require [SecureContext] for Web Crypto

NEW
Unassigned

Status

()

defect
P2
normal
2 years ago
2 months ago

People

(Reporter: ryan.sleevi, Unassigned)

Tracking

(Depends on 2 bugs, {dev-doc-needed, leave-open, site-compat})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [domsecurity-active])

Attachments

(3 attachments, 9 obsolete attachments)

4.91 KB, patch
ttaubert
: review+
ttaubert
: feedback+
ttaubert
: checkin+
Details | Diff | Splinter Review
5.58 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
4.75 KB, patch
keeler
: review+
ttaubert
: checkin+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
WebCrypto CR requires [SecureContext], as captured on https://github.com/w3c/webcrypto/issues/28 and reflected in both the ED and CR.

Firefox does not currently require this.

Chrome partially does, and is in the process of aligning with the spec ( https://codereview.chromium.org/2639593002/ - WebCrypto was implemented before [SecureContext] support in Chrome, hence spec divergence)

It would be great it Firefox could align with the spec on requiring a SecureContext for WebCrypto.
Though we did this from the start?
Flags: needinfo?(ttaubert)
(Reporter)

Comment 2

2 years ago
We had a report of compat issue in https://groups.google.com/a/chromium.org/d/msg/security-dev/DtOFo51WFMo/Ypy4gakaAgAJ (and linked bug)

I couldn't find where SecureContext was being enforced w/in the FF code, either in the generated bindings or the WebCryptoTask

My understanding was that FF didn't implement the SecureContext requirement because Chrome's was buggy (which we're trying to fix) and because the spec didn't normatively require it (which it now does)
It wasn't feasible to mark things [SecureContext] until Bug 1286312 landed, which it (recently) has, so this is probably unblocked.
Depends on: 1286312
Speaking historically, I will fess up to not implementing it the first time around because (1) we didn't have [SecureContext] yet, (2) the whole "powerful features" debate was still ongoing, and (3) as a result of the above, the arguments that there were some valid use cases for it on HTTP won the day.

Now that (1) and (2) have settled down, I think we should absolutely go ahead with this if we can convince ourselves the compat risk is acceptable.
Ok, so this is easy. Tests succeed locally and it looks like we don't need any further changes to get this landed.

Richard, how do we determine the compat risk here and what do we define as acceptable?
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Flags: needinfo?(ttaubert)
Attachment #8835006 - Flags: feedback?(rlb)
Tim: How about we add a telemetry probe (say WEBCRYPTO_SECURE_CONTEXT), and let that ride a release or two ahead of the actual change?

It seems like it would be best to measure this in units of what would fail if we added the SecureContext annotation, so probably in dom::Crypto::Subtle.  It looks like you can call JS_GetIsSecureContext on some derivative of mParent to get the flag you want to collect, as here:

http://searchfox.org/mozilla-central/source/dom/workers/WorkerScope.cpp#179

Something like JS_GetIsSecureContext(js::GetObjectCompartment(mParent->GetGlobalJSObject()));

It would also be good to have an overall idea of how often crypto.subtle is accessed in the first place.  I think the WebIDL folks might have some usage counter stuff for this that you can just turn on.  It looks like you might just be able to add "UseCounter", e.g.:

http://searchfox.org/mozilla-central/source/dom/webidl/OfflineResourceList.webidl#28
https://mzl.la/2kU3TsA

As to what risk is acceptable, I would say let's take a look at what the measurements look like.  Obviously, the lower the impact, the better :)
Comment on attachment 8835006 [details] [diff] [review]
0001-Bug-1333140-Require-SecureContext-for-Web-Crypto.patch

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

This looks fine to me, modulo the below nit.  As noted in my other comments, though, let's hold on this until we get some telemetry.

::: dom/webidl/Crypto.webidl
@@ +3,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 origin of this IDL file is
> + * https://w3c.github.io/webcrypto/Overview.html#crypto-interface

Probably better to refer to the slightly more official version:

https://www.w3.org/TR/WebCryptoAPI/#crypto-interface
Attachment #8835006 - Flags: feedback?(rlb) → feedback+
Priority: -- → P2
Whiteboard: [domsecurity-active]
Should we ask a DOM peer as well for the UseCounter changes?
Attachment #8835725 - Flags: review?(rlb)
Comment on attachment 8835725 [details] [diff] [review]
0001-Bug-1333140-Collect-telemetry-about-non-HTTPS-usage-.patch

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

LGTM modulo one nit below.  Adding r? to bz since he's helped out with WebCrypto stuff in the past.

::: toolkit/components/telemetry/Histograms.json
@@ +8822,5 @@
>      "n_values": 30,
>      "description": "Algorithms used with WebCrypto (see table in WebCryptoTask.cpp)"
>    },
> +  "WEBCRYPTO_SECURE_CONTEXT": {
> +    "expires_in_version": "56",

Nit: I would push this out a little further, say to 60.
Attachment #8835725 - Flags: review?(rlb)
Attachment #8835725 - Flags: review?(bzbarsky)
Attachment #8835725 - Flags: review+
Comment on attachment 8835723 [details] [diff] [review]
0002-Bug-1333140-Require-SecureContext-for-Web-Crypto.patch

You may well need some web platform test changes here too, right?
Comment on attachment 8835725 [details] [diff] [review]
0001-Bug-1333140-Collect-telemetry-about-non-HTTPS-usage-.patch

I'm not super-happy with that JSAPI stuff there.  Far better would be to have IsSecureContext on nsIGlobalObject.  Please file a followup bug to do that, and reference this code from that bug?

Apart from that, this is likely to run into false positives from code that just walks all properties of default objects, which does exist out there.  It's not clear to me to what extent this will affect your numbers.  I _might_ be better to measure the thing we really care about, which are actual API calls to crypto.subtle.*.

Measuring both and comparing might be informative....
Attachment #8835725 - Flags: review?(bzbarsky) → review+
Note that we already have overall counters for WEBCRYPTO_METHOD, so we would just have to split them out by secure / not.

Tim: If you want to do that, you could just set a flag on the SubtleCrypto object so that you don't have to query IsSecureContext() multiple times.  It shouldn't change.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #13)
> I'm not super-happy with that JSAPI stuff there.  Far better would be to
> have IsSecureContext on nsIGlobalObject.  Please file a followup bug to do
> that, and reference this code from that bug?

Sure, will do.

> Apart from that, this is likely to run into false positives from code that
> just walks all properties of default objects, which does exist out there. 
> It's not clear to me to what extent this will affect your numbers.  I
> _might_ be better to measure the thing we really care about, which are
> actual API calls to crypto.subtle.*.
> 
> Measuring both and comparing might be informative....

Indeed, very good point.

(In reply to Richard Barnes [:rbarnes] from comment #14)
> Note that we already have overall counters for WEBCRYPTO_METHOD, so we would
> just have to split them out by secure / not.

Good idea.

> Tim: If you want to do that, you could just set a flag on the SubtleCrypto
> object so that you don't have to query IsSecureContext() multiple times.  It
> shouldn't change.

Did that.
Attachment #8835725 - Attachment is obsolete: true
Attachment #8868965 - Flags: review?(dkeeler)
Attachment #8868965 - Flags: review?(bzbarsky)
Attachment #8868965 - Flags: feedback?(benjamin)
Attachment #8868965 - Attachment is obsolete: true
Attachment #8868965 - Flags: review?(dkeeler)
Attachment #8868965 - Flags: review?(bzbarsky)
Attachment #8868965 - Flags: feedback?(benjamin)
Attachment #8868995 - Flags: review?(dkeeler)
Attachment #8868995 - Flags: review?(bzbarsky)
Attachment #8868995 - Flags: feedback?(benjamin)
Now also including the WPT fixes. We of course will have to wait for the Telemetry data to come back, and if that looks promising write an intent to ship before we can land this.
Attachment #8835723 - Attachment is obsolete: true
Attachment #8869019 - Flags: review?(bzbarsky)
Comment on attachment 8868995 [details] [diff] [review]
0001-Bug-1333140-Collect-telemetry-about-non-HTTPS-usage-.patch, v3

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

LGTM - just a couple of questions.

::: dom/base/Crypto.cpp
@@ +9,5 @@
>  #include "nsIRandomGenerator.h"
>  #include "MainThreadUtils.h"
>  #include "nsXULAppAPI.h"
>  
> +#include "mozilla/Telemetry.h"

Is this #include necessary here?

::: toolkit/components/telemetry/Histograms.json
@@ +10722,5 @@
>      "n_values": 20,
>      "description": "Methods invoked under window.crypto.subtle (0=encrypt, 1=decrypt, 2=sign, 3=verify, 4=digest, 5=generateKey, 6=deriveKey, 7=deriveBits, 8=importKey, 9=exportKey, 10=wrapKey, 11=unwrapKey)"
>    },
> +  "WEBCRYPTO_METHOD_SECURE": {
> +    "alert_emails": ["ttaubert@mozilla.com"],

Should we just use seceng-telemetry@mozilla.com?
Attachment #8868995 - Flags: review?(dkeeler) → review+
Comment on attachment 8868995 [details] [diff] [review]
0001-Bug-1333140-Collect-telemetry-about-non-HTTPS-usage-.patch, v3

So this will take how many were secure vs insecure, but not the per-method-type breakdown, right?

Also, this will measure the number of calls, not the number of pages or documents, so can be skewed by one page making ridiculous numbers of calls.  Is that ok?

Apart from that and the "please file a followup to have a way to get this information without JSAPI" bit, r=me
Attachment #8868995 - Flags: review?(bzbarsky) → review+
Comment on attachment 8869019 [details] [diff] [review]
0002-Bug-1333140-Require-SecureContext-for-WebCrypto-API.patch, v2

In the spec, [SecureContext] is also ont he overall CryptoKey interface and the overall SubtleCrypto interface.

Did we skip adding those on purpose?  If not, we should probably just add them.
Attachment #8869019 - Flags: review?(bzbarsky) → review+
Comment on attachment 8868995 [details] [diff] [review]
0001-Bug-1333140-Collect-telemetry-about-non-HTTPS-usage-.patch, v3

data-r=me
Attachment #8868995 - Flags: feedback?(benjamin) → feedback+
(In reply to David Keeler [:keeler] (use needinfo?) from comment #19)
> ::: dom/base/Crypto.cpp
> @@ +9,5 @@
> >  #include "nsIRandomGenerator.h"
> >  #include "MainThreadUtils.h"
> >  #include "nsXULAppAPI.h"
> >  
> > +#include "mozilla/Telemetry.h"
> 
> Is this #include necessary here?

Oops, no.

> ::: toolkit/components/telemetry/Histograms.json
> @@ +10722,5 @@
> >      "n_values": 20,
> >      "description": "Methods invoked under window.crypto.subtle (0=encrypt, 1=decrypt, 2=sign, 3=verify, 4=digest, 5=generateKey, 6=deriveKey, 7=deriveBits, 8=importKey, 9=exportKey, 10=wrapKey, 11=unwrapKey)"
> >    },
> > +  "WEBCRYPTO_METHOD_SECURE": {
> > +    "alert_emails": ["ttaubert@mozilla.com"],
> 
> Should we just use seceng-telemetry@mozilla.com?

Yeah, didn't know that existed.
(In reply to Boris Zbarsky [:bz] (work week until 5/26) from comment #21)
> In the spec, [SecureContext] is also ont he overall CryptoKey interface and
> the overall SubtleCrypto interface.
> 
> Did we skip adding those on purpose?  If not, we should probably just add
> them.

Didn't skip them on purpose, will add.
(In reply to Boris Zbarsky [:bz] (work week until 5/26) from comment #20)
> So this will take how many were secure vs insecure, but not the
> per-method-type breakdown, right?

Correct.

> Also, this will measure the number of calls, not the number of pages or
> documents, so can be skewed by one page making ridiculous numbers of calls. 
> Is that ok?

That's a good point, I should have thought about that. Will amend the patch to only report once per SubtleCrypto instance. This could still be skewed by pages with tons of iframes that all use WebCrypto but I don't think it's worth guarding against that.
This now records telemetry only once per SubtleCrypto instance.

Carrying over data-r=bsmedberg
Attachment #8868995 - Attachment is obsolete: true
Attachment #8872279 - Flags: review?(dkeeler)
Attachment #8872279 - Flags: feedback+
This adds [SecureContext] to SubtleCrypto and CryptoKey now as well. Boris doesn't accept review requests currently, maybe Olli can help?
Attachment #8869019 - Attachment is obsolete: true
Attachment #8872280 - Flags: review?(bugs)
(In reply to Tim Taubert [:ttaubert] from comment #27)
> This adds [SecureContext] to SubtleCrypto and CryptoKey now as well. Boris
> doesn't accept review requests currently, maybe Olli can help?

OTOH, this won't land for a while anyway. Need some telemetry first.
Comment on attachment 8872280 [details] [diff] [review]
0002-Bug-1333140-Require-SecureContext-for-WebCrypto-API.patch, v3

>  * The origin of this IDL file is
>- * https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#crypto-interface
>+ * https://www.w3.org/TR/WebCryptoAPI/#crypto-interface

You mean https://w3c.github.io/webcrypto/Overview.html right?
We don't try to implement TR specs if there is some newer available. TR stand for trash for that reason (and not technical report) ;)
Attachment #8872280 - Flags: review?(bugs) → review+
Comment on attachment 8872279 [details] [diff] [review]
0001-Bug-1333140-Collect-telemetry-about-non-HTTPS-usage-.patch, v4

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

LGTM - just some nits.

::: dom/base/SubtleCrypto.cpp
@@ +24,5 @@
>  
>  
>  
>  SubtleCrypto::SubtleCrypto(nsIGlobalObject* aParent)
> +  : mParent(aParent), mRecordedTelemetry(false)

nit: let's initialize one member per line

@@ +41,5 @@
> +    return;
> +  }
> +
> +  mRecordedTelemetry = true;
> +  JSObject *global = mParent->GetGlobalJSObject();

nit: * to the left
Attachment #8872279 - Flags: review?(dkeeler) → review+
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #20)
> Apart from that and the "please file a followup to have a way to get this
> information without JSAPI" bit, r=me

Filed bug 1368929 for nsIGlobalObject::IsSecureContext().
(In reply to David Keeler [:keeler] (use needinfo?) from comment #30)
> >  SubtleCrypto::SubtleCrypto(nsIGlobalObject* aParent)
> > +  : mParent(aParent), mRecordedTelemetry(false)
> 
> nit: let's initialize one member per line

Done.

> > +  mRecordedTelemetry = true;
> > +  JSObject *global = mParent->GetGlobalJSObject();
> 
> nit: * to the left

Done. Thanks!
(In reply to Olli Pettay [:smaug] from comment #29)
> >  * The origin of this IDL file is
> >- * https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#crypto-interface
> >+ * https://www.w3.org/TR/WebCryptoAPI/#crypto-interface
> 
> You mean https://w3c.github.io/webcrypto/Overview.html right?
> We don't try to implement TR specs if there is some newer available. TR
> stand for trash for that reason (and not technical report) ;)

Yeah... fixed :)
r=keeler data-r=bsmedberg
Attachment #8872279 - Attachment is obsolete: true
Attachment #8872915 - Flags: review+
Attachment #8872915 - Flags: feedback+
Flags: needinfo?(ttaubert)
Keywords: leave-open

Comment 36

2 years ago
Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/917caf00a8b4
Collect telemetry about (non-)HTTPS usage of crypto.subtle r=keeler data-r=bsmedberg
Looking at the telemetry data we have 12.83% of calls to crypto.subtle.* methods from a non-secure context on Beta 55, over the last 7 days.

It's maybe worth noting that 12.83% is rather high, we recorded 10.31% non-secure uses since we started measuring. 7% on Nightly.

Benjamin, what's a good way to reason about this data? Can we somehow compare it to the overall number of pings to see what percentage of users would be affected? Do we have experience evaluating similar data before?
Flags: needinfo?(ttaubert) → needinfo?(benjamin)
(Reporter)

Comment 39

2 years ago
Likely related to the discussion here: https://twitter.com/mikewest/status/880767995936747521
I provided a tiny bit of advice on IRC, but these kinds of questions are best addressed to a product manager. I suggested abovens.
Flags: needinfo?(benjamin)
(In reply to Ryan Sleevi from comment #39)
> Likely related to the discussion here:
> https://twitter.com/mikewest/status/880767995936747521

That's a good pointer, thanks, I missed that. So Twitter/AMP likely skews those numbers, but it's probably not something we want to break either...
(In reply to Tim Taubert [:ttaubert] from comment #41)
> (In reply to Ryan Sleevi from comment #39)
> > Likely related to the discussion here:
> > https://twitter.com/mikewest/status/880767995936747521
> 
> That's a good pointer, thanks, I missed that. So Twitter/AMP likely skews
> those numbers, but it's probably not something we want to break either...

To clarify, AMP loads a polyfill if crypto.subtle isn't available. We wouldn't break anyone's Twitter if we restrict to [SecureContext].

The problem however is that our telemetry numbers currently aren't worth much because we can't tell which sites use the API from a non-secure context. The only comparison we have is the data hovering at around 1-2% (not of all users, only of users using the WebCrypto API) for about two weeks before Google noticed the uptick caused by Twitter/AMP as well.

Jeff, Andreas, what's your opinion on this? Is this enough evidence to move forward with the restriction?
Flags: needinfo?(jgriffiths)
Flags: needinfo?(abovens)
(In reply to Tim Taubert [:ttaubert] from comment #42)
> The problem however is that our telemetry numbers currently aren't worth
> much because we can't tell which sites use the API from a non-secure
> context.

That wasn't worded well. I wanted to say that unfortunately we can't tell how much Twitter/AMP skews those numbers.
My suggestion is to move forward with the restriction. AMP falling back to a polyfill when the API isn't natively available is likely to cover most instances of non-secure usage of crypto.subtle, and for the rest, there's sitecompat outreach: I'm sure this won't be a problem, given that other vendors are likely to move forward with the restriction as well as per https://groups.google.com/a/chromium.org/d/msg/blink-dev/ZD3NWqkk-bo/tgcy_SJQBQAJ
Flags: needinfo?(abovens)
what Andreas said
Flags: needinfo?(jgriffiths)
It doesn't make sense to keep the histogram around when we enforce [SecureContext]. Let's remove it, it has served its purpose.
Attachment #8933281 - Flags: review?(dkeeler)

Comment 47

a year ago
Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d02d52b92ee
Require [SecureContext] for WebCrypto API r=bz,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc91ce755717
Remove WEBCRYPTO_METHOD_SECURE histogram r=keeler
Looks like this won't be too easy. WebCrypto seems the first secure context API exposed to and used in JS sandboxes, as well as workers. We have to figure these two out before we can actually ship the API restriction.
Keywords: leave-open
Workers shouldn't be an issue, right?  I believe we fully support those, at least in the web context...  workers used in our chrome might be a separate issue.

Bug 1273687 tracks the sandbox situation.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #50)
> Workers shouldn't be an issue, right?  I believe we fully support those, at
> least in the web context...  workers used in our chrome might be a separate
> issue.

I saw a bunch of WPT failures that test WebCrypto in workers, that's why I assumed it to be a more general problem with workers:

https://treeherder.mozilla.org/logviewer.html#?job_id=149068416&repo=mozilla-inbound&lineNumber=2295
> https://treeherder.mozilla.org/logviewer.html#?job_id=149068416&repo=mozilla-inbound&lineNumber=2295

That's running based on WebCryptoAPI/derive_bits_keys/ecdh_bits.worker.js

The file isn't named *.https.worker.js, so it's not loaded over https by the test harness.  It should be renamed accordingly.  Same applies to various other tests under WebCryptoAPI/, though there are some (e.g. under WebCryptoAPI/generateKey/) that are named right.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #53)
> The file isn't named *.https.worker.js, so it's not loaded over https by the
> test harness.  It should be renamed accordingly.  Same applies to various
> other tests under WebCryptoAPI/, though there are some (e.g. under
> WebCryptoAPI/generateKey/) that are named right.

Ah, that makes sense. I just tested this with a single worker test and it runs correctly which means there indeed are no problems with workers and [SecureContext] APIs.

I wonder why Chrome doesn't hit that since they updated their implementation and the WPTs.

https://chromium.googlesource.com/chromium/src.git/+/7d57a8fa0d13db4fcb178bb58bb9389fa7670d74
https://github.com/w3c/web-platform-tests/commit/ea527511d7f742f5ad939339df3bad2619382a00

Comment 55

a year ago
It seems they forgot to rename many of the worker resources (though did rename some). Not sure how that happened though. I created https://github.com/w3c/web-platform-tests/pull/8995 to rectify the situation. Hope that helps and unblocks us here.
Assignee: ttaubert → nobody
Status: ASSIGNED → NEW

The leave-open keyword is there and there is no activity for 6 months.
:ckerschb, maybe it's time to close this bug?

Flags: needinfo?(ckerschb)

Comment 58

2 months ago

I don't think this should be closed, without HTTPS

console.log(self.SubtleCrypto)
console.log(self.crypto)

do not log undefined as they should.

(In reply to Release mgmt bot [:sylvestre / :calixte] from comment #57)

The leave-open keyword is there and there is no activity for 6 months.
:ckerschb, maybe it's time to close this bug?

I am not sure - JC Jones is the webcrypto expert - 302 to him.

Flags: needinfo?(ckerschb) → needinfo?(jjones)
Component: DOM: Security → DOM: Web Crypto
Flags: needinfo?(jjones)

I'm going to try and pick this up here at the end of the quarter. It's added to my backlog, so removing the ni.

You need to log in before you can comment on or make changes to this bug.