Full Text Bug Listing

Jiewen Tan

Reported 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 ); };

Radar WebKit Bug Importer

Comment 1 2017-01-11 18:37:24 PST

<rdar://problem/29982683>

Daniel Bates

Comment 2 2017-06-13 13:26:39 PDT

*** Bug 173323 has been marked as a duplicate of this bug. ***

Daniel Bates

Comment 3 2017-06-13 13:36:48 PDT

*** Bug 173325 has been marked as a duplicate of this bug. ***

Daniel Bates

Comment 4 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.

Jiewen Tan

Comment 5 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?

Daniel Bates

Comment 6 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.

Saam Barati

Comment 7 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?

Daniel Bates

Comment 8 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.

Jiewen Tan

Comment 9 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?

Daniel Bates

Comment 10 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.

Daniel Bates

Comment 11 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.

Jiewen Tan

Comment 12 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.

Alexey Proskuryakov

Comment 13 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.

Oliver Hunt

Comment 14 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.

Alexey Proskuryakov

Comment 15 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.

Jiewen Tan

Comment 16 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.

Alexey Proskuryakov

Comment 17 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.

Frederic Jacobs

Comment 18 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.

Alexey Proskuryakov

Comment 19 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.

Oliver Hunt

Comment 20 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

Alexey Proskuryakov

Comment 21 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.

Oliver Hunt

Comment 22 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?"

Alexey Proskuryakov

Comment 23 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.

Jiewen Tan

Comment 24 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.

Frederic Jacobs

Comment 25 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.

Frederic Jacobs

Comment 26 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

Sam Sneddon [:gsnedders]

Comment 27 2021-08-17 10:54:26 PDT

forward duping to where this fixed :( *** This bug has been marked as a duplicate of bug 227725 ***

Attachments
Patch (3.52 KB, patch) 2017-06-13 13:37 PDT, Daniel Bates	ap: review-	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Bug 166959