Closed Bug 440046 Opened 16 years ago Closed 11 years ago

expose secure PRNG in the DOM (window.crypto.getRandomValues)

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
relnote-firefox --- 21+

People

(Reporter: ericjung, Assigned: ddahl)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: [sec-assigned:bsmith])

Attachments

(3 files, 53 obsolete files)

1.41 KB, patch
KaiE
: review+
Details | Diff | Splinter Review
3.15 KB, patch
Details | Diff | Splinter Review
16.82 KB, patch
ddahl
: review+
Details | Diff | Splinter Review
nsCrypto.random() is unimplemented. This method should provide a cryptographically secure pseudo-random number. The interface documentation should be updated to specify more detail about the return type (e.g., is it a base64-encoded hex string of the random bytes, or what?)

window.crypto.random(8) throws an unimplemented exception due to this.

Relevant URLS:

http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsCrypto.cpp#2464

http://mxr.mozilla.org/mozilla-central/source/dom/public/idl/base/nsIDOMCrypto.idl#52

See also bug #354840 and bug #322529
Blocks: 354840
Blocks: 322529
Assignee: nobody → kaie
Component: Security → Security: PSM
OS: Windows XP → All
QA Contact: toolkit → psm
Hardware: PC → All
Shawn Wilsher says we should add a new method to nsIRandomGenerator for this, then delegate nsIDOMCrypto.random() to that new method.
Version: unspecified → Trunk
Version: Trunk → unspecified
If you specify the behavior of this method, I think you should submit it to WHATWG or the w3c webapps group (I don't know what's popular currently).
(In reply to comment #0)
> (e.g., is it a
> base64-encoded hex string of the random bytes, or what?)

Meant to say "base64-encoded string" here, not 'base64-encoded hex string"

(In reply to comment #2)
> If you specify the behavior of this method, I think you should submit it to
> WHATWG or the w3c webapps group (I don't know what's popular currently).
> 

OK. Pointers on where to do that?
Status: NEW → ASSIGNED
Assignee: kaie → eric.jung
Status: ASSIGNED → NEW
(In reply to comment #0)
> nsCrypto.random() is unimplemented. This method should provide a
> cryptographically secure pseudo-random number. The interface documentation
> should be updated to specify more detail about the return type (e.g., is it a
> base64-encoded hex string of the random bytes, or what?)

I think we already have a description of the intended interface behavior. From http://developer.mozilla.org/en/docs/JavaScript_crypto

"For instance, to obtain a ten byte random number using the cryptographic engine, simply call: var myrandom = window.crypto.random(10);"

So, the parameter specifies the number of random bytes you get.
No encoding here, I'd say. Just raw bytes.

If you want a random number between 0 and 1000, you could get 2 bytes, cast to an int (gives you 0-65535), and use number%1000.
Maybe it would be wise to research whether other browsers have already implemented this interface, and whether they all do it the same way. Then we should probably follow.
(In reply to comment #4)


> If you want a random number between 0 and 1000, you could get 2 bytes, cast 
> to an int (gives you 0-65535), and use number%1000.

There's a well known problem with that approach.  It introduces statistically
significant bias into the results. Not all those results are equally probable.

As a cryptographer and a library developer, I think that the most useful sort of random numbers in JS (and the easiest kind to implement) is arrays of 32-bit words.  Obviously in C you can cast left and right, but in JS casting from strings is tricky.  Has window.crypto.random every been implemented, or can we change the spec in WhatWG to be arrays of words?  Or should we create a different interface, window.crypto.randomWords()?
> If you want a random number between 0 and 1000, you could get 2 bytes, cast to
> an int (gives you 0-65535), and use number%1000.
This introduces interesting levels of bias (as Nelson pointed out) it's better that you do Math.floor((number/65536)*1000), that's why random values are returned between 0 and 1, it makes people less likely to use % and so avoids the various subtle bias issues.
(In reply to comment #8)
> > If you want a random number between 0 and 1000, you could get 2 bytes, cast to
> > an int (gives you 0-65535), and use number%1000.
> This introduces interesting levels of bias (as Nelson pointed out) it's better
> that you do Math.floor((number/65536)*1000), that's why random values are
> returned between 0 and 1, it makes people less likely to use % and so avoids
> the various subtle bias issues.

Unfortunately, that's biased too.  What you really want to do is throw out numbers >= 65000, then use either number%1000 or floor(number/65).  However, for cryptographic use, particularly on the client, it isn't all that common to need a number between 0 and 1000... you usually want either a random byte, a random word, or a random number between 0 and some huge prime.
Attached patch Implementation of crypto.random (obsolete) — Splinter Review
This patch (against the Mozilla 1.9.1 branch) implements window.crypto.random as per the specification at https://developer.mozilla.org/en/JavaScript_crypto and using nsIRandomGenerator as suggested in comment #1 (although no new method on nsIRandomGenerator seemed to be required as in comment #1, so perhaps I'm missing something...).

Since this is not (?) a standardized DOM method yet, there are options about what this function should actually return.  My opinion would be to keep this function as it is (returning a string of raw bytes).

We could also consider adding another function that can take an input string containing a base-10 integer and return an output string containing a random base-10 integer modulo the input.  Why strings containing base 10 integers instead of just (32-bit) integer values?  Because in cryptography we often want to work with long integers, hundreds or thousands of bits in length, and there is no standard mechanism in Javascript for representing such integers.  However, safely generating random integers, large or small, modulo a given value is important and mildly tricky as Nelson notes in comment #6, so it may be worth doing right once as opposed to leaving it to Javascript application developers.
Comment on attachment 392058 [details] [diff] [review]
Implementation of crypto.random


> NS_IMETHODIMP
> nsCrypto::Random(PRInt32 aNumBytes, nsAString& aReturn)
> {
>-  return NS_ERROR_NOT_IMPLEMENTED;
>+  nsresult rv = NS_OK;
>+
>+  if (aNumBytes < 0) {
>+    return NS_ERROR_ILLEGAL_VALUE;
>+  }
>+  PRUint32 numBytes = static_cast<PRUint32>(aNumBytes);

I would say that if aNumBytes is zero, just return rv 
and skip the rest.
Revision of patch based on comment #11.
Attachment #392058 - Attachment is obsolete: true
Assignee: eric.jung → douglas
Comment on attachment 392195 [details] [diff] [review]
Implementation of crypto.random (take 2)

I agree that implementing the existing interface is a good idea.

It's funny and maybe unfortunate that original authors described this interface as returning double-byte strings, and I see you call the necessary conversion function to turn your array of bytes into a string of double-byte chars.

Will NS_ConvertASCIItoUTF16 always do the trivial thing, keep the input byte as the lower value byte, and add zero for the higher value byte? That would be good.

I assumse NS_ConvertASCIItoUTF16 will not do any character encoding mappings and transform them to other values, so I'm OK with this implementation.

r=kaie
Attachment #392195 - Flags: review+
> Will NS_ConvertASCIItoUTF16 always do the trivial thing

It does this:

751      *mDestination++ = (output_type)(unsigned_input_type)(*aSource++);

With |output_type| == PRUnichar and |unsigned input type| == |unsigned char|, with aSource being a char* and mDestination being a PRUnichar*.
Specifically, see the LossyConvertEncoding template in xpcom/string/public/nsUTF8Utils.h and its use in AppendASCIItoUTF16 in xpcom/string/src/nsReadableUtils.cpp and the use of AppendASCIItoUTF16 in the NS_ConvertASCIItoUTF16 constructor in xpcom/string/public/nsString.h
This patch is an implementation of crypto.random(numBytes), with proper error handling. This version calls PK11_GenerateRandom directly without any fluff, and handles out-of-memory conditions. It also handles failures in the random number generator correctly (e.g., if the numBytes exceeds PRNG_MAX_REQUEST_SIZE).
Attachment #538301 - Flags: review?
Seeing as how it has been two years since there was any activity on this bug, I thought I'd give it a push along.

Some people have been asking for a cryptographic, high-quality RNG, so it is a little surprising that this bug hasn't been fixed for awhile. The version in attachment 538301 [details] [diff] [review] makes the call to PK11_GenerateRandom directly, and then deals with all of the possible errors.
Comment on attachment 538301 [details] [diff] [review]
Implementation of crypto.random() with proper error handling

Reviews need to have a target, and Kai seems like a sensible one here. Thanks for the patch!
Attachment #538301 - Flags: review? → review?(kaie)
Comment on attachment 538301 [details] [diff] [review]
Implementation of crypto.random() with proper error handling

ddahl has been doing some next-gen browser crypto API work.  In a sense more functionality is better.  But it seems to me APIs should have a complexity budget, especially for technically difficult concepts like crypto, and it seems at least possible that both APIs might exceed it.  It's a possibility worth considering, at the least.
Attachment #538301 - Flags: review?(ddahl)
Comment on attachment 538301 [details] [diff] [review]
Implementation of crypto.random() with proper error handling

Awesome! this is great to see happening. Looks good, however, I should defer to Kai and Brian Smith for any 'real' review here:)
Attachment #538301 - Flags: review?(ddahl)
Attachment #538301 - Flags: review?(bsmith)
Attachment #538301 - Flags: review+
Thanks ddahl for the endorsement! If anybody has questions, I'll continue to monitor the bug to respond.
I haven't reviewed the patch yet. We should consider exposing the same or similar API as WebKit/Chrome. See:

https://bugs.webkit.org/show_bug.cgi?id=22049
http://code.google.com/p/chromium/issues/detail?id=45580
Summary: nsIDOMCrypto.random() unimplemented → expose secure PRNG in window.crypto
Comment on attachment 538301 [details] [diff] [review]
Implementation of crypto.random() with proper error handling

Nit: we can do SetLength on the string and use its buffer as a target of PK11_GenerateRandom access through BeginWriting.  On a failure do Truncate() on that string.
"spec" for what's in WebKit can be found here:

http://wiki.whatwg.org/wiki/Crypto
There are a couple advantages to the http://wiki.whatwg.org/wiki/Crypto that were discussed on the WhatWG mailing list:

1) That design avoids the malloc, which allows for much higher performance for sites that want to generate lots of random numbers (they can re-used the same ArrayBuffer).
2) That design avoids any tricky issues with Unicode characters (e.g., unpaired surrogates).

I considered implementing the crypto.random API as you have, but decided the ArrayBuffer approach was better in a number of dimensions.
This version stuffs the bytes into aReturn, and then "aerates" (fans out) the bytes across the appropriate PRUnichar units. As a result, it avoids an additional buffer allocation, thus being faster. Only one memory allocation is needed (if at all), which is performed at aReturn.SetLength.
Attachment #538301 - Attachment is obsolete: true
Attachment #538301 - Flags: review?(kaie)
Attachment #538301 - Flags: review?(bsmith)
Responsive to Honza Bambas (:mayhemer) and Adam Barth:
This version addresses the concerns/optimizations that you raised. Fully debugged and tested.
@Sean: The http://wiki.whatwg.org/wiki/Crypto approach has zero allocations, which is less than one.  :)

Also, you haven't addressed the unicode unpaired surrogate issue.
Actually, your new implementation uses double the memory!
Not exactly true; ArrayBufferView has to be allocated by someone. In that case, the JS caller. :)

The Unicode unpaired surrogate issue is addressed. All Unicode units are in the range 0-255, so they can't have unpaired surrogates (U+0000 - U+00FF).

I am not seeing how it allocates "double the memory" compared to the last version. Please explain.

Note that this implementation is fully compatible with expectations of other functions such as btoa. btoa expects bytes (characters in the range U+0000 - U+00FF). Anything else generates an NS_ERROR_DOM_INVALID_CHARACTER_ERR.
> Not exactly true; ArrayBufferView has to be allocated by someone. In that case, the JS caller. :)

Indeed, but the ArrayBuffer can be re-used across multiple calls to the API, saving many allocations in the long run.  That's why PK11_GenerateRandom and most randomness APIs have the caller supply the buffer instead of calling malloc themselves.

> I am not seeing how it allocates "double the memory" compared to the last version.

Assuming you're using 16 bits to represent each Unicode character, half the bits in your implementation are zeros (and therefore wasted).  With ArrayBuffers, you get to use all the bits, which means you use half the memory.

I guess I'm missing what the advantage is of using strings to store random binary data.
(In reply to comment #28)
> @Sean: The http://wiki.whatwg.org/wiki/Crypto approach has zero allocations,
> which is less than one.  :)

I am not so sure about the allocation issue. 0 < 1, but we may want to allocate buffers used for cryptographic purposes a different way (e.g. aligned cache line size and/or with different contiguity guarantees than normal) to minimize cache timing attacks. On the other hand, presumably we could ask the JS engine to re-align the buffers as necessary, if this would be helpful.

What could one could use this API for safely without additional API support? Adam, have you investigated this? E.g. I don't think you can implement HMAC safely without an additional guaranteed-constant-time array comparison function. Similarly, I am pretty sure you couldn't implement a timing-attack-resistant AES with it. 

I noticed the comment in the WebKit bug "Folks from Firefox agreed to implement the [WebKit] API" but I'm not sure who agreed to what.
(In reply to comment #32)
> (In reply to comment #28)
> > @Sean: The http://wiki.whatwg.org/wiki/Crypto approach has zero allocations,
> > which is less than one.  :)
> 
> I am not so sure about the allocation issue. 0 < 1, but we may want to
> allocate buffers used for cryptographic purposes a different way (e.g.
> aligned cache line size and/or with different contiguity guarantees than
> normal) to minimize cache timing attacks. On the other hand, presumably we
> could ask the JS engine to re-align the buffers as necessary, if this would
> be helpful.

Yeah, that's not really an issue for this API.  There's a larger question of what sorts of data types ddahl's DOMCrypt API should be written in terms of.  You can certainly imagine an API for allocating AlignedArrayBuffers (or whatever) My inclination is to worry about those issues when we come to them.

> What could one could use this API for safely without additional API support?

http://code.google.com/p/crypto-js/
http://crypto.stanford.edu/sjcl/

These kinds of libraries can go a long way, but they can't really generate strong randomness without help from the browser.

> Adam, have you investigated this? E.g. I don't think you can implement HMAC
> safely without an additional guaranteed-constant-time array comparison
> function. Similarly, I am pretty sure you couldn't implement a
> timing-attack-resistant AES with it. 

I haven't investigated those issues, but those questions seem orthogonal to whether to use DOMStrings or ArrayBuffers as the type to hold binary data.

> I noticed the comment in the WebKit bug "Folks from Firefox agreed to
> implement the [WebKit] API" but I'm not sure who agreed to what.

Brendan Eich said that he wanted to implement the API.
> I guess I'm missing what the advantage is of using strings to store random
> binary data.

Preserves the existing nsIDOMCrypto interface, which hasn't changed in a decade.
Preserves the documentation floating out there on the Internet that says "use window.crypto.random() on Mozilla" (quite unclear why this documentation exists in the first place, given that the API never worked).
Compatible with APIs and callers that use strings rather than new (and not widely implemented--think about backwards compatibility) ArrayBuffer implementations.
Not called getRandomBytes or similar--so someday in the future, somebody else can write something that uses this ArrayBuffer stuff while still preserving the existing API.
ECMAScript rev 3 (~1999)-compatible.
At the end of the day the issue of strings vs. ArrayBuffers is a general JavaScript language issue, one that I don't take a position on here. Just trying to get some attention on a bug that really should have been dealt with in 2002. Maybe if it was solved then, there would be proportionately more attention to optimizing immutable string allocations instead of creating various other abstractions. :)

> Indeed, but the ArrayBuffer can be re-used across multiple calls to the API, > saving many allocations in the long run.

Won't deny it.
Just would like to point out that eventually the randomness has to go *somewhere*. You may keep on re-filling the buffer a million times, but it doesn't matter unless you use that randomness to do something interesting (and likely something that will allocate new buffers anyway).

Typically cryptographic implementations just need a small amount of high-quality cryptographic randomness at a time. Even if you need a "large amount" because you are doing heavy network traffic (streaming an encrypted video over AJAX?) the network bandwidth and latency, not to mention the "interesting" work of manipulating images or video, will clearly dominate the CPU over memory allocations.

In any event, drbg.c PRNG_MAX_REQUEST_SIZE is hardcoded to 65,536 bytes max.
(In reply to comment #34)
> > I guess I'm missing what the advantage is of using strings to store random
> > binary data.
> 
> Preserves the existing nsIDOMCrypto interface, which hasn't changed in a
> decade.

AFAICT, window.crypto.random() doesn't work anywhere, so compatibility is not an issue. The nsIDOMCrypto interface was created when strings were the only choice. Array buffers seem clearly better to me. 

> Compatible with APIs and callers that use strings rather than new (and not
> widely implemented--think about backwards compatibility) ArrayBuffer
> implementations.

You can always convert a typed array into a string and carry on as before.
(In reply to comment #34)
> (In reply to comment #31)
> > Indeed, but the ArrayBuffer can be re-used across multiple calls
> > to the API, saving many allocations in the long run.
> 
> Won't deny it. Just would like to point out that eventually the randomness
> has to go *somewhere*. [...] (and likely [do] something that will allocate
> new buffers anyway).

Technically, non-allocating API is preferable, because non-allocating call can easily be wrapped [by caller] into another call, with allocation strategy of his choice.

The inverse is impossible, as well as replacing allocation strategy.
This patch tries to follow the WHATWG spec for window.crypto.random using an ArrayBuffer

See: http://wiki.whatwg.org/wiki/Crypto#getRandomValues 

I will write tests tomorrow, just looking for feedback for now
Attachment #546927 - Flags: feedback?(khuey)
Looks like the method (in the whatwg wiki) should actually be called getRandomValues() - so maybe this is not the correct bug for this work?
Comment on attachment 546927 [details] [diff] [review]
v 0.1 WIP implement random() as described in whatwg spec

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

::: security/manager/ssl/src/nsCrypto.cpp
@@ +2513,5 @@
> +  }
> +  
> +  PRUint8* resultbuffer;
> +  // TODO: what length to use here?
> +  rv = rg->GenerateRandomBytes(16, &resultbuffer);

I would have expected you to be able to generate the random data directly into the arraybuffer without needing to do a memcpy later.  Also, arraybuffers have an length, so that's how much randomness you should generate.
(In reply to comment #38)

This is the correct bug, but I think that the bug itself should be changed to "implement crypto.getRandomValues()". Also, yes, it should be named getRandomValues() instead of random().
(In reply to comment #39)
> I would have expected you to be able to generate the random data directly
> into the arraybuffer without needing to do a memcpy later.  
Ah, cool. I will fix that. I was looking at some other uses of ArrayBuffer on mxr...

> Also,
> arraybuffers have an length, so that's how much randomness you should
> generate.
Ok, will do. thanks.
Comment on attachment 546927 [details] [diff] [review]
v 0.1 WIP implement random() as described in whatwg spec

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

I'll prefix by saying that I haven't read the spec, only the bug and your patch.

::: dom/interfaces/base/nsIDOMCrypto.idl
@@ +49,5 @@
>    DOMString                 importUserCertificates(in DOMString nickname,
>                                                     in DOMString cmmfResponse,
>                                                     in boolean doForcedBackup);
>    DOMString                 popChallengeResponse(in DOMString challenge);
> +  void                      random(in jsval aData);

Is this call supposed to modify the input (which is what it does, from the code)?  If so, this should be an inout param.

::: security/manager/ssl/src/nsCrypto.cpp
@@ +2464,5 @@
>    return NS_ERROR_NOT_IMPLEMENTED;
>  }
>  
>  NS_IMETHODIMP
> +nsCrypto::Random(const jsval& aData)

Once you change this to be an inout param it won't be const anymore.

@@ +2469,2 @@
>  {
> +  // TODO: do I need to unwrap this object?

No.

@@ +2480,5 @@
> +        NS_WARNING("error: crypto.random() cannot get ArrayBuffer");
> +        return NS_OK;
> +      }
> +    }
> +  }

You should turn this inside out and just early return if it's not an arraybuffer.

@@ +2500,5 @@
> +  if (!cx) {
> +    NS_WARNING("error: internalError");
> +
> +    return NS_OK;
> +  }

You don't need any of this.  Just mark your method with [implicit_jscontext] and XPConnect will pass the JSContext to you.

@@ +2509,5 @@
> +    do_GetService("@mozilla.org/security/random-generator;1", &rv);
> +  if (NS_FAILED(rv)) {
> +    JS_ReportError(cx, "%s%s\n", JS_ERROR, "unable to continue without random generator");
> +    return NS_OK;
> +  }

Don't JS_ReportError, just return a failure code here.  XPConnect will translate that into an exception.

@@ +2513,5 @@
> +  }
> +  
> +  PRUint8* resultbuffer;
> +  // TODO: what length to use here?
> +  rv = rg->GenerateRandomBytes(16, &resultbuffer);

You really want a non-allocating wrapper over PK11_GenerateRandom here ....

Once you have that, you should just pass in JS_GetArrayBufferData(buffer) as the void* and  JS_GetArrayBufferByteLength(buffer) for the length.

The idea is that the caller passes in a blank ArrayBuffer with a preset length, right?

@@ +2517,5 @@
> +  rv = rg->GenerateRandomBytes(16, &resultbuffer);
> +  if (NS_FAILED(rv)) {
> +    JS_ReportError(cx, "%s%s\n", JS_ERROR, "random data generation failed");
> +
> +    return NS_OK;

Again, just propagate the failure code back.

Eventually we'll fix XPConnect's exception handling to reflect spec DOMExceptions instead of Mozilla error codes.

@@ +2522,5 @@
> +  }
> +  
> +  // fill in the ArrayBuffer with the random bytes
> +  memcpy(JS_GetArrayBufferData(buffer), resultbuffer, 16);
> +  // NS_FREE(resultBuffer);

And once you have a non-allocating call you'll be able to skip this.
Attachment #546927 - Flags: feedback?(khuey)
What is the plan for the e10s (mobile) version? I think we need the e10s version in the same release that we ship the non-e10s version. (Doesn't need to be done at exactly the same time.)

Mini preparation for security review:

1. Should we restrict the maximum number of random bytes that should be generated--at once and/or per document? The application is almost definitely doing something wrong if it is generating more than a few dozen at a time. If we limit the maximum number we return (at once) then we don't have to worry about performance. Otherwise, how long does it take to generate hundreds of megabytes of random values? We must be careful to avoid allowing scripts to make Firefox unresponsive using this API.

2. Do we need to worry about entropy problems if the application makes us generate too many total random bytes?

3. Should we attempt to make the state for each origin completely independent, to avoid any cross-origin attacks on the PRNG implementation and/or attacks on our other crypto usages (SSL, in particular)?
For what it's worth, the way we address these issues in WebKit is by using a separate RC4 instance that periodically re-filled it's entropy from the OS.  I'm not sure if that's appropriate for Firefox, but I thought I'd mention it.
(In reply to comment #43)
> 1. Should we restrict the maximum number of random bytes that should be generated--at once and/or per document?

What if a script actually needs that many random bytes (think a TrueCrypt webapp, albeit impractical)? Unfortunately, the API is synchronous so you'll just have to let the application become unresponsive until the API is revised to accommodate generating many bytes asynchronously.
If we're expecting to generate significant quantities of random numbers we should just make this asynchronous from the beginning.
(In reply to comment #45)
> (In reply to comment #43)
> > 1. Should we restrict the maximum number of random bytes that should be generated--at once and/or per document?
> 
> What if a script actually needs that many random bytes (think a TrueCrypt
> webapp, albeit impractical)? Unfortunately, the API is synchronous so you'll
> just have to let the application become unresponsive until the API is
> revised to accommodate generating many bytes asynchronously.

No, we can limit the number of bytes that we generate at once, and make the application make multiple calls if it needs a huge number (that way, we can process events between the calls).

(In reply to comment #46)
> If we're expecting to generate significant quantities of random numbers we
> should just make this asynchronous from the beginning.

It is very unusual to generate such a large number of random bytes. I would rather put the burden on those unusual applications to jump through some hoops, rather than complicate the vast majority of applications--especially since complication leads to bugs and bugs in this area can be very serious.
> > > 1. Should we restrict the maximum number of random bytes that should be generated--at once and/or per document?
> > 
> > What if a script actually needs that many random bytes (think a TrueCrypt
> > webapp, albeit impractical)? Unfortunately, the API is synchronous so you'll
> > just have to let the application become unresponsive until the API is
> > revised to accommodate generating many bytes asynchronously.
> 
> No, we can limit the number of bytes that we generate at once, and make the
> application make multiple calls if it needs a huge number (that way, we can
> process events between the calls).

To clarify: the problem isn't the application becoming unresponsive; the problem is the application causing Firefox to become unresponsive, which is unacceptable.
(In reply to comment #47)
> No, we can limit the number of bytes that we generate at once, and make the
> application make multiple calls if it needs a huge number (that way, we can
> process events between the calls).

That sounds fine to me, but that behavior should be specced.
(In reply to comment #47)
> It is very unusual to generate such a large number of random bytes. I would
> rather put the burden on those unusual applications to jump through some
> hoops

Applications don't know anything about the PRNG capabilities, so that won't help. The application won't know how many bytes it should request at a time. An async API (possibly with progress events, though that may be a little overkill) is the best solution in this case. We should submit an async proposal to WhatWG and just implement sync for now.
(In reply to comment #43)
> What is the plan for the e10s (mobile) version? I think we need the e10s
> version in the same release that we ship the non-e10s version. (Doesn't need
> to be done at exactly the same time.)

CCing mfinkle and dougt for e10s advice

I'm thinking a new bug should be filed for the e10s (mobile) version.
Assuming that the v0.1 patch is the only one requiring e10s investigation, the only problem here is that the nsIRandomGenerator service is unavailable in content processes. It would be a simple matter to send a synchronous IPDL message that instantiates nsIRandomGenerator in the chrome process, generates some number of bytes, and returns those back over the wire.
Adam, is it the case that you implemented this using RC4 because you didn't have any crypto library linked into WebKit and you wanted a simple implementation?

I also saw this comment in the webkit bug: "Implementing this functionality on Window object alone is not ideal, because workers (http://whatwg.org/ww) won't have access to it then." How would a worker access this API, since workers do not have access to the window object?
> Adam, is it the case that you implemented this using RC4 because you didn't have any crypto library
> linked into WebKit and you wanted a simple implementation?

I implemented it that way because I wanted a buffer between web sites interacting with randomness and the OS entropy pool.  Also, in WebKit, the call to get randomness from the OS can be slow, depending on the platform.

> I also saw this comment in the webkit bug: "Implementing this functionality on Window object alone
> is not ideal, because workers (http://whatwg.org/ww) won't have access to it then." How would a
> worker access this API, since workers do not have access to the window object?

Eventually we should make the crypto object available to workers, but there's no particular rush on that.
AFAIK, secure random gens work this way: collect real entropy usually from the system, then seed a linear gen with it, hash/hmac/what-ever the results, occasionally re-seed with entropy.  There should be approved specs on algorithms like these.  Advantages: we collect entropy only in small doses, which is a must, otherwise it might lead to block of the demanding thread (=Firefox' main thread) ; we can have seed values and generators instantiated on each process/window and keep them this way separated each from other protecting us from any cross attacks.  Also would be a good e10s solution as the generators would run on content processes, reseeding can be driven by the chrome process sometimes sending async messages to content processes (the last point would need a sec review).
That's is the way arc4random works on BSD systems -- it uses the arc4 seed generator periodically reseeding from a "true" random source (i presume they mean something equivalent to /dev/urandom rather than time() but these days who knows? ;) ).  Per the man page the stream generator has  ~2^1700 internal states, so theoretically a single seed point should be sufficient to avoid touching real random ever again ;)

WebKit uses the BSD arc4random internally and a quick look at the code implies it reseeds from hardware every 1.6 million queries.  Presumably this could be further improved by having a secondary thread periodically querying the true random source in preparation for subsequent stirring.  Such a model should be able to get to essentially non-blocking behaviour.
If y'all want a library to do this "properly", I'm working on one over at

https://github.com/bitwiseshiftleft/crandom

The goal is to be:
cross-platform -- not there yet... should compile for any CPU but needs Windows support
secure -- 256-bit AES or ChaCha12, cannot be run in reverse if compromised
simple -- currently ~1kloc for the lib, including variants for several architectures
fast -- hundreds of MB/s, call overhead is a bottleneck
slim -- <256B storage

It's still a work in progress, but if you have any particular requirements (Windows support...), I'm happy to add them so you can get started with it.

</self-promotion>

Alternatively, you can port the BSD arc4random code.  It's even simpler, plenty fast, and should be secure enough.
Added the GetRandomValues method

Changed to PK11_GenerateRandom to avoid allocation. Throws an error if the ArrayBuffer's length is over 65K
Attachment #546927 - Attachment is obsolete: true
Attachment #547264 - Flags: feedback?(khuey)
(In reply to comment #57)
> If y'all want a library to do this "properly", I'm working on one over at
> 

While I am not a cryptographer, I am sure NSS does this properly.
Comment on attachment 547264 [details] [diff] [review]
v 0.2 Switched to PK11_GenerateRandom

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

The DOMish bits look ok.  I'm assuming that the NS_WARNINGs are for debugging and that you don't intend to leave them in.

You still need to handle bsmith's security issues, of course :-)

::: dom/interfaces/base/nsIDOMCrypto.idl
@@ +50,5 @@
>                                                     in DOMString cmmfResponse,
>                                                     in boolean doForcedBackup);
>    DOMString                 popChallengeResponse(in DOMString challenge);
>    DOMString                 random(in long numBytes);
> +  void                      getRandomValues(in jsval aData);

ddahl tells me that inout jsval doesn't work on IRC.  File a bug on that?

::: security/manager/ssl/src/nsCrypto.cpp
@@ +2477,5 @@
> +  // Is it an object?
> +  if (!JSVAL_IS_OBJECT(aData)) {
> +    NS_WARNING("error: crypto.getRandomValues() expects an ArrayBuffer as first argument");
> +    return NS_ERROR_FAILURE;
> +  } else {

no else after return.

@@ +2484,5 @@
> +    // Is it an array buffer?
> +    if (!js_IsArrayBuffer(obj)) {
> +        NS_WARNING("error: crypto.getRandomValues() expects an ArrayBuffer as first argument");
> +        return NS_ERROR_FAILURE;
> +    } else {

again

@@ +2489,5 @@
> +      buffer = js::ArrayBuffer::getArrayBuffer(obj);
> +      if (!buffer) {
> +        NS_WARNING("error: crypto.getRandomValues() cannot get ArrayBuffer");
> +        return NS_ERROR_FAILURE;
> +      }

We should assert that buffer is not null here.

@@ +2496,5 @@
> +
> +  nsresult rv;
> +
> +  PRUint32 len = static_cast<PRUint32>(::JS_GetArrayBufferByteLength(buffer));
> +  PRUint8* data = static_cast<unsigned char*>(::JS_GetArrayBufferData(buffer));

You'll need to do some sort of sanity checking here, presumably.

@@ +2500,5 @@
> +  PRUint8* data = static_cast<unsigned char*>(::JS_GetArrayBufferData(buffer));
> +
> +  rv = PK11_GenerateRandom(data, len);
> +
> +  if (NS_FAILED(rv)) 

Don't use nsresults for something that is an SECStatus.
Attachment #547264 - Flags: feedback?(khuey) → feedback+
(In reply to comment #59)
> While I am not a cryptographer, I am sure NSS does this properly.

Oh, OK, I thought you were concerned than NSS would block and/or drain system entropy.  Carry on...
(In reply to comment #61)
> Oh, OK, I thought you were concerned than NSS would block and/or drain
> system entropy.  Carry on...

I think it does block, and that is an issue for sure in a world of e10s
(In reply to comment #43)
> Mini preparation for security review:
> 
> 1. Should we restrict the maximum number of random bytes that should be
> generated--at once and/or per document? 

It looks like PRNG_MAX_REQUEST_SIZE is pegged at 65536, which seems like a hard limit for how the prng is designed (I could be wrong, just a quick run through gdb tells me this). In my tests - on a very fast core i7 running linux - a debug build takes 2ms to generate 65536 random values.
The patch doesn't look compatible with the spec and WebKit's implementation [1].

getRandomValues expects an ArrayBufferView, not an ArrayBuffer.
In SpiderMonkey it needs to be checked (confusingly) with js_isTypedArray and then use TypedArray::getBuffer and TypedArray::getByteOffset to fill the correct 'view' range with the random bytes.



[1] http://trac.webkit.org/browser/trunk/Source/WebCore/page/Crypto.cpp
(In reply to comment #64)
> The patch doesn't look compatible with the spec and WebKit's implementation
> [1].
> 
> getRandomValues expects an ArrayBufferView, not an ArrayBuffer.
> In SpiderMonkey it needs to be checked (confusingly) with js_isTypedArray
> and then use TypedArray::getBuffer and TypedArray::getByteOffset to fill the
> correct 'view' range with the random bytes.

Ah, right. I was wondering about that. In which case, the TypedArray API seems very confusing. I don't even see any actual uses of it in mxr. I am getting some errors using it like so: 

/home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.cpp: In member function ‘virtual nsresult nsCrypto::GetRandomValues(const jsval&, JSContext*)’:
/home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.cpp:2488:12: error: ‘getBuffer’ is not a member of ‘js::TypedArray’
/home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.cpp:2496:25: error: ‘getByteLength’ is not a member of ‘js::TypedArray’
/home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.cpp:2503:40: error: ‘getByteLength’ is not a member of ‘js::TypedArray’
/home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.cpp:2504:47: error: ‘getByteOffset’ is not a member of ‘js::TypedArray’

The code looks like:

// Is it an ArrayBufferView?
  if (!js_IsTypedArray(obj))
    return NS_ERROR_FAILURE;
 
  buffer = js::TypedArray::getBuffer(obj);

...

JSUint32 buffer_len = js::TypedArray::getByteLength(buffer);

...

PRUint32 len = static_cast<PRUint32>(::js::TypedArray::getByteLength(buffer));
  PRUint8* data = static_cast<unsigned char*>(::js::TypedArray::getByteOffset(buffer));
 
I hear this API is voodoo, it also doesn't help that I spend 99% of my time in JS:)

pastebin: http://pastebin.mozilla.org/1277804
Sorry my mistake.
It is possible to do something like:

JSObject *obj = JSVAL_TO_OBJECT(val);
if (!js_IsTypedArray(obj))
  return NS_ERROR_FAILURE;
js::TypedArray *typedArray = js::TypedArray::fromJSObject(obj);

And then use directly:

typedArray->byteLength
typedArray->data
(In reply to comment #66)
> ...
> And then use directly:
> 
> typedArray->byteLength
> typedArray->data

Thanks!

Adam:

How does the google implementation handle the number of values returned? Is there a MAX values returned? Do you throw if the ArrayBuffer's length is more than the MAX? Or do you just re-create the ArrayBufferView with it's length set to the MAX you can generate?
> How does the google implementation handle the number of values returned? Is
> there a MAX values returned? Do you throw if the ArrayBuffer's length is
> more than the MAX? Or do you just re-create the ArrayBufferView with it's
> length set to the MAX you can generate?

There isn't a MAX in the WebKit implementation.  Having a max makes a bunch of sense.  IMHO, we should throw if you pass in an array that's bigger than the max.  Once your patch lands, I'll change WebKit to match.
Attached patch v 0.3 Uses ArrayBufferViews (obsolete) — Splinter Review
Just posting a WIP, need more validation on arrayBufferView types, also need to report better errors. tests to follow.
Attachment #547264 - Attachment is obsolete: true
thanks to the discussion in bug 673267, I fixed the method signature problem where I was unable to use an inout in the IDL. The method sig is: nsCrypto::GetRandomValues(jsval* aData, JSContext* aCtx)
 
However, now, JSVAL_TO_OBJECT is raising an ASSERTION:

Breakpoint 1, nsCrypto::GetRandomValues (this=0x7fffdb47b200, aData=0x7fffffff53e0, aCtx=0x7fffd24fa000) at /home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.cpp:2481
2481	  obj = JSVAL_TO_OBJECT(*aData);
(gdb) print aData
$1 = (jsval *) 0x7fffffff53e0
(gdb) x 0x7fffffff53e0
0x7fffffff53e0:	0x00000000
(gdb) n
Assertion failure: JSVAL_IS_OBJECT(v), at ../../../../dist/include/jsapi.h:220

Program received signal SIGABRT, Aborted.
0x00007ffff7bcdb3b in raise () from /lib/x86_64-linux-gnu/libpthread.so.0
(gdb) n
Single stepping until exit from function raise,
which has no line number information.
nsProfileLock::FatalSignalHandler (signo=0, info=0x7fffffff4ae0, context=0x0) at /home/ddahl/code/moz/obj-x86_64-unknown-linux-gnu-debug/toolkit/profile/nsProfileLock.cpp:167
167	{
(gdb) n
169	    RemovePidLockFiles(PR_TRUE);
(gdb) n
[Thread 0x7fffd4bfe700 (LWP 9553) exited]
172	    struct sigaction *oldact = nsnull;
(gdb) c
Continuing.

Program /home/ddahl/code/moz/obj-x86_64-unknown-linux-gnu-debug/dist/bin/firefox (pid = 9520) received signal 6.
Stack:
UNKNOWN [/lib/x86_64-linux-gnu/libpthread.so.0 +0x0000FC60]
raise+0x0000002B [/lib/x86_64-linux-gnu/libpthread.so.0 +0x0000FB3B]
JS_Assert+0x0000005B [/home/ddahl/code/moz/obj-x86_64-unknown-linux-gnu-debug/dist/bin/libxul.so +0x0240150B]


I attached to the thread and there was no stack:(
Did you adjust the types so that you're passing JSVAL_IS_OBJECT/JSVAL_TO_OBJECT a jsval and not a jsval*?
(In reply to comment #71)
> Did you adjust the types so that you're passing
> JSVAL_IS_OBJECT/JSVAL_TO_OBJECT a jsval and not a jsval*?

That did not work for me, so, for now I have punted on the method signature:
/home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.cpp: In member function ‘virtual nsresult nsCrypto::GetRandomValues(jsval*, JSContext*)’:
/home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.cpp:2476:29: error: conversion from ‘jsval*’ to non-scalar type ‘jsval’ requested
/home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.cpp:2479:40: error: conversion from ‘jsval*’ to non-scalar type ‘jsval’ requested

Also, there is this: http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLAudioElement.cpp#187

Which is almost the same as what I am doing - the const jsval is used to create a new object anyway, correct? So is there harm in it being a const? The AudioAPI also just uses an in param.
(In reply to comment #72)
> Also, there is this:
> http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/
> nsHTMLAudioElement.cpp#187
> 
> Which is almost the same as what I am doing - the const jsval is used to
> create a new object anyway, correct? So is there harm in it being a const?
> The AudioAPI also just uses an in param.

Sure, but that's not modifying the original value ...

It may work with it as an in param, but that doesn't mean it's correct, or that that won't break in the future.
Upon reflection, I somewhat regret kicking the hornet's nest on this bug. Originally I wanted to help resolve an NS_ERROR_NOT_IMPLEMENTED issue but there are several intractable security problems with exposing cryptographic RNGs to webpages.

As I stated last month (and as some people appear to have "re-discovered"), there is a hard stop in PK11_GenerateRandom to generate more than 64KB of data at once. This is likely for three reasons: a) to reduce the entropy drain of applications, b) to prevent DoS when too many random values are requested (due to a) and because even if a PRNG is used, it takes time to generate values), and c) there aren't practical uses of large quantities of cryptographic-quality random values. Most cryptographic algorithms are satisfied with a small quantity (e.g., 128 to 4096 bits) of randomness. Yet there are some people who appear to want huge quantities. It is unclear what the point is. If you want huge quantities, you don't really need cryptographic entropy, and it would take forever to generate (i.e., bits derived from processes that are computationally infeasible to model, such as lava lamps or Blum Blum Shub). If you want large quantities, then use something like the Mersenne twister algorithm, which will get you high quality random numbers (but not cryptographic quality random numbers) very fast. These metrics are fundamentally at odds with one another.

Extracting entropy out of the system, particularly out of the same pool, is badness. Both my original implementation and the various other implementations proposed suffer from the same problem. These naïve implementations allow any webpage to extract an unlimited quantity of entropy from the system, which it can then use to predict future values (and depending on the implementation, past values as well). Don't underestimate the ingenuity of attackers or grad students who are hungry for publications in this regard. The smallest detectable bias in the output can be magnified and exploited by extracting additional bits--and then if the SSL thread takes its bits from the same pool, those bits can be predicted. Past values may be vulnerable as well depending on the implementation. The goal is not to predict all the bits but to reduce the likely search space from 2^128 to something smaller that is computationally feasible given other searches or attacks. (All this is arguably made worse by having all this happen asynchronously, because the user won't even be able to notice the slowdown in the browsing experience.)

Chrome's implementation is superficially better in that it uses some kind of same-origin segmentation. But not much better, a point I may address in a follow-on post if time permits.

The easiest solution out is just to have the server generate the random bits. That way, the webpage can also be assured that the randomness isn't borked by a bad underlying client implementation (of which there are many examples).

I am not going to dismiss this effort is completely futile, but I think there were probably very good reasons why nsCrypto::Random() was never implemented in the first place (although the only way to find out is probably to ask the original maintainer ca. 2002). First the goals need to be clearly delineated. If you want large quantities of randomness for things like the next Game of Life(tm) or some random sparklies on the screen, then don't ask for cryptographic quality ones, and don't put it on any "crypto"-oriented object. If you want small quantities of true cryptographic quality randomness, then there is a lot more work that has to be done. At a minimum, only cryptographic RNGs that have been thoroughly analyzed in the literature should be implemented. A skilled cryptographer should do a detailed cryptanalysis of the implementation before it goes into production. This is a lot more work than is represented in this bug in its current form.
IMHO, your post is just FUD.  If you're worried about these issue, just add an intermediate generator like we did in WebKit.  Getting entropy from the server is problematic if you want to use that entropy to hide things from the server (e.g., the secure chat use case) or of there's no server available (e.g., the offline use case).
This patch (for crypto.getRandomValues) may still need some GC functionality and other things I am unaware of.
Attachment #547520 - Attachment is obsolete: true
Attachment #547820 - Flags: review?(kaie)
Sean, you're right that there's little use for huge amounts of cryptographic randomness.  But I agree with Adam Barth: if this is implemented correctly, there will not be an attack, and good randomness on the client will open up opportunities that getting entropy from a server will not.

If you're worried, just seed an intermediate generator (something fast and cryptographic, so NOT Mersenne twister or Blum Blum Shub) from the system entropy pool, one for each page that requests randomness.  Even if that generator is completely broken and its state is recovered, that's only 256 bits revealed from the system pool.  And if you do it right, the intermediate generator will be essentially unbreakable anyway.
I definitely think we should be doing a separate prng per window or per origin.  That's what Math.random does, for similar reasons...
(In reply to comment #55)
> which is a must, otherwise it might lead to block of the demanding thread
> (=Firefox' main thread)

I filed bug 673706. IMO, if this is possible, then we need to change the implementation of PK11_GenerateRandom (and/or Softoken) or find an alternative solution that prevents this blocking.

> we can have seed values and generators instantiated on each process/window 
> and keep them this way separated each from other protecting us from any
> cross attacks.

+1

> Also would be a good e10s
> solution as the generators would run on content processes

We should continue to do the generation in the chrome process. Currently Gecko is designed to have all crypto state living in the PKCS#11 tokens and accessed through pk11wrap (part of NSS). This is how we ensure that the FIPS mode of operation is possible. Content processes currently don't link to NSS. If we have content processes doing the generation themselves then we would either need to link them to NSS, create a new FIPS-certified PRNG, or decide that FIPS certification is not important for this API. If we choose this latter option, then some distributors of Firefox (on Linux) will probably ask for an option to disable this API, which is something we should definitely avoid.
(In reply to comment #69)
> const JSUint32 MAX_BUFFER_LENGTH = static_cast<JSUint32>(65536);

Let's not hard-code this limit (which might not be correct, depending on the configuration of Firefox's PKCS#11 tokens).

(In reply to comment #77)
> Sean, you're right that there's little use for huge amounts of cryptographic
> randomness.  But I agree with Adam Barth: if this is implemented correctly,
> there will not be an attack, and good randomness on the client will open up
> opportunities that getting entropy from a server will not.

When DOMCrypt is implemented, there will be other ways for a script to do similar attacks--e.g. by generating creating many keys/keypairs. And, even now without DOMCrypt, an attacker can get similar effects (more slowly) by creating many TLS connections or by sending a lot of small messages with WebSockets.

We also need to consider bug 524596 and bug 420964.
In reply to comment 57:
I believe NSS's DRBG (PRNG) meets your criteria today.

In reply to comment 74:
The reason that NSS limits the amount of output for any single call to 2^16 bytes, or 2^19 bits, is because that's what NIST requires to get it FIPS 140-2
validated.  Search for max_number_of_bits_per_request in table 2, p. 34-45 of
http://csrc.nist.gov/publications/nistpubs/800-90/SP800-90revised_March2007.pdf
Oh, I meant to add, NSS forces the DRBG to be reseeded at least once every 
2^48 requests, each of which may return up to 2^19 bits.  IMO, a browser is
unlikely to hit this limit in any one session.

All these limits come from table 2 in NIST SP 800-90 2007.
From a cryptographic perspective, using a different seed per origin/window is unnecessary.  If a cryptographic attack existed against NSS's DRBG when using the same seed, that would still be considered enough of an attack for the whole DRBG -- in all of its usage contexts -- to be considered insecure and needing replacement.  Either you trust that the DRBG is safe to use in any context, or not safe at all.  

At best, using a different seed per origin/window makes a cryptographic attack slightly harder, in that an attacker has to be creative in getting lots of bits generated from the same seed.  But as noted above in comment 80, an attacker can already get lots of bits generated from the main (only) seed by causing lots of SSL sessions to be initiated, and they expose random bits directly in the client.random value in the SSL handshake.
Hi Douglas, I would really vote in favor of having proxy RNGs rather than exposing the system's one. I still don't understand the need for a crypto-strength RNG accessible via javascript (i.e. by the server).. but.. if it *really* has to happen, by implementing proxy ones and limiting them to each window / origin (maybe origin is better ?) you can mitigate attacks coming from the server (generating a lot of SSL handshakes is a little more noticeable than just executing a JS function.. :D).

With respect of comment #80, I am also *very very* worried about exposing DOMCrypt to pages because of the security nightmares that will inevitably follow.. I do really hope that DOMCrypt implementation (if it really has to happen.. (bad idea)), will restrict the objects DOMCrypt is allowed to operate with/on to the single page/domain and use only proxy RNGs... I fear we're going to open another iframe/XMLHttpRequest()-like security can of worms... but that's another story... :(
(In reply to comment #85)
> With respect of comment #80, I am also *very very* worried about exposing
> DOMCrypt to pages because of the security nightmares that will inevitably
> follow.. I do really hope that DOMCrypt implementation (if it really has to
> happen.. (bad idea)), will restrict the objects DOMCrypt is allowed to
> operate with/on to the single page/domain and use only proxy RNGs

Indeed, the design calls for single-origin keypairs. The spec is here: https://wiki.mozilla.org/Privacy/Features/DOMCryptAPISpec/Latest

also see bug 649154
The tests are crashing Firefox - we create a TypedArray, fill it with random values via PK11_GenerateRandom and then when you try to access the ArrayBufferView, Firefox crashes.  I am not sure if this is related, but I get Assertion failure: isInt32(), at /home/ddahl/code/moz/mozilla-central/js/src/jsvalue.h:592 in the console. I will attach a bt.
Attachment #547820 - Attachment is obsolete: true
Attachment #551189 - Flags: feedback?(luke)
Attachment #547820 - Flags: review?(kaie)
Attached file Crash Backtrace (obsolete) —
Luke:

I reproduce this by entering this into the web console:

var buf = new ArrayBuffer(32);
var arBuf = new Int8Array(buf);
window.crypto.getRandomValues(arBuf);
arBuf;
cc'd Nikhil Marathe and mrbkap as this crash happens since the recent TypedArray changes.

If you would like a new bug filed, let me know.
Comment on attachment 551189 [details] [diff] [review]
v 0.5 Updated for the jsapi changes to js::TypedArray

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

I'm not really a typed-array expert, so you'd probably want to find one.  But since I'm here:

::: security/manager/ssl/src/nsCrypto.cpp
@@ +2477,5 @@
> +  // Is it an object?
> +  if (!JSVAL_IS_OBJECT(aData))
> +    return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
> +
> +  JSObject* obj = JSVAL_TO_OBJECT(aData);

Confoundingly, JSVAL_IS_OBJECT(JSVAL_NULL) is true.  What you want above is: if (JSVAL_IS_PRIMITIVE(aData)).
Attachment #551189 - Flags: feedback?(luke)
Whiteboard: [sr:bsmith]
Whiteboard: [sr:bsmith]
(In reply to Luke Wagner [:luke] from comment #91)
> Comment on attachment 551189 [details] [diff] [review]
> Confoundingly, JSVAL_IS_OBJECT(JSVAL_NULL) is true.  What you want above is:
> if (JSVAL_IS_PRIMITIVE(aData)).

Ok, made this change and pull --updated my repo after a few days away and I see no crashes happening.
Attached file Backtrace 2 (obsolete) —
I spoke too soon, this crash happened on quit!
Attachment #551190 - Attachment is obsolete: true
Attached patch v 0.6 Latest patch (obsolete) — Splinter Review
Attachment #551189 - Attachment is obsolete: true
Depends on: 678515
Comment on attachment 552528 [details] [diff] [review]
v 0.6 Latest patch

>+  JSUint32 buffer_len = static_cast<JSUint32>(JS_GetTypedArrayLength(tarray));
>+  
>+  // Abort if the ArrayBufferView is requesting too many or too few values
>+  if (buffer_len < MIN_BUFFER_LENGTH || buffer_len > MAX_BUFFER_LENGTH) {
>+    return NS_ERROR_ABORT;
>+  }
>+
>+  PRUint32 len = static_cast<PRUint32>(JS_GetTypedArrayLength(tarray));

Why call JS_GetTypedArrayLength(tarray) twice?

>+  PRUint8* data = static_cast<unsigned char*>(JS_GetArrayBufferData(tarray));

tarray is the view, not the buffer, right?

/be
(In reply to Brendan Eich [:brendan] from comment #95)
> Comment on attachment 552528 [details] [diff] [review]
> v 0.6 Latest patch
> 
> >+  JSUint32 buffer_len = static_cast<JSUint32>(JS_GetTypedArrayLength(tarray));
> >+  
> >+  // Abort if the ArrayBufferView is requesting too many or too few values
> >+  if (buffer_len < MIN_BUFFER_LENGTH || buffer_len > MAX_BUFFER_LENGTH) {
> >+    return NS_ERROR_ABORT;
> >+  }
> >+
> >+  PRUint32 len = static_cast<PRUint32>(JS_GetTypedArrayLength(tarray));
> 
> Why call JS_GetTypedArrayLength(tarray) twice?
> 
> >+  PRUint8* data = static_cast<unsigned char*>(JS_GetArrayBufferData(tarray));

yeah - that is a mistake, I just need the cast to PRUint8 here

> 
> tarray is the view, not the buffer, right?
> 
Correct. I must be using the new TypedArray API incorrectly
It looks like I never write to the ArrayBufferView's array data. The result 'data' in the patch is always an initial zero'd-out array. Should I not be using the JS_-prefixed API? Is this an internal API?
A bit puzzled why this patch crashed during the test run. Also, the resulting arrayBufferView is not updated with the random values. It does not seem like the TypedArray api has changed all that much, but I must be using it wrong.

Side note: the changes to a js content script in this patch is only while testing with a debug build. (felipe tells me it is due to e10s while I am not running an e10s build - that change will be reverted.)
Attachment #552528 - Attachment is obsolete: true
Attachment #553192 - Flags: feedback?(brendan)
Comment on attachment 553192 [details] [diff] [review]
v 0.7 was getting the typedarray data twice

>+  PRUint8* data = static_cast<unsigned char*>(JS_GetArrayBufferData(view));

I believe this should be:

  PRUint8* data = static_cast<unsigned char*>(JS_GetArrayBufferData(JS_GetTypedArrayBuffer(view)));

The SpiderMonkey typed array APIs seem to lose type safety by indirecting via JSObject*, and they don't assert correct class to make up for that at runtime in debug builds. Please file a bug.

/be
Filed Bug 679112 - crash: SpiderMonkey typed array APIs seem to lose type safety based on above feedback
debugging with brendan. The only other difference was manually editing objdir/dist/include/nsIDOMCrypto.h to change the order of the args in the macro 'NS_DECL_NSIDOMCRYPTO'

Perhaps I did not understand what to do here?
Attachment #552525 - Attachment is obsolete: true
Attachment #553192 - Attachment is obsolete: true
Attachment #553192 - Flags: feedback?(brendan)
Huh?  Why are you editing the generated files?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #102)
> Huh?  Why are you editing the generated files?

It was edited while debugging.
David, which changeset are your patches based on?  In particular, does that changeset have the fix for bug 671453?
(In reply to Boris Zbarsky (:bz) from comment #104)
> David, which changeset are your patches based on?  In particular, does that
> changeset have the fix for bug 671453?

I do have that fix, I am on:
changeset:   74252:ffb2a6be641a from Thu Aug 11 14:24:52 2011 -0400
Sanity checking via a obj dir clobber gives me a build error: 

/home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.cpp:2475:1: error: prototype for ‘nsresult nsCrypto::GetRandomValues(JSContext*, const jsval&)’ does not match any in class ‘nsCrypto’
/home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.h:90:3: error: candidate is: virtual nsresult nsCrypto::GetRandomValues(const jsval&, JSContext*)

Which points to this macro: NS_DECL_NSIDOMCRYPTO

in dist/include/nsIDOMCrypto.h we see this:

  /* [implicit_jscontext] void getRandomValues (in jsval aData); */
  NS_SCRIPTABLE NS_IMETHOD GetRandomValues(const jsval & aData, JSContext* cx) = 0;
(In reply to David Dahl :ddahl from comment #106)

> in dist/include/nsIDOMCrypto.h we see this:
> 
>   /* [implicit_jscontext] void getRandomValues (in jsval aData); */
>   NS_SCRIPTABLE NS_IMETHOD GetRandomValues(const jsval & aData, JSContext*
> cx) = 0;

I meant to also point out that the args seems reversed here, does the JSContext* go first?
It occurred to me that I am not referencing the JSContext* in this method anymore, so I removed it and the [implicit_jscontext] form the idl. It builds, but still crashes:

(gdb) bt
#0  0x00007f0e07cb15ad in nanosleep () from /lib/x86_64-linux-gnu/libc.so.6
#1  0x00007f0e07cb143c in sleep () from /lib/x86_64-linux-gnu/libc.so.6
#2  0x00007f0e0325f5cc in ah_crap_handler (signum=6) at /home/ddahl/code/moz/mozilla-central/toolkit/xre/nsSigHandlers.cpp:121
#3  0x00007f0e03264ee7 in nsProfileLock::FatalSignalHandler (signo=6, info=0x7fff0fd3d9f0, context=0x7fff0fd3d8c0) at /home/ddahl/code/moz/obj-x86_64-unknown-linux-gnu-debug/toolkit/profile/nsProfileLock.cpp:226
#4  <signal handler called>
#5  0x00007f0e0894cb3b in raise () from /lib/x86_64-linux-gnu/libpthread.so.0
#6  0x00007f0e04ee1fb2 in CrashInJS () at /home/ddahl/code/moz/mozilla-central/js/src/jsutil.cpp:95
#7  0x00007f0e04ee200a in JS_Assert (s=0x7f0e053ee366 "JSVAL_IS_DOUBLE_IMPL(data)", file=0x7f0e053ee330 "/home/ddahl/code/moz/mozilla-central/js/src/jsvalue.h", ln=707)
    at /home/ddahl/code/moz/mozilla-central/js/src/jsutil.cpp:103
#8  0x00007f0e041793e4 in js::Value::toPrivate (this=0x7f0df01265c8) at /home/ddahl/code/moz/mozilla-central/js/src/jsvalue.h:707
#9  0x00007f0e04ecb78e in JS_GetTypedArrayBuffer (obj=0x7f0df0126560) at /home/ddahl/code/moz/mozilla-central/js/src/jstypedarray.cpp:2075
#10 0x00007f0e04451965 in nsCrypto::GetRandomValues (this=0x7f0de136e8e0, aData=...) at /home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.cpp:2513
#11 0x00007f0e0499a48d in NS_InvokeByIndex_P (that=0x7f0de136e8e0, methodIndex=10, paramCount=1, params=0x7fff0fd3e1b0)
Need testing buddies to take this and run with it, push to try, etc. It should be good to go.

/be
Attachment #553372 - Attachment is obsolete: true
Assignee: douglas → ddahl
(In reply to Brendan Eich [:brendan] from comment #109)
> Created attachment 553552 [details] [diff] [review]
> v0.9 with correct [implicit_jscontext] parameter order
> 
We still have a crash. I thought it might be debug only, but my opt build is segfaulting. I am fully clobbering opt again just to be sure.

http://pastebin.mozilla.org/1302135
(In reply to Brendan Eich [:brendan] from comment #111)
> Created attachment 553634 [details] [diff] [review]
> v0.10, add missing type-safety assertions to jstypedarray.cpp

With the latest patch I am seeing this crash:
#6  0x00007fd60c9e20d2 in CrashInJS () at /home/ddahl/code/moz/mozilla-central/js/src/jsutil.cpp:95
#7  0x00007fd60c9e212a in JS_Assert (s=0x7fd60ceee486 "JSVAL_IS_DOUBLE_IMPL(data)", file=0x7fd60ceee450 "/home/ddahl/code/moz/mozilla-central/js/src/jsvalue.h", ln=707)
    at /home/ddahl/code/moz/mozilla-central/js/src/jsutil.cpp:103
#8  0x00007fd60bc793e4 in js::Value::toPrivate (this=0x7fd5f7c1d5c8) at /home/ddahl/code/moz/mozilla-central/js/src/jsvalue.h:707
#9  0x00007fd60c9cb886 in JS_GetTypedArrayBuffer (obj=0x7fd5f7c1d560) at /home/ddahl/code/moz/mozilla-central/js/src/jstypedarray.cpp:2082
#10 0x00007fd60bf51948 in nsCrypto::GetRandomValues (this=0x7fd5f61f9600, aData=..., aCtx=0x7fd5ef069400) at /home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.cpp:2507
(In reply to David Dahl :ddahl from comment #112)


I believe line:
PRUint8* data = static_cast<unsigned char*>(JS_GetArrayBufferData(JS_GetTypedArrayBuffer(view)));

Should be:
PRUint8* data = static_cast<PRUint8*>(JS_GetTypedArrayData(view));


Though former line should probably not crash, anyways it won't be correct from a getRandomValues point of view (it fills from the start of the underlying ArrayBuffer rather than at the offset of the ArrayBufferView).


Also:
JSUint32 buffer_len = static_cast<JSUint32>(JS_GetTypedArrayLength(view));

Should be:
JSUint32 buffer_len = static_cast<JSUint32>(JS_GetTypedArrayByteLength(view));

As the former gives the length of the array not the byte length that we want here for use with PK11_GenerateRandom.
(the byte length is different from the array length for types other than TypedArray::TYPE_INT8 or TypedArray::TYPE_UINT8)
(In reply to Cedric Vivier [cedricv] from comment #114)
> (the byte length is different from the array length for types other than
> TypedArray::TYPE_INT8 or TypedArray::TYPE_UINT8)

So this patch is now working with those changes, also my tests all pass, which test each type of ArrayBuffer. In which case, using JS_GetTypedArrayByteLength should always be used, correct? Just making sure.

Cedric: I will attach a new patch with tests, do you have time to build and test it real quick?
Brendan: Should this patch be broken up - with a new bug - separating the TypedArray tweaks?
Attachment #553634 - Attachment is obsolete: true
Attachment #553774 - Flags: review?(brendan)
The typedarray api tweaks should go in the new bug you filed.
Depends on: 679112
(In reply to David Dahl :ddahl from comment #115)
> (In reply to Cedric Vivier [cedricv] from comment #114)
> In which case, using
> JS_GetTypedArrayByteLength should always be used, correct? Just making sure.

Yup.

> Cedric: I will attach a new patch with tests, do you have time to build and
> test it real quick?

Tried it, looks good :)
Attached patch v 0.11 removed typedarray bits (obsolete) — Splinter Review
Attachment #553774 - Attachment is obsolete: true
Attachment #553810 - Flags: review?(brendan)
Attachment #553774 - Flags: review?(brendan)
Blocks: 673432
Attached patch More test coverage (obsolete) — Splinter Review
More test coverage (I've ended up writing tests all day today.. took a chance at this one to end the day yay)
Attachment #553823 - Flags: review?
Attached patch More test coverage v2 (obsolete) — Splinter Review
More test coverage v2. Also make sure that we fail if no exception is thrown when expected.
Attachment #553823 - Attachment is obsolete: true
Attachment #553826 - Flags: review?(ddahl)
Attachment #553823 - Flags: review?
Attachment #553826 - Attachment is obsolete: true
Attachment #553826 - Flags: review?(ddahl)
Comment on attachment 553830 [details] [diff] [review]
More test coverage v2 (minus typo)

I have found 2 syntax errors so far in trying to run the tests with this patch.

I don't understand why you are doing this:

+  var arr = new window[aType + "Array"](aLength);
+  var gotNonZeroValue = false;
 
-  var buf = new ArrayBuffer(aLength);
-  var arBuf = new arrayTypes[aType](buf);

Its much less readable to me.

I think the tests were pretty comprehensive how they were
Attachment #553830 - Flags: feedback-
I do this because we intend to test against the ArrayBufferView length, not the ArrayBuffer length, so creating the underlying ArrayBuffer first is unnecesary and misleading.

gotNonZeroValue as a boolean instead of incrementing a 'result' integer so that the loop is simpler (no more multiple conditionals) and exits as soon something is non-zero (which is what the test cares about).


The test was not failing if an exception is not caught where expected.
If an exception is indeed caught, they were succeeding without checking that it is the correct exception type according to the spec.


window[aType + "Array"] just avoids the noisy hashtable that gives aType+"Array" for each aType. Maybe it's less clear I agree, so we can put back the hashtable.
Paths for the ABORT case (array > 65355 bytes) with ArrayBufferView whose BYTES_PER_ELEMENT is greater than 1 were not tested since the typed array were created by an ArrayBuffer of given byte length directly and ABORT cases were tested with Uint8 only.

With additional tests patch v2, it checks for instance that an Int16Array of length 32768 is filled and that ABORT is thrown if the length is 32769 (which means its underlying store exceed the API's maximum 65535 bytes).
(In reply to Cedric Vivier [cedricv] from comment #124)
> I do this because we intend to test against the ArrayBufferView length, not
> the ArrayBuffer length, so creating the underlying ArrayBuffer first is
> unnecesary and misleading.
Perhaps i was misled by the MDN docs: https://developer.mozilla.org/en/JavaScript_typed_arrays - all of the typed array docs show an arrayBuffer being created and passed into the typedarray constructor.

> 
> gotNonZeroValue as a boolean instead of incrementing a 'result' integer so
> that the loop is simpler (no more multiple conditionals) and exits as soon
> something is non-zero (which is what the test cares about).
> 

Sounds good.

> 
> The test was not failing if an exception is not caught where expected.
> If an exception is indeed caught, they were succeeding without checking that
> it is the correct exception type according to the spec.
> 
That is important, right.

> 
> window[aType + "Array"] just avoids the noisy hashtable that gives
> aType+"Array" for each aType. Maybe it's less clear I agree, so we can put
> back the hashtable.

I just cringe when I see tests doing things that you have to look at 3 times to understand them. tests are documentation as well.

The main problem is this patch does not work for me.
Comment on attachment 553810 [details] [diff] [review]
v 0.11 removed typedarray bits

>+nsCrypto::GetRandomValues(const jsval& aData, JSContext* aCtx)
>+{
>+  // Is it an object?
>+  if (JSVAL_IS_PRIMITIVE(aData))
>+    return NS_ERROR_DOM_TYPE_MISMATCH_ERR;

This branch is untested. Also, you're not in js/src/, so

if (expr) {
  stmt;
}

Same elsewhere in this patch

>+  // Is it an ArrayBufferView?
>+  if (!js_IsTypedArray(view))
>+    return NS_ERROR_DOM_TYPE_MISMATCH_ERR;

This one as well.

>+  // Abort if the ArrayBufferView is requesting too many or too few values
>+  if (buffer_len < MIN_BUFFER_LENGTH || buffer_len > MAX_BUFFER_LENGTH) {
>+    return NS_ERROR_ABORT;

If someone passes in an empty ArrayBufferView, you probably want to return NS_OK. If you don't want to do that, update the spec and use a standard error code.

>+  PRUint8* data = static_cast<unsigned char*>(JS_GetTypedArrayData(view));

static_cast<PRUint8*>?

>--- a/security/manager/ssl/tests/mochitest/bugs/Makefile.in
>+++ b/security/manager/ssl/tests/mochitest/bugs/Makefile.in
> _TEST_FILES = \
>         test_bug480509.html \
>         test_bug483440.html \
>         test_bug484111.html \
>+        test_bug440046.html \

>--- /dev/null
>+++ b/security/manager/ssl/tests/mochitest/bugs/test_bug440046.html
>@@ -0,0 +1,78 @@
>+function testNsCryptoGetRandomValues(aLength, aType, aShouldPass)
>+{
>+  console.log(aLength + ": " + aType + ": " + aShouldPass);

This line should go, along with the aShouldPass parameter, which you don't use here.

>+function onWindowLoad()
>+{
>+  for (var i = 0; i < testData.length; i++) {
>+    if (testData[i].type == "Float64") { 
>+      SimpleTest.finish();
>+    }

<del>Why can't you just move this out of the loop?</del> Why is that change in a separate patch?
Version: unspecified → Trunk
(In reply to David Dahl :ddahl from comment #126)
> (In reply to Cedric Vivier [cedricv] from comment #124)
> > window[aType + "Array"] just avoids the noisy hashtable that gives
> > aType+"Array" for each aType. Maybe it's less clear I agree, so we can put
> > back the hashtable.
> 
> I just cringe when I see tests doing things that you have to look at 3 times
> to understand them. tests are documentation as well.

And I guess I'm an outlier in finding |new window[aType + "Array"]()| perfectly readable :)
(In reply to Ms2ger from comment #127)
> Comment on attachment 553810 [details] [diff] [review]
> v 0.11 removed typedarray bits
> 
> >+nsCrypto::GetRandomValues(const jsval& aData, JSContext* aCtx)
> >+{
> >+  // Is it an object?
> >+  if (JSVAL_IS_PRIMITIVE(aData))
> >+    return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
> 
> This branch is untested. Also, you're not in js/src/, so
> 
> if (expr) {
>   stmt;
> }
Ok, cool. I was unsure, In JS, I am usually told to not brace single line statements. I prefer braces anyway.

> 
> Same elsewhere in this patch

gotcha.

> If someone passes in an empty ArrayBufferView, you probably want to return
> NS_OK. If you don't want to do that, update the spec and use a standard
> error code.

Right. Maybe Adam has an idea on this, I will return NS_OK for now.

> 
> >+  PRUint8* data = static_cast<unsigned char*>(JS_GetTypedArrayData(view));
> 
> static_cast<PRUint8*>?
> 
I think I tried several different ways of casting, and it ended up working like this. PK11_GenerateRanddom takes an unsigned char* 

> >--- a/security/manager/ssl/tests/mochitest/bugs/Makefile.in
> >+++ b/security/manager/ssl/tests/mochitest/bugs/Makefile.in
> > _TEST_FILES = \
> >         test_bug480509.html \
> >         test_bug483440.html \
> >         test_bug484111.html \
> >+        test_bug440046.html \
> 
> >--- /dev/null
> >+++ b/security/manager/ssl/tests/mochitest/bugs/test_bug440046.html
> >@@ -0,0 +1,78 @@
> >+function testNsCryptoGetRandomValues(aLength, aType, aShouldPass)
> >+{
> >+  console.log(aLength + ": " + aType + ": " + aShouldPass);
> 
> This line should go, along with the aShouldPass parameter, which you don't
> use here.
> 
ok

> >+function onWindowLoad()
> >+{
> >+  for (var i = 0; i < testData.length; i++) {
> >+    if (testData[i].type == "Float64") { 
> >+      SimpleTest.finish();
> >+    }
> 
> <del>Why can't you just move this out of the loop?</del>
The SimpleTest.finish() ? I think finish was being called too early. I'll try it again.

> Why is that change
> in a separate patch?
It will all be consolidated into a single patch.
Attachment #553810 - Attachment is obsolete: true
Attachment #553830 - Attachment is obsolete: true
Attachment #554235 - Flags: review?(Ms2ger)
Attachment #553810 - Flags: review?(brendan)
Comment on attachment 554235 [details] [diff] [review]
v 0.12 comments addressed, tests tweaked for potential NS_OK

Is it intended that we fail silently if length is too large?
abarth, which error would you recommend to throw for too large arrays?
What's too large? Chrome worked on every uint8array I tested, up to 600MB. Anything higher crashed the tab before I could call crypto.getRandomValues on the array.
(In reply to Eli Grey (:sephr) from comment #132)
> What's too large?

NSS has an upper limit per call to PK11_GenerateRandom of 65536 bytes
(In reply to Cedric Vivier [cedricv] from comment #131)
> Comment on attachment 554235 [details] [diff] [review]
> v 0.12 comments addressed, tests tweaked for potential NS_OK
> 
> Is it intended that we fail silently if length is too large?
> abarth, which error would you recommend to throw for too large arrays?

I was thinking not, I would prefer to throw some kind of an error
(In reply to David Dahl :ddahl from comment #134)
> I was thinking not, I would prefer to throw some kind of an error

+1, failing silently is not developer-friendly.

Or perhaps we should remove that limit and fill the array through multiple PK11_GenerateRandom if the array is larger than its maximum supported value?
(In reply to Cedric Vivier [cedricv] from comment #131)
> Comment on attachment 554235 [details] [diff] [review]
> v 0.12 comments addressed, tests tweaked for potential NS_OK
> 
> Is it intended that we fail silently if length is too large?
> abarth, which error would you recommend to throw for too large arrays?

I should have been clearer; I meant to suggest returning NS_OK for zero-length arrays and to keeping the error for too long arrays (though actually making them work would be even better, of course).
Comment on attachment 554235 [details] [diff] [review]
v 0.12 comments addressed, tests tweaked for potential NS_OK

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

This looks pretty good, IMO. I would like to see tests for calling getRandomValues with (at least) null, undefined, a string and a number. (Theoretically these should throw TypeError, but TYPE_MISMATCH_ERR is fine for now.) You'll need a review from a PSM peer, though, and preferably also from someone who knows about typed arrays.

::: dom/interfaces/base/nsIDOMCrypto.idl
@@ +51,5 @@
>                                                     in boolean doForcedBackup);
>    DOMString                 popChallengeResponse(in DOMString challenge);
>    DOMString                 random(in long numBytes);
> +  [implicit_jscontext]
> +  void                      getRandomValues(in jsval aData);

You need to update the uuid here, from mozilla.pettay.fi/cgi-bin/mozuuid.pl, for example.

::: security/manager/ssl/src/nsCrypto.cpp
@@ +2506,5 @@
> +    return NS_OK;
> +  }
> +
> +  PRUint32 len = static_cast<PRUint32>(buffer_len);
> +  PRUint8* data = static_cast<unsigned char*>(JS_GetTypedArrayData(view));

Maybe declare data as unsigned char* instead of PRUint8*, then?

@@ +2512,5 @@
> +  SECStatus srv = PK11_GenerateRandom(data, len);
> +
> +  if (srv == SECFailure) {
> +    return NS_ERROR_FAILURE;
> +  }

<http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/util/seccomon.h#91> claims that "if (srv != SECSuccess)" is preferred. (Doesn't actually matter in this case, because PK11_GenerateRandom only returns SECFailure and SECSuccess, but better to be safe.)

::: security/manager/ssl/tests/mochitest/bugs/test_bug440046.html
@@ +80,5 @@
> +      try {
> +        testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);
> +      } 
> +      catch (ex) {
> +        var result = ex.toString().search(/NS_ERROR_DOM_TYPE_MISMATCH_ERR/);

How about ok(ex instanceof DOMException && ex.code === DOMException.TYPE_MISMATCH_ERR, "Expected TYPE_MISMATCH_ERR, got " + ex + ".")?
Attachment #554235 - Flags: review?(Ms2ger) → feedback+
(In reply to Ms2ger from comment #137)
> Comment on attachment 554235 [details] [diff] [review]
> v 0.12 comments addressed, tests tweaked for potential NS_OK
> ::: security/manager/ssl/tests/mochitest/bugs/test_bug440046.html
> @@ +80,5 @@
> > +      try {
> > +        testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);
> > +      } 
> > +      catch (ex) {
> > +        var result = ex.toString().search(/NS_ERROR_DOM_TYPE_MISMATCH_ERR/);
> 
> How about ok(ex instanceof DOMException && ex.code ===
> DOMException.TYPE_MISMATCH_ERR, "Expected TYPE_MISMATCH_ERR, got " + ex +
> ".")?

I'd rather use the technique in the additional patch, so that if an exception is not thrown it tells directly which test failed rather than fail later at "Ran 10 tests".

Also we need to instantiate ArrayBufferView directly, not through ArrayBuffer to test against the intended length (with more at least one Int16 or Int32 type [BYTES_PER_ELEMENT not 1]).
Attached patch v 0.13 Comments addressed (obsolete) — Splinter Review
Attachment #554235 - Attachment is obsolete: true
Attachment #554879 - Flags: review?(kaie)
Attachment #554879 - Flags: review?(kaie) → review?(bsmith)
Whiteboard: [sr:bsmith] → [secr:bsmith]
(In reply to David Dahl :ddahl from comment #139)
> Created attachment 554879 [details] [diff] [review]
> v 0.13 Comments addressed

Try results here: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&pusher=ddahl@mozilla.com

Looks good
Comment on attachment 554879 [details] [diff] [review]
v 0.13 Comments addressed

> interface nsIDOMCrypto : nsISupports
> ...
>+  [implicit_jscontext]
>+  void                      getRandomValues(in jsval aData);

As you're doing strict checking in the function,
and your argument is a flexible type =>
What about adding a quick comment that explain what parameter values are acceptable?


>+NS_IMETHODIMP
>+nsCrypto::GetRandomValues(const jsval& aData, JSContext* aCtx)

I cannot find any reference to variable aCtx in your code.
Is this variable referenced by one of the macros you're using,
or can it be removed? (From both here and the interface declaration)


>+  // PK11_GenerateRandom will not generate more than 65536 values at once

I'm surprised. If this were a kind of 16 bit limitation, I would have expected that the limit is by one smaller, 65535, the max of a unsigned 16 bit value.

Where did you see this limit?
(In reply to Kai Engert (:kaie) from comment #141)
> Comment on attachment 554879 [details] [diff] [review]
> v 0.13 Comments addressed
> 
> > interface nsIDOMCrypto : nsISupports
> > ...
> >+  [implicit_jscontext]
> >+  void                      getRandomValues(in jsval aData);
> 
> As you're doing strict checking in the function,
> and your argument is a flexible type =>
> What about adding a quick comment that explain what parameter values are
> acceptable?
Yeah, that is confusing, will do.

> 
> 
> >+NS_IMETHODIMP
> >+nsCrypto::GetRandomValues(const jsval& aData, JSContext* aCtx)
> 
> I cannot find any reference to variable aCtx in your code.
> Is this variable referenced by one of the macros you're using,
> or can it be removed? (From both here and the interface declaration)
That does seem to be detritus at this point, I removed the [implicit_jscontext] and the JSContext* arg and it works just fine.  

> 
> 
> >+  // PK11_GenerateRandom will not generate more than 65536 values at once
> 
> I'm surprised. If this were a kind of 16 bit limitation, I would have
> expected that the limit is by one smaller, 65535, the max of a unsigned 16
> bit value.
> 
> Where did you see this limit?
In my manual testing and via gdb. Here is my manual test:

This works:
var buf = new ArrayBuffer(65536);
var arBuf = new Int8Array(buf);
window.crypto.getRandomValues(arBuf);

This does not:
var buf = new ArrayBuffer(65537);
var arBuf = new Int8Array(buf);
window.crypto.getRandomValues(arBuf);

I believe the random generation would bail here:
http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/freebl/drbg.c#577
bsmith: any update on when you can review this?
Comment on attachment 554879 [details] [diff] [review]
v 0.13 Comments addressed

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

It would be great to get a final review on the JS API stuff from Luke. Please re-request review when you address the review comments and I will do the review right away. Sorry for the delay.

We should update the "spec" to note that an implementation should not request "too many" values in a single call because the brower's PRNG may be limited in how much it returns technically or by regulatory requirements.

::: security/manager/ssl/src/nsCrypto.cpp
@@ +2473,5 @@
> +NS_IMETHODIMP
> +nsCrypto::GetRandomValues(const jsval& aData, JSContext* aCtx)
> +{
> +  // Is it an object?
> +  if (JSVAL_IS_PRIMITIVE(aData)) {

Is this check needed? Is there some way that js_IsTypedArray(view) below could return true for a primitive value?

If this check is needed, please add a comment as to why it is needed.

@@ +2491,5 @@
> +    case js::TypedArray::TYPE_FLOAT32:
> +    case js::TypedArray::TYPE_FLOAT64:
> +      return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
> +    default:
> +      break;

Instead of blacklisting the floating-point types, let's whitelist the integer types that we want to allow.

@@ +2502,5 @@
> +
> +  // Abort if the ArrayBufferView is requesting too many or too few values
> +  if (buffer_len < MIN_BUFFER_LENGTH || buffer_len > MAX_BUFFER_LENGTH) {
> +    // return NS_OK as the WHATWG spec does not specify a MAX_BUFFER_LENGTH
> +    return NS_OK;

We should return NS_ERROR_FAILURE here instead of NS_OK, so that a JS exception to be raised.

We don't need to check against buffer_len > MAX_BUFFER_LENGTH. PK11_GenerateRandom will fail if we request too much, and the (srv != SECSuccess) check below will cause us to return NS_ERROR_FAILURE.

Do we need to fail when zero bytes are requested, or should the call just be a no-op? What does the WebKit implementation do?

::: security/manager/ssl/tests/mochitest/bugs/test_bug440046.html
@@ +91,5 @@
> +}
> +
> +
> +</script>
> +</body></html>

We should have tests for the other error cases:
1. A null output buffer view (to show that the !JS_IS_PRIMITIVE check is necessary)
2. A zero-length output buffer view (to exercise the that the < 1 check)
3. A large buffer view (to exercise the case where PK11_GenerateRandom fails).
Attachment #554879 - Flags: review?(bsmith) → review-
(In reply to Brian Smith (:bsmith) from comment #144)
> Comment on attachment 554879 [details] [diff] [review] [diff] [details] [review]
> v 0.13 Comments addressed
> 
> Review of attachment 554879 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> We should update the "spec" to note that an implementation should not
> request "too many" values in a single call because the brower's PRNG may be
> limited in how much it returns technically or by regulatory requirements.

By "implementation", do you mean the API consumer? (Usually called 'author' in a spec context.)

> ::: security/manager/ssl/src/nsCrypto.cpp
> @@ +2473,5 @@
> > +NS_IMETHODIMP
> > +nsCrypto::GetRandomValues(const jsval& aData, JSContext* aCtx)
> > +{
> > +  // Is it an object?
> > +  if (JSVAL_IS_PRIMITIVE(aData)) {
> 
> Is this check needed? Is there some way that js_IsTypedArray(view) below
> could return true for a primitive value?

Yes. Without this check, we'd assert and dereference an invalid pointer in js_IsTypedArray.

> @@ +2502,5 @@
> > +
> > +  // Abort if the ArrayBufferView is requesting too many or too few values
> > +  if (buffer_len < MIN_BUFFER_LENGTH || buffer_len > MAX_BUFFER_LENGTH) {
> > +    // return NS_OK as the WHATWG spec does not specify a MAX_BUFFER_LENGTH
> > +    return NS_OK;
> 
> We should return NS_ERROR_FAILURE here instead of NS_OK, so that a JS
> exception to be raised.

We should return NS_OK for buffer_len == 0, because in that case not doing anything is the correct behaviour.

For buffer_len > MAX_BUFFER_LENGTH, IMO, we should just call into NSS repeatedly.

In any case, we shouldn't throw non-standard exceptions unless, perhaps, in cases where we really have no idea what happened.
(In reply to Ms2ger from comment #145)
> > We should update the "spec" to note that an implementation should not
> > request "too many" values in a single call because the brower's PRNG may be
> > limited in how much it returns technically or by regulatory requirements.
> 
> By "implementation", do you mean the API consumer? (Usually called 'author'
> in a spec context.)

Right. s/implementation/caller/

> We should return NS_OK for buffer_len == 0, because in that case not doing
> anything is the correct behaviour.

That sounds OK with me.

> For buffer_len > MAX_BUFFER_LENGTH, IMO, we should just call into NSS
> repeatedly.

I disagree here. The NSS upper bound is really high, and no reasonable caller of this API will request more than a few hundred bytes of random values, so even allowing thousands of random bytes to be requested at once is already excessive.

> In any case, we shouldn't throw non-standard exceptions unless, perhaps, in
> cases where we really have no idea what happened.

We should raise whatever exception normally gets raised by DOM APIs when a parameter value is too large.
(In reply to Ms2ger from comment #145)
> We should return NS_OK for buffer_len == 0, because in that case not doing
> anything is the correct behaviour.
> 
> For buffer_len > MAX_BUFFER_LENGTH, IMO, we should just call into NSS
> repeatedly.
> 
> In any case, we shouldn't throw non-standard exceptions unless, perhaps, in
> cases where we really have no idea what happened.

I think NSS' hard limit is what both Adam and Brian thought made the most sense, see comment 68
(In reply to Brian Smith (:bsmith) from comment #146)
> We should raise whatever exception normally gets raised by DOM APIs when a
> parameter value is too large.

This would require the spec to change. Should we return  NS_ERROR_INVALID_ARG or something like that?
Added 2 new checks. Altered the > MAX_BUFFER_LENGTH test to catch a new NS_ERROR_INVALID_ARG, which seems to throw NS_ERROR_ILLEGAL_VALUE. Again, if the spec needs to change to account for this possible Exception that would be great - or whatever is the best error to throw in this case.
Attachment #554879 - Attachment is obsolete: true
Attachment #566635 - Flags: review?(bsmith)
Comment on attachment 566635 [details] [diff] [review]
v 0.14 comments addressed, additional checks added to the test

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

::: security/manager/ssl/src/nsCrypto.cpp
@@ +36,5 @@
>   * the provisions above, a recipient may use your version of this file under
>   * the terms of any one of the MPL, the GPL or the LGPL.
>   *
>   * ***** END LICENSE BLOCK ***** */
> +

Undo this whitespace change.

@@ +2501,5 @@
> +
> +  // PK11_GenerateRandom will not generate more than 65536 values at once
> +  const JSUint32 MAX_BUFFER_LENGTH = static_cast<JSUint32>(65536);
> +  const JSUint32 MIN_BUFFER_LENGTH = static_cast<JSUint32>(1);
> +  JSUint32 buffer_len = static_cast<JSUint32>(JS_GetTypedArrayByteLength(view));

PRUint32 dataLen = JS_GetTypedArrayByteLength(view);

(rename to be match "data" below, and no cast needed).

@@ +2502,5 @@
> +  // PK11_GenerateRandom will not generate more than 65536 values at once
> +  const JSUint32 MAX_BUFFER_LENGTH = static_cast<JSUint32>(65536);
> +  const JSUint32 MIN_BUFFER_LENGTH = static_cast<JSUint32>(1);
> +  JSUint32 buffer_len = static_cast<JSUint32>(JS_GetTypedArrayByteLength(view));
> +

Remove MAX_BUFFER_LENGTH and MIN_BUFFER_LENGTH.

@@ +2504,5 @@
> +  const JSUint32 MIN_BUFFER_LENGTH = static_cast<JSUint32>(1);
> +  JSUint32 buffer_len = static_cast<JSUint32>(JS_GetTypedArrayByteLength(view));
> +
> +  // Abort if the ArrayBufferView is requesting too few values
> +  if (buffer_len < MIN_BUFFER_LENGTH) {

Just use "< 1" here.

@@ +2508,5 @@
> +  if (buffer_len < MIN_BUFFER_LENGTH) {
> +    return NS_OK;
> +  }
> +  // Throw NS_ERROR_INVALID_ARG if too many values are requested. 
> +  // This is not in the spec

Remove these comments.

@@ +2511,5 @@
> +  // Throw NS_ERROR_INVALID_ARG if too many values are requested. 
> +  // This is not in the spec
> +  if (buffer_len > MAX_BUFFER_LENGTH) {
> +    return  NS_ERROR_INVALID_ARG;
> +  }

Replace this check with the one I mentioned below.

@@ +2513,5 @@
> +  if (buffer_len > MAX_BUFFER_LENGTH) {
> +    return  NS_ERROR_INVALID_ARG;
> +  }
> +
> +  PRUint32 len = static_cast<PRUint32>(buffer_len);

This cast should not be necessary.

@@ +2519,5 @@
> +
> +  SECStatus srv = PK11_GenerateRandom(data, len);
> +
> +  if (srv != SECSuccess) {
> +    return NS_ERROR_FAILURE;

// PK11_GenerateRandom returns 
// SEC_ERROR_INVALID_ARGS when too many random
// bytes have been requested.
return PR_GetError() == SEC_ERROR_INVALID_ARGS
           ?  NS_ERROR_INVALID_ARG
           :  NS_ERROR_FAILURE;

::: security/manager/ssl/tests/mochitest/bugs/test_bug440046.html
@@ +16,5 @@
> +                 { len: 32, type: "Uint8", pass: true },
> +                 { len: 32, type: "Uint16", pass: true },
> +                 { len: 32, type: "Uint32", pass: true },
> +                 { len: 65536, type: "Uint8", pass: true },
> +                 { len: 65538, type: "Uint8", pass: false }, 

{ len: 65537, type: "Uint8", pass: false },

@@ +46,5 @@
> +  window.crypto.getRandomValues(arBuf);
> +
> +
> +  for (var i = 0; i < arBuf.length; i++) {
> +    if (arBuf[i] < 1) {

For signed integer array buffer views, there will be negative numbers. If all the values are negative, then this test will wrongly fail.

@@ +53,5 @@
> +    result += arBuf[i];
> +    if (i > 16) {
> +      break;
> +    }
> +  }  

I understand what you are trying to do but this test doesn't really make sense. It is hard to test a PRNG when you don't have a way of seeding it to get deterministic results. However, we already have tests for the PRNG separately, so we should be able to rely on its contract.

Consequently, I think the test should be: if the array buffer length is larger than 4 bytes, then ensure there is at least one non-zero value in it. Also, do the check by breaking on a non-zero element, instead of adding the values together and checking the sum. This test would cause a false failure 1/2^32 of the time, but that is better than most of our automated tests. :)

@@ +55,5 @@
> +      break;
> +    }
> +  }  
> +
> +  ok(result != 0, "Non-zero result: " + result  +  " found in the  " + aType + ": " + aLength + " ArrayBufferView");

We shouldn't call ok here when the test is supposed to fail. It should call ok() conditionally based on the value of pass.

@@ +75,5 @@
> +        } 
> +        else {
> +          ok(ex.toString().search(/TYPE_MISMATCH_ERR/), "Expected TYPE_MISMATCH_ERR, got " + ex + ".");
> +
> +        }

This doesn't work if the call fails but returns the wrong error code. Instead of "pass: false", we should have pass: "ILLEGAL_VALUE" or pass: "TYPE_MISMATCH_ERR" or something like that.

@@ +98,5 @@
> +    ok(a[0] === undefined, "The array buffer is unchanged, still 0 length");
> +  } 
> +  catch (ex) {
> +    ok(false, "A zero-length array buffer view should not fail");
> +  }  

In addition to zero-length, we should have the following tests:
1. A one-element test.
2. A test where the array buffer view isn't the whole array buffer. In particular, we should have a test where the array buffer view isn't at the start of the array buffer, and we should verify that the parts of the array buffer outside of the view are not modified. I suggest initializing the array buffer to all ones, then generating the random values on some subrange of the array buffer, and then verifying that the rest of the array buffer is still all ones. (This way, we can verify that the new function doesn't reset values outside of the buffer view to zeros too.)
Attachment #566635 - Flags: review?(bsmith) → review-
Attachment #566635 - Attachment is obsolete: true
Attachment #567805 - Flags: review?(bsmith)
Please allow Uint8ClampedArray. Also, I updated the spec to use QUOTA_EXCEEDED_ERR, please update accordingly.
Comment on attachment 567805 [details] [diff] [review]
v 0.15 Comments addressed, new tests added

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

r-, but we are getting close.

::: security/manager/ssl/src/nsCrypto.cpp
@@ +2487,5 @@
> +  // Throw if the wrong type of ArrayBufferView is passed in
> +  // (Part of the WHATWG spec)
> +  switch (JS_GetTypedArrayType(view)) {
> +    case js::TypedArray::TYPE_INT8:
> +    case js::TypedArray::TYPE_UINT8:

Add support for the clamped uint8 arrays here. (My understanding is that, for the purposes of this function, clamped uint8 arrays are exactly like regular UINT8 arrays. Is that correct?)

@@ +2512,5 @@
> +
> +  // PK11_GenerateRandom returns 
> +  // SEC_ERROR_INVALID_ARGS when too many random
> +  // bytes have been requested.
> +  if (srv != SECSuccess) {

Pleaes fix the line length here.

::: security/manager/ssl/tests/mochitest/bugs/test_bug440046.html
@@ +50,5 @@
> +      pass = true;
> +      break;
> +    }
> +  }
> +  is(pass, true,  "Non-zero result: " + i  +  " found in the  " + aType + ": " + aLength + " ArrayBufferView");

It is better to move this is() call to the if (testData[i].pass) case below, since this check is only correct for tests that are supposed to pass.

@@ +61,5 @@
> +      testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);
> +    } 
> +    else {
> +      try {
> +        testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);

Need to ensure that the line after the call is never reached.

@@ +63,5 @@
> +    else {
> +      try {
> +        testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);
> +      } 
> +      catch (ex) {

This test still isn't right. It needs to check that the 8th test fails with the quota exceeded error, and it needs to check that the 9th and 10th tests fail with type mismatch errors. Instead, it tests that the 8th, 9th, and 10th tests fail with either a type mismatch error or a quota exceeded error.

@@ +85,5 @@
> +  try {
> +    window.crypto.getRandomValues(null);    
> +  } 
> +  catch (ex) {
> +    ok(ex.toString().search(/TYPE_MISMATCH_ERR/), "Expected TYPE_MISMATCH_ERR, got " + ex + ".");

Do the Webkit guys agree that TYPE_MISMATCH_ERR is the correct error for a null input buffer view?

@@ +116,5 @@
> +  for (var i = 0; i < view2.byteLength; i++) {
> +    view2[i] = 1;
> +  }
> +  window.crypto.getRandomValues(view);
> +  is(view2[0], 1, "view2 is unchanged");

We should also have a test just like this, but calling window.crypto.getRandomValues on view2, so that we test the case where the view doesn't start at the beginning of the buffer.

Also, let's test all 16 values of the other view to make sure they are all unchanged.
Attachment #567805 - Flags: review?(bsmith) → review-
(In reply to Brian Smith (:bsmith) from comment #153)
> @@ +85,5 @@
> > +  try {
> > +    window.crypto.getRandomValues(null);    
> > +  } 
> > +  catch (ex) {
> > +    ok(ex.toString().search(/TYPE_MISMATCH_ERR/), "Expected TYPE_MISMATCH_ERR, got " + ex + ".");
> 
> Do the Webkit guys agree that TYPE_MISMATCH_ERR is the correct error for a
> null input buffer view?

WebIDL requires TypeError.
(In reply to Brian Smith (:bsmith) from comment #153)
> Comment on attachment 567805 [details] [diff] [review] [diff] [details] [review]

> ::: security/manager/ssl/tests/mochitest/bugs/test_bug440046.html
> @@ +50,5 @@
> > +      pass = true;
> > +      break;
> > +    }
> > +  }
> > +  is(pass, true,  "Non-zero result: " + i  +  " found in the  " + aType + ": " + aLength + " ArrayBufferView");
> 
> It is better to move this is() call to the if (testData[i].pass) case below,
> since this check is only correct for tests that are supposed to pass.
> 
I was initializing the 'pass' variable to false in order to catch when we do not pass this test, however unlikely that might be.

Hrmm. looks like I need to add an Error macro?

'WARNING: Huh, someone is throwing non-DOM errors using the DOM module!: file /home/ddahl/code/moz/mozilla-central/dom/base/nsDOMException.cpp, line 131'
Attachment #567805 - Attachment is obsolete: true
Attachment #567915 - Flags: review?(bsmith)
(In reply to David Dahl :ddahl from comment #155)

> Hrmm. looks like I need to add an Error macro?
> 
> 'WARNING: Huh, someone is throwing non-DOM errors using the DOM module!:
> file /home/ddahl/code/moz/mozilla-central/dom/base/nsDOMException.cpp, line
> 131'

Do I need to add something like what is done in workers/Exceptions and avoid the DOM errors? I am using DOM errors elsewhere...
http://mxr.mozilla.org/mozilla-central/source/dom/workers/Exceptions.h#69

http://mxr.mozilla.org/mozilla-central/source/dom/workers/Exceptions.cpp#230
No, you need to add a string corresponding to QUOTA_ERR to domerr.msg.
Attachment #567915 - Attachment is obsolete: true
Attachment #568173 - Flags: review?(bsmith)
Attachment #567915 - Flags: review?(bsmith)
Attachment #568173 - Attachment is obsolete: true
Attachment #568543 - Flags: review?(bsmith)
Attachment #568173 - Flags: review?(bsmith)
Comment on attachment 568543 [details] [diff] [review]
v 18 updated test where mobile will see a differently-cased error string

>--- a/dom/base/domerr.msg
>+++ b/dom/base/domerr.msg
>@@ -95,16 +95,21 @@ DOM_MSG_DEF(NS_ERROR_DOM_INDEXEDDB_NOT_A
> 
>+/* WHATWG DOM error for window.crypto.getRandomValues
>+   http://wiki.whatwg.org/wiki/Crypto#getRandomValues
>+*/
>+DOM_MSG_DEF(NS_ERROR_DOM_QUOTA_EXCEEDED_ERR, "Quota exceeded error")

Can you please move this one and DATA_CLONE_ERR to a block right below TYPE_MISMATCH_ERR and add a link to <http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html>?

>--- a/security/manager/ssl/src/nsCrypto.cpp
>+++ b/security/manager/ssl/src/nsCrypto.cpp
>+NS_IMETHODIMP
>+nsCrypto::GetRandomValues(const jsval& aData, JSContext* aCtx)
>+{
>+  // Is it an object?
>+  if (JSVAL_IS_PRIMITIVE(aData)) {
>+    return NS_ERROR_DOM_TYPE_ERR;

Hmm, that's not actually the right TypeError... Might as well throw NS_ERROR_DOM_NOT_OBJECT_ERR.
No longer depends on: 679112
(In reply to Ms2ger from comment #161)
> Comment on attachment 568543 [details] [diff] [review] [diff] [details] [review]
> v 18 updated test where mobile will see a differently-cased error string
> 
> >--- a/dom/base/domerr.msg
> >+++ b/dom/base/domerr.msg
> >@@ -95,16 +95,21 @@ DOM_MSG_DEF(NS_ERROR_DOM_INDEXEDDB_NOT_A
> > 
> >+/* WHATWG DOM error for window.crypto.getRandomValues
> >+   http://wiki.whatwg.org/wiki/Crypto#getRandomValues
> >+*/
> >+DOM_MSG_DEF(NS_ERROR_DOM_QUOTA_EXCEEDED_ERR, "Quota exceeded error")
> 
> Can you please move this one and DATA_CLONE_ERR to a block right below
> TYPE_MISMATCH_ERR and add a link to
> <http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html>?
> 

Like this?
DOM_MSG_DEF(NS_ERROR_DOM_TYPE_MISMATCH_ERR, "The type of an object is incompatible with the expected type of the parameter associated to the object")

/* WHATWG DOM error for window.crypto.getRandomValues
   http://wiki.whatwg.org/wiki/Crypto#getRandomValues
*/
DOM_MSG_DEF(NS_ERROR_DOM_QUOTA_EXCEEDED_ERR, "Quota exceeded error")

/* http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html */
DOM_MSG_DEF(NS_ERROR_DOM_DATA_CLONE_ERR, "The object could not be cloned.")
Attachment #568543 - Attachment is obsolete: true
Attachment #568733 - Flags: review?(bsmith)
Attachment #568543 - Flags: review?(bsmith)
I should have been clearer, I meant

/* http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html */
DOM_MSG_DEF(NS_ERROR_DOM_QUOTA_EXCEEDED_ERR, "Quota exceeded error")
DOM_MSG_DEF(NS_ERROR_DOM_DATA_CLONE_ERR, "The object could not be cloned.")

No need to upload a new patch just for this, just change it after addressing any other review comments.
Attachment #568733 - Attachment is obsolete: true
Attachment #573386 - Flags: review?(bsmith)
Attachment #568733 - Flags: review?(bsmith)
Last I checked, bsmith was blocked on SPDY work
After SPDY landed, is this now possible?
Comment on attachment 573386 [details] [diff] [review]
v 20 rebased, built and tested on birch

>--- a/security/manager/ssl/src/nsCrypto.cpp
>+++ b/security/manager/ssl/src/nsCrypto.cpp
>+nsCrypto::GetRandomValues(const jsval& aData, JSContext* aCtx)
>+{
>+  PRUint32 len = dataLen;

What's the point of this variable? Can't you pass dataLen to PK11_GenerateRandom?
Comment on attachment 573386 [details] [diff] [review]
v 20 rebased, built and tested on birch

smaug, please review the test and the changes under dom/. Pay special attention to the error codes reported, to make sure they make sense.

dougt, is removing "MOZ_DISABLE_DOMCRYPTO=1" really all that is needed for birch? What about all the other methods of window.crypto that require UI?
Attachment #573386 - Flags: superreview?(doug.turner)
Attachment #573386 - Flags: review?(bugs)
Comment on attachment 573386 [details] [diff] [review]
v 20 rebased, built and tested on birch

># HG changeset patch
># Parent 48e52668c10a482ba743683b1e3a16c88118e7d1
># User David Dahl <ddahl@mozilla.com>
>
>try: -b do -p linux-android -u xpcshell,mochitests,browser-chrome,mochitest-6,mochitest-7,mochitest-8 -t none --post-to-bugzilla Bug 440046
>
>diff --git a/dom/base/domerr.msg b/dom/base/domerr.msg
>--- a/dom/base/domerr.msg
>+++ b/dom/base/domerr.msg
>@@ -64,31 +64,37 @@ DOM_MSG_DEF(NS_ERROR_DOM_SYNTAX_ERR, "An
> DOM_MSG_DEF(NS_ERROR_DOM_RANGE_BAD_BOUNDARYPOINTS_ERR, "The boundary-points of a range does not meet specific requirements.")
> DOM_MSG_DEF(NS_ERROR_DOM_RANGE_INVALID_NODE_TYPE_ERR, "The container of an boundary-point of a range is being set to either a node of an invalid type or a node with an ancestor of an invalid type.")
> 
> /* DOM error codes from http://www.w3.org/TR/DOM-Level-3/ */
> 
> DOM_MSG_DEF(NS_ERROR_DOM_VALIDATION_ERR, "A call to a method would make the Node invalid with respect to \"partial validity\", so the operation was not done")
> DOM_MSG_DEF(NS_ERROR_DOM_TYPE_MISMATCH_ERR, "The type of an object is incompatible with the expected type of the parameter associated to the object")
> 
>+/* WHATWG DOM error for window.crypto.getRandomValues
>+   http://wiki.whatwg.org/wiki/Crypto#getRandomValues
>+*/
>+DOM_MSG_DEF(NS_ERROR_DOM_QUOTA_EXCEEDED_ERR, "Quota exceeded error")
>+
>+/* http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html */
>+DOM_MSG_DEF(NS_ERROR_DOM_DATA_CLONE_ERR, "The object could not be cloned.")
>+
> /* SVG DOM error codes from http://www.w3.org/TR/SVG11/svgdom.html */
> 
> DOM_MSG_DEF(NS_ERROR_DOM_SVG_WRONG_TYPE_ERR, "Unknown or invalid type")
> DOM_MSG_DEF(NS_ERROR_DOM_SVG_INVALID_VALUE_ERR, "One of the parameters has an invalid value")
> DOM_MSG_DEF(NS_ERROR_DOM_SVG_MATRIX_NOT_INVERTABLE, "The matrix could not be computed")
> 
> /* DOM error codes from http://www.w3.org/TR/DOM-Level-3-XPath/ */
> 
> DOM_MSG_DEF(NS_ERROR_DOM_INVALID_EXPRESSION_ERR, "The expression is not a legal expression.")
> DOM_MSG_DEF(NS_ERROR_DOM_TYPE_ERR, "The expression cannot be converted to return the specified type.")
> 
> /* HTML5 error codes http://dev.w3.org/html5/spec/Overview.html */
> 
>-DOM_MSG_DEF(NS_ERROR_DOM_DATA_CLONE_ERR, "The object could not be cloned.")
>-
> /* IndexedDB error codes http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html */
> 
> DOM_MSG_DEF(NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR, "The operation failed for reasons unrelated to the database itself and not covered by any other error code.")
> DOM_MSG_DEF(NS_ERROR_DOM_INDEXEDDB_NON_TRANSIENT_ERR, "This error occurred because an operation was not allowed on an object. A retry of the same operation would fail unless the cause of the error is corrected.")
> DOM_MSG_DEF(NS_ERROR_DOM_INDEXEDDB_NOT_FOUND_ERR, "The operation failed because the requested database object could not be found. For example, an object store did not exist but was being opened.")
> DOM_MSG_DEF(NS_ERROR_DOM_INDEXEDDB_CONSTRAINT_ERR, "A mutation operation in the transaction failed because a constraint was not satisfied. For example, an object such as an object store or index already exists and a new one was being attempted to be created.")
> DOM_MSG_DEF(NS_ERROR_DOM_INDEXEDDB_DATA_ERR, "Data provided to an operation does not meet requirements.")
> DOM_MSG_DEF(NS_ERROR_DOM_INDEXEDDB_NOT_ALLOWED_ERR, "A mutation operation was attempted on a database that did not allow mutations.")
>diff --git a/dom/base/nsDOMError.h b/dom/base/nsDOMError.h
>--- a/dom/base/nsDOMError.h
>+++ b/dom/base/nsDOMError.h
>@@ -61,17 +61,17 @@
> #define NS_ERROR_DOM_NAMESPACE_ERR               NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM,14)
> #define NS_ERROR_DOM_INVALID_ACCESS_ERR          NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM,15)
> #define NS_ERROR_DOM_VALIDATION_ERR              NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM,16)
> #define NS_ERROR_DOM_TYPE_MISMATCH_ERR           NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM,17)
> /* 18: SECURITY_ERR */
> /* 19: NETWORK_ERR */
> /* 20: ABORT_ERR */
> /* 21: URL_MISMATCH_ERR */
>-/* 22: QUOTA_EXCEEDED_ERR */
>+#define NS_ERROR_DOM_QUOTA_EXCEEDED_ERR          NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM,22)
> /* 23: TIMEOUT_ERR */
> /* 24: NOT_READABLE_ERR */
> #define NS_ERROR_DOM_DATA_CLONE_ERR              NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM,25)
> /* 26: ENCODING_ERR */
> 
> /* DOM error codes from http://www.w3.org/TR/DOM-Level-2/range.html */
> 
> #define NS_ERROR_DOM_RANGE_BAD_BOUNDARYPOINTS_ERR NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM_RANGE, 1)
>diff --git a/dom/interfaces/base/nsIDOMCrypto.idl b/dom/interfaces/base/nsIDOMCrypto.idl
>--- a/dom/interfaces/base/nsIDOMCrypto.idl
>+++ b/dom/interfaces/base/nsIDOMCrypto.idl
>@@ -16,16 +16,17 @@
>  *
>  * The Initial Developer of the Original Code is
>  * Netscape Communications Corporation.
>  * Portions created by the Initial Developer are Copyright (C) 2000
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>  *   Johnny Stenback <jst@netscape.com>
>+ *   David Dahl <ddahl@mozilla.com>
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either of the GNU General Public License Version 2 or later (the "GPL"),
>  * or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>  * in which case the provisions of the GPL or the LGPL are applicable instead
>  * of those above. If you wish to allow use of your version of this file only
>  * under the terms of either the GPL or the LGPL, and not to allow others to
>  * use your version of this file under the terms of the MPL, indicate your
>@@ -33,25 +34,27 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "domstubs.idl"
> 
>-[scriptable, uuid(12b6d899-2aed-4ea9-8c02-2223ab7ab592)]
>+[scriptable, uuid(9311137d-3495-4c9a-97a2-21f3a18ad203)]
> interface nsIDOMCrypto : nsISupports
> {
>   readonly attribute DOMString        version;
>   attribute boolean         enableSmartCardEvents;
> 
>   nsIDOMCRMFObject          generateCRMFRequest(/* ... */);
>   DOMString                 importUserCertificates(in DOMString nickname,
>                                                    in DOMString cmmfResponse,
>                                                    in boolean doForcedBackup);
>   DOMString                 popChallengeResponse(in DOMString challenge);
>   DOMString                 random(in long numBytes);
>+  [implicit_jscontext]
>+  void                      getRandomValues(in jsval aData);
>   DOMString                 signText(in DOMString stringToSign,
>                                      in DOMString caOption /* ... */);
>   void                      logout();
>   void                      disableRightClick();
> };
>diff --git a/mobile/confvars.sh b/mobile/confvars.sh
>--- a/mobile/confvars.sh
>+++ b/mobile/confvars.sh
>@@ -42,18 +42,16 @@ MOZ_APP_VERSION=10.0a1
> 
> MOZ_BRANDING_DIRECTORY=mobile/branding/unofficial
> MOZ_OFFICIAL_BRANDING_DIRECTORY=mobile/branding/official
> # MOZ_APP_DISPLAYNAME is set by branding/configure.sh
> 
> MOZ_SAFE_BROWSING=
> MOZ_SERVICES_SYNC=1
> 
>-MOZ_DISABLE_DOMCRYPTO=1
>-
> if test "$LIBXUL_SDK"; then
> MOZ_XULRUNNER=1
> else
> MOZ_XULRUNNER=
> MOZ_PLACES=1
> fi
> 
> if test "$OS_TARGET" = "Android"; then
>diff --git a/security/manager/ssl/src/nsCrypto.cpp b/security/manager/ssl/src/nsCrypto.cpp
>--- a/security/manager/ssl/src/nsCrypto.cpp
>+++ b/security/manager/ssl/src/nsCrypto.cpp
>@@ -17,16 +17,17 @@
>  *
>  * The Initial Developer of the Original Code is
>  * Netscape Communications Corporation.
>  * Portions created by the Initial Developer are Copyright (C) 2001
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>  *   Javier Delgadillo <javi@netscape.com>
>+ *   David Dahl <ddahl@mozilla.com>
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either the GNU General Public License Version 2 or later (the "GPL"), or
>  * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>  * in which case the provisions of the GPL or the LGPL are applicable instead
>  * of those above. If you wish to allow use of your version of this file only
>  * under the terms of either the GPL or the LGPL, and not to allow others to
>  * use your version of this file under the terms of the MPL, indicate your
>@@ -71,16 +72,19 @@
> #include "nsIScriptSecurityManager.h"
> #include "nsXPIDLString.h"
> #include "nsIGenKeypairInfoDlg.h"
> #include "nsIDOMCryptoDialogs.h"
> #include "nsIFormSigningDialog.h"
> #include "nsIJSContextStack.h"
> #include "jsapi.h"
> #include "jsdbgapi.h"
>+#include "jstypedarray.h"
>+#include "jsfriendapi.h"
>+#include "jswrapper.h"
> #include <ctype.h>
> #include "nsReadableUtils.h"
> #include "pk11func.h"
> #include "keyhi.h"
> #include "cryptohi.h"
> #include "seccomon.h"
> #include "secerr.h"
> #include "sechash.h"
>@@ -2446,16 +2450,70 @@ nsCrypto::PopChallengeResponse(const nsA
> }
> 
> NS_IMETHODIMP
> nsCrypto::Random(PRInt32 aNumBytes, nsAString& aReturn)
> {
>   return NS_ERROR_NOT_IMPLEMENTED;
> }
> 
>+NS_IMETHODIMP
>+nsCrypto::GetRandomValues(const jsval& aData, JSContext* aCtx)
>+{
>+  // Is it an object?
>+  if (JSVAL_IS_PRIMITIVE(aData)) {
>+    return NS_ERROR_DOM_NOT_OBJECT_ERR;
>+  }
>+
>+  JSObject* view = js::UnwrapObject(JSVAL_TO_OBJECT(aData));
>+
>+  // Is it an ArrayBufferView?
>+  if (!js_IsTypedArray(view)) {
>+    return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
>+  }
>+
>+  // Throw if the wrong type of ArrayBufferView is passed in
>+  // (Part of the WHATWG spec)
>+  switch (JS_GetTypedArrayType(view)) {
>+    case js::TypedArray::TYPE_INT8:
>+    case js::TypedArray::TYPE_UINT8:
>+    case js::TypedArray::TYPE_UINT8_CLAMPED:
>+    case js::TypedArray::TYPE_INT16:
>+    case js::TypedArray::TYPE_UINT16:
>+    case js::TypedArray::TYPE_INT32:
>+    case js::TypedArray::TYPE_UINT32:
>+      break;
>+    default:
>+
>+      return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
>+  }
>+
>+  PRUint32 dataLen = JS_GetTypedArrayByteLength(view);
>+
>+  // Abort if the ArrayBufferView is requesting too few values
>+  if (dataLen < 1) {
>+    return NS_OK;
>+  }
>+
>+  PRUint32 len = dataLen;
>+  unsigned char* data = static_cast<unsigned char*>(JS_GetTypedArrayData(view));
>+
>+  SECStatus srv = PK11_GenerateRandom(data, len);
>+
>+  // PK11_GenerateRandom returns
>+  // SEC_ERROR_INVALID_ARGS when too many random
>+  // bytes have been requested.
>+  if (srv != SECSuccess) {
>+    return PR_GetError() == SEC_ERROR_INVALID_ARGS 
>+      ? NS_ERROR_DOM_QUOTA_EXCEEDED_ERR : NS_ERROR_FAILURE;
>+  }
>+
>+  return NS_OK;
>+}
>+
> static void
> GetDocumentFromContext(JSContext *cx, nsIDocument **aDocument)
> {
>   // Get the script context.
>   nsIScriptContext* scriptContext = GetScriptContextFromJSContext(cx);
>   if (!scriptContext) {
>     return;
>   }
>diff --git a/security/manager/ssl/tests/mochitest/bugs/Makefile.in b/security/manager/ssl/tests/mochitest/bugs/Makefile.in
>--- a/security/manager/ssl/tests/mochitest/bugs/Makefile.in
>+++ b/security/manager/ssl/tests/mochitest/bugs/Makefile.in
>@@ -44,16 +44,17 @@ relativesrcdir	= security/ssl/bugs
> 
> include $(DEPTH)/config/autoconf.mk
> include $(topsrcdir)/config/rules.mk
> 
> _TEST_FILES = \
>         test_bug480509.html \
>         test_bug483440.html \
>         test_bug484111.html \
>+        test_bug440046.html \
>         $(NULL)
> 
> _CHROME_FILES = \
>         test_bug413909.html \
>         test_bug480619.html \
>         test_bug644006.html \
>         $(NULL)
> 
>diff --git a/security/manager/ssl/tests/mochitest/bugs/test_bug440046.html b/security/manager/ssl/tests/mochitest/bugs/test_bug440046.html
>new file mode 100644
>--- /dev/null
>+++ b/security/manager/ssl/tests/mochitest/bugs/test_bug440046.html
>@@ -0,0 +1,161 @@
>+<!DOCTYPE HTML>
>+<html><head>
>+  <title>Test bug 440046</title>
>+  <script type="text/javascript" src="/MochiKit/packed.js"></script>
>+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>        
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
>+</head>
>+<body onload="onWindowLoad()">
>+<script class="testbody" type="text/javascript">
>+SimpleTest.waitForExplicitFinish();
>+
>+var testData = [ { len: 32, type: "Int8", pass: true },
>+                 { len: 32, type: "Int16", pass: true },
>+                 { len: 32, type: "Int32", pass: true },
>+                 { len: 32, type: "Uint8", pass: true },
>+                 { len: 32, type: "Uint16", pass: true },
>+                 { len: 32, type: "Uint32", pass: true },
>+                 { len: 65536, type: "Uint8", pass: true },
>+                 { len: 32, type: "Uint8Clamped", pass: true },
>+                 { len: 65537, type: "Uint8", pass: false },
>+                 { len: 32, type: "Float32", pass: false },
>+                 { len: 32, type: "Float64", pass: false } ];
>+
>+
>+var testCount = 0;
>+
>+function testNsCryptoGetRandomValues(aLength, aType)
>+{
>+  var arrayTypes = {
>+    Int8:         Int8Array,
>+    Int16:        Int16Array,
>+    Int32:        Int32Array,
>+    Uint8:        Uint8Array,
>+    Uint16:       Uint16Array,
>+    Uint32:       Uint32Array,
>+    Float32:      Float32Array,
>+    Float64:      Float64Array,
>+    Uint8Clamped: Uint8ClampedArray,
>+  };
>+
>+  testCount++;
>+
>+  var buf = new ArrayBuffer(aLength);
>+  var arBuf = new arrayTypes[aType](buf);
>+
>+  var pass = false;
>+  window.crypto.getRandomValues(arBuf);
>+
>+  for (var i = 0; i < arBuf.length; i++) {
>+    if (i != 0) {
>+      pass = true;
>+      break;
>+    }
>+  }
>+  is(pass, true,  "Non-zero result: " + i  +  " found in the  " + aType + ": " + aLength + " ArrayBufferView");
>+}
>+
>+function onWindowLoad()
>+{
>+  window.removeEventListener("load", onWindowLoad, false);
>+  for (var i = 0; i < testData.length; i++) {
>+    if (testData[i].pass) {
>+      testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);
>+    }
>+    else {
>+      // failing tests are dealt with here
>+      if (i == 8) {
>+        try {
>+          testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);
>+        }
>+        catch (ex) {
>+          if (ex.toString().search(/QUOTA_EXCEEDED_ERR/)) {
>+            ok(true, "Extended length array buffer fails, NS_ERROR_DOM_QUOTA_EXCEEDED_ERR thrown");
>+          }
>+        }
>+      } // 8
>+
>+      if (i == 9) {
>+        try {
>+          testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);
>+        }
>+        catch (ex) {
>+          if (ex.toString().search(/TYPE_MISMATCH_ERR/)) {
>+            ok(true, "Expected TYPE_MISMATCH_ERR: Float32Array is not valid, got " + ex + ".");
>+          }
>+          else {
>+            ok(false, "Unexpected error: " + ex);
>+          }
>+        }
>+      } // 9
>+
>+      if (i == 10) {
>+        try {
>+          testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);
>+        }
>+        catch (ex) {
>+          if (ex.toString().search(/TYPE_MISMATCH_ERR/)) {
>+            ok(true, "Expected TYPE_MISMATCH_ERR: Float64Array is not valid, got " + ex + ".");
>+          }
>+          else {
>+            ok(false, "Unexpected error: " + ex);
>+          }
>+        }
>+      }
>+    }
>+  }
>+  // Count the tests in the testData array
>+  ok(testCount == 11, "11 tests run via testData");
>+
>+  // Test a null argument
>+  try {
>+    window.crypto.getRandomValues(null);
>+  }
>+  catch (ex) {
>+    // We search for different errors based on if the callee is JS or Native code
>+    var test = ex.toString().search(/TYPE_ERR/);
>+    var testMobile = ex.toString().search(/TypeErr/);
>+    ok((test > -1) || (testMobile > -1),
>+       "Expected TYPE_ERR, got " + ex + ".");
>+  }
>+
>+  // Test a zero-length buffer view
>+  try {
>+    var a = new Int8Array(0);
>+    window.crypto.getRandomValues(a);
>+    ok(a[0] === undefined, "The array buffer is unchanged, still 0 length");
>+  }
>+  catch (ex) {
>+    ok(false, "A zero-length array buffer view should not fail");
>+  }
>+
>+  // Test a one-length buffer view
>+  try {
>+    var a = new Int8Array(1);
>+    window.crypto.getRandomValues(a);
>+    ok(a[0] !== 0, "The array buffer has one random value");
>+  }
>+  catch (ex) {
>+    ok(false, "A one-length array buffer view should not fail");
>+  }
>+
>+  // Test a range of an array buffer view
>+  var buffer = new ArrayBuffer(32);
>+  var view = new Int8Array(buffer, 0, 16);
>+  var view2 = new Int8Array(buffer, 16, 16);
>+  for (var i = 0; i < view2.byteLength; i++) {
>+    view2[i] = 1;
>+  }
>+  window.crypto.getRandomValues(view);
>+  for (var i=0; i < view.byteLength; i++) {
>+    is(view2[i], 1, "view2 is unchanged");
>+  }
>+
>+  // test an offset view
>+  window.crypto.getRandomValues(view2);
>+  ok(view2[0] != 1, "view2 has been updated ");
>+
>+  SimpleTest.finish();
>+}
>+</script>
>+</body></html>
Attachment #573386 - Flags: review?(evilpies)
Comment on attachment 573386 [details] [diff] [review]
v 20 rebased, built and tested on birch

>+[scriptable, uuid(9311137d-3495-4c9a-97a2-21f3a18ad203)]
> interface nsIDOMCrypto : nsISupports
> {
>   readonly attribute DOMString        version;
>   attribute boolean         enableSmartCardEvents;
> 
>   nsIDOMCRMFObject          generateCRMFRequest(/* ... */);
>   DOMString                 importUserCertificates(in DOMString nickname,
>                                                    in DOMString cmmfResponse,
>                                                    in boolean doForcedBackup);
>   DOMString                 popChallengeResponse(in DOMString challenge);
>   DOMString                 random(in long numBytes);
>+  [implicit_jscontext]
>+  void                      getRandomValues(in jsval aData);
Add some comment what this method is doing.
Also, is there need for implicit_jscontext?

>+
>+  PRUint32 len = dataLen;
Useless variable 'len'

>+  SECStatus srv = PK11_GenerateRandom(data, len);
I have no idea about this, so someone else needs to review this.
Attachment #573386 - Flags: review?(bugs) → review+
Target Milestone: --- → mozilla11
Target Milestone: mozilla11 → mozilla12
Someone familiar with JS API should review this too.
Comment on attachment 573386 [details] [diff] [review]
v 20 rebased, built and tested on birch

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

::: security/manager/ssl/src/nsCrypto.cpp
@@ +2462,5 @@
> +  if (JSVAL_IS_PRIMITIVE(aData)) {
> +    return NS_ERROR_DOM_NOT_OBJECT_ERR;
> +  }
> +
> +  JSObject* view = js::UnwrapObject(JSVAL_TO_OBJECT(aData));

I am not sure that we need to unwrap here.

@@ +2488,5 @@
> +
> +  PRUint32 dataLen = JS_GetTypedArrayByteLength(view);
> +
> +  // Abort if the ArrayBufferView is requesting too few values
> +  if (dataLen < 1) {

dataLen == 0

::: security/manager/ssl/tests/mochitest/bugs/test_bug440046.html
@@ +152,5 @@
> +  }
> +
> +  // test an offset view
> +  window.crypto.getRandomValues(view2);
> +  ok(view2[0] != 1, "view2 has been updated ");

Some of these tests seem fishy, because what prevents this from producing '1' ?
(In reply to Tom Schuster (evilpie) from comment #174)
> > +  // test an offset view
> > +  window.crypto.getRandomValues(view2);
> > +  ok(view2[0] != 1, "view2 has been updated ");
> 
> Some of these tests seem fishy, because what prevents this from producing
> '1' ?

I know. Unfortunately, there isn't really a great way to test the PRNG when we don't have the ability to control the seed in the test. :( However, as long as the PRNG doesn't always/usually/often return 1, the test will find the type of bug that it is intended to find (ignoring the slice parameters of the array buffer, and always writing to the start of the buffer).
Target Milestone: mozilla12 → ---
Could you generate multiple random values in this way and only fail if they were all 1?  Assuming the chance of a false positive is already fairly small, it would make it even smaller, at a fairly small additional testing cost.
Comment on attachment 573386 [details] [diff] [review]
v 20 rebased, built and tested on birch

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

::: security/manager/ssl/tests/mochitest/bugs/test_bug440046.html
@@ +59,5 @@
> +{
> +  window.removeEventListener("load", onWindowLoad, false);
> +  for (var i = 0; i < testData.length; i++) {
> +    if (testData[i].pass) {
> +      testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);

needs to be surrounded with:

   try {
      ...
   } catch (ex) {
      ok(false, "unexpected error ...")
   }

@@ +65,5 @@
> +    else {
> +      // failing tests are dealt with here
> +      if (i == 8) {
> +        try {
> +          testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);

failedWithCorrectError = false;

@@ +68,5 @@
> +        try {
> +          testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);
> +        }
> +        catch (ex) {
> +          if (ex.toString().search(/QUOTA_EXCEEDED_ERR/)) {

failedWithCorrectError = ex.toString().search(/QUOTA_EXCEEDED_ERR/);

Is search(/QUOTA_EXCEEDED_ERR/) really the best way to test that the correct exception is being thrown? A DOM reviewer should help with this.

@@ +71,5 @@
> +        catch (ex) {
> +          if (ex.toString().search(/QUOTA_EXCEEDED_ERR/)) {
> +            ok(true, "Extended length array buffer fails, NS_ERROR_DOM_QUOTA_EXCEEDED_ERR thrown");
> +          }
> +        }

ok(failedWithCorrectError, "Extended length array buffer fails, NS_ERROR_DOM_QUOTA_EXCEEDED_ERR thrown");

and similarly with the other error tests, #9 and #10.

@@ +111,5 @@
> +  try {
> +    window.crypto.getRandomValues(null);
> +  }
> +  catch (ex) {
> +    // We search for different errors based on if the callee is JS or Native code

Why? This seems very strange that there would be a difference here.
Attachment #573386 - Flags: superreview?(kaie)
Attachment #573386 - Flags: superreview?(doug.turner)
Attachment #573386 - Flags: review?(doug.turner)
Attachment #573386 - Flags: review?(bsmith)
Attachment #573386 - Flags: review+
Comment on attachment 573386 [details] [diff] [review]
v 20 rebased, built and tested on birch

Actually, I would like to see the questions about mobile answered before I r+.
Attachment #573386 - Flags: review?(bugs)
Attachment #573386 - Flags: review-
Attachment #573386 - Flags: review+
It seems like the easiest way to do the window.crypto stuff on mobile would be to use #ifdef to make the implementations of the not-implemented-on-mobile functions unconditionally return a "not implemented" error on mobile.
Comment on attachment 573386 [details] [diff] [review]
v 20 rebased, built and tested on birch

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

Assuming that somebody checks on the Unwrap r+

::: security/manager/ssl/src/nsCrypto.cpp
@@ +2457,5 @@
>  
> +NS_IMETHODIMP
> +nsCrypto::GetRandomValues(const jsval& aData, JSContext* aCtx)
> +{
> +  // Is it an object?

I am not a big fan of this kind of comments (but not my module so .. ;)

@@ +2481,5 @@
> +    case js::TypedArray::TYPE_INT32:
> +    case js::TypedArray::TYPE_UINT32:
> +      break;
> +    default:
> +

Nit: get rid of the space

@@ +2485,5 @@
> +
> +      return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
> +  }
> +
> +  PRUint32 dataLen = JS_GetTypedArrayByteLength(view);

I think somebody already mentioned that, but move this down.
Attachment #573386 - Flags: review?(evilpies) → review+
(In reply to Brian Smith (:bsmith) from comment #177)
> Comment on attachment 573386 [details] [diff] [review]
> v 20 rebased, built and tested on birch
> 
> 
> @@ +65,5 @@
> > +    else {
> > +      // failing tests are dealt with here
> > +      if (i == 8) {
> > +        try {
> > +          testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);
> 
> failedWithCorrectError = false;
> 
> @@ +68,5 @@
> > +        try {
> > +          testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);
> > +        }
> > +        catch (ex) {
> > +          if (ex.toString().search(/QUOTA_EXCEEDED_ERR/)) {
> 
> failedWithCorrectError = ex.toString().search(/QUOTA_EXCEEDED_ERR/);
> 
> Is search(/QUOTA_EXCEEDED_ERR/) really the best way to test that the correct
> exception is being thrown? A DOM reviewer should help with this.

I think the issue here is the interaction with the e10s mobile patch and how it throws exceptions a little bit differently inside the content process, and what is marshalled ove ris not quite the same exception object. Since we are not going to worry about e10s right now, we can no doubt simplify this test. 
> 
> @@ +71,5 @@
> > +        catch (ex) {
> > +          if (ex.toString().search(/QUOTA_EXCEEDED_ERR/)) {
> > +            ok(true, "Extended length array buffer fails, NS_ERROR_DOM_QUOTA_EXCEEDED_ERR thrown");
> > +          }
> > +        }
> 
> ok(failedWithCorrectError, "Extended length array buffer fails,
> NS_ERROR_DOM_QUOTA_EXCEEDED_ERR thrown");
> 
> and similarly with the other error tests, #9 and #10.

Same here. this can be simplified too.
> 
> @@ +111,5 @@
> > +  try {
> > +    window.crypto.getRandomValues(null);
> > +  }
> > +  catch (ex) {
> > +    // We search for different errors based on if the callee is JS or Native code
> 
> Why? This seems very strange that there would be a difference here.

Again, we were having to marshal JS-generated errors inside the content process. I'll fix this.
Target Milestone: --- → mozilla12
Version: Trunk → Other Branch
Attached patch Patch 21 (obsolete) — Splinter Review
Fixed tests, tweaked some nits in nsCrypto.cpp. Changed the test so we ignore mobile for now, as it seems impractical to have the mobile patch and this one land concurrently
Attachment #573386 - Attachment is obsolete: true
Attachment #583634 - Flags: review?(bsmith)
Attachment #573386 - Flags: superreview?(kaie)
Attachment #573386 - Flags: review?(doug.turner)
Attachment #573386 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #172)
> Comment on attachment 573386 [details] [diff] [review]
> >+  [implicit_jscontext]
> >+  void                      getRandomValues(in jsval aData);
> Add some comment what this method is doing.
> Also, is there need for implicit_jscontext?

Apparently not. The test suite passes without it.
(In reply to Brian Smith (:bsmith) from comment #179)
> It seems like the easiest way to do the window.crypto stuff on mobile would
> be to use #ifdef to make the implementations of the
> not-implemented-on-mobile functions unconditionally return a "not
> implemented" error on mobile.

That is already in place - so I think we should just ignore mobile until this lands, then devise the correct approach in bug 673432.
Attached patch Patch 22 (obsolete) — Splinter Review
Attachment #583634 - Attachment is obsolete: true
Attachment #583648 - Flags: review?(bsmith)
Attachment #583634 - Flags: review?(bsmith)
1. There is new support for typed arrays in XPConnect [1]. Should we implement this using that new support?

2. I talked to Andreas and he said that we will need the XUL implementation for B2G. And, we will need it for tablets. So, we should get together to figure out how the mobile versions are going to work.

3. I still don't understand why the test doesn't check for exceptions like this:
   pass = false;
   try {
      ...   
   } catch (e if e instanceof QuotaExceededError) {
      psas = true;
   }
   ok(pass, "The correct exception was thrown");

Isn't this how exceptions are normally handled in (chrome) JS?

[1] http://bholley.wordpress.com/2011/12/13/typed-arrays-supported-in-xpconnect/
(In reply to Brian Smith (:bsmith) from comment #186)
> 1. There is new support for typed arrays in XPConnect [1]. Should we
> implement this using that new support?

No, this requires the caller to explicitly pass the length of the array. That's not a problem for internal APIs, but doesn't fly for Web APIs.

> 3. I still don't understand why the test doesn't check for exceptions like
> this:
>    pass = false;
>    try {
>       ...   
>    } catch (e if e instanceof QuotaExceededError) {

This is a SpiderMonkey-ism that would make it harder for other browsers and standards test suites to reuse our tests, so we shouldn't do this.
(In reply to Ms2ger from comment #187)
> >    try {
> >       ...   
> >    } catch (e if e instanceof QuotaExceededError) {
> 
> This is a SpiderMonkey-ism that would make it harder for other browsers and
> standards test suites to reuse our tests, so we shouldn't do this.

We discussed this on IRC. These tests should look like this:

   try {
      ...
      pass = false;
   } catch (e) {
      pass = e instanceof QuotaExceededError &&
             e.code == DOMException.QUOTA_EXCEEDED_ERROR;
   }
   ok(pass, "Correct exception was thrown for too-large array");

But, we do not implement the DOM exception types yet (bug 691017), so for now we will have to write:

   try {
      ...
      pass = false;
   } catch (e) {
      try {
         // bug 691017
         todo(e instanceof QuotaExceededError, "Exception was the correct type");
      } catch (e) { }
      pass = e instanceof DOMException &&
             e.code == DOMException.QUOTA_EXCEEDED_ERROR;
   }
   ok(pass, "Correct exception was thrown for too-large array");

I filed follow-up bug 713433 for fixing this.
David Dahl: Please don't forget to update the patch fix some of the previous review comments.
(In reply to Brian Smith (:bsmith) from comment #188)
>       try {
>          // bug 691017
>          todo(e instanceof QuotaExceededError, "Exception was the correct
> type");
>       } catch (e) { }

I'd write this as

todo("QuotaExceededError" in window && e instanceof QuotaExceededError,
     "Exception was the correct type");

to avoid the extra throw/catch
(In reply to David Dahl :ddahl from comment #183)
> (In reply to Olli Pettay [:smaug] from comment #172)
> > Comment on attachment 573386 [details] [diff] [review]
> > >+  [implicit_jscontext]
> > >+  void                      getRandomValues(in jsval aData);
> > Add some comment what this method is doing.
> > Also, is there need for implicit_jscontext?
> 
> Apparently not. The test suite passes without it.

Hmm. On second thought - and after an attempted clobber build - it failed without the [implicit_jscontext]
Attached patch Patch 23 (obsolete) — Splinter Review
Updated patch, tested build, added todo() check
Attachment #583648 - Attachment is obsolete: true
Attachment #585931 - Flags: review?(bsmith)
Attachment #583648 - Flags: review?(bsmith)
(In reply to David Dahl :ddahl from comment #191)
> (In reply to David Dahl :ddahl from comment #183)
> > (In reply to Olli Pettay [:smaug] from comment #172)
> > > Comment on attachment 573386 [details] [diff] [review]
> > > >+  [implicit_jscontext]
> > > >+  void                      getRandomValues(in jsval aData);
> > > Add some comment what this method is doing.
> > > Also, is there need for implicit_jscontext?
> > 
> > Apparently not. The test suite passes without it.
> 
> Hmm. On second thought - and after an attempted clobber build - it failed
> without the [implicit_jscontext]

How so? The only effect [implicit_jscontext] should have is passing the JSContext argument to your method, and you don't use that argument.
Comment on attachment 585931 [details] [diff] [review]
Patch 23

And we've got a much nicer jsval API:

>+nsCrypto::GetRandomValues(const jsval& aData, JSContext* aCtx)
>+{
>+  // Make sure this is a JavaScript object
>+  if (JSVAL_IS_PRIMITIVE(aData)) {

if (!aData.isObject()) {

>+  JSObject* view = js::UnwrapObject(JSVAL_TO_OBJECT(aData));

JSObject* view = js::UnwrapObject(&aData.toObject());
(In reply to Ms2ger from comment #193)
> How so? The only effect [implicit_jscontext] should have is passing the
> JSContext argument to your method, and you don't use that argument.
Right you are. Forgot to remove the JSContext from the method.


(In reply to Ms2ger from comment #194)
> Comment on attachment 585931 [details] [diff] [review]
> Patch 23
> 
> And we've got a much nicer jsval API:
> JSObject* view = js::UnwrapObject(&aData.toObject());

Nice and clean. New patch on its way.
Attached patch Patch 24 (obsolete) — Splinter Review
Attachment #585931 - Attachment is obsolete: true
Attachment #586072 - Flags: review?(bsmith)
Attachment #585931 - Flags: review?(bsmith)
Attached patch Patch 25 (obsolete) — Splinter Review
removed the unwrap call
Attachment #586072 - Attachment is obsolete: true
Attachment #586095 - Flags: review?(bsmith)
Attachment #586072 - Flags: review?(bsmith)
Summary: expose secure PRNG in window.crypto → expose secure PRNG in the DOM (window.crypto.getRandomValues)
Blocks: 720463
After meeting with folks on the WebAPI team this week, some issues came up about the WHATWG spec for this method.

window.crypto.getRandomValues() will probably need to change to a moz-prefixed window.crypto.mozGenerateRandomValues(). 

The feedback from the WebAPI team was that the WHATWG spec is suboptimal for JS developers. It might be better if it looks like:

    ArrayBuffer mozGenerateRandomValues(int aLength);
(In reply to David Dahl :ddahl from comment #198)
> The feedback from the WebAPI team was that the WHATWG spec is suboptimal for
> JS developers. It might be better if it looks like:
>     ArrayBuffer mozGenerateRandomValues(int aLength);


What?!
IMHO this is absolutely ridiculous bikeshedding.

getRandomValues works and delivers what it needs to do in an with a very efficient and flexible API.

mozGenerateRandomValues has absolutely NO technical advantage :
1) if you want to generate many (large) arrays or do circular buffers, it requires a lot of object creation and GC work.
2) you don't want to get returned an ArrayBuffer, it is completely inflexible!! You'll then have to do gymnastics around it to provide the ArrayBufferView you want (in the type you want)


For the very discussable "JS developers" argument - if *anyone* prefers to make it inefficient but with an array as return value, there is nothing preventing a JavaScript (or library) developer to do this :

function mozGenerateRandomValues(aLength) {
   let buffer = new ArrayBufferView(aLength);
   window.crypto.getRandomValues(buffer);
   return buffer;
}

(but again, this is so inflexible and inefficient that I hope nobody doing crypto would need this)
(In reply to Cedric Vivier [:cedricv] from comment #199)
I apologize if it sounded aggressive, but in the current 'high' of the CSS prefixing debate, I am quite shocked of this news.

Is there any _technical_ argument in favor of the latter API?
This landed in WebKit a year ago. We should not prefix or change the API.
(In reply to Cedric Vivier [:cedricv] from comment #200)
> Is there any _technical_ argument in favor of the latter API?

1. The ArrayBufferView can be altered in the midst of being written to
2. The WebAPI team does not want to use ArrayBufferViews at all in DOM APIs
3. outparams are bad form going forward

bent, Jonas - I thought there might be more reasons, what am I missing here?
(In reply to David Dahl :ddahl from comment #202)
> 1. The ArrayBufferView can be altered in the midst of being written to
How so??

> 2. The WebAPI team does not want to use ArrayBufferViews at all in DOM APIs
Why?
An ArrayBuffer is not supposed to be exposed in APIs, it is merely a backing store for an ArrayBufferView! It has no useful interface attached to it.

> 3. outparams are bad form going forward
For this particular 'low-level' API it does not really make sense otherwise on both performance and usability, as already discussed long time ago in the WHATWG list and here in comments 25, 30, 31, 36.

On illustrate the usability/flexibility side :


Use Case #1
I want to get a new array of 65k random bytes (uint8)

var buffer = new UInt8Array(65536);
window.crypto.getRandomValues(buffer);
--
var buffer = mozGenerateRandomValues(65536);

PERF/MEM - Same.
TERSENESS - mozGenerateRandomValues wins per 1 line
WINNER - mozGenerateRandomValues

===
Use Case #2
I want to get a new array of 32k signed shorts (int16)

var buffer = new Int16Array(32768);
window.crypto.getRandomValues(buffer);
--
var temp = mozGenerateRandomValues(65536);
buffer = new Int16Array(temp);

PERF/MEM - Same
TERSENESS - getRandomValues does not need a temp variable
WINNER getRandomValues

==
Use Case #3
I want to repeatedly fill one fixed size buffer with new random data (eg. circular buffer).

window.crypto.getRandomValues(buffer);
--
var temp = mozGenerateRandomValues(buffer.length);
buffer.set(buffer, 0);

PERF/MEM - getRandomValues does not need to allocate <fixed size> at each fill, reduces GC pressure
TERSENESS - getRandomValues wins per 1 line
WINNER getRandomValues

==

Use Case #4
I want to fill a (possibly dynamic) subset [start:end] of an existing buffer with new random data
 
var subset = new Int8Array(buffer, start, end - start);
window.crypto.getRandomValues(subset);
--
var temp = mozGenerateRandomValues(end - start);
buffer.set(temp, start);

PERF/MEM - getRandomValues does not need to allocate a temporary subset array
TERSENESS - Same.
WINNER getRandomValues.
(In reply to David Dahl :ddahl from comment #202)
> 2. The WebAPI team does not want to use ArrayBufferViews at all in DOM APIs

So what's the plan to allow passing, well, array views without deep-copying array buffers everytime?

The WebGL spec allows passing ArrayBufferViews because, well, that's the only known way to avoid that performance overhead.
I've been asked to provide some feedback on the API here, so here's my thoughts:

Most javascript APIs work by returning new objects. It's very rare for APIs to take an object and modify it and use that as a way to produce a result. The only example I can think of is the almost universally hated XPathEvaluator.evaluate API.

I don't see why things need to be different just because we're dealing with binary data?

The argument that lead to XPathEvaluator.evaluate's design was that people were concern about object creation time. Is that the argument here too?

If so, can we get some numbers on that before we design the API around the assumption that object creation is slow? Especially given that modern javascript engines and GC strategies have dramatically changed which operations are slow and which aren't. I'd particularly want to test in modern Chrome versions since they have a good GC strategy which we're working towards in Firefox.
(In reply to Jonas Sicking (:sicking) from comment #205)
> I don't see why things need to be different just because we're dealing with
> binary data?

Contrary to the API you cited, which 'just' instantiates one object (usually of fixed size and/or has lazy properties), dealing with binary data can be much more costly on both performance and memory usage since it is unbounded.
If you want to fill, say, 512KB of random data... you need to allocate 512KB every time.


> The argument that lead to XPathEvaluator.evaluate's design was that people
> were concern about object creation time. Is that the argument here too?

No, the object creation time itself is insignificant (nowadays anyways), the "new *Array(x)" is not important.
The allocation time and memory usage for binary data storage IS.

Furthermore, since you lose flexibility when using a returning API here - you also need to _copy_ the binary data around for many use cases, you lose twice (see comment 203).


Performance aside, it's also more cumbersome to use (comment 203 again), in Use Case #2, you even have to calculate 32768 * Int16Array.BYTES_PER_ITEM instead of just passing around the array you want to fill.
(In reply to Cedric Vivier [:cedricv] from comment #206)
> (In reply to Jonas Sicking (:sicking) from comment #205)
> > I don't see why things need to be different just because we're dealing with
> > binary data?
> 
> Contrary to the API you cited, which 'just' instantiates one object (usually
> of fixed size and/or has lazy properties), dealing with binary data can be
> much more costly on both performance and memory usage since it is unbounded.
> If you want to fill, say, 512KB of random data... you need to allocate 512KB
> every time.

"every time" indicates that it's something done multiple times.

Are there common use cases for wanting to generate 512KB of random data many times over?

> Furthermore, since you lose flexibility when using a returning API here -
> you also need to _copy_ the binary data around for many use cases, you lose
> twice (see comment 203).

I don't see which of the use cases in comment 203 that causes multiple copies to happen. The one case I can think of is if you want to generate random numbers and then send the buffer to a worker thread and do this many times over.

In that case it's likely that the buffer needs to be copied into shared memory as part of sending the buffer to the other thread. Whereas if you were able to write into an existing buffer, you can transfer the buffer back and forth between the window and the worker and write into the same shared buffer each time.

Is that the case you are concerned about? Do you think that will be a common use case?

> Performance aside, it's also more cumbersome to use (comment 203 again), in
> Use Case #2, you even have to calculate 32768 * Int16Array.BYTES_PER_ITEM
> instead of just passing around the array you want to fill.

This comes down to which use cases are most common I think. Counting the number of use cases which are smaller in any given syntax doesn't tell a lot. What we need to do is to look at which use cases are common.


One possible solution to all of this is to make getRandomValues either take a number, in which case it returns a new ArrayBuffer, or an ArrayBuffer, in which case it fills it with random data.
(In reply to Jonas Sicking (:sicking) from comment #207)
> Are there common use cases for wanting to generate 512KB of random data many
> times over?
I see at least two :

Example 1 - If you have a lot of data to encrypt, you might need to be able to fetch random data from the random pool many times over (eg. each message with a new key).
Example 2 - Demo scene, animations, procedural content/textures using noise as seed


> I don't see which of the use cases in comment 203 that causes multiple
> copies to happen.

Maybe it's not obvious, but buffer.set means "copy". It's equivalent to memcpy in typed array speak.


> The one case I can think of is if you want to generate
> random numbers and then send the buffer to a worker thread and do this many
> times over.
> Is that the case you are concerned about? Do you think that will be a common
> use case?

This an interesting use case. Changing the API though does not provide any benefit for it.

> > Performance aside, it's also more cumbersome to use (comment 203 again), in
> > Use Case #2, you even have to calculate 32768 * Int16Array.BYTES_PER_ITEM
> > instead of just passing around the array you want to fill.
> 
> This comes down to which use cases are most common I think. Counting the
> number of use cases which are smaller in any given syntax doesn't tell a
> lot. What we need to do is to look at which use cases are common.

It's not only about terseness, it's also about performance and principle of least surprise, having to think when you need to count how many bytes you want to actually fill a typed array entirely, regardless of his type, is more 'surprising' than "fill this array entirely".


> One possible solution to all of this is to make getRandomValues either take
> a number, in which case it returns a new ArrayBuffer, or an ArrayBuffer, in
> which case it fills it with random data.

This would make the API and documentation more complex for little reason IMHO.
Your suggestion could be as easily done in JavaScript alone if one *really* prefers to have an API that returns a new array every time but *really* feels bad about being explicit about it (eg. "new Uint8Array(length)" before the getRandomValues call).
The inverse is not possible.
(In reply to Cedric Vivier [:cedricv] from comment #208)
> (In reply to Jonas Sicking (:sicking) from comment #207)
> > Are there common use cases for wanting to generate 512KB of random data many
> > times over?
> I see at least two :
> 
> Example 1 - If you have a lot of data to encrypt, you might need to be able
> to fetch random data from the random pool many times over (eg. each message
> with a new key).

Would you really need 512KB of random data for each message? I would have expected that you'd need a 1-2Kbit sized key for each message which is significantly less data. If a caller cares about performance you'd probably just do a single call to get 512KB of data and then use separate chunks of that to encrypt the messages.

But I'm not an expert in crypto so please let me know if I'm wrong.


> Example 2 - Demo scene, animations, procedural content/textures using noise
> as seed

Demo scene would never use crypographically secure numbers in a performance sensitive way. That's just wasted cycles. Instead what you'd do is you'd generate a seed once, and then use a faster random number generator based on that seed.

I would expect the same thing if all you need is procedural content/textures using noise.

> > I don't see which of the use cases in comment 203 that causes multiple
> > copies to happen.
> 
> Maybe it's not obvious, but buffer.set means "copy". It's equivalent to
> memcpy in typed array speak.

So again, what's the use case for wanting to fill the middle of a buffer with random numbers? Can you give an estimate for how common such a use case is?

> > > Performance aside, it's also more cumbersome to use (comment 203 again), in
> > > Use Case #2, you even have to calculate 32768 * Int16Array.BYTES_PER_ITEM
> > > instead of just passing around the array you want to fill.
> > 
> > This comes down to which use cases are most common I think. Counting the
> > number of use cases which are smaller in any given syntax doesn't tell a
> > lot. What we need to do is to look at which use cases are common.
> 
> It's not only about terseness, it's also about performance and principle of
> least surprise, having to think when you need to count how many bytes you
> want to actually fill a typed array entirely, regardless of his type, is
> more 'surprising' than "fill this array entirely".

It's also about following the style of the rest of the language. I would have more of an understanding for wanting to optimize for performance if anyone could show that this API will be used in performance critical code paths, but generally cryptographically strong numbers aren't known for being used in performance critical code. Quite the opposite, they are known for being used where you care about correctness rather than performance.

I'm definitely willing to create an API optimized for performance rather than fit with the rest of the language if someone can show there's a need for that. So far comment 203 just lists various theoretical things people could do with a random number API.
(In reply to Jonas Sicking (:sicking) from comment #209)
> Would you really need 512KB of random data for each message? I would have
> expected that you'd need a 1-2Kbit sized key for each message which is
> significantly less data. If a caller cares about performance you'd probably
> just do a single call to get 512KB of data and then use separate chunks of
> that to encrypt the messages.

Yes, maybe, yes... the thing with the current API as it is now, we don't have to think about use cases that we might overlook... it's flexible and efficient no matter what you actually want to do with it.


> So again, what's the use case for wanting to fill the middle of a buffer
> with random numbers?

Same as above. 
It might or might not.. it's just possible to do, efficiently, if you want to do it!

[[
One *existing* use case I'm just thinking about now: jsLinux ;)
To simulate the OS easily, it uses one big typed array to simulate the memory - therefore to implement /dev/random, as this API would allow, you'd rather just write to the subset of memory needed - zero-copy.
]]


> Can you give an estimate for how common such a use case is?

No, but the same as above applies again.

Can you give me an estimate on how much happier developers would be to have an incompatible and less flexible API than what shipped already in another browser? ;)


> It's also about following the style of the rest of the language. 

Despite you keep moving the argument on performance... but my main point all along is more about flexibility and ease of use, I haven't seen a rebuttal about that.

ArrayBuffers are not flexible nor easy to use at all, ArrayBufferViews are the interface you typically manipulate typed arrays with! As bjacob pointed out as well!

About style, returning an object, yes. Returning a list, yes. There's precedent on that, I agree - anyways it's not possible to do otherwise, an outparam would not be practical to use in JavaScript ;)

Returning a filled unbounded memory buffer, there's not much precedent in JavaScript here. The precedents in WebGL and upcoming WebCL are actually to fill ArrayBufferViews, not allocate them

In many languages, allocating to fill is considered a code smell. It's not because JavaScript is 'new' with binary/typed/native data that it should not follow the conventions here.

Also, besides subjective style, how about following the many many years of experience from all existing/well-known crypto APIs (native or otherwise) ? 
They are all caller-allocated.


In a nutshell :

1) The getRandomValues API has been discussed by the WHATWG, in public, one year ago, returning an array has been dismissed as inflexible/inefficient by consensus.
2) It's been discussed here in comments 25-30-31-36 and more.
3) A specification has been written.
4) It has been implemented and has been available in WebKit for almost one year now, no negative feedback known. 
5) The API is flexible and efficient for any use case - it builds on the many many years of experience from most if not all existing other crypto APIs (native or otherwise) ... which do not allocate themselves as well. 
6) We have a patch (almost) ready in this bug, delivering value to web developers and already compatible with any content already using getRandomValues (WebKit/mobile sites?).

Why don't we ship? :(
(In reply to Cedric Vivier [:cedricv] from comment #210)
> 1) The getRandomValues API has been discussed by the WHATWG, in public, one
> year ago, returning an array has been dismissed as inflexible/inefficient by
> consensus.

I'm assuming this is the thread you're talking about:

  http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2011-February/030249.html

What I see is that you proposed this filling API and no one challenged it really. Everyone basically says, "Good idea!" and that's the end. That's fine (it is a good idea - I don't mean to imply otherwise), but Jonas has pointed out several issues that were never raised in that discussion, so I don't think we should necessarily be bound by the consensus reached there.

> 2) It's been discussed here in comments 25-30-31-36 and more.

I find it hard to draw consensus from the comments in this bug. Several people agree that having a filling API is better than one which causes allocation, but then everyone also seems to agree that generating large amounts of random data is both uncommon and potentially hazardous (if it drains available entropy). The conclusion I draw from those two positions is that the allocation strategy really doesn't matter here...

> 3) A specification has been written.

Lots of specifications have been written, and we have to continually evaluate the ones which we think are worth implementing and which are not.

> 4) It has been implemented and has been available in WebKit for almost one
> year now, no negative feedback known. 

It's always hard to know how much this kind of thing matters... Are lots of people using this API if it's only available in WebKit? Surely some are, but how can we draw any kind of conclusions about developer satisfaction with this API based on "no negative feedback known" from an unknown number of web pages?

> 5) The API is flexible and efficient for any use case - it builds on the
> many many years of experience from most if not all existing other crypto
> APIs (native or otherwise) ... which do not allocate themselves as well.

In JS? I know lots of C libraries do this sort of thing, but this isn't C. In JS in-out parameters are rare and most APIs return objects. That's the world that we live in here. However, as Jonas said, making an API that breaks these conventions is certainly possible provided we have a compelling reason. Do we?

> 6) We have a patch (almost) ready in this bug, delivering value to web
> developers and already compatible with any content already using
> getRandomValues (WebKit/mobile sites?).

And it could easily be changed to return a new array rather than modifying one that is passed in if we decide that's best. We're not talking about a huge change that will require going back to the drawing board, after all.
Calling malloc each time you generate random numbers is wasteful.  Most crypto APIs let their clients use an already allocated buffer, including NSS:

http://mxr.mozilla.org/security/source/security/nss/lib/pk11wrap/pk11pub.h#286
(In reply to Adam Barth from comment #212)
> Calling malloc each time you generate random numbers is wasteful.

Sure. So far I haven't really been talking about malloc, just about JS object allocation. JS objects come from a pool (more or less) so allocating a JS object is not necessarily going to call malloc. ArrayBuffers do malloc a separate buffer if they're larger than a certain size (not sure what that size is), but it's not necessary to call malloc in all cases there either.
All, please do your API bikeshedding on the whatwg mailing list instead of in this bug. Thanks.
(In reply to ben turner [:bent] from comment #211)
> Several
> people agree that having a filling API is better than one which causes
> allocation, but then everyone also seems to agree that generating large
> amounts of random data is both uncommon and potentially hazardous (if it
> drains available entropy). The conclusion I draw from those two positions is
> that the allocation strategy really doesn't matter here...

... and this has absolutely no relationship with your proposed API change... :(
In both cases you could generate large amounts.

I gave plenty example use cases, you dismissed them as 'theoritical'.
I gave one *existing* use case (jsLinux/emscripten kind of apps), you also seem to dismiss it as "non-important"... :/


> In JS? I know lots of C libraries do this sort of thing, but this isn't C.
> In JS in-out parameters are rare and most APIs return objects.

I think you are a bit confused to consider this an out parameter.
In IDL, this is an "in ArrayBufferView" argument, the passed object is immutable.

Again, I gave examples here of current JS APIs that fills typed arrays with data... on the other hand I've seen no example of an API that allocates and returns a new typed array/buffer of developer-requested size (there is none - besides the constructor).


> making an API that
> breaks these conventions is certainly possible provided we have a compelling
> reason. Do we?

At least, it doesn't break crypto APIs conventions (as implemented in high-level languages as well) and general CS conventions at least.
JavaScript isn't a silo where all other CS rules and conventions do not apply.


Again, what is the technically compelling reason for changing this API to return a new array?! :/

The only argument I've seen so far is the subjective/debatable 'style' - despite all other drawbacks - in style, flexibility, and ease of use included - myself and others pointed out but got dismissed to focus the discussion on supposedly non-needed performance... (not needed EVER? ;) )


I second Ms2ger, please post your concerns on the WHATWG list if you still the API is so broken currently that it's worth the effort.
I think I might have a proposal that is palatable to everyone.

The main problem that I have with a in/out argument is that the syntax tends to be a pain to use. This is something I used to run into a lot in Perl back when I was writing perl. I'd end up with code like:

$tmpstr = someFunction();
$tmpstr ~= s/expression/;
doStuff($tmpStr);

Rather than being able to write:

doStuff(regexp(someFunction(), s/expression/));


The same thing feels like you'd end up with here:

var tmpBuffer = new UInt8Array(65536);
window.crypto.getRandomValues(tmpBuffer);
doStuff(tmpBuffer);


However, if we make getRandomValues return the buffer it just modified, then you can write this as:

doStuff(window.crypto.getRandomValues(new UInt8Array(65536));

It'll also give us a nice extension point to pass in a size instead of a buffer if we want to (now or in the future).


Does this sound acceptable to everyone?
(In reply to Jonas Sicking (:sicking) from comment #216)
> I think I might have a proposal that is palatable to everyone.

This bug is not the correct forum for this discussion.
> Does this sound acceptable to everyone?

Sounds fine to me.
(In reply to Jonas Sicking (:sicking) from comment #216)
> It'll also give us a nice extension point to pass in a size instead of a
> buffer if we want to (now or in the future).
> Does this sound acceptable to everyone?

Sounds good. I agree it's an improvement that does not have any drawback.
Should the IDL return type be 'any' or 'ArrayBufferView' ?
> Should the IDL return type be 'any' or 'ArrayBufferView' ?

I've updated the spec to return an ArrayBufferView.
Attached patch Patch 26 (obsolete) — Splinter Review
getRandomValues now also returns the typed array
Attachment #586095 - Attachment is obsolete: true
Attachment #586095 - Flags: review?(bsmith)
Now, to delay this patch a little more, bsmith would like me to rebase this over the patch on bug 673432. I am of the opinion that landing this as is has value and working out the refactoring for mobile in bug 673432 next. Thoughts?
(In reply to David Dahl :ddahl from comment #222)
> Thoughts?

Ship it and go shopping.
(In reply to Ms2ger from comment #223)
> (In reply to David Dahl :ddahl from comment #222)
> > Thoughts?
> 
> Ship it and go shopping.

I am looking into bsmith's patch in bug 673432 right now, but not having much luck getting it to link. The largish set of changes - even though for the most part things are just being moved around make me think the review will take some time.
Comment on attachment 599411 [details] [diff] [review]
Patch 26

Looking for a DOM peer to review the DOM parts of this patch. I imagine we will also need an sr?
Attachment #599411 - Flags: review?(bugs)
(In reply to David Dahl :ddahl from comment #224)
> (In reply to Ms2ger from comment #223)
> > (In reply to David Dahl :ddahl from comment #222)
> > > Thoughts?
> > 
> > Ship it and go shopping.
> 
I am also thinking this is prudent.

> I am looking into bsmith's patch in bug 673432 right now, but not having
> much luck getting it to link. The largish set of changes - even though for
> the most part things are just being moved around make me think the review
> will take some time.

It looks like bsmith's patch is a bit of scope creep (good scope creep, FWIW), and the additions I will have to work out will take some time plus reviews. (I am blocked right now in bug 673432), I think we should land this for desktop and be done with this bug.

Who wants to do a final pass on this bug for the DOM/XPCOM parts? Who should I ask for sr?
Attachment #599411 - Flags: superreview?(jst)
Comment on attachment 599411 [details] [diff] [review]
Patch 26

> DOM_MSG_DEF(NS_ERROR_DOM_INVALID_EXPRESSION_ERR, "The expression is not a legal expression.")
> DOM_MSG_DEF(NS_ERROR_DOM_TYPE_ERR, "The expression cannot be converted to return the specified type.")
> 
> /* HTML5 error codes http://dev.w3.org/html5/spec/Overview.html */
> 
>-DOM_MSG_DEF(NS_ERROR_DOM_DATA_CLONE_ERR, "The object could not be cloned.")
>-
Just add the new error here, so that you don't need to move this one.
(I don't understand the move anyway.)

>+  SECStatus srv = PK11_GenerateRandom(data, dataLen);
I don't know about this method. Ask someone else whether using it is the right thing to do.
Attachment #599411 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #227)
> > /* HTML5 error codes http://dev.w3.org/html5/spec/Overview.html */
> > 
> >-DOM_MSG_DEF(NS_ERROR_DOM_DATA_CLONE_ERR, "The object could not be cloned.")
> >-
> Just add the new error here, so that you don't need to move this one.
> (I don't understand the move anyway.)
This part of the patch is out of date anyway because of bug 720208. Required errors are already added.
Attached patch Patch 27 (obsolete) — Splinter Review
Rebased on latest m-c, fixed domerr bits
Attachment #599411 - Attachment is obsolete: true
Attachment #601098 - Flags: superreview?(jst)
Attachment #599411 - Flags: superreview?(jst)
(In reply to Olli Pettay [:smaug] from comment #227)
> >+  SECStatus srv = PK11_GenerateRandom(data, dataLen);
> I don't know about this method. Ask someone else whether using it is the
> right thing to do.

I am confident we have had enough eyes on the crypto parts of this patch, but I will ask for another review from bsmith
Comment on attachment 601098 [details] [diff] [review]
Patch 27

I don't think I have seen an r+ from bsmith on the crypto parts of this patch yet. I think the consensus is to land this and then follow up with the new mobile crypto object in bug 673432
Attachment #601098 - Flags: review?(bsmith)
Comment on attachment 601098 [details] [diff] [review]
Patch 27

sr=jst on the DOM API addition.
Attachment #601098 - Flags: superreview?(jst) → superreview+
Whiteboard: [secr:bsmith] → [sec-assigned:bsmith]
Comment on attachment 601098 [details] [diff] [review]
Patch 27

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

There is a new patch coming very, very soon.
Attachment #601098 - Flags: review?(bsmith)
*ping*
bsmith:

In applying this latest patch, I get: 

$ hg qpush
applying latest-window-crypto-getRandomValues
unable to find 'dom/base/nsDOMCrypto.cpp' for patching

There do not appear to be any non-obsolete patches to apply that create dom/base/nsDOMCrypto.cpp

Which other patch does this depend on?
(In reply to Brian Smith (:bsmith) from comment #237)
> Created attachment 633306 [details] [diff] [review]
> New patch which builds and seems to work -- just needs cleanup [v2]

This all just works now. Thanks! Going to clean up the error checking and push to try later today
Tryserver build errors on Android:

/usr/bin/ccache /tools/android-ndk-r5c/toolchains/arm-linux-androideabi-4.4.3/prebuilt/linux-x86/bin/arm-linux-androideabi-g++ -o SSLServerCertVerification.o -c  -fvisibility=hidden -DNSS_ENABLE_ECC -DDLL_PREFIX=\"lib\" -DDLL_SUFFIX=\".so\"  -DXPCOM_TRANSLATE_NSGM_ENTRY_POINT=1 -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -I/builds/slave/try-andrd/build/obj-firefox/dist/include/nss -I/builds/slave/try-andrd/build/security/manager/ssl/src -I. -I../../../../dist/include  -I/builds/slave/try-andrd/build/obj-firefox/dist/include/nspr -I/builds/slave/try-andrd/build/obj-firefox/dist/include/nss      -fPIC -isystem /tools/android-ndk-r5c/platforms/android-5/arch-arm/usr/include  -pedantic -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wno-long-long -mandroid -fno-short-enums -fno-exceptions -Wno-psabi -march=armv7-a -mthumb -mfpu=vfp -mfloat-abi=softfp -I/builds/slave/try-andrd/build/obj-firefox/build/stlport -I/tools/android-ndk-r5c/sources/cxx-stl/stlport/stlport -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -std=gnu++0x -pipe  -DNDEBUG -DTRIMMED -g -Os -freorder-blocks -fno-reorder-functions -fomit-frame-pointer  -isystem /tools/android-ndk-r5c/platforms/android-5/arch-arm/usr/include  -DMOZILLA_CLIENT -include ../../../../mozilla-config.h -MD -MF .deps/SSLServerCertVerification.pp /builds/slave/try-andrd/build/security/manager/ssl/src/SSLServerCertVerification.cpp
/builds/slave/try-andrd/build/security/manager/ssl/src/nsNSSComponent.cpp:180: error: expected class-name before '{' token
/builds/slave/try-andrd/build/security/manager/ssl/src/nsNSSComponent.cpp: In member function 'nsresult nsNSSComponent::PostCRLImportEvent(const nsCSubstring&, nsIStreamListener*)':
/builds/slave/try-andrd/build/security/manager/ssl/src/nsNSSComponent.cpp:1217: error: 'nsIRunnable' was not declared in this scope
/builds/slave/try-andrd/build/security/manager/ssl/src/nsNSSComponent.cpp:1217: error: template argument 1 is invalid
/builds/slave/try-andrd/build/security/manager/ssl/src/nsNSSComponent.cpp:1217: error: invalid type in declaration before '=' token
/builds/slave/try-andrd/build/security/manager/ssl/src/nsNSSComponent.cpp:1217: error: invalid conversion from 'CRLDownloadEvent*' to 'int'
/builds/slave/try-andrd/build/security/manager/ssl/src/nsNSSComponent.cpp:1222: error: 'NS_DispatchToMainThread' was not declared in this scope
/builds/slave/try-andrd/build/security/manager/ssl/src/nsNSSComponent.cpp: In static member function 'static nsresult nsNSSComponent::GetNewPrompter(nsIPrompt**)':
/builds/slave/try-andrd/build/security/manager/ssl/src/nsNSSComponent.cpp:2416: error: 'NS_IsMainThread' was not declared in this scope
/builds/slave/try-andrd/build/security/manager/ssl/src/nsNSSComponent.cpp: In member function 'virtual nsresult PipUIContext::GetInterface(const nsIID&, void**)':
/builds/slave/try-andrd/build/security/manager/ssl/src/nsNSSComponent.cpp:3092: error: 'NS_IsMainThread' was not declared in this scope
/builds/slave/try-andrd/build/security/manager/ssl/src/nsNSSComponent.cpp: In function 'nsresult getNSSDialogs(void**, const nsIID&, const char*)':
/builds/slave/try-andrd/build/security/manager/ssl/src/nsNSSComponent.cpp:3109: error: 'NS_IsMainThread' was not declared in this scope
make[7]: *** [nsNSSComponent.o] Error 1
make[7]: *** Waiting for unfinished jobs....
(In reply to David Dahl :ddahl from comment #239)
> Tryserver build errors on Android:
> 

Please ignore these errors, just a confvars.sh setting. Updated the setting, re-pushed to try server, build completed, all mochitests pass: 

https://tbpl.mozilla.org/?tree=Try&rev=8b8916541cba
Attachment #601098 - Attachment is obsolete: true
Attachment #633306 - Attachment is obsolete: true
Attachment #634157 - Flags: review?(bsmith)
Comment on attachment 634157 [details] [diff] [review]
Patch, now builds and tests pass on android

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

Looks pretty good. Please have a DOM peer review the changes under dom/ (including the tests). I will re-review the changes to security/manager and we'll have Kai sr them.

::: dom/interfaces/base/nsIDOMCrypto.idl
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsISupports.idl"
>   
>  [scriptable, uuid(eadb45d6-aec2-4b70-95f4-ffdf1f86738f)]

You must generate a new UUID whenever you change the interface.

::: security/manager/ssl/src/nsRandomGenerator.cpp
@@ +38,5 @@
>    return NS_OK;
>  }
> +
> +NS_IMETHODIMP
> +nsRandomGenerator::GetRandomValues(PRUint32 aLength, unsigned char* aValues)

Please name this GenerateRandomBytes.

You need to ensure NSS is initialized the first time the function is called, e.g.:
   NS_ENSURE_TRUE(NS_IsMainThread(), NS_ERROR_NOT_SAME_THREAD);

   static bool nssInitialized = false;
   if (!nssInitialized) {
       nsresult rv;
       nsCOMPtr<nsINSSComponent> nssComponent = do_GetService(nssComponentCID, &rv);
       NS_ENSURE_SUCCESS(rv, rv);
       nssInitialized = true;
   }

@@ +47,5 @@
> +
> +  SECStatus srv = PK11_GenerateRandom(aValues, aLength);
> +
> +  if (srv != SECSuccess) {
> +    return PR_GetError();

PR_GetError returns a PRErrorCode, not an nsresult. You can use NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_SECURITY,
                              -1 * PR_GetError());

to map an NSS error to an nsresult.

::: security/manager/ssl/src/nsRandomGenerator.h
@@ +18,5 @@
>  public:
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIRANDOMGENERATOR
> +
> +  NS_IMETHOD GetRandomValues(PRUint32 aLength, unsigned char* aValues);

static nsresult, not NS_IMETHOD.
Attachment #634157 - Flags: review?(bsmith)
Although this has some PSM changes, it is mostly a DOM bug now, so I'm changing the component.

One question for DOM peers: Would it be better to have the tests be mochitests instead of xpcshell tests, since this is an API exposed to content? Is there a convenient way to run xpcshell tests like this in a web context?
Assignee: ddahl → nobody
Component: Security: PSM → DOM
QA Contact: psm → general
Target Milestone: mozilla12 → mozilla16
Version: Other Branch → Trunk
Attached patch Updated Patch (obsolete) — Splinter Review
Updated via your comments. Thanks. All tests pass.
Attachment #634157 - Attachment is obsolete: true
Attachment #635095 - Flags: review?(bsmith)
Things build on Mac, Android and Linux. Windows complains of:

e:/builds/moz2_slave/try-w32/build/security/manager/ssl/src/nsRandomGenerator.cpp(50) : error C2373: 'nsRandomGenerator::GenerateRandomBytes' : redefinition; different type modifiers

        e:\builds\moz2_slave\try-w32\build\security\manager\ssl\src\nsRandomGenerator.h(22) : see declaration of 'nsRandomGenerator::GenerateRandomBytes'
The previous patch would not build on Windows due to the method signature  when overriding the method, using NS_IMETHODIMP return value.
Attachment #635095 - Attachment is obsolete: true
Attachment #635095 - Flags: review?(bsmith)
Attachment #635510 - Flags: review?(bsmith)
Ping. Brian: I think most of the other related bugs are reviewed - just waiting on this review.  Would love to land this ASAP.
Assignee: nobody → ddahl
Comment on attachment 635510 [details] [diff] [review]
Latest Patch, updated to build on Windows

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

Kai, please super-review the changes to PSM (Makefile.in, nsRandomGenerator.h, nsRandomGenerator.cpp).

::: dom/base/Crypto.cpp
@@ +34,5 @@
>  
> +NS_IMETHODIMP
> +Crypto::GetRandomValues(const jsval& aData, JSContext *cx,
> +                        jsval* _retval NS_OUTPARAM)
> +{

I would feel better if we had a

   *retval = nsnull;

here.

@@ +64,5 @@
> +  }
> +
> +  PRUint32 dataLen = JS_GetTypedArrayByteLength(view, cx);
> +  // Abort if the ArrayBufferView is requesting too few values
> +  if (dataLen == 0) {

Here, you need to set _retval before returning.

Also, you need to change the tests to test for this error, by checking that the return value is a reference to the view passed as the parameter.

It might be clearer if you wrote it as:

if (dataLen > 0) {
   unsigned char* data = ...;
   nsresult rv = nsRandomGenerator::
       GenerateRandomBytes(dataLen, data);   
   NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
}

*_retval = OBJECT_TO_JSVAL(view); // or whatever; see comment below.
return NS_OK;

@@ +70,5 @@
> +  }
> +
> +  unsigned char* data = static_cast<unsigned char*>(JS_GetArrayBufferViewData(view, cx));
> +  
> +  nsRefPtr<nsRandomGenerator> randomGen = new nsRandomGenerator();

We made GenerateRandomBytes a static member so that you wouldn't have to create an instance. Just call it as nsRandomGenerator::GenerateRandomBytes(dataLen, data);

@@ +76,5 @@
> +  nsresult rv = randomGen->GenerateRandomBytes(dataLen, data);
> +  
> +  NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
> +  
> +  *_retval = OBJECT_TO_JSVAL(view);

AFAICT, the function is supposed to return a reference to the input parameter, but this isn't obvious (to me) the way it is written now.

Why not _retval = NS_ADDREF(aData) or something similar?

Have a JS an/dor DOM person review the JSAPI usage, especially regarding reference counting and conversions.

::: dom/tests/mochitest/crypto/test_getRandomValues.html
@@ +43,5 @@
> +  var buf = new ArrayBuffer(aLength);
> +  var arBuf = new arrayTypes[aType](buf);
> +
> +  var pass = false;
> +  window.crypto.getRandomValues(arBuf);

check the return value === arBuf.

@@ +67,5 @@
> +      else {
> +        // failing tests are dealt with here
> +        if (i == 8) {
> +          try {
> +            testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);

There should be a call to fail() after this call to catch the case where the function does not throw an exception when it is supposed to.

@@ +79,5 @@
> +        } // 8
> +
> +        if (i == 9) {
> +          try {
> +            testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);

ditto.

@@ +90,5 @@
> +        } // 9
> +
> +        if (i == 10) {
> +          try {
> +            testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);

ditto.

@@ +110,5 @@
> +  ok(testCount == 11, "11 tests run via testData");
> +
> +  // Test a null argument
> +  try {
> +    window.crypto.getRandomValues(null);

ditto.

@@ +120,5 @@
> +
> +  // Test a zero-length buffer view
> +  try {
> +    var a = new Int8Array(0);
> +    window.crypto.getRandomValues(a);

test that the return value === a.

@@ +130,5 @@
> +
> +  // Test a one-length buffer view
> +  try {
> +    var a = new Int8Array(1);
> +    window.crypto.getRandomValues(a);

ditto.

@@ +153,5 @@
> +  // test an offset view
> +  window.crypto.getRandomValues(view2);
> +  ok(view2[0] != 1, "view2 has been updated ");
> +
> +  // test the return value

Better to do this check in all test cases, because the code paths for different cases (zero vs. non-zero length, for example) are different.
Attachment #635510 - Flags: superreview?(kaie)
Attachment #635510 - Flags: review?(bsmith)
Attachment #635510 - Flags: review+
Comment on attachment 635510 [details] [diff] [review]
Latest Patch, updated to build on Windows

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

JSAPI usage is good.

::: dom/base/Crypto.cpp
@@ +34,5 @@
>  
> +NS_IMETHODIMP
> +Crypto::GetRandomValues(const jsval& aData, JSContext *cx,
> +                        jsval* _retval NS_OUTPARAM)
> +{

Just add aRetval = aData; here, then you can keep the early returns.

::: dom/tests/mochitest/crypto/Makefile.in
@@ +21,5 @@
> +_TEST_FILES += test_no_legacy.html
> +endif
> +
> +libs::	$(_TEST_FILES)
> +	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/tests/$(relativesrcdir)

This should use MOCHITEST_FILES now.
(In reply to Brian Smith (:bsmith) from comment #250)
> Comment on attachment 635510 [details] [diff] [review]
> Latest Patch, updated to build on Windows
> 
> Review of attachment 635510 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Kai, please super-review the changes to PSM (Makefile.in,
> nsRandomGenerator.h, nsRandomGenerator.cpp).
> 
> ::: dom/base/Crypto.cpp
> @@ +34,5 @@
> >  
> > +NS_IMETHODIMP
> > +Crypto::GetRandomValues(const jsval& aData, JSContext *cx,
> > +                        jsval* _retval NS_OUTPARAM)
> > +{
> 
> I would feel better if we had a
> 
>    *retval = nsnull;
> 
> here.

No, don't set jsvals to nsnull.

> @@ +76,5 @@
> > +  nsresult rv = randomGen->GenerateRandomBytes(dataLen, data);
> > +  
> > +  NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
> > +  
> > +  *_retval = OBJECT_TO_JSVAL(view);
> 
> AFAICT, the function is supposed to return a reference to the input
> parameter, but this isn't obvious (to me) the way it is written now.
> 
> Why not _retval = NS_ADDREF(aData) or something similar?

If you addref a jsval, you're gonna have a bad time...

> Have a JS an/dor DOM person review the JSAPI usage, especially regarding
> reference counting and conversions.

Done.
(In reply to Brian Smith (:bsmith) from comment #250)
> Comment on attachment 635510 [details] [diff] [review]

> @@ +67,5 @@
> > +      else {
> > +        // failing tests are dealt with here
> > +        if (i == 8) {
> > +          try {
> > +            testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);
> 
> There should be a call to fail() after this call to catch the case where the
> function does not throw an exception when it is supposed to.

A manual test shows that this is correctly handled in the test. I just set failedWithCorrectError to false and see the following errors during test run:

mochitest-plain failed:
21 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/crypto/test_getRandomValues.html | Expected TYPE_MISMATCH_ERR: Float64Array is not valid, got [Exception... "The type of an object is incompatible with the expected type of the parameter associated to the object"  code: "17" nsresult: "0x80530011 (TypeMismatchError)"  location: "http://mochi.test:8888/tests/dom/tests/mochitest/crypto/test_getRandomValues.html Line: 47"].
To rerun your failures please run 'make mochitest-plain-rerun-failures' exit 1
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=0a502f2c9802
Attachment #635510 - Attachment is obsolete: true
Attachment #635510 - Flags: superreview?(kaie)
Attachment #640797 - Flags: review+
removed forced test failure. Going to re-push to try. whoops.
Attachment #640797 - Attachment is obsolete: true
Try run for 0a502f2c9802 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0a502f2c9802
Results (out of 13 total builds):
    exception: 8
    failure: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ddahl@mozilla.com-0a502f2c9802
(In reply to David Dahl :ddahl from comment #256)
> Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=d59d5b1b8906

seeing: 12308 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_interfaces.html | Unexpected interface name in global scope: CryptoLegacy
(In reply to David Dahl :ddahl from comment #258)
> (In reply to David Dahl :ddahl from comment #256)
> > Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=d59d5b1b8906
> 
> seeing: 12308 ERROR TEST-UNEXPECTED-FAIL |
> /tests/dom/tests/mochitest/general/test_interfaces.html | Unexpected
> interface name in global scope: CryptoLegacy

Thanks. This is a problem with the patch in bug 683262 that was pointed out by Olli in bug 683262 comment 8. In that bug, we need to rename nsIDOMCryptoLegacy to nsICryptoLegacy.
Flags: sec-review?(bsmith)
Comment on attachment 640801 [details] [diff] [review]
Unbitrotted, forgot to remove forced test failure

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

::: security/manager/ssl/src/nsRandomGenerator.cpp
@@ +61,5 @@
> +  if (aLength == 0) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  SECStatus srv = PK11_GenerateRandom(aValues, aLength);

Ryan Sleevi told me that Mozilla's patch for
window.crypto.getRandomValues is likely to block
on an internal lock of NSS if there is a (possibly
unrelated) slow PKCS #11 module. I suspect he
is referring to this PK11_GenerateRandom call.

That blocking issue can be avoided by using NSS's
own PRNG directly. Replace this PK11_GenerateRandom
call with:

  PK11SlotInfo *slot = PK11_GetInternalSlot();
  SECStatus srv = PK11_GenerateRandomOnSlot(slot, aValues, aLength);
  PK11_FreeSlot(slot);
Comment on attachment 640801 [details] [diff] [review]
Unbitrotted, forgot to remove forced test failure

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

::: security/manager/ssl/src/nsRandomGenerator.cpp
@@ +61,5 @@
> +  if (aLength == 0) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  SECStatus srv = PK11_GenerateRandom(aValues, aLength);

Yes, if there are multiple slots, there are several locks involved that may be triggered

PK11_GetBestSlotMultipleWithAttribues (called by PK11_GetBestSlot) will use the locks on PK11_GetSlotList iterating the slot (see PK11_GetFirstSafe/PK11_GetNextSafe)

PK11_IsPresent may block for some time (it will attempt a slot lock, and also attempt a session lock)

PK11_DoesMechanism may also invoke the slot locks


Wan-Teh's proposed change will avoid these locks, but at the cost that it will not use the RNG from any hardware devices. 

As far as the WHATWG proposal goes, this is not a required behaviour, and thus offers the best performance for users. As implemented in Chromium/Google Chrome, we do not consider any external devices for RNG.
(In reply to Ryan Sleevi from comment #261)
> Comment on attachment 640801 [details] [diff] [review]
> Unbitrotted, forgot to remove forced test failure
> 
> Review of attachment 640801 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: security/manager/ssl/src/nsRandomGenerator.cpp
> @@ +61,5 @@
> > +  if (aLength == 0) {
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +
> > +  SECStatus srv = PK11_GenerateRandom(aValues, aLength);
> 
> Yes, if there are multiple slots, there are several locks involved that may
> be triggered
> 
> PK11_GetBestSlotMultipleWithAttribues (called by PK11_GetBestSlot) will use
> the locks on PK11_GetSlotList iterating the slot (see
> PK11_GetFirstSafe/PK11_GetNextSafe)
> 
> PK11_IsPresent may block for some time (it will attempt a slot lock, and
> also attempt a session lock)
> 
> PK11_DoesMechanism may also invoke the slot locks
> 
> 
> Wan-Teh's proposed change will avoid these locks, but at the cost that it
> will not use the RNG from any hardware devices. 
> 
> As far as the WHATWG proposal goes, this is not a required behaviour, and
> thus offers the best performance for users. As implemented in
> Chromium/Google Chrome, we do not consider any external devices for RNG.

Thanks! I will update this patch as soon as I have a time slot free.
This patch is updated to reflect Wan-Teh's above comments. Asking for a review as the code has changed.

Passes all tests with the soon to be uploaded patch in bug 683262
Attachment #640801 - Attachment is obsolete: true
Attachment #683420 - Flags: review?(wtc)
Attached patch Latest patch unbitrot (obsolete) — Splinter Review
Attachment #683420 - Attachment is obsolete: true
Attachment #683420 - Flags: review?(wtc)
Attached patch Latest patch unbitrot (obsolete) — Splinter Review
Attachment #686655 - Attachment is obsolete: true
Comment on attachment 691978 [details] [diff] [review]
Latest patch unbitrot

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

ddahl: I took a look at this patch. I didn't review test_getRandomValues.html.
In general it looks correct. Here are my comments. Thanks.

::: dom/base/Crypto.cpp
@@ +49,5 @@
> +    return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
> +  }
> +
> +  // Throw if the wrong type of ArrayBufferView is passed in
> +  // (Part of the WHATWG spec)

Should we say WebCryptoAPI spec instead of WHATWG spec?

@@ +53,5 @@
> +  // (Part of the WHATWG spec)
> +  switch (JS_GetTypedArrayType(view)) {
> +    case TYPE_INT8:
> +    case TYPE_UINT8:
> +    case TYPE_UINT8_CLAMPED:

The WebCryptoAPI spec does not mention Uint8Clamped, but I remember
seeing this in the WebKit implementation of crypto.GetRandomValues.
Do you know whether Uint8Clamped should be allowed?

@@ +63,5 @@
> +    default:
> +      return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
> +  }
> +
> +  PRUint32 dataLen = JS_GetTypedArrayByteLength(view);

Use uint32_t instead of PRUint32.

@@ +65,5 @@
> +  }
> +
> +  PRUint32 dataLen = JS_GetTypedArrayByteLength(view);
> +  // Abort if the ArrayBufferView is requesting too few values
> +  if (dataLen > 0) {

In addition, you should also check if the ArrayBufferView is requesting too many values.
This requirement comes from the spec:

  2. If the byteLength of array is greater than 65536, throw a QuotaExceededError and
     terminate the algorithm.

@@ +71,5 @@
> +      static_cast<unsigned char*>(JS_GetArrayBufferViewData(view));
> +    nsresult rv = nsRandomGenerator::GenerateRandomBytes(dataLen, data); 
> +    NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
> +  }
> +  

Nit: you may want to get rid of these spaces at the end of lines.

::: mobile/android/confvars.sh
@@ +13,5 @@
>  # MOZ_APP_DISPLAYNAME is set by branding/configure.sh
>  
>  MOZ_SAFE_BROWSING=1
>  
> +# MOZ_DISABLE_DOMCRYPTO=0

Why is this configuration variable commented out?

::: security/manager/ssl/src/Makefile.in
@@ +99,5 @@
>  EXPORTS += \
>    CryptoTask.h \
>    nsNSSShutDown.h \
>    ScopedNSSTypes.h \
> +  nsRandomGenerator.h \

Nit: should we list this file in alphabetical order?

::: security/manager/ssl/src/nsRandomGenerator.cpp
@@ +48,5 @@
>    return NS_OK;
>  }
> +
> +nsresult
> +nsRandomGenerator::GenerateRandomBytes(PRUint32 aLength, unsigned char* aValues)

The function above declares aBuffer as a uint8_ **. Perhaps we should also use
uint8_ instead of unsigned char here?

@@ +61,5 @@
> +    nssInitialized = true;
> +  }
> +
> +  if (aLength == 0) {
> +    return NS_ERROR_FAILURE;

If aLength is 0, we can also do nothing and just return NS_OK. It is
up to you how you want to handle this corner case.

@@ +64,5 @@
> +  if (aLength == 0) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  SECStatus srv = PK11_GenerateRandom(aValues, aLength);

Please copy lines 37-39 from the function above. This function should also use
PK11_GenerateRandomOnSlot().

::: security/manager/ssl/src/nsRandomGenerator.h
@@ +19,5 @@
>  public:
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIRANDOMGENERATOR
> +
> +  static nsresult GenerateRandomBytes(PRUint32 aLength, unsigned char* aValues);

It is better for aLength be the second argument.

If you can use size_t in this file, I suggest declaring aLength as a size_t.
Comment on attachment 691978 [details] [diff] [review]
Latest patch unbitrot

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

::: security/manager/ssl/src/nsRandomGenerator.h
@@ +19,5 @@
>  public:
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIRANDOMGENERATOR
> +
> +  static nsresult GenerateRandomBytes(PRUint32 aLength, unsigned char* aValues);

I just noticed that nsRandomGenerator already has a method named GenerateRandomBytes.
It is confusing to add a method with the same name. Can this be avoided?

I suggest using PRUint32 to uint32_t.
(In reply to Wan-Teh Chang from comment #267)
> Comment on attachment 691978 [details] [diff] [review]
> Latest patch unbitrot
> 
> Review of attachment 691978 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: security/manager/ssl/src/nsRandomGenerator.h
> @@ +19,5 @@
> >  public:
> >    NS_DECL_ISUPPORTS
> >    NS_DECL_NSIRANDOMGENERATOR
> > +
> > +  static nsresult GenerateRandomBytes(PRUint32 aLength, unsigned char* aValues);
> 
> I just noticed that nsRandomGenerator already has a method named
> GenerateRandomBytes.
> It is confusing to add a method with the same name. Can this be avoided?

Ok, one question, in the current patch, the second GenerateRandomBytes method does some checking to make sure NSS is initialized, should this part be incorporated into the existing method?
Looks like I have this working using only the existing nsIRandomGenerator object, without any duplication
Attachment #691978 - Attachment is obsolete: true
Attachment #694459 - Flags: review?(wtc)
Comment on attachment 691978 [details] [diff] [review]
Latest patch unbitrot

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

::: security/manager/ssl/src/nsRandomGenerator.cpp
@@ +58,5 @@
> +    nsresult rv;
> +    nsCOMPtr<nsINSSComponent> nssComponent(do_GetService(kNSSComponentCID, &rv));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    nssInitialized = true;
> +  }

I don't know whether the code to initialize NSS should be
added to the existing GenerateRandomBytes method. I suspect
right now the caller is required to ensure NSS is initialized
before calling GenerateRandomBytes, so it seems better to
continue to require that.
Comment on attachment 694459 [details] [diff] [review]
Latest Patch, consolidated GenerateRandomBytes

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

Hi David: I found a serious bug (marked with "BUG"), and suggested
some other minor changes. I didn't review test_getRandomValues.html.

::: dom/base/Crypto.cpp
@@ +35,5 @@
>  
> +NS_IMETHODIMP
> +Crypto::GetRandomValues(const jsval& aData, JSContext *cx, jsval* _retval)
> +{
> +  *_retval = aData;

This line probably should be removed because it seems redundant
with line 89 below:

89	  *_retval = OBJECT_TO_JSVAL(view);
90	 
91	  return NS_OK;

@@ +49,5 @@
> +    return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
> +  }
> +
> +  // Throw if the wrong type of ArrayBufferView is passed in
> +  // (Part of the WHATWG spec)

This comment should be updated to say WebCryptoAPI spec.

@@ +53,5 @@
> +  // (Part of the WHATWG spec)
> +  switch (JS_GetTypedArrayType(view)) {
> +    case TYPE_INT8:
> +    case TYPE_UINT8:
> +    case TYPE_UINT8_CLAMPED:

The WebCryptoAPI spec does not list Uint8ClampedArray.
Should we add it to the spec?

@@ +64,5 @@
> +      return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
> +  }
> +  
> +  uint32_t dataLen = JS_GetTypedArrayByteLength(view);
> +  // Abort if the ArrayBufferView is requesting too few values

This comment implies we abort if dataLen is 0, but I don't see
where we do that.

@@ +65,5 @@
> +  }
> +  
> +  uint32_t dataLen = JS_GetTypedArrayByteLength(view);
> +  // Abort if the ArrayBufferView is requesting too few values
> +  if (dataLen > 0) {

You are still not checking if dataLen is > 65536. The spec
requires that we throw a QuotaExceededError in that case.

@@ +82,5 @@
> +
> +    rv = randomGenerator->GenerateRandomBytes(dataLen, &buf); 
> +    NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
> +
> +    data = static_cast<unsigned char*>(buf);

BUG: you should do
    memcpy(data, buf, dataLen);
    NS_Free(buf);

::: security/manager/ssl/src/nsRandomGenerator.cpp
@@ +11,5 @@
> +#include "nsThreadUtils.h"
> +#include "nsCOMPtr.h"
> +#include "nsIServiceManager.h"
> +
> +static NS_DEFINE_CID(kNSSComponentCID, NS_NSSCOMPONENT_CID);

I think you can revert this part of the patch.
Attachment #694459 - Flags: review?(wtc) → review-
(In reply to Wan-Teh Chang from comment #271)
> Comment on attachment 694459 [details] [diff] [review]
> Latest Patch, consolidated GenerateRandomBytes
> 
> ::: dom/base/Crypto.cpp
> @@ +35,5 @@
> >  
> > +NS_IMETHODIMP
> > +Crypto::GetRandomValues(const jsval& aData, JSContext *cx, jsval* _retval)
> > +{
> > +  *_retval = aData;
> 
> This line probably should be removed because it seems redundant
> with line 89 below:
> 
> 89	  *_retval = OBJECT_TO_JSVAL(view);
> 90	 
> 91	  return NS_OK;

Ok, removed.

> @@ +49,5 @@
> > +    return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
> > +  }
> > +
> > +  // Throw if the wrong type of ArrayBufferView is passed in
> > +  // (Part of the WHATWG spec)
> 
> This comment should be updated to say WebCryptoAPI spec.

Ok, updated.

> 
> @@ +53,5 @@
> > +  // (Part of the WHATWG spec)
> > +  switch (JS_GetTypedArrayType(view)) {
> > +    case TYPE_INT8:
> > +    case TYPE_UINT8:
> > +    case TYPE_UINT8_CLAMPED:
> 
> The WebCryptoAPI spec does not list Uint8ClampedArray.
> Should we add it to the spec?
> 
Perhaps we do need to add it to the spec. I remember going back and adding this to the patch after review by ms2ger - it was in the whatwg spec and in the webkit impl.

> @@ +64,5 @@
> > +      return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
> > +  }
> > +  
> > +  uint32_t dataLen = JS_GetTypedArrayByteLength(view);
> > +  // Abort if the ArrayBufferView is requesting too few values
> 
> This comment implies we abort if dataLen is 0, but I don't see
> where we do that.

Patch updated to check for this and abort
> 
> @@ +65,5 @@
> > +  }
> > +  
> > +  uint32_t dataLen = JS_GetTypedArrayByteLength(view);
> > +  // Abort if the ArrayBufferView is requesting too few values
> > +  if (dataLen > 0) {
> 
> You are still not checking if dataLen is > 65536. The spec
> requires that we throw a QuotaExceededError in that case.
> 
Updated patch. I think this patch was truncated at one point after moving things to nsRandomGenerator - and that is whey some of these things are missing.

> @@ +82,5 @@
> > +
> > +    rv = randomGenerator->GenerateRandomBytes(dataLen, &buf); 
> > +    NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
> > +
> > +    data = static_cast<unsigned char*>(buf);
> 
> BUG: you should do
>     memcpy(data, buf, dataLen);
>     NS_Free(buf);
> 
Ah! thanks. Whoops.

> ::: security/manager/ssl/src/nsRandomGenerator.cpp
> @@ +11,5 @@
> > +#include "nsThreadUtils.h"
> > +#include "nsCOMPtr.h"
> > +#include "nsIServiceManager.h"
> > +
> > +static NS_DEFINE_CID(kNSSComponentCID, NS_NSSCOMPONENT_CID);
> 
> I think you can revert this part of the patch.
Ok, done.
Attached patch Latest Patch, comments addressed (obsolete) — Splinter Review
Attachment #694459 - Attachment is obsolete: true
Attachment #697686 - Flags: review?(wtc)
Comment on attachment 697686 [details] [diff] [review]
Latest Patch, comments addressed

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

::: dom/base/Crypto.cpp
@@ +52,5 @@
> +  // (Part of the Web Crypto API spec)
> +  switch (JS_GetTypedArrayType(view)) {
> +    case TYPE_INT8:
> +    case TYPE_UINT8:
> +    case TYPE_UINT8_CLAMPED:

I don't think is valid as is. Uint8ClampedArray values should be in the range [0, 255], this could end up creating values outside this range. Either we just don't care or don't allow the usage of clamped arrays here.

@@ +61,5 @@
> +      break;
> +    default:
> +      return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
> +  }
> +  

Whitespace.

@@ +65,5 @@
> +  
> +  uint32_t dataLen = JS_GetTypedArrayByteLength(view);
> +  // Abort if the ArrayBufferView is requesting too few values
> +  if (dataLen > 0) {
> +    unsigned char* data = 

Here, too.
(In reply to Tom Schuster [:evilpie] from comment #274)
> Comment on attachment 697686 [details] [diff] [review]
> Latest Patch, comments addressed
> 
> Review of attachment 697686 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/Crypto.cpp
> @@ +52,5 @@
> > +  // (Part of the Web Crypto API spec)
> > +  switch (JS_GetTypedArrayType(view)) {
> > +    case TYPE_INT8:
> > +    case TYPE_UINT8:
> > +    case TYPE_UINT8_CLAMPED:
> 
> I don't think is valid as is. Uint8ClampedArray values should be in the
> range [0, 255], this could end up creating values outside this range. Either
> we just don't care or don't allow the usage of clamped arrays here.

I just looked up the old spec:  "array is an ArrayBufferView. If array is not of an integer type (i.e., Int8Array, Uint8Array, Int16Array, Uint16Array, Int32Array, or Uint32Array), throw a TypeMismatchError and abort these steps."

Looks like Uint8ClampedArray is not supposed to be here, or ?? - see comment 153
Blocks: 827829
(In reply to David Dahl :ddahl from comment #275)
> Looks like Uint8ClampedArray is not supposed to be here, or ?? - see comment
> 153

After a little experimentation, a uint8clampedarray works (there has always been a test for this too), and you always get back positive integers below 256 - if you try to manually set an index to something like -512 - it is set to 0.

Not sure if we should remove the clamped array support or not.
Changed JS_GetTypedArrayType(view) to JS_GetArrayBufferViewType(view) after ion monkey merge changes
Attachment #697686 - Attachment is obsolete: true
Attachment #697686 - Flags: review?(wtc)
Attachment #699481 - Flags: review?(wtc)
Attachment #699481 - Attachment is obsolete: true
Attachment #699481 - Flags: review?(wtc)
Attachment #702565 - Flags: review?(bsmith)
Comment on attachment 702565 [details] [diff] [review]
Latest patch - folded in blassy's patch from bug 683262

Camilo, can you please review this patch? I will look it over when you are done.
Attachment #702565 - Flags: review?(cviecco)
Comment on attachment 702565 [details] [diff] [review]
Latest patch - folded in blassy's patch from bug 683262

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

::: dom/base/Crypto.cpp
@@ +61,5 @@
> +      break;
> +    default:
> +      return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
> +  }
> +  

Whitespace. There is more. You should adjust your editor to highlight it. When you enable the color extension you can also see it in diffs.

@@ +87,5 @@
> +
> +  } else if (dataLen == 0) {
> +    NS_WARNING("ArrayBufferView length is 0, cannot continue");
> +    return NS_OK;
> +  } else if (dataLen > 65536) { 

I don't think this works. In general I find this if { return } else if { } confusing.
Comment on attachment 702565 [details] [diff] [review]
Latest patch - folded in blassy's patch from bug 683262

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

::: dom/base/Crypto.cpp
@@ +87,5 @@
> +
> +  } else if (dataLen == 0) {
> +    NS_WARNING("ArrayBufferView length is 0, cannot continue");
> +    return NS_OK;
> +  } else if (dataLen > 65536) { 

One thing for style:

I would put the error handling after you get the len:
if (datalen ==0){
   warn
   return
}
if (datalen> 2^16) {
  warn
  return   
}
//and now continue with processing (datalen>0 && < 2^16)

::: dom/tests/mochitest/crypto/test_getRandomValues.html
@@ +18,5 @@
> +                 { len: 65536, type: "Uint8", pass: true },
> +                 { len: 32, type: "Uint8Clamped", pass: true },
> +                 { len: 65537, type: "Uint8", pass: false },
> +                 { len: 32, type: "Float32", pass: false },
> +                 { len: 32, type: "Float64", pass: false } ];

I would add a 1,16,31 and 33 byte tests.

::: security/manager/ssl/src/nsRandomGenerator.cpp
@@ +27,5 @@
>    uint8_t *buf = reinterpret_cast<uint8_t *>(NS_Alloc(aLength));
>    if (!buf)
>      return NS_ERROR_OUT_OF_MEMORY;
>  
> +  PK11SlotInfo *slot = PK11_GetInternalSlot();

It is yood that are going to use PK11_GenerateRandomOnSlot but uou  must verify that you actually got a slot. (slot!=null). 

also scoped types remove the need of cleanup:

ScopedPK11SlotInfo slot(PK11_GetInternalSlot());
Attachment #702565 - Flags: review?(cviecco) → review-
Camilo:

I have fixed up the patch with all of your comments. All tests pass, but we crash at the end:

TEST-PASS | unknown test url | ArrayBuffer result is argument buffer
Assertion failure: module->refCount == 0, at pk11util.c:837

Program received signal SIGABRT, Aborted.
0x00002aaaab93e425 in raise () from /lib/x86_64-linux-gnu/libc.so.6
(gdb) 
(gdb) 
(gdb) bt
#0  0x00002aaaab93e425 in raise () from /lib/x86_64-linux-gnu/libc.so.6
#1  0x00002aaaab941b8b in abort () from /lib/x86_64-linux-gnu/libc.so.6
#2  0x00002aaaabe15c6d in PR_Assert (s=0x2aaaaca4a86e "module->refCount == 0", file=0x2aaaaca4a6d7 "pk11util.c", ln=837)
    at /home/ddahl/code/moz/mc/src/nsprpub/pr/src/io/prlog.c:554
#3  0x00002aaaac944e91 in SECMOD_SlotDestroyModule (module=0x2aaac4af3020, fromSlot=1) at pk11util.c:837
#4  0x00002aaaac93e832 in PK11_DestroySlot (slot=0x2aaac4ad8000) at pk11slot.c:440
#5  0x00002aaaac93dd6f in PK11_FreeSlot (slot=0x2aaac4ad8000) at pk11slot.c:453
#6  0x00002aaaafabf495 in mozilla::TypeSpecificDelete<PK11SlotInfoStr> (value=0x2aaac4ad8000)
    at /home/ddahl/code/moz/mc/src/security/manager/ssl/src/ScopedNSSTypes.h:201
#7  0x00002aaaafabf473 in mozilla::TypeSpecificScopedPointerTraits<PK11SlotInfoStr>::release (value=0x2aaac4ad8000) at ../../../../dist/include/mozilla/Scoped.h:263
#8  0x00002aaaafabf42f in mozilla::Scoped<mozilla::TypeSpecificScopedPointerTraits<PK11SlotInfoStr> >::~Scoped (this=0x7fffffff6f78)
    at ../../../../dist/include/mozilla/Scoped.h:90
#9  0x00002aaaafabf405 in mozilla::TypeSpecificScopedPointer<PK11SlotInfoStr>::~TypeSpecificScopedPointer (this=0x7fffffff6f78)
    at ../../../../dist/include/mozilla/Scoped.h:267
#10 0x00002aaaafabcc75 in mozilla::TypeSpecificScopedPointer<PK11SlotInfoStr>::~TypeSpecificScopedPointer (this=0x7fffffff6f78)
    at ../../../../dist/include/mozilla/Scoped.h:267
#11 0x00002aaaafb43de7 in nsRandomGenerator::GenerateRandomBytes (this=0x2aaac3d080e0, aLength=16, aBuffer=0x7fffffff7038)
    at /home/ddahl/code/moz/mc/src/security/manager/ssl/src/nsRandomGenerator.cpp:46
#12 0x00002aaaaf0485d5 in mozilla::dom::Crypto::GetRandomValues (this=0x2aaac41fd040, aData=..., cx=0x2aaad41fb480, _retval=0x7fffffff7330)
    at /home/ddahl/code/moz/mc/src/dom/base/Crypto.cpp:88

Do I need to free the ScopedSlotInfo object?
Review comments addressed, thanks again Camilo.
Attachment #702565 - Attachment is obsolete: true
Attachment #702565 - Flags: review?(bsmith)
Attachment #709907 - Flags: review?(cviecco)
Comment on attachment 709907 [details] [diff] [review]
Latest patch, review comments addressed

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

::: dom/base/Crypto.cpp
@@ +73,5 @@
> +
> +  }  else if (dataLen > 0 && dataLen < 65536) {
> +    unsigned char* data =
> +      static_cast<unsigned char*>(JS_GetArrayBufferViewData(view));
> +    uint8_t *buf;

Nit: declare these closer to where they are used.

Can JS_GetArrayBufferViewData() ever return nullptr? If so, you need to check for null.

@@ +84,5 @@
> +      NS_WARNING("unable to continue without random number generator");
> +      return rv;
> +    }
> +
> +    rv = randomGenerator->GenerateRandomBytes(dataLen, &buf);

Nit: Do you remember, many comments ago, that we created the static GenerateRandomBytes method to avoid the need to allocate and free buf? Why did we undo that? I agree that it is a little confusing to have two overloaded GenerateRandomBytes methods but OTOH we already have to deal with such overloading throughout XPCOM. The other way, with the new static GenerateRandomBytes method was more efficient and thus better, especially as it also had no do_GetService overhead. (As you may recall from hundreds of comments ago, the whole design of this API was centered around minimizing such overhead.)

@@ +89,5 @@
> +    NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
> +
> +    memcpy(data, buf, dataLen);
> +    NS_Free(buf);
> +

Nit: Remove this blank line.

::: dom/tests/mochitest/crypto/test_getRandomValues.html
@@ +1,5 @@
> +<!DOCTYPE HTML>
> +<html><head>
> +  <title>Test window.crypto.getRandomValues</title>
> +  <script type="text/javascript" src="/MochiKit/packed.js"></script>
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>        

nit: trailing whitespace.
(In reply to Brian Smith (:bsmith) from comment #284)
> Comment on attachment 709907 [details] [diff] [review]
>
> Can JS_GetArrayBufferViewData() ever return nullptr? If so, you need to
> check for null.
Not sure if it might return nullptr. I'll check it out.

> 
> @@ +84,5 @@
> > +      NS_WARNING("unable to continue without random number generator");
> > +      return rv;
> > +    }
> > +
> > +    rv = randomGenerator->GenerateRandomBytes(dataLen, &buf);
> 
> Nit: Do you remember, many comments ago, that we created the static
> GenerateRandomBytes method to avoid the need to allocate and free buf? Why
> did we undo that? I agree that it is a little confusing to have two
> overloaded GenerateRandomBytes methods but OTOH we already have to deal with
> such overloading throughout XPCOM. The other way, with the new static
> GenerateRandomBytes method was more efficient and thus better, especially as
> it also had no do_GetService overhead. (As you may recall from hundreds of
> comments ago, the whole design of this API was centered around minimizing
> such overhead.)

Wan-Teh did not like that approach, I think he said it might be confusing and redundant.
Brian:

see comment 267 for wtc's take on the redundant methods
Ran these patches through try server, looks good: https://tbpl.mozilla.org/?tree=Try&rev=345dbe167292
Comment on attachment 709907 [details] [diff] [review]
Latest patch, review comments addressed

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

::: dom/base/Crypto.cpp
@@ +40,5 @@
> +  if (!aData.isObject()) {
> +    return NS_ERROR_DOM_NOT_OBJECT_ERR;
> +  }
> +
> +  JSObject* view = &aData.toObject();

Can this call ever fail?

@@ +70,5 @@
> +    return NS_OK;
> +  } else if (dataLen > 65536) {
> +    return NS_ERROR_DOM_QUOTA_EXCEEDED_ERR;
> +
> +  }  else if (dataLen > 0 && dataLen < 65536) {

There is no need for the condition check anymore (if we are here those conditions have been meet. We can remove the else if.

@@ +73,5 @@
> +
> +  }  else if (dataLen > 0 && dataLen < 65536) {
> +    unsigned char* data =
> +      static_cast<unsigned char*>(JS_GetArrayBufferViewData(view));
> +    uint8_t *buf;

ditto for brian's comment-> if it can be null then you must check for it.

::: dom/tests/mochitest/crypto/test_getRandomValues.html
@@ +21,5 @@
> +                 { len: 32, type: "Float32", pass: false },
> +                 { len: 32, type: "Float64", pass: false } ];
> +
> +
> +var testCount = 0;

Using testcount is a silly way to ensure completeness. See comments below

@@ +47,5 @@
> +  var b = window.crypto.getRandomValues(arBuf);
> +  ok(b === arBuf, "ArrayBuffer result is argument buffer");
> +
> +  for (var i = 0; i < arBuf.length; i++) {
> +    if (i != 0) {

I do not understand this test. This sems that it will always be true for any array whose length is >1. What is the purpose of this?

@@ +62,5 @@
> +  var failedWithCorrectError = false;
> +  try {
> +    for (var i = 0; i < testData.length; i++) {
> +      if (testData[i].pass) {
> +        testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);

Put this on a try/catch block and put a fail on the catch section "ok(false,'This should not have happened'").

@@ +65,5 @@
> +      if (testData[i].pass) {
> +        testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);
> +      }
> +      else {
> +        // failing tests are dealt with here

Can you write this part to be more generic?. It maybe add the reason to the array so that you can print it (ie move the error info from the code to the data, so that we do not have to be aware of the index of the test case)

try {
  testNs.....
  ok(false, "succeded when it should have failed!"+ someDataRelatedComment)
}
catch(ex) {
  failedWithCorrectError = ex.toString().search(testData[i].regexerr);
  ok(ailedWithCorrectError, "testData[i].testString");
}

@@ +68,5 @@
> +      else {
> +        // failing tests are dealt with here
> +        if (i == 8) {
> +          try {
> +            testNsCryptoGetRandomValues(testData[i].len, testData[i].type, testData[i].pass);

put a fail case after this too, since we expect to fail it should not reach it.

@@ +138,5 @@
> +  catch (ex) {
> +    ok(false, "A one-length array buffer view should not fail");
> +  }
> +
> +  // Test a 16 byte length buffer

Can you add these to the  testData on top? seems cleaner

@@ +171,5 @@
> +  }
> +
> +  // test an offset view
> +  var b = window.crypto.getRandomValues(view2);
> +  ok(view2[0] != 1, "view2 has been updated ");

This will fail every 256 runs (on average). Better check the whole thing so that it almost never happens
Attachment #709907 - Flags: review?(cviecco) → review-
(In reply to Camilo Viecco (:cviecco) from comment #288)
> Comment on attachment 709907 [details] [diff] [review]
> Latest patch, review comments addressed
> 
> Review of attachment 709907 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/Crypto.cpp
> @@ +40,5 @@
> > +  if (!aData.isObject()) {
> > +    return NS_ERROR_DOM_NOT_OBJECT_ERR;
> > +  }
> > +
> > +  JSObject* view = &aData.toObject();
> 
> Can this call ever fail?

<ddahl_> Waldo: can something like this fail: JSObject* view = &aData.toObject(); // aData is supposed to be a typedArray
<njn> billm: pong
<Waldo> ddahl_: no
<ddahl_> ok, thanks
<Waldo> ddahl_: at least, not assuming aData is really an object
<Waldo> which the comment claims
<ddahl_> Waldo: i do this first: 
<ddahl_>  if (!aData.isObject()) {
<ddahl_>     return NS_ERROR_DOM_NOT_OBJECT_ERR;
<ddahl_>   }
<Waldo> if aData's not an object you'll assert
<Waldo> so sounds like you're good, then

> 
> @@ +70,5 @@
> > +    return NS_OK;
> > +  } else if (dataLen > 65536) {
> > +    return NS_ERROR_DOM_QUOTA_EXCEEDED_ERR;
> > +
> > +  }  else if (dataLen > 0 && dataLen < 65536) {
> 
> There is no need for the condition check anymore (if we are here those
> conditions have been meet. We can remove the else if.
Done.

> 
> @@ +73,5 @@
> > +
> > +  }  else if (dataLen > 0 && dataLen < 65536) {
> > +    unsigned char* data =
> > +      static_cast<unsigned char*>(JS_GetArrayBufferViewData(view));
> > +    uint8_t *buf;
> 
> ditto for brian's comment-> if it can be null then you must check for it.
> 

Yep, it can in one circumstance return NULL: 
http://mxr.mozilla.org/mozilla-central/source/js/src/jstypedarray.cpp#3949

> ::: dom/tests/mochitest/crypto/test_getRandomValues.html
> @@ +21,5 @@
> > +                 { len: 32, type: "Float32", pass: false },
> > +                 { len: 32, type: "Float64", pass: false } ];
> > +
> > +

I think we found the major flaws in the tests on irc yesterday.
Attachment #709907 - Attachment is obsolete: true
Attachment #710907 - Flags: review?(cviecco)
Comment on attachment 710907 [details] [diff] [review]
Latest patch, review comments addressed

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

::: dom/tests/mochitest/crypto/test_getRandomValues.html
@@ +47,5 @@
> +  var b = window.crypto.getRandomValues(arBuf);
> +  ok(b === arBuf, "ArrayBuffer result is argument buffer");
> +
> +  for (var i = 0; i < arBuf.length; i++) {
> +    if (arBuf.length > 6) {

Nit: (no need to do) put the condition outside the test so that is more clear.
Attachment #710907 - Flags: review?(cviecco) → review+
Comment on attachment 710907 [details] [diff] [review]
Latest patch, review comments addressed

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

r+ with comment addressed.

::: dom/base/Crypto.cpp
@@ +84,5 @@
> +  void *dataptr = JS_GetArrayBufferViewData(view);
> +  if (dataptr == nullptr) {
> +    NS_WARNING("ArrayBufferView is NULL, cannot continue");
> +    return NS_OK;
> +  }

NS_ENSURE_TRUE(dataptr, NS_ERROR_FAILURE);
Attached patch Patch for landing (obsolete) — Splinter Review
removed the NS_WARNING as per bsmith's comments
Attachment #710907 - Attachment is obsolete: true
Attachment #714672 - Attachment is obsolete: true
Attachment #714744 - Flags: review+
After this has relanded, will someone convert it to WebIDL?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #297)
> After this has relanded, will someone convert it to WebIDL?

I'll probably have a look.
Looks like the patch in bug 673432 will now block this.
No longer blocks: 673432
Depends on: 673432
(In reply to David Dahl :ddahl from comment #299)
> Looks like the patch in bug 673432 will now block this.

Could you just disable the test on B2G? I'd rather not block this bug even longer...
disabled tests and pushed to try https://tbpl.mozilla.org/?tree=Try&rev=c5bad072279b
Try run for c5bad072279b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c5bad072279b
Results (out of 14 total builds):
    success: 13
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ddahl@mozilla.com-c5bad072279b
Try run for c5bad072279b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c5bad072279b
Results (out of 15 total builds):
    success: 14
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ddahl@mozilla.com-c5bad072279b
disabled B2G tests
Attachment #714744 - Attachment is obsolete: true
Attachment #714864 - Flags: review+
I know this bug is already at c#304, before this... but ddahl in IRC suggested I ask here (probably best to move it to a newsgroup thread though)...

I'm curious on why the perfectly legitimate objections to window.crypto.random which have been raised over the past years, (and prevented us from implementing it), don't also apply to window.crypto.getRandomValues.

Such that the end result is that we now find it worthwhile to implement getRandomValues on our end.
https://hg.mozilla.org/mozilla-central/rev/a235e0d9dd70
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 842265
I suspect that the target milestone of Mozilla 16 is wrong and we should read Mozilla 21. Feel free to reverse my change if I'm mistaken.
Target Milestone: mozilla16 → mozilla21
(In reply to Jean-Yves Perrier [:teoli] from comment #308)
> I suspect that the target milestone of Mozilla 16 is wrong and we should
> read Mozilla 21. Feel free to reverse my change if I'm mistaken.

You are correct
Blocks: 842818
Mentioned this on:
https://developer.mozilla.org/en-US/docs/Firefox_21_for_developers
The documentation already was in good enough state (I worked on this a while back):
https://developer.mozilla.org/en-US/docs/DOM/window.crypto.getRandomValues
It's all probably fine.
Flags: sec-review?(brian)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.