1030963 - remove proprietary window.crypto functions/properties

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

We should remove the proprietary window.crypto functions and properties. See https://developer.mozilla.org/en-US/docs/JavaScript_crypto for what will be affected by this change.
These functions have never been (and never will be) standardized. The implementation has near-nonexistent test coverage. What few tests exist were written as a result of finding easily-encountered bugs years after the original implementation landed[0][1][2]. As it is exposed to web content, it represents a considerable attack surface. It is not well-maintained. It is incompatible with our process-separation and sandboxing efforts. It is not supported or enabled on Firefox OS.
Meanwhile, we are making progress on implementing the webcrypto specification[3]. When complete, webcrypto should provide compatible functionality for what these functions are currently being used to do. Any functionality not implementable using webcrypto is available to addons (see the interfaces in security/manager/ssl/public).

Note: this does not include window.crypto.subtle or window.crypto.getRandomValues, which are part of webcrypto and do not need to be removed.

[0] bug 849553
[1] bug 934716
[2] bug 935618
[3] bug 865789

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch 1/4: build bits

So far people seem to think this is a good idea, so I'm going to start the review process. This removes the MOZ_DISABLE_CRYPTOLEGACY build option that will no longer be necessary as a result of this.

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch 2/4: content bits

This mainly removes tests in content/ that aren't necessary anymore.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch 3/4: dom bits

This removes the bindings from the dom, I think.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch 4/4: security bits

This removes the implementation and related UI in security/manager. Let me know if you want this broken up further into frontend/backend (so the UI bits can be reviewed by someone else).

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8447488 [details] [diff] [review]
patch 4/4: security bits

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

Overall, I think this looks really good.

Are you going to do the removal of window.pkcs11 in another bug? When this project is done, there shouldn't be any DOM code left in PSM. Any remaining bits needed by WebCrypto should be moved to the WebCrypto module. Please describe how/when this will happen.

It seems unnecessary to remove the feature of the certificate manager being updated when you insert/remove your smartcard. I'm sure there is some some way to keep this feature without implementing it through the DOM. However, it is up to Dolske whether it is worthwhile to keep that feature.

::: security/manager/ssl/src/nsCrypto.cpp
@@ +39,5 @@
>  #define JS_ERR_BAD_MODULE_NAME            -6
>  #define JS_ERR_BAD_DLL_NAME               -7
>  #define JS_ERR_BAD_MECHANISM_FLAGS        -8
>  #define JS_ERR_BAD_CIPHER_ENABLE_FLAGS    -9
>  #define JS_ERR_ADD_DUPLICATE_MOD          -10

Please delete this stuff too. (Seems to be totally unused even before your patch.)

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8447484 [details] [diff] [review]
patch 1/4: build bits

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

Didn't look at the other patches, but I guess you're also removing the CONFIG['MOZ_DISABLE_CRYPTOLEGACY'] in various moz.builds?

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 8447485 [details] [diff] [review]
patch 2/4: content bits

r=jst for the code changes here, but do we have a good understanding of the implications of the smart card event related change for the people in the world that do use that stuff? Not saying we shouldn't do this, just that we should call that out if this has any real implications on such users.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #5)
> Are you going to do the removal of window.pkcs11 in another bug?

I'm not sure what you mean - there doesn't appear to be a window.pkcs11 property...

> When this
> project is done, there shouldn't be any DOM code left in PSM. Any remaining
> bits needed by WebCrypto should be moved to the WebCrypto module. Please
> describe how/when this will happen.

My plan was to do this, see what's left, and remove/move the rest as necessary in follow-up bugs.

> It seems unnecessary to remove the feature of the certificate manager being
> updated when you insert/remove your smartcard. I'm sure there is some some
> way to keep this feature without implementing it through the DOM. However,
> it is up to Dolske whether it is worthwhile to keep that feature.

Ok - if necessary we can keep that functionality (particularly exposed to chrome/addons).

> ::: security/manager/ssl/src/nsCrypto.cpp
> @@ +39,5 @@
> >  #define JS_ERR_BAD_MODULE_NAME            -6
> >  #define JS_ERR_BAD_DLL_NAME               -7
> >  #define JS_ERR_BAD_MECHANISM_FLAGS        -8
> >  #define JS_ERR_BAD_CIPHER_ENABLE_FLAGS    -9
> >  #define JS_ERR_ADD_DUPLICATE_MOD          -10
> 
> Please delete this stuff too. (Seems to be totally unused even before your
> patch.)

Sounds good.

(In reply to Mike Hommey [:glandium] from comment #6)
> Didn't look at the other patches, but I guess you're also removing the
> CONFIG['MOZ_DISABLE_CRYPTOLEGACY'] in various moz.builds?

Yes indeed - there should be no more MOZ_DISABLE_CRYPTOLEGACY anywhere after these patches have landed.

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #7)
> r=jst for the code changes here, but do we have a good understanding of the
> implications of the smart card event related change for the people in the
> world that do use that stuff? Not saying we shouldn't do this, just that we
> should call that out if this has any real implications on such users.

Honestly, I think the handling of smart card events can be emulated by refreshing the page, logging in/out, opening and closing the certificate manager, etc. in most cases. If it's left exposed to addons, that's probably sufficient (so I'll see if I can update the patch to do that).

I've communicated the intent to remove this functionality here:
https://groups.google.com/forum/#!topic/mozilla.dev.tech.crypto/hR_bjx9OVJA
https://groups.google.com/forum/#!topic/mozilla.dev.platform/1zv68u5kWfY (cross-posted for visibility)

Thanks for the reviews!

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I added some "see also" references to bugs about <keygen>. That list is not complete. Since we are telling people that <keygen> is what they should use instead for some of the removed functionality, it would be good to triage those bugs and others while doing this work.

Comment 10

[Justin Dolske [:Dolske]]

Comment on attachment 8447488 [details] [diff] [review]
patch 4/4: security bits

Erp, sorry, thought I already did this but I was thinking of bug 1024871.

I don't really need to signoff on basic removals, so long as NSS/PSM peers on onboard with and DOM peers are ok with any webcompat bits.

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch 1/4 v2: build bits

Turns out we'll still need some sort of build variable for whether or not to support smartcards, so rather than removing MOZ_DISABLE_CRYPTOLEGACY, I replaced it with MOZ_NO_SMARTCARDS.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch 2/4: content bits rebased

Rebased - carrying over r+.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch 3/4: dom bits rebased

Rebased - carrying over r+.

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch 4/4: security bits (with smartcard support)

Brian - I'd appreciate if you took a final look at this. Basically, the only significant changes from the last patch are that smartcards are still supported/used. 

The important parts are:
- using MOZ_NO_SMARTCARDS instead of MOZ_DISABLE_CRYPTOLEGACY
- changes to nsSmartCardMonitor.cpp
- use of observers on smartcard-insert/remove in device_manager.js and certManager.js

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8472415 [details] [diff] [review]
patch 1/4 v2: build bits

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

Please land this all folded in one changeset, otherwise you'll be creating changesets that likely won't build.

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8472427 [details] [diff] [review]
patch 4/4: security bits (with smartcard support)

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

::: security/manager/pki/resources/content/certManager.js
@@ +47,4 @@
>  function LoadCerts()
>  {
> +  Services.obs.addObserver(smartcardObserver, "smartcard-insert", false);
> +  Services.obs.addObserver(smartcardObserver, "smartcard-remove", false);

I guess that we don't have a way of testing this automatically. Perhaps this is something for QA to test manually. Many ThinkPads come with built-in smartcard readers so it should be possible for them to test this, though they may need help procuring and configuring a smartcard for that test.

::: security/manager/pki/resources/content/device_manager.js
@@ +39,5 @@
>  {
>    bundle = document.getElementById("pippki_bundle");
>    secmoddb = Components.classes[nsPKCS11ModuleDB].getService(nsIPKCS11ModuleDB);
> +  Services.obs.addObserver(smartcardObserver, "smartcard-insert", false);
> +  Services.obs.addObserver(smartcardObserver, "smartcard-remove", false);

Ditto.

Ideally, this repeated code (the addObserver calls, DeregisterSmartCardObservers, and smartcardObserver) would be encapsulated into a resusable function, e.g. addSmartCardObserver( onSmartCardChange). But, OK.

Nit "smart card" is two words, so correct camelCasing of it would be "smartCard"/"SmartCard", not "smartcard".

::: security/manager/ssl/src/nsCrypto.cpp
@@ +54,5 @@
>    SECStatus srv = SECMOD_DeleteModule(modName.get(), &modType);
>    if (srv == SECSuccess) {
>      SECMODModule *module = SECMOD_FindModule(modName.get());
>      if (module) {
> +#ifndef MOZ_NO_SMARTCARDS

Nit: Should this be MOZ_NO_SMART_CARDS since "smart card" is two words? (everywhere).

Comment 17

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch folded

Thanks for the reviews, everyone. Here is the patch folded together. I changed MOZ_NO_SMARTCARDS to MOZ_NO_SMART_CARDS and used the proper camel case for "smart card" in code I added. I didn't refactor the add/remove observer calls since there wasn't a significantly shorter way to do it and it's a common enough paradigm that I thought duplicating those few lines of code wouldn't be a maintenance challenge in the future. In any case, a lot of that code could use refactoring and/or updating to more modern practices.

https://hg.mozilla.org/integration/mozilla-inbound/rev/68499003df5e

Comment 18

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

For the record, I did run this through try, but since the try repo just got reset, there's not much evidence. In any case:

https://tbpl.mozilla.org/?tree=Try&rev=d62649dedbc2
https://tbpl.mozilla.org/?tree=Try&rev=1d3ce76e5ddd

(In the first changeset, there was a bug that prevented smart card events from working as expected. However, newly-added tests caught this and I fixed it.)

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/68499003df5e

Comment 21

[Jean-Yves Perrier [:teoli]]

Doc updated:
https://developer.mozilla.org/en-US/Firefox/Releases/34
and I moved the old pages in the Archive:
https://developer.mozilla.org/en-US/docs/Archive/Mozilla/JavaScript_crypto

I kept Window.crypto and Window.crypto.getRandomValues for the Web Crypto API

Comment 22

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Thanks!

Comment 23

[Vasil Badev]

This change removes crypto.signText function, that is used in most of online banking sites in Bulgaria for signing transactions with certificates issued on smart cards. Web Crypto API can't replace this function. Contrary to the section 2.4 from specification (https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#document-signing), it do not provide API to sign text with keys "re-provisioned out-of-band by the web application" such as certificates on smart card issued by third parity CA or government. It is just not possible to get key that is not generated, imported or derived by web application.

This leaves users with choice to use Internet Explorer (if they use Windows), or to stay on old version of Firefox. Sites that rely on this functionality have to choose to drop support for browsers different from Internet Explorer, to try to create custom extension that directly calls to NSS using ctypes or to create custom java applet.

Comment 24

[Justin Dolske [:Dolske]]

What do Bulgarian users of Chrome, Safari, or everything on mobile do on those sites?

Comment 25

[Vasil Badev]

They can use other browsers to see account balance and get reports of mony transfers, but can make new transfers only with Internet Explorer and old versions of Firefox. Probbably we have very paranoid banks that insist every transaction to be signed with qualified signature.

I do not know what banks will do, but probably they will move to one time passwords sent by SMS. It is probably too late to complain about this change, so I will try to reimplement crypto.signText as extension.

Comment 26

[Vladislav Evgeniev]

Is there a usage statistics for the removed crypto functions, i.e. how many people are affected by this change?
Though I understand that this functions are not standardized, removing crypto.signText leaves the users no options to sign data from the browser.
In Bulgaria many banks are signing the transactions in their online banking systems to improve security. Also I know of some web-based document exchange systems that rely on this functionality.
The removal of crypto.signText prevents many Bulgarian Mac OS and Linux users from using online banking. Some banks offer OTP, but OTP is not a viable alternative to PKI.

Comment 27

[Graham Leggett]

I've just been alerted that a client's decade old claims management system has suddenly stopped working after an upgrade to Firefox 33 and their business is at a standstill. It turns out it is because crypto.signText has suddenly vanished from the browser.

Obviously before thinking that removing crypto.signText was a good idea, a replacement standards complaint technology now exists supported natively by Firefox that is a drop in replacement with the same functionality, being a) part of the browser and therefore impossible to spoof, b) show the user some text, and if they agree with that text, c) allow them to sign it with a digital certificate.

Can you point me at a link confirming what that replacement technology is?

Comment 28

[Vasil Badev]

There is no alternative to crypto.signText. WebCrypto can be replacement if Key Discovery API become part of the standard and there is certificate based key selector. This stackoverflow answer gives good overview of the situation http://stackoverflow.com/a/26407109/85106, and linked youtube video shows what is missing in WebCryptoAPI to replace crypto.signText.
The only possible workaround for now is to use Firefox ESR and hope that Mozilla developers will change their minds on the removal of this feature before ESR 38 is released in May 2015.

Comment 29

[Graham Leggett]

It seems the webcrypto API at https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html is still at a draft stage and warns:

"Implementors should be aware that this specification is not stable. Implementors who are not taking part in the discussions are likely to find the specification changing out from under them in incompatible ways."

Comment 30

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Graham Leggett from comment #27)
> I've just been alerted that a client's decade old claims management system
> has suddenly stopped working after an upgrade to Firefox 33 and their
> business is at a standstill. It turns out it is because crypto.signText has
> suddenly vanished from the browser.
> 
> Obviously before thinking that removing crypto.signText was a good idea, a
> replacement standards complaint technology now exists supported natively by
> Firefox that is a drop in replacement with the same functionality, being a)
> part of the browser and therefore impossible to spoof, b) show the user some
> text, and if they agree with that text, c) allow them to sign it with a
> digital certificate.
> 
> Can you point me at a link confirming what that replacement technology is?

Please see bug 1083118 for any further discussion of window.crypto.signText.

Comment 31

[Mihail Daskalov]

(In reply to Vasil Badev from comment #25)
> They can use other browsers to see account balance and get reports of mony
> transfers, but can make new transfers only with Internet Explorer and old
> versions of Firefox. Probbably we have very paranoid banks that insist every
> transaction to be signed with qualified signature.
> 
> I do not know what banks will do, but probably they will move to one time
> passwords sent by SMS. It is probably too late to complain about this
> change, so I will try to reimplement crypto.signText as extension.

In the past there was a secclab extension that this sort of this thing (created for the use by Spanish Government sites).

The question is however, whether users should be made to install whatever "extensions" various people come up with, or ActiveX control. What level of trust is put into a base product such as Mozilla Firefox versus "Extensions" having access on your behalf to crypto functions with you PIN code in there hands.

Comment 32

[Tero]

(In reply to Vasil Badev from comment #25)
> They can use other browsers to see account balance and get reports of mony
> transfers, but can make new transfers only with Internet Explorer and old
> versions of Firefox. Probbably we have very paranoid banks that insist every
> transaction to be signed with qualified signature.
> 
> I do not know what banks will do, but probably they will move to one time
> passwords sent by SMS. It is probably too late to complain about this
> change, so I will try to reimplement crypto.signText as extension.

Well, I for one can tell you about Banco Santander Rio from Argentina, they use signed transactions on corporate accounts to authorize any kind of outbound money movement. At least as of 33.0.2 update I am unable to authorize any payment, luckily I have a laptop with FF v32 or I will be obliged to use IE (yuck).