901742 - Make window.crypto an immutable property to content scripts

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, defect)

Description

[David Dahl :ddahl]

In an effort to add some additional "defense in depth" for JS cryptography development, making the crypto object immutable would help with trust issues in the DOM. Perhaps I wrong?

I do wonder if this will have any effect on the official W3C spec.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

If you want that (it is not clear to me why), I guess crypto property in window should 
be [unforgeable] and the Crypto interface should be [unforgeable].
http://dev.w3.org/2006/webapi/WebIDL/#Unforgeable

Comment 2

[Masatoshi Kimura [:emk]]

Doesn't it break scripts using a global variable 'crypto'?

Comment 3

[Boris Zbarsky [:bzbarsky]]

It would, yes.

Comment 4

[David Bruant]

(In reply to David Dahl :ddahl from comment #0)
> In an effort to add some additional "defense in depth" for JS cryptography
> development, making the crypto object immutable would help with trust issues
> in the DOM. Perhaps I wrong?
By that reasoning, we should make pretty much everything immutable.

The threat model here is something like: an untrusted script runs in the global context and modifies the "crypto" of the global object.
First off, you shouldn't let untrusted scripts run in the global context in the first place. You can use something like Caja [1] to encapsulate untrusted scripts so they don't mess with your globals.

In case you're forced to do so (script@src with cross-origin URL), what you can do is something like:

    (function(global){
      "use strict";

      var crypto = global.crypto;

      // whatever
    })(this);

If you run this code before any untrusted code, your "crypto" variable contains a reference to the object you're actually interested in and no untrusted code can mess with it.
  

> I do wonder if this will have any effect on the official W3C spec.
Given that the change you're asking isn't standard (yet?) and emk and bz suggest this change could have web-compat issues, I'm WONTFIX-ing for now. Feel free to reopen if it's considered in the standard or if something changes.


[1] https://developers.google.com/caja/

Comment 5

[Boris Zbarsky [:bzbarsky]]

David, if you're not a module owner or peer for a module, please do not wontfix bugs in it.

Comment 6

[David Bruant]

woop. Ok. My bad :-s

Comment 7

[Florian Bender]

I think this is nonetheless something we should consider, even if only to make window.crypto more predictable. (Sometimes you're constrained in where to put your scripts, i.e. due to performance concerns, and some environments (server or client or network) inject JS into sites so they could override window.crypto whilst you think you have the "real thing".)

Can we have telemetry, to see how much of a web compat issue this is? If it's a small (or even medium sized) issue, we might get away with evangelism. I doubt that there are that many sites that rely on crypto, even less overwrite it (minus polyfills?). 

In any case, would making individual properties of window.crypto, e.g. getRandomValues(), immutable help or make sense at all? Can we have something like crypto itself not being replacable but mutable (i.e. no `crypto = { getRandomValues : 0 }` but allow `crypto.foo = true`)? Together with making all properties of the WebCrypto API being immutable, that'll allow polyfills but makes sure that the "native" functions are always the "real" ones.

Comment 8

[David Bruant]

(In reply to Florian Bender from comment #7)
> I think this is nonetheless something we should consider, even if only to
> make window.crypto more predictable. (Sometimes you're constrained in where
> to put your scripts, i.e. due to performance concerns, and some environments
> (server or client or network) inject JS into sites so they could override
> window.crypto whilst you think you have the "real thing".)
By that reasoning, we should make pretty much everything immutable... hmm... and all of comment #4, especially the code snippet and the paragraphs right before and right after.
Alternatively, you can make "crypto" immutable yourself if that's something you care about.

If you inject untrusted code before yours, the game is over anyway.

> In any case, would making individual properties of window.crypto, e.g.
> getRandomValues(), immutable help or make sense at all? Can we have
> something like crypto itself not being replacable but mutable (i.e. no
> `crypto = { getRandomValues : 0 }` but allow `crypto.foo = true`)? Together
> with making all properties of the WebCrypto API being immutable, that'll
> allow polyfills but makes sure that the "native" functions are always the
> "real" ones.
With the pattern I showed in the code above, you're guaranteed that the inner "crypto" variable always contains the correct object. No way to attack this unless you find a security vulnerability in the JS engine. From this, you can choose to either Object.freeze it yourself (preferably deeply) or isolate separate method.

Comment 9

[Florian Bender]

You said that already, and I get your point (and mostly agree). Though, as I said, it'd make the crypto interface a bit more predictable, that's the least we can gain from that. After all, crypto is a rather special case compared to other primitives, and it'd be a big win if you can rely on the crypto functions being the functions you expect to be (that does not mean that you can rely on them being completely safe when you use them!).

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

You should bring raise this as a spec issue.  If the spec changes we can come back and implement this in Gecko.  But it's definitely not clear that there's any benefit to doing this, so I think we will want to see what other vendors think before doing this.