Bug 166959 - [WebCrypto] WebCrypto API should be on top of SecureContext
Summary: [WebCrypto] WebCrypto API should be on top of SecureContext
Status: RESOLVED DUPLICATE of bug 227725
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
: 173323 (view as bug list)
Depends on: 158121
Blocks:
  Show dependency treegraph
 
Reported: 2017-01-11 18:36 PST by Jiewen Tan
Modified: 2021-08-17 10:54 PDT (History)
16 users (show)

See Also:


Attachments
Patch (3.52 KB, patch)
2017-06-13 13:37 PDT, Daniel Bates
ap: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2017-01-11 18:36:06 PST
WebCrypto API should be on top of SecureContext per the latest spec: https://www.w3.org/TR/WebCryptoAPI/

Here are the updated IDLs:

[Exposed=(Window,Worker)]
interface Crypto {
  [SecureContext] readonly attribute SubtleCrypto subtle;
  ArrayBufferView getRandomValues(ArrayBufferView array);
};

[SecureContext,Exposed=(Window,Worker)]
interface CryptoKey {
  readonly attribute KeyType type;
  readonly attribute boolean extractable;
  readonly attribute object algorithm;
  readonly attribute object usages;
};

[SecureContext,Exposed=(Window,Worker)]
interface SubtleCrypto {
  Promise<any> encrypt(AlgorithmIdentifier algorithm,
                       CryptoKey key,
                       BufferSource data);
  Promise<any> decrypt(AlgorithmIdentifier algorithm,
                       CryptoKey key,
                       BufferSource data);
  Promise<any> sign(AlgorithmIdentifier algorithm,
                    CryptoKey key,
                    BufferSource data);
  Promise<any> verify(AlgorithmIdentifier algorithm,
                      CryptoKey key,
                      BufferSource signature,
                      BufferSource data);
  Promise<any> digest(AlgorithmIdentifier algorithm,
                      BufferSource data);

  Promise<any> generateKey(AlgorithmIdentifier algorithm,
                          boolean extractable,
                          sequence<KeyUsage> keyUsages );
  Promise<any> deriveKey(AlgorithmIdentifier algorithm,
                         CryptoKey baseKey,
                         AlgorithmIdentifier derivedKeyType,
                         boolean extractable,
                         sequence<KeyUsage> keyUsages );
  Promise<ArrayBuffer> deriveBits(AlgorithmIdentifier algorithm,
                          CryptoKey baseKey,
                          unsigned long length);
  
  Promise<CryptoKey> importKey(KeyFormat format,
                         (BufferSource or JsonWebKey) keyData,
                         AlgorithmIdentifier algorithm,
                         boolean extractable,
                         sequence<KeyUsage> keyUsages );
  Promise<any> exportKey(KeyFormat format, CryptoKey key);

  Promise<any> wrapKey(KeyFormat format,
                       CryptoKey key,
                       CryptoKey wrappingKey,
                       AlgorithmIdentifier wrapAlgorithm);
  Promise<CryptoKey> unwrapKey(KeyFormat format,
                         BufferSource wrappedKey,
                         CryptoKey unwrappingKey,
                         AlgorithmIdentifier unwrapAlgorithm,
                         AlgorithmIdentifier unwrappedKeyAlgorithm,
                         boolean extractable,
                         sequence<KeyUsage> keyUsages );
};
Comment 1 Radar WebKit Bug Importer 2017-01-11 18:37:24 PST
<rdar://problem/29982683>
Comment 2 Daniel Bates 2017-06-13 13:26:39 PDT
*** Bug 173323 has been marked as a duplicate of this bug. ***
Comment 3 Daniel Bates 2017-06-13 13:36:48 PDT
*** Bug 173325 has been marked as a duplicate of this bug. ***
Comment 4 Daniel Bates 2017-06-13 13:37:19 PDT
Created attachment 312799 [details]
Patch

We cannot test this change using a layout test as we do not support testing with a non-localhost server. We could write a TestWebKitAPI test for this change. I'm unclear how much value there is to do so given that we have existing unit tests to ensure SecurityOrigin::isPotentionallyTrustworthy(), the core algorithm for Secure Contexts, works correctly. Let me know if it would be preferred to add a unit test for this change.
Comment 5 Jiewen Tan 2017-06-13 13:44:58 PDT
Comment on attachment 312799 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312799&action=review

Thanks for adding the SecureContext attribute.

> Source/JavaScriptCore/ChangeLog:9
> +        Adds SubtleCrypto to the list of common identifiers so as to support toggling this

Not familiar with this. Needs someone else to take a look.

> Source/WebCore/ChangeLog:10
> +        <https://w3c.github.io/webcrypto/Overview.html#crypto-interface> (W3C Editor's Draft 24 October 2016).

Should we change this to https://www.w3.org/TR/WebCryptoAPI/ since it is official and has a newer date?
Comment 6 Daniel Bates 2017-06-13 13:47:56 PDT
(In reply to Jiewen Tan from comment #5)
> > Source/WebCore/ChangeLog:10
> > +        <https://w3c.github.io/webcrypto/Overview.html#crypto-interface> (W3C Editor's Draft 24 October 2016).
> 
> Should we change this to https://www.w3.org/TR/WebCryptoAPI/ since it is
> official and has a newer date?

Will change before landing.
Comment 7 Saam Barati 2017-06-13 13:47:58 PDT
Comment on attachment 312799 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312799&action=review

> Source/WebCore/ChangeLog:12
> +        As a result of this change SubtleCrypto is only exposed in a secure context. That is,
> +        it is only available on an HTTPS page or an HTTPS page embedded in another HTTPS page.

Can this be tested?
Comment 8 Daniel Bates 2017-06-13 13:51:26 PDT
(In reply to Saam Barati from comment #7)
> Comment on attachment 312799 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312799&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        As a result of this change SubtleCrypto is only exposed in a secure context. That is,
> > +        it is only available on an HTTPS page or an HTTPS page embedded in another HTTPS page.
> 
> Can this be tested?

See comment #4.
Comment 9 Jiewen Tan 2017-06-13 13:58:38 PDT
(In reply to Daniel Bates from comment #4)
> Created attachment 312799 [details]
> Patch
> 
> We cannot test this change using a layout test as we do not support testing
> with a non-localhost server. We could write a TestWebKitAPI test for this
> change. I'm unclear how much value there is to do so given that we have
> existing unit tests to ensure SecurityOrigin::isPotentionallyTrustworthy(),
> the core algorithm for Secure Contexts, works correctly. Let me know if it
> would be preferred to add a unit test for this change.

We have some imported W3C tests:
LayoutTests/imported/w3c/web-platform-tests/WebCryptoAPI/secure_context/

Do your change make affect them properly?
Comment 10 Daniel Bates 2017-06-13 14:30:46 PDT
(In reply to Jiewen Tan from comment #9)
> (In reply to Daniel Bates from comment #4)
> > Created attachment 312799 [details]
> > Patch
> > 
> > We cannot test this change using a layout test as we do not support testing
> > with a non-localhost server. We could write a TestWebKitAPI test for this
> > change. I'm unclear how much value there is to do so given that we have
> > existing unit tests to ensure SecurityOrigin::isPotentionallyTrustworthy(),
> > the core algorithm for Secure Contexts, works correctly. Let me know if it
> > would be preferred to add a unit test for this change.
> 
> We have some imported W3C tests:
> LayoutTests/imported/w3c/web-platform-tests/WebCryptoAPI/secure_context/
> 
> Do your change make affect them properly?

No, because we need to add support infrastructure to run the test using a non-localhost server.
Comment 11 Daniel Bates 2017-06-13 14:32:03 PDT
(In reply to Daniel Bates from comment #10)
> (In reply to Jiewen Tan from comment #9)
> > (In reply to Daniel Bates from comment #4)
> > > Created attachment 312799 [details]
> > > Patch
> > > 
> > > We cannot test this change using a layout test as we do not support testing
> > > with a non-localhost server. We could write a TestWebKitAPI test for this
> > > change. I'm unclear how much value there is to do so given that we have
> > > existing unit tests to ensure SecurityOrigin::isPotentionallyTrustworthy(),
> > > the core algorithm for Secure Contexts, works correctly. Let me know if it
> > > would be preferred to add a unit test for this change.
> > 
> > We have some imported W3C tests:
> > LayoutTests/imported/w3c/web-platform-tests/WebCryptoAPI/secure_context/
> > 
> > Do your change make affect them properly?
> 
> No, because we need to add support infrastructure to run the test using a
> non-localhost server.

I forgot to mention, that with the proposed patch (attachment #312799 [details]) applied then the tests in LayoutTests/imported/w3c/web-platform-tests/WebCryptoAPI/secure_context pass when you run them from a non-localhost server.
Comment 12 Jiewen Tan 2017-06-13 15:38:36 PDT
Comment on attachment 312799 [details]
Patch

Saam and Dan have explained the concept of common identifiers to me. Looks good then. r=me.
Comment 13 Alexey Proskuryakov 2017-06-14 16:11:38 PDT
Does this change mean that WebCrypto will no longer be available in http pages? If so, the benefit is unclear to me.
Comment 14 Oliver Hunt 2017-06-14 16:20:16 PDT
(In reply to Alexey Proskuryakov from comment #13)
> Does this change mean that WebCrypto will no longer be available in http
> pages? If so, the benefit is unclear to me.

It does not make sense to provide access to cryptographic APIs in an insecure environment, and i believe that we're the only browser left that has not restricted this.
Comment 15 Alexey Proskuryakov 2017-06-14 17:15:56 PDT
Comment on attachment 312799 [details]
Patch

If that's the reason, I think that we should just plain not make this change. There is no benefit to anyone from it.
Comment 16 Jiewen Tan 2017-06-14 18:25:07 PDT
(In reply to Alexey Proskuryakov from comment #15)
> Comment on attachment 312799 [details]
> Patch
> 
> If that's the reason, I think that we should just plain not make this
> change. There is no benefit to anyone from it.

I think the committee add SecureContext mainly because they try to avoid leaking keying materials when they are transported between servers and clients. I am not involved in the discussions so I cannot recover more information.

As for me, I am kind of neutral for this particular reason to add SecureContext. One thing is that I believe with all the functionalities we provided developers can protect their keying materials well without SecureContext. The other thing however is if developers begin to get used to SecureContext, they might solely rely on HTTPS to protect their keying materials. And therefore if users suddenly load those pages with HTTP, keying materials will be exposed then. To prevent this scenario, we should add support of SecureContext despite whether this idea itself is good or not. Or we could ask the committee to drop this requirement, which seems impossible for me.
Comment 17 Alexey Proskuryakov 2017-06-14 21:41:02 PDT
I think that it's mostly about the idea that all new functionality in HTML should be limited to https, forcing people to use https. We generally don't respect that, and I don't think that we should start doing that.

In this case, it was natural to claim security benefits, but I don't think that there is any validity to that claim.
Comment 18 Frederic Jacobs 2017-06-15 02:56:55 PDT
Seconding this argument

> The other thing however is if developers begin to get used to SecureContext, they might solely rely on HTTPS to protect their keying materials. And therefore if users suddenly load those pages with HTTP, keying materials will be exposed then. To prevent this scenario, we should add support of SecureContext despite whether this idea itself is good or not.


If a website is using the WebCrypto API, it is likely store sensitive keying information on the client-side. It would be unfortunate that a downgrade attack to their server (either because of a HSTS bug or missing headers) would enable an attacker to get a signing oracle that signs or decrypt anything they want.

I think it would be wise to follow the spec.
Comment 19 Alexey Proskuryakov 2017-06-15 13:05:31 PDT
> If a website is using the WebCrypto API, it is likely store sensitive keying information on the client-side.

I don't think that's accurate. There are lots of uses that don't involve storing secrets locally.

WebCrypto is "subtle" for a reason, there are many mistakes that can be done with it. Yes, using http with anything sensitive would be a bad mistake, but guarding against it by blocking the feature is a truly weird idea. We don't block password form fields, which are a lot more sensitive in practice.
Comment 20 Oliver Hunt 2017-06-15 13:54:26 PDT
(In reply to Alexey Proskuryakov from comment #19)
> > If a website is using the WebCrypto API, it is likely store sensitive keying information on the client-side.
> 
> I don't think that's accurate. There are lots of uses that don't involve
> storing secrets locally.
> 
> WebCrypto is "subtle" for a reason, there are many mistakes that can be done
> with it. Yes, using http with anything sensitive would be a bad mistake, but
> guarding against it by blocking the feature is a truly weird idea. We don't
> block password form fields, which are a lot more sensitive in practice.

Given your strong feelings on the matter, could you work with Hixie (or whoever the current editor is) to remove the SecureContext restriction from the spec? Otherwise we are needlessly violating the spec where every other browser is in compliance.

My belief is that this should be restricted to sites that are secure to prevent accidental foot-gun interaction with user private data, and my understanding was that that was the consensus of the other browsers. Again, if you feel otherwise please make your case for a spec change and add a link to the thread into this bug so others can follow. If you don't believe that the spec is wrong then i'll happily r+ this as this is a low risk change, with a moderate-high reward depending on where you stand vis a vis the footgun effect :D
Comment 21 Alexey Proskuryakov 2017-06-15 17:36:10 PDT
Do we argue against the other "https only" features, or are we silently ignoring those requirements? I thought it was the latter.
Comment 22 Oliver Hunt 2017-06-17 01:07:51 PDT
(In reply to Alexey Proskuryakov from comment #21)
> Do we argue against the other "https only" features, or are we silently
> ignoring those requirements? I thought it was the latter.

The only reason this hasn't come up before was because we did not implement the SecureContext component of the spec correctly, and have out of date IDL for some of the existing features.

Again, if you feel that this is incorrect, could you raise the issue with the working group and get the secure context attribute remove from web crypto apis.

Your current argument is literally "we don't already match the spec, so why should we fix this now?"
Comment 23 Alexey Proskuryakov 2017-06-17 04:21:30 PDT
Sure, it would be good to improve the spec. It has seen a lot of influence from people new to web technologies. 

It would also be useful to discuss actual issues that need fixing. Frederic's comment 18 is actually somewhat convincing to me, except that blocking all crypto functionality seems like overkill.
Comment 24 Jiewen Tan 2017-06-19 12:23:05 PDT
(In reply to Alexey Proskuryakov from comment #23)
> Sure, it would be good to improve the spec. It has seen a lot of influence
> from people new to web technologies. 
> 
> It would also be useful to discuss actual issues that need fixing.
> Frederic's comment 18 is actually somewhat convincing to me, except that
> blocking all crypto functionality seems like overkill.

Actually, some people comments on the spec that at least hash could somehow be an exception. Maybe we could follow this by pointing out some functionalities are not necessarily protected by SecureContext.
Comment 25 Frederic Jacobs 2017-06-20 04:46:18 PDT
Even if you only expose window.crypto.subtle.digest on non-SecureContext, I think that's detrimental to security. Someone out there will do an integrity check of some resource using window.crypto.subtle.digest, not realizing that any network attacker can redefine that function (and even his application source code).

The idea of having things in window.crypto.subtle that require SecureContext and other not seems very confusing from a Web developers perspective.

I'm fully aware that the compatibility is not good yet. But requiring developers in the future to check whether a sub-component of subtle is present is gonna be making cross-browser support really painful for frameworks who need to specify what is available over what origin.

If you want to use browser crypto APIs, use HTTPS. I have trouble seeing why we're arguing so hard about not complying to a standard, especially when not doing so will likely result in developers shooting themselves in the foot and increased code/documentation complexity for us and web developers.
Comment 26 Frederic Jacobs 2017-08-04 13:47:12 PDT
Both Chrome and Firefox are going along with deprecating it. Chrome already did it. FireFox says it's going to happen in 2017 Q4 or 2018 Q1. 

https://groups.google.com/forum/#!msg/mozilla.dev.platform/wwmN1eqeRyw/Bv7ELvsrAgAJ
Comment 27 Sam Sneddon [:gsnedders] 2021-08-17 10:54:26 PDT
forward duping to where this fixed :(

*** This bug has been marked as a duplicate of bug 227725 ***