369428 - nsExternalAppHandler::SetUpTempFile uses a poor source of randomness, resulting in predictable filenames

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Jesse Ruderman]

nsExternalAppHandler::SetUpTempFile uses srand/rand (a PRNG not suited for security) to create its random filenames.  Worse, it calls srand with a predictable value (the current time) rather than relying on something else to call srand beforehand.

As long as Firefox needs to use random filenames for helper app files (due to bug 230606), it should make sure they're truly unpredictable.  Do we have something that can provide good random bits to C++ code, maybe in NSS?

Discovered by Michal Zalewski.  Split from bug 369390, which describes how this
can be combined with several other bugs to pull off a pretty good exploit with
a little user interaction.

Comment 1

[Brendan Eich [:brendan]]

Why aren't we using mkstemp(3) or whatever's the latest in the POSIX-y world, at least on Linux? I'm assuming this is a bug in XP code.

/be

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yes, this is XP code.  I believe there are existing bugs on using better temp-file-creation functions when available.

Comment 3

[georgi - hopefully not receiving bugspam]

on linux trunk there are 2 open file descriptors to /dev/random:
ls -la /proc/`pidof firefox-bin`/fd.
why not just read(2) from it?
(it is another story that /dev/random blocks when there is not enough entropy resulting in an unknown delay in read(2), so probably better use /dev/urandom).

nss should also have some crypto strong prng so why not just export something from nss?

imho Math.random() should be deliberately different from the real random stuff to avoid attacks on predicting pseudo randomness.

Comment 4

[georgi - hopefully not receiving bugspam]

(In reply to comment #3)
> 
> imho Math.random() should be deliberately different from the real random stuff
> to avoid attacks on predicting pseudo randomness.
> 

... and avoid draining entropy if aplicable.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

NSS is optional...

Comment 6

[Jay Patel [:jay]]

Too late for this 1.8.1.2/1.8.0.10, let's try to get this figured out for next time.

Comment 7

[Brian Crowder]

Attachment: Moved seeding code away

How about this as a naive, easy fix to the seeding problem?  We'll seed the OS RNG at ::Init time instead of in SetUpTemp, and then consume randoms in the salting routine just as we do now, nothing else changes.  This should at least make this algorithm significantly less attackable, though certainly not ironclad.

Comment 8

[Brian Crowder]

Attachment: better comments thanks to reed on IRC

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 254193 [details] [diff] [review]
better comments thanks to reed on IRC

r=dveditz

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 254193 [details] [diff] [review]
better comments thanks to reed on IRC

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v
>retrieving revision 1.309
>diff -u -p -8 -r1.309 nsExternalHelperAppService.cpp
>--- uriloader/exthandler/nsExternalHelperAppService.cpp	21 Dec 2006 07:03:22 -0000	1.309
>+++ uriloader/exthandler/nsExternalHelperAppService.cpp	6 Feb 2007 20:45:54 -0000
>@@ -504,16 +504,22 @@ nsresult nsExternalHelperAppService::Ini
> #ifdef PR_LOGGING
>   if (!mLog) {
>     mLog = PR_NewLogModule("HelperAppService");
>     if (!mLog)
>       return NS_ERROR_OUT_OF_MEMORY;
>   }
> #endif
> 
>+  // turn PR_Now() into milliseconds since epoch
>+  // and salt rand with that. 
>+  double fpTime;
>+  LL_L2D(fpTime, PR_Now());
>+  srand((uint)(fpTime * 1e-6 + 0.5));

Pre-existing code, I know, but PR_Now returns usec since epoch in 64 bits. To convert to ms you want 1e-3. Most OSes do not give ticks at ms resolution, more like 10ms. So you might rather 1e-4.

/be

Comment 11

[Brian Crowder]

Attachment: precision change in PR_Now -> seed conversion

Carrying over dveditz r+ and other flags

Comment 12

[Boris Zbarsky [:bzbarsky]]

So I should note that this patch is only as good as the caller's inability to trigger other code the calls srand().

Offhand,

  MakeRandomString() in nsDownloader.cpp 
  Various XPInstall code
  nsMIMEInfoOS2::LaunchWithFile
  BuildMemName in mapiutl.cpp (once only, though)
  GenerateGlobalRandomBytes in nsMsgComposeSecure.cpp (only once)
  GenerateGlobalRandomBytes in nsMsgCompUtils.cpp (only once)

call srand() on the current time.  Worse yet, there's no guarantee that someone won't add code that calls srand() on some constant value, or value under the control of unprivileged script.  It only takes one failure point for us to lose, unfortunately.

See also bug 327161 where we had similar issues with the UUID generator and fixed them by having it save and restore the rng state.  This might be the way to go here if we want something we're sure is safe.  The other option, of course, being to use a private generator of some sort.

Also, as far as the 1e6 vs 1e4 thing... if you look at the place this was copied from (profile manager), the comments say:

612     // use 1e-6, granularity of PR_Now() on the mac is seconds

Not sure that's still true on OSX, of course.  ;)

Comment 13

[Jay Patel [:jay]]

Comment on attachment 254212 [details] [diff] [review]
precision change in PR_Now -> seed conversion

Approved for both branches, a=jay.  Looks like dvedtiz and brendan have reviewed the patch, so let's get it in asap!

Comment 14

[Boris Zbarsky [:bzbarsky]]

Er... Please see comment 12.  I think this patch just gives us a false sense of security, basically; it doesn't really fix the problem.

If you think the marginal attack difficulty increase it provides is worth it for the branch, I guess we can take it...

It'd also be nice if this had gotten review from someone who actually owns this module at some point, of course.  ;)

Comment 15

[Daniel Veditz [:dveditz]]

In Firefox netwerk appears to be the only place with an accessible srand call. XPInstall calls it in the "wizard" (the old native installer code, not addon installs), and the mail code isn't in Firefox. We should fix this better, especially for SeaMonkey since it *does* have mail code, but this is a quick improvement we can get into 2.0.0.2 at the last minute.

The networking MakeRandomString() could probably be forced, we should fix it there, too.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Like I said, I'm ok landing this on branch.  I don't think we should take it on trunk -- we should fix it for real there.

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 254212 [details] [diff] [review]
precision change in PR_Now -> seed conversion

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v
>retrieving revision 1.309
>diff -u -p -8 -r1.309 nsExternalHelperAppService.cpp
>--- uriloader/exthandler/nsExternalHelperAppService.cpp	21 Dec 2006 07:03:22 -0000	1.309
>+++ uriloader/exthandler/nsExternalHelperAppService.cpp	6 Feb 2007 22:19:39 -0000
>@@ -504,16 +504,21 @@ nsresult nsExternalHelperAppService::Ini
> #ifdef PR_LOGGING
>   if (!mLog) {
>     mLog = PR_NewLogModule("HelperAppService");
>     if (!mLog)
>       return NS_ERROR_OUT_OF_MEMORY;
>   }
> #endif
> 
>+  // turn PR_Now() into milliseconds since epoch and seed rand with that.

Comment's wrong.  Could use 1e-3 to make it true.

>+  double fpTime;
>+  LL_L2D(fpTime, PR_Now());
>+  srand((uint)(fpTime * 1e-4 + 0.5));

/be

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #12)
> It only takes one failure point for us to lose, unfortunately.

Yes, and we need a bug on this. Dan, can you file one?

> Also, as far as the 1e6 vs 1e4 thing... if you look at the place this was
> copied from (profile manager), the comments say:
> 
> 612     // use 1e-6, granularity of PR_Now() on the mac is seconds
> 
> Not sure that's still true on OSX, of course.  ;)

It's not -- that dates from Mac OS 9 or even earlier. Lies in comments, news at 11. Mac OS X gettimeofday is fine-grained, as this test pgm shows:

#include <stdio.h>
#include <sys/types.h>
#include <sys/time.h>

main() {
    int tv_sec = -1;

    for (;;) {
        struct timeval tv;
        gettimeofday(&tv, NULL);
        if (tv_sec < 0)
            tv_sec = tv.tv_sec;
        else if (tv.tv_sec > tv_sec)
            return 0;
        printf("%d.%d\n", tv.tv_sec, tv.tv_usec);
    }
}

/be

Comment 19

[Scott MacGregor]

Comment on attachment 254212 [details] [diff] [review]
precision change in PR_Now -> seed conversion

I'm happy to review this but per bz, this needs a module owner review and since bsmedberg is on paternity leave that means: jst, bz or darin.

bz has already been active in this bug so I'll start there :)

Comment 20

[Brian Crowder]

Attachment: same philosophy for netwerk

Alright, same change philosophically for netwerk.

Comment 21

[Brian Crowder]

On looking at the PR_Now implementation, I think 1e-6 is better, so I changed back to that, and it is that in both locations.

Comment 22

[Brian Crowder]

Comment on attachment 254228 [details] [diff] [review]
same philosophy for netwerk

changing sr? to bz on reed's recommendation

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 254228 [details] [diff] [review]
same philosophy for netwerk

nsDownloader is a component, not a service unlike nsExternalHelperAppService.cpp.  So the network change just makes a minor tweak to the exact moment when we seed the rng in network.

I guess using a static boolean the way some of mailnews works wouldn't help us much either, since it would need to user to have hit the code before the attack comes, right?

Comment 24

[Brian Crowder]

Could just make a static C++ object that calls srand() on startup, I suppose.  Ugly, though.

Comment 25

[Boris Zbarsky [:bzbarsky]]

I can live with ugly for branch-only, I think.  Like I said, I don't really want this approach happening on trunk to start with; we should put in the time to really make it work there.

ccing some seamonkey and mailnews folks who might want to fix the mail issues, by the way.

Comment 26

[Brendan Eich [:brendan]]

(In reply to comment #21)
> On looking at the PR_Now implementation, I think 1e-6 is better, so I changed
> back to that, and it is that in both locations.

Why not use sub-second resolution? It's there and it could help defeat short-term exploits based on the server measuring the client's time.

The comment is *still* wrong!

/be

Comment 27

[Brian Crowder]

Attachment: an ugly hack

Have a look at this ugly hack, then.  I'm building/testing it now.

Comment 28

[Brian Crowder]

http://mxr.mozilla.org/seamonkey/source/nsprpub/pr/src/md/mac/mactime.c#118 <--- this is the implementation of PR_Now we're discussing, right?  I agree it should use a finer-grained clock.  Should I fix that bug first, separately and then use the subsecond resolution here, or do we just want to get this hack over with?  (and I'll fix my comments to say "seconds" instead of "milliseconds")

Comment 29

[Brian Crowder]

Note to self, mainly: bug 369561 is the PR_Now bug I just submitted.  Couldn't find any other bugs about PR_Now on the Mac.

Comment 30

[Brendan Eich [:brendan]]

(In reply to comment #28)
> http://mxr.mozilla.org/seamonkey/source/nsprpub/pr/src/md/mac/mactime.c#118
> <--- this is the implementation of PR_Now we're discussing, right?

No. Mac OS X is "Unix". See http://mxr.mozilla.org/seamonkey/source/nsprpub/pr/src/md/unix/unix.c#2991

/be

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 254231 [details] [diff] [review]
an ugly hack

sr=bzbarsky.  Worst-case, we have a compiler that doesn't run static ctors right (not an issue on Windows/Mac/Linux), and then the other srand calls will handle the situation anyway.

Again, please do not land this on trunk!

Comment 32

[Brian Crowder]

Attachment: use microseconds in seed

Oh, lovely.  Sorry for the confusion, then, and yay unix.  Let's use microseconds.

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 254235 [details] [diff] [review]
use microseconds in seed

r=dveditz

approved for 1.8/1.8.0 branches, a=dveditz

Comment 34

[Brian Crowder]

No landing on trunk, which I assume means we need another strategy before I can mark fixed?

1.8:
uriloader/exthandler/nsExternalHelperAppService.cpp: 1.290.4.12
netwerk/base/src/nsDownloader.cpp:                   1.18.28.1

1.8.0:
Checking in uriloader/exthandler/nsExternalHelperAppService.cpp: 1.290.4.2.4.1
netwerk/base/src/nsDownloader.cpp:                               1.18.38.1

Comment 35

[Boris Zbarsky [:bzbarsky]]

I think the best option for trunk is to guarantee that app startup calls srand(), and forbid calls to srand from code elsewhere in the app, unless that code does an rng save/restore around the srand call....

Comment 36

[Boris Zbarsky [:bzbarsky]]

Although, if a plug-in calls srand() we're hosed too, no?

So maybe the right solution is to use our own rng and not use the standard library one at all....

Comment 37

[Brian Crowder]

Using our own RNG across the browser might not be a bad idea, at all.  If (when, seems likely) I do an Mersenne Twister implementation for JS, it could be duplicated in NSPR, or an API could be exposed for it, and we could use that everywhere?

Comment 38

[Boris Zbarsky [:bzbarsky]]

Sounds pretty good to me!

Comment 39

[Jo Hermans]

(In reply to comment #32)
> Created an attachment (id=254235) [details]
> use microseconds in seed
> 
> Oh, lovely.  Sorry for the confusion, then, and yay unix.  Let's use
> microseconds.
> 

Note that it's a bad idea to use a timestamp to initialize srand(). I got burned a few months ago in my daytime job for this, when a piece on dynamically loaded code using srand(time(NULL)). Using microseconds makes this scenario almost impossible, although other solutions exists too (/dev/urandom etc ...)

(In reply to comment #36)
> Although, if a plug-in calls srand() we're hosed too, no?

That was actually the attack-vector that was used. Either Gecko has to provide its own implementation, or use other protection. I had to xor every output from rand() and random() with an offset (properly initialized and re-initialized once in a while) to defeat any attempts of guessing and re-seeding. Messy.

Comment 40

[Brian Crowder]

Letting go of this bug for now.

Comment 41

[Benjamin Smedberg]

We're going to need at least the nasty hack on trunk as well. Shawn, can you take this?

Comment 42

[Boris Zbarsky [:bzbarsky]]

Note that the reason the nasty hack is nasty is that any srand() anywhere else in the code _will_ screw it over.  That's why we need to either do what the UUID generator does or make all srand() calls happen at startup only.

Comment 43

[Nelson Bolyard (seldom reads bugmail)]

Does this code run after NSS has been initialized? 
If so, you've got access to a very good (FIPS 140-1 certified) 
cryptographic quality PRNG with a single function call:  
   PK11_GenerateRandom(unsigned char *data,int len)

Comment 44

[Boris Zbarsky [:bzbarsky]]

> Does this code run after NSS has been initialized? 

See comment 5...  That said, I do think that that's the way to go.  I can't think of a good reason to build Gecko without crypto nowadays.

Comment 45

[Christian :Biesinger (don't email me, ping me on IRC)]

US export regulations?

Comment 46

[Benjamin Smedberg]

US export regulations don't really affect us any more, and the toolkit doesn't build with crypto disabled anyway. So I'm perfectly happy with removing that option and always having crypto on.

Comment 47

[Christian :Biesinger (don't email me, ping me on IRC)]

They still affect several countries.

Comment 48

[Boris Zbarsky [:bzbarsky]]

They affect them by prohibiting all export to them, whether with or without crypto.

Comment 49

[Christian :Biesinger (don't email me, ping me on IRC)]

Even without crypto? wow.

Comment 50

[[e-mail]]

(In reply to comment #46)
>toolkit doesn't build with crypto disabled anyway.
I thought that was just the old password manager.
(Not that the new password manager will be happy without crypto, but...)

Comment 51

[Shawn Wilsher :sdwilsh]

Attachment: trunk v1.0

Not asking for review yet because I haven't tested it.  I'm mostly posting it to make sure I'm using the NSS stuff correctly, and to have bsmedberg tell me what I did wrong with the Makefiles, because what I did feels like a hack.

Comment 52

[Shawn Wilsher :sdwilsh]

Comment on attachment 280225 [details] [diff] [review]
trunk v1.0

Just for the record, this doesn't actually work :(

It only gets the extension and the rest is empty.  It's possible I'm using our string api wrong too...

Comment 53

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 280225 [details] [diff] [review]
trunk v1.0

Here are some review questions.

>+  nsCAutoString saltedTempLeafName;
>+  SECStatus srv = PK11_GenerateRandom(reinterpret_cast<unsigned char *>(saltedTempLeafName.BeginWriting()),
>+                                      SALT_SIZE);

Can saltedTempLeafName.BeginWriting() return NULL?
When it returns non-NULL, is the buffer always at least SALT_SIZE bytes long?

PK11_GenerateRandom returns a random BINARY string, wherein each byte has an
equal probability of containing any value in the range [0..255.].
Do you really want a binary file name, as opposed to a name that can be typed
(e.g. following the string "rm " :) ?  
Perhaps you should base64-encode the name first.  If you use base64, be sure 
to scan the results for any (platform dependent) path separator characters
and remove or substitute them, to keep them out of the file name.

>+  mTempFile->Append(NS_ConvertUTF8toUTF16(saltedTempLeafName));

Not all binary strings are valid UTF8.  I would expect the above call to 
fail with high probability (high frequency) due to invalid UTF8 characters
in the input string.  This is yet another reason to do something like 
base64 encoding of the name.

Comment 54

[Nelson Bolyard (seldom reads bugmail)]

You can call NSSBase64_EncodeItem to encode the binary data into ASCII. 
The only two non-alphanumeric characters that can occur in base64 encoded
data are + and /, so it probably suffices to replace all / with - to 
avoid path separators.  

Comment 55

[Shawn Wilsher :sdwilsh]

Attachment: trunk v1.1

Nelson, can you take a look at the NSS api usage please?

biesi, can you take a look at the exthandler stuff, and bsmedberg, please check over the Makefile changes?  Thanks!

Comment 56

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 280257 [details] [diff] [review]
trunk v1.1

r=nelson for the NSS part, with one change.

>+#define SALT_SIZE 8

>+  unsigned char data[SALT_SIZE];
>+  SECStatus srv = PK11_GenerateRandom(data, SALT_SIZE);

>+  char *base64encodedData = NSSBase64_EncodeItem(nsnull, nsnull, SALT_SIZE,
>+                                                 &binary_item);

Base64 encoding produces 4 characters of output for every 3 bytes
of input.  If the input is not a multiple of 3 bytes in length,
it pads the output with trailing '=' characters. 
For 8 bytes of input, there will always be one pad byte added,
and that will result in the last character of output always 
being '='.  So, if it is desirable that no character of the name
be predictable, I suggest you change SALT_SIZE to a multiple of 3.
I'd suggest 9.  Alternatively, you could strip trailing '=' characters
from the base64-encoded salt string.

Comment 57

[Shawn Wilsher :sdwilsh]

That explains why I was getting more than 8 characters of output then (clearly, I've never used base64 encoding before!), but the code as it stands truncates the output string to SALT_SIZE.

Comment 58

[Nelson Bolyard (seldom reads bugmail)]

OK, I don't really require any change.  I just wanted you to be aware
so that you could make the code produce the result you intended.

Each character output from the base64 encoder uses 6 bits from the input.
If you limit the output to 8 characters, you're using just 48 bits of 
the random data.  That may or may not be satisfactory to you.  

Comment 59

[Benjamin Smedberg]

Comment on attachment 280257 [details] [diff] [review]
trunk v1.1

>Index: docshell/build/Makefile.in

>+EXTRA_DEPS += $(NSS_DEP_LIBS)
>+EXTRA_DSO_LIBS += sqlite3

The sqlite3 line isn't right. Just beacuse NSS has sqlite deps doesn't mean we should explicitly link against it. You may need MOZ_FIX_LINK_PATH see also 391096

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp

>+  // At this point we do not have a filename for the temp file.  For security
>+  // purposes, this cannot be predictable, so we use a cryptographic quality
>+  // PRNG to generate one.
>+  unsigned char data[SALT_SIZE];
>+  SECStatus srv = PK11_GenerateRandom(data, SALT_SIZE);
>+  if (SECSuccess != srv)
>+    return NS_ERROR_FAILURE;
>+
>+  SECItem binary_item;
>+  binary_item.data = data;
>+  binary_item.len = SALT_SIZE;
>+  char *base64encodedData = NSSBase64_EncodeItem(nsnull, nsnull, SALT_SIZE,
>+                                                 &binary_item);
>+  if (!base64encodedData)
>+    return NS_ERROR_FAILURE;

Does NSS need too be initialized for any of these calls?

Comment 60

[Nelson Bolyard (seldom reads bugmail)]

> Does NSS need too be initialized for any of these calls?
Yes, I mentioned that in comment 43.

Comment 61

[Shawn Wilsher :sdwilsh]

OK, someone help me out here.  Comment 44 seems to indicate that we don't care if someone wants to build without NSS - that is their problem.

Wouldn't this code be called after NSS is initialized, or no?  This isn't clear to me, nor am I really sure about when we initialize it in general.

Comment 62

[Christian :Biesinger (don't email me, ping me on IRC)]

Normally we initialize it when we first use PSM, e.g. when first connecting to an HTTPS page or when password manager reads the stored passwords.

Therefore, you can't rely on NSS being initialized. GetService a PSM component to ensure it. Some places in the code do that:

http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpHandler.cpp#1499
http://lxr.mozilla.org/seamonkey/source/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp#2140

Comment 63

[Boris Zbarsky [:bzbarsky]]

Or we could expose this functionality via PSM in general and avoid doing raw NSS calls here.  I suggested to Shawn that he check with Kai as to which is preferable.

Comment 64

[Shawn Wilsher :sdwilsh]

Yeah, sorry.  I should have updated the bug.  I plan on sending an e-mail off to Kai later today about this.

Comment 65

[Damon Sicore (:damons)]

Targeting M9 as this bug will block beta.

Comment 66

[Shawn Wilsher :sdwilsh]

I still haven't heard back from Kai as to what he wants me to do, so I'm blocked on that.

Comment 67

[Shawn Wilsher :sdwilsh]

Got an e-mail from Kai - I hope to have a patch for this within a week.

Comment 68

[Shawn Wilsher :sdwilsh]

Attachment: security/ code v1.0

OK, I think this is what we need for the backend.  Looking for feedback on it before I hook it up the the external helper app code...

Comment 69

[Kai Engert (:KaiE:)]

Comment on attachment 283075 [details] [diff] [review]
security/ code v1.0

>+[scriptable, uuid(2362d97a-747a-4576-8863-697667309209)]
>+interface nsIRandomGenerator : nsISupports {

Will this accessible from chrome only, or from sandbox JS, too?

If sandbox JS can access this, I wonder if we're ok to allow web sites to drain our pool of true random data.


>+  AUTF8String generateString(in unsigned long aLength);

Why AUTF8String?
I think ACString is better.

Using AUTF8String might suggest to readers that this string is a valid encoded UTF-8 String, but it's not.


>+nsRandomGenerator::GenerateString(PRUint32 aLength, nsACString &retVal)
>+{
>+  PRUint32 size = static_cast<PRUint32>((aLength + 0.5) * 4. / 3.);

What's the purpose of this calculation?
If you have a good reason to, please explain in a source code comment.


>+  unsigned char *data = new unsigned char[size];
>+  if (!data)
>+    return NS_ERROR_OUT_OF_MEMORY;

You never delete data, I think you need a 
  delete [] data
statement before you return.

You must delete if you exit after a failure from PK11_GenerateRandom,
and you can always delete it after you called NSSBase64_EncodeItem
(before you test for the error).

Comment 70

[Shawn Wilsher :sdwilsh]

Attachment: security/ code v1.1

(In reply to comment #69)
> Will this accessible from chrome only, or from sandbox JS, too?
Chrome only, as far as I know.

> Why AUTF8String?
> I think ACString is better.
Agreed.

> >+nsRandomGenerator::GenerateString(PRUint32 aLength, nsACString &retVal)
> >+{
> >+  PRUint32 size = static_cast<PRUint32>((aLength + 0.5) * 4. / 3.);
> 
> What's the purpose of this calculation?
> If you have a good reason to, please explain in a source code comment.
Comment added.

> You never delete data, I think you need a 
>   delete [] data
> statement before you return.
> 
> You must delete if you exit after a failure from PK11_GenerateRandom,
> and you can always delete it after you called NSSBase64_EncodeItem
> (before you test for the error).
Whoops.  I really don't want to add more leaks to the tree :/

Comment 71

[georgi - hopefully not receiving bugspam]

(In reply to comment #69)
> I wonder if we're ok to allow web sites to drain
> our pool of true random data.
> 

window.crypto probably may be used to drain entropy.
crypto.generateCRMFRequest
if crypto.random() gets implemented it will drain the entropy.

does lack of entropy scare you?

Comment 72

[georgi - hopefully not receiving bugspam]

imo this may be a problem if nss PRNG returns data when there is no entropy.
though on 2.6 kernel i couldn't make the entropy 0 with a local user.

Comment 73

[Nelson Bolyard (seldom reads bugmail)]

Shawn, don't fear the use of NSS's FIPS certified PRNG any more than you'd
fear using /dev/urandom (if it was FIPS certified).  

Comment 74

[Dave Townsend [:mossop]]

(In reply to comment #70)
> Created an attachment (id=283303) [details]
> security/ code v1.1

You are using NSSBase64_EncodeItem here which has the "feature" that it adds linebreaks (\r\n as I recall) at every 64th character. While I believe this is a longer string than you are using, I think it would be better to avoid linebreaks in the output from this generic component.

fwiw I ended up using PL_Base64Encode rather than the NSS base64 encoder.

Comment 75

[Shawn Wilsher :sdwilsh]

Attachment: security/ code v1.2

Comment 76

[Shawn Wilsher :sdwilsh]

Attachment: uriloader/ code v2.0

Comment 77

[Christian :Biesinger (don't email me, ping me on IRC)]

does the patch work in clobber builds? (I don't think it will, because security is in tier_toolkit, which is after tier_gecko where uriloader lives)

your indentation in uriloader/exthandler/Makefile.in is wrong

Comment 78

[Christian :Biesinger (don't email me, ping me on IRC)]

>Using AUTF8String might suggest to readers that this string is a valid encoded
>UTF-8 String, but it's not.

It actually is a valid UTF-8 string...

Comment 79

[Kai Engert (:KaiE:)]

(In reply to comment #71)
> does lack of entropy scare you?

All I wanted is that we think about it, from the comments I conclude it's not a problem.


(In reply to comment #78)
> >Using AUTF8String might suggest to readers that this string is a valid encoded
> >UTF-8 String, but it's not.
> 
> It actually is a valid UTF-8 string...

A random series of bytes is likely to be "invalid input" for a UTF-8 decoder.

Comment 80

[Kai Engert (:KaiE:)]

(In reply to comment #79)
> (In reply to comment #78)
> > >Using AUTF8String might suggest to readers that this string is a valid encoded
> > >UTF-8 String, but it's not.
> > 
> > It actually is a valid UTF-8 string...
> 
> A random series of bytes is likely to be "invalid input" for a UTF-8 decoder.

Oh, I see, the data returned is base64 encoded and therefore only returning characters from the 7bit ASCII set. This makes it a valid UTF-8 string, of course.

Comment 81

[Kai Engert (:KaiE:)]

Comment on attachment 283383 [details] [diff] [review]
security/ code v1.2

>+[scriptable, uuid(2362d97a-747a-4576-8863-697667309209)]
>+interface nsIRandomGenerator : nsISupports {
>+ /**
>+  * Generates a random ASCII string of the specified length.
>+  *
>+  * @param aLength
>+  *        The length of the string to generate.
>+  * @return an ASCII string.

I think this comment doesn't match your implementation and is misleading.

You're not returning a "random ASCII string".
By that description, I would expect a string that makes use of all possible ASCII characters.

But you're returning only a subset of those characters, the possible base64 characters.


In addition you say, the function will return a string of aLength, but that's wrong.
You will return a string that is longer, because of the base64 encoding.


I'd expect your generateString function to return a buffer of random bytes of exactly size aLength.


If you want the interface to return a base64-encoded version of that buffer, I think the function name and comments should explain that, in particular that the returned buffer will be longer than aLength.



>+NS_IMETHODIMP
>+nsRandomGenerator::GenerateString(PRUint32 aLength, nsACString &retVal)
>+{
>+  // This calculation gets the right size of the data string that we need.
>+  // Base64 encoding produces 4 characters of output for every 3 bytes of input.
>+  PRUint32 size = static_cast<PRUint32>((aLength + 1) * 4. / 3.);


If I'm reading your code right, then:

- requested output size is aLength, for example: 5

- your buffer size for raw random data: 8

- for a 8 bytes buffer, the base64 encoder will produce an ascii string: >= 12

- finally you'll only assign 5 of the 12 bytes to the result value.


I think your function should only request the required amount of random data.

Comment 82

[Kai Engert (:KaiE:)]

(In reply to comment #81)
> In addition you say, the function will return a string of aLength, but that's
> wrong.
> You will return a string that is longer, because of the base64 encoding.

I'm taking this statement back, I later saw you're truncating the encoded buffer.

Comment 83

[Shawn Wilsher :sdwilsh]

Yes - biesi pointed out that I had my three and four backwards (and that I'm using floating point math for no good reason).

I'll update the comments, and I'll have to move the idl someplace else, because like biesi said, this won't work with clobber builds.

Comment 84

[Shawn Wilsher :sdwilsh]

Attachment: security/ code v1.3

After talking to a few people online, I've moved the idl file to netwerk/base/public (assuming it's OK with biesi).

I do believe I've addressed all previous review comments as well.

Comment 85

[Shawn Wilsher :sdwilsh]

Attachment: uriloader/ code v2.1

The only thing that has changed here is the name of the method we are calling, and dropping the requires for the Makefile.

Comment 86

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 283739 [details] [diff] [review]
security/ code v1.3

necko isn't really a good place but we crossed that bridge years ago

netwerk/base/public/nsIRandomGenerator.idl

the comment should be indented by one more character

Comment 87

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 283740 [details] [diff] [review]
uriloader/ code v2.1

it would, I think, make more sense to use the RNG as a service

Comment 88

[Shawn Wilsher :sdwilsh]

Comments addressed locally - I'll put a new patch up once kaie is done with the review.

Comment 89

[Kai Engert (:KaiE:)]

Comment on attachment 283739 [details] [diff] [review]
security/ code v1.3

>+[scriptable, uuid(2362d97a-747a-4576-8863-697667309209)]
>+interface nsIRandomGenerator : nsISupports {
>+ /**
>+  * Generates a random string that is base64 encoded of the specified length.
>+  *
>+  * @param aLength
>+  *        The length of the string to generate.
>+  * @return a base64 encoded string that is of length aLength.
>+  */
>+  AUTF8String generateBase64EncodedString(in unsigned long aLength);
>+};


I think when someone looks at this interface, he will be surprised.
You gave the interface a very generic name, but there is no function that returns raw random data.

What you offer is a function that produces data in a format specific to your application context.
Who else will ever prefer random data in that special format? I think other consumers will prefer to obtain the raw data and do their own encoding.

I propose to do this:


>+interface nsIRandomGenerator : nsISupports {
>+ /**
>+  * Generate the specified amount of random bytes, returned
      as a string (the buffer may contain the zero byte).
>+  *
>+  * @param aLength
>+  *        The amount of random bytes requested.
>+  * @return A string that contains aLength random bytes.
>+  */
>+  ACString generateRandomBytes(in unsigned long aLength);
>+};


This would be a general purpose interface for other consumers, too.
You can base64 encode the buffer that is returned in your application code.

Comment 90

[Kai Engert (:KaiE:)]

I'm not sure if ACString is a valid transport mechanism for buffers that may contain the zero byte.

In other interfaces I've seen something like this:

+   *  @param length The number of bytes
+   *  @param data The bytes
+   */
+  void export(out unsigned long length,
+              [retval, array, size_is(length)] out octet data);

Comment 91

[Kai Engert (:KaiE:)]

I'm ok with the proposed API if there are more people who expect the base64-encoded string version of random data to be useful for other application code, too. In that case we could add my proposed API in addition, with a future patch.

Comment 92

[Kai Engert (:KaiE:)]

Comment on attachment 283739 [details] [diff] [review]
security/ code v1.3

Please forgive me if I sound like nit picking.

Your interface claims that the data returned is in base64 format. Someone who looks only at the interface description will conclude the returned string is a valid base64 encoded sequence of characters.

But I think it's not?

Let's say the request length is 5.

Your calculation will allocate a buffer of
  (5+1) * 3 / 4 => 4.5 => rounded up => 5
bytes

Let's say the random buffer contains
  qqqqq

echo -n "qqqqq" | base64

=> cXFxcXE=

You truncate that to 5 characters.

echo -n "cXFxc" | base64 -d
qqqbase64: invalid input


I think you should at least change the description and function name to make it clear the returned string will only contain characters from the base64 encoding, but it will no necessarily be a valid base64 string.

But I personally think this is another argument for moving the base64 transformation to your module.

Comment 93

[[e-mail]]

(In reply to comment #90)
>I'm not sure if ACString is a valid transport mechanism for buffers that may
>contain the zero byte.
It is; string and wstring are the unsafe buffers.

Comment 94

[Kai Engert (:KaiE:)]

Attachment: Proposed changes for security/ code (v1.4)

This is a new revision of Shawn's "security/ code" patch that implements the API I'd prefer.

For now I used "out octet". It that's not acceptable, we could change this to ACString (but other crypto code is using "out octet", too)

Comment 95

[Kai Engert (:KaiE:)]

Attachment: Proposed changes for uriloader/ code (v2.2)

This is a new revision of Shawn's "uriloader/ code" patch that makes use of the changed API. I moved the base64 calls over to this patch.

Comment 96

[Shawn Wilsher :sdwilsh]

Attachment: security/ code v1.5

Comment 97

[Shawn Wilsher :sdwilsh]

Attachment: uriloader/ code v2.3

Comment 98

[Shawn Wilsher :sdwilsh]

Comment on attachment 284405 [details] [diff] [review]
security/ code v1.5

Please note that there is bitrot with this patch.  I've fixed it locally.

Comment 99

[Kai Engert (:KaiE:)]

Comment on attachment 284405 [details] [diff] [review]
security/ code v1.5

Look good to me, thanks for the changes! r=kengert

Nit: @param aLength
     The length of the string to generate.

We're not producing a string anymore.

Comment 100

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 284406 [details] [diff] [review]
uriloader/ code v2.3

+  // We will request rw random bytes, and transform that to a base64 string,

rw?

Comment 101

[Kai Engert (:KaiE:)]

(In reply to comment #100)
> (From update of attachment 284406 [details] [diff] [review])
> +  // We will request rw random bytes, and transform that to a base64 string,
> 
> rw?

Raw

Comment 102

[Robert Relyea]

Comment on attachment 284405 [details] [diff] [review]
security/ code v1.5

r+. my review focused on the random number function itself. I'm presuming kai has properly verified the xpcom boiler plate.

Also note, there is a random number generate function defined for the window.crypto object in the docs and the idl, but it currently not implemented (see security/manager/ssl/src/nsCrypto.cpp -- nsCrypto::Random() for the 'implementation' and  dom/public/idl/base/nsIDOMCrypto.idl  for the definition, and http://developer.mozilla.org/en/docs/JavaScript_crypto for the documentation (including the not at the bottom that says it is unimplemented).

bob

Comment 103

[Shawn Wilsher :sdwilsh]

Attachment: security/ code v1.6

This just fixes the bitrot that occurred.

Comment 104

[Shawn Wilsher :sdwilsh]

Checking in security/manager/ssl/src/Makefile.in;
new revision: 1.84; previous revision: 1.83
Checking in security/manager/ssl/src/nsNSSModule.cpp;
new revision: 1.44; previous revision: 1.43
Checking in security/manager/ssl/src/nsRandomGenerator.cpp;
initial revision: 1.1
Checking in security/manager/ssl/src/nsRandomGenerator.h;
initial revision: 1.1
Checking in netwerk/base/public/Makefile.in;
new revision: 1.117; previous revision: 1.116
Checking in netwerk/base/public/nsIRandomGenerator.idl;
initial revision: 1.1
Checking in uriloader/exthandler/nsExternalHelperAppService.cpp;
new revision: 1.348; previous revision: 1.347

Comment 105

[Shawn Wilsher :sdwilsh]

Some strange orangeness on the test boxen has caused me to back this out, although it's not clear that this checkin even caused it.

Comment 106

[Dave Townsend [:mossop]]

It was not this code, the test was at fault since it relies on external resources and failed again later (bug 399611)

Comment 107

[Shawn Wilsher :sdwilsh]

Take two...

Checking in netwerk/base/public/Makefile.in;
new revision: 1.119; previous revision: 1.118
Checking in netwerk/base/public/nsIRandomGenerator.idl;
new revision: 1.3; previous revision: 1.2
Checking in security/manager/ssl/src/Makefile.in;
new revision: 1.86; previous revision: 1.85
Checking in security/manager/ssl/src/nsNSSModule.cpp;
new revision: 1.46; previous revision: 1.45
Checking in security/manager/ssl/src/nsRandomGenerator.cpp;
new revision: 1.3; previous revision: 1.2
Checking in security/manager/ssl/src/nsRandomGenerator.h;
new revision: 1.3; previous revision: 1.2
Checking in uriloader/exthandler/nsExternalHelperAppService.cpp;
new revision: 1.350; previous revision: 1.349