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

RESOLVED FIXED in mozilla1.9beta1

Status

Core Graveyard
File Handling
RESOLVED FIXED
11 years ago
a year ago

People

(Reporter: Jesse Ruderman, Assigned: sdwilsh)

Tracking

({fixed1.8.0.10, fixed1.8.1.2})

Trunk
mozilla1.9beta1
fixed1.8.0.10, fixed1.8.1.2
Dependency tree / graph
Bug Flags:
blocking1.9 +
blocking1.8.1.2 +
blocking1.8.0.10 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low] stepping stone, URL)

Attachments

(3 attachments, 16 obsolete attachments)

3.49 KB, patch
dveditz
: review+
Details | Diff | Splinter Review
5.25 KB, patch
Biesinger
: review+
Biesinger
: superreview+
Details | Diff | Splinter Review
11.86 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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.
(Reporter)

Updated

11 years ago
Group: security
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
(Reporter)

Updated

11 years ago
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.10?
Yes, this is XP code.  I believe there are existing bugs on using better temp-file-creation functions when available.
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.
(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.
NSS is optional...

Comment 6

11 years ago
Too late for this 1.8.1.2/1.8.0.10, let's try to get this figured out for next time.
Flags: blocking1.8.1.3?
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.11?
Flags: blocking1.8.0.10?

Comment 7

11 years ago
Created attachment 254192 [details] [diff] [review]
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.
Assignee: file-handling → crowder
Status: NEW → ASSIGNED

Comment 8

11 years ago
Created attachment 254193 [details] [diff] [review]
better comments thanks to reed on IRC
Attachment #254192 - Attachment is obsolete: true
Flags: blocking1.8.1.3?
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.11?
Flags: blocking1.8.0.10?
Comment on attachment 254193 [details] [diff] [review]
better comments thanks to reed on IRC

r=dveditz
Attachment #254193 - Flags: superreview?(mscott)
Attachment #254193 - Flags: review+
Attachment #254193 - Flags: approval1.8.1.2?
Attachment #254193 - Flags: approval1.8.0.10?
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

11 years ago
Created attachment 254212 [details] [diff] [review]
precision change in PR_Now -> seed conversion

Carrying over dveditz r+ and other flags
Attachment #254193 - Attachment is obsolete: true
Attachment #254212 - Flags: superreview?
Attachment #254212 - Flags: review+
Attachment #254212 - Flags: approval1.8.1.2?
Attachment #254212 - Flags: approval1.8.0.10?
Attachment #254193 - Flags: superreview?(mscott)
Attachment #254193 - Flags: approval1.8.1.2?
Attachment #254193 - Flags: approval1.8.0.10?

Updated

11 years ago
Attachment #254212 - Flags: superreview? → superreview?(mscott)
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.  ;)

Updated

11 years ago
Flags: blocking1.8.1.2?
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10?
Flags: blocking1.8.0.10+

Comment 13

11 years ago
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!
Attachment #254212 - Flags: superreview?(mscott)
Attachment #254212 - Flags: approval1.8.1.2?
Attachment #254212 - Flags: approval1.8.1.2+
Attachment #254212 - Flags: approval1.8.0.10?
Attachment #254212 - Flags: approval1.8.0.10+

Updated

11 years ago
Whiteboard: needs landing on 1.8 and 1.8.0 branches
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.  ;)
Whiteboard: needs landing on 1.8 and 1.8.0 branches
Whiteboard: needs landing on 1.8 and 1.8.0 branches
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.
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 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
Attachment #254212 - Flags: superreview?(mscott)
(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

11 years ago
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 :)
Attachment #254212 - Flags: superreview?(mscott) → superreview?(bzbarsky)

Comment 20

11 years ago
Created attachment 254228 [details] [diff] [review]
same philosophy for netwerk

Alright, same change philosophically for netwerk.
Attachment #254212 - Attachment is obsolete: true
Attachment #254228 - Flags: superreview?(mscott)
Attachment #254228 - Flags: review?(dveditz)
Attachment #254228 - Flags: approval1.8.1.2?
Attachment #254228 - Flags: approval1.8.0.10?
Attachment #254212 - Flags: superreview?(bzbarsky)

Comment 21

11 years ago
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

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

changing sr? to bz on reed's recommendation
Attachment #254228 - Flags: superreview?(mscott) → superreview?(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?
Attachment #254228 - Flags: superreview?(bzbarsky)
Attachment #254228 - Flags: superreview-
Attachment #254228 - Flags: review?(dveditz)
Attachment #254228 - Flags: review-

Comment 24

11 years ago
Could just make a static C++ object that calls srand() on startup, I suppose.  Ugly, though.
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.
(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

11 years ago
Created attachment 254231 [details] [diff] [review]
an ugly hack

Have a look at this ugly hack, then.  I'm building/testing it now.
Attachment #254228 - Attachment is obsolete: true
Attachment #254231 - Flags: superreview?(bzbarsky)
Attachment #254231 - Flags: review?(dveditz)
Attachment #254231 - Flags: approval1.8.1.2?
Attachment #254231 - Flags: approval1.8.0.10?
Attachment #254228 - Flags: approval1.8.1.2?
Attachment #254228 - Flags: approval1.8.0.10?

Comment 28

11 years ago
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

11 years ago
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.
(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 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!
Attachment #254231 - Flags: superreview?(bzbarsky) → superreview+

Comment 32

11 years ago
Created attachment 254235 [details] [diff] [review]
use microseconds in seed

Oh, lovely.  Sorry for the confusion, then, and yay unix.  Let's use microseconds.
Attachment #254231 - Attachment is obsolete: true
Attachment #254235 - Flags: superreview?(bzbarsky)
Attachment #254235 - Flags: review?(dveditz)
Attachment #254235 - Flags: approval1.8.1.2?
Attachment #254235 - Flags: approval1.8.0.10?
Attachment #254231 - Flags: review?(dveditz)
Attachment #254231 - Flags: approval1.8.1.2?
Attachment #254231 - Flags: approval1.8.0.10?
Attachment #254235 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 254235 [details] [diff] [review]
use microseconds in seed

r=dveditz

approved for 1.8/1.8.0 branches, a=dveditz
Attachment #254235 - Flags: review?(dveditz)
Attachment #254235 - Flags: review+
Attachment #254235 - Flags: approval1.8.1.2?
Attachment #254235 - Flags: approval1.8.1.2+
Attachment #254235 - Flags: approval1.8.0.10?
Attachment #254235 - Flags: approval1.8.0.10+

Comment 34

11 years ago
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
Keywords: fixed1.8.0.10, fixed1.8.1.2
OS: Mac OS X → All
Hardware: PC → All
Whiteboard: needs landing on 1.8 and 1.8.0 branches
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....
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

11 years ago
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?
Sounds pretty good to me!

Comment 39

11 years ago
(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

11 years ago
Letting go of this bug for now.
Assignee: crowder → file-handling
Status: ASSIGNED → NEW
Flags: blocking1.9?
Whiteboard: [sg:low]

Comment 41

10 years ago
We're going to need at least the nasty hack on trunk as well. Shawn, can you take this?
Assignee: file-handling → comrade693+bmo
Flags: blocking1.9? → blocking1.9+
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.
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)
> 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.
US export regulations?

Comment 46

10 years ago
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.
They still affect several countries.
They affect them by prohibiting all export to them, whether with or without crypto.
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Even without crypto? wow.

Comment 50

10 years ago
(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...)
(Assignee)

Updated

10 years ago
Target Milestone: --- → mozilla1.9 M9
(Assignee)

Comment 51

10 years ago
Created attachment 280225 [details] [diff] [review]
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.
(Assignee)

Comment 52

10 years ago
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 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.
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.  
(Assignee)

Comment 55

10 years ago
Created attachment 280257 [details] [diff] [review]
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!
Attachment #280225 - Attachment is obsolete: true
Attachment #280257 - Flags: review?(nelson)
Attachment #280257 - Flags: review?(cbiesinger)
Attachment #280257 - Flags: review?(benjamin)
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.
Attachment #280257 - Flags: review?(nelson) → review+
(Assignee)

Comment 57

10 years ago
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.
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.  
(Reporter)

Updated

10 years ago
Whiteboard: [sg:low] → [sg:low] stepping stone

Comment 59

10 years ago
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?
Attachment #280257 - Flags: review?(benjamin) → review-
> Does NSS need too be initialized for any of these calls?
Yes, I mentioned that in comment 43.
(Assignee)

Comment 61

10 years ago
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.
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
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.
(Assignee)

Comment 64

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

Updated

10 years ago
Attachment #280257 - Flags: review?(cbiesinger)

Updated

10 years ago
Target Milestone: mozilla1.9 M9 → ---
Targeting M9 as this bug will block beta.
Target Milestone: --- → mozilla1.9 M9
(Assignee)

Comment 66

10 years ago
I still haven't heard back from Kai as to what he wants me to do, so I'm blocked on that.
(Assignee)

Comment 67

10 years ago
Got an e-mail from Kai - I hope to have a patch for this within a week.
(Assignee)

Comment 68

10 years ago
Created attachment 283075 [details] [diff] [review]
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...
Attachment #280257 - Attachment is obsolete: true
Attachment #283075 - Flags: review?(kengert)

Comment 69

10 years ago
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).
Attachment #283075 - Flags: review?(kengert) → review-
(Assignee)

Comment 70

10 years ago
Created attachment 283303 [details] [diff] [review]
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 :/
Attachment #283075 - Attachment is obsolete: true
Attachment #283303 - Flags: review?(kengert)
(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?




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.
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).  
(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.
(Assignee)

Comment 75

10 years ago
Created attachment 283383 [details] [diff] [review]
security/ code v1.2
Attachment #283303 - Attachment is obsolete: true
Attachment #283383 - Flags: review?(kengert)
Attachment #283303 - Flags: review?(kengert)
(Assignee)

Comment 76

10 years ago
Created attachment 283385 [details] [diff] [review]
uriloader/ code v2.0
Attachment #283385 - Flags: superreview?(cbiesinger)
Attachment #283385 - Flags: review?(cbiesinger)
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

>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

10 years ago
(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

10 years ago
(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

10 years ago
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.
Attachment #283383 - Flags: review?(kengert) → review-

Comment 82

10 years ago
(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.
(Assignee)

Comment 83

10 years ago
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.
(Assignee)

Comment 84

10 years ago
Created attachment 283739 [details] [diff] [review]
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.
Attachment #283383 - Attachment is obsolete: true
Attachment #283739 - Flags: superreview?(cbiesinger)
Attachment #283739 - Flags: review?(kengert)
Attachment #283739 - Flags: review?(cbiesinger)
(Assignee)

Comment 85

10 years ago
Created attachment 283740 [details] [diff] [review]
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.
Attachment #283385 - Attachment is obsolete: true
Attachment #283740 - Flags: superreview?(cbiesinger)
Attachment #283740 - Flags: review?(cbiesinger)
Attachment #283385 - Flags: superreview?(cbiesinger)
Attachment #283385 - Flags: review?(cbiesinger)
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
Attachment #283739 - Flags: superreview?(cbiesinger) → superreview+
Attachment #283739 - Flags: review?(cbiesinger) → review+
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
Attachment #283740 - Flags: superreview?(cbiesinger)
Attachment #283740 - Flags: superreview+
Attachment #283740 - Flags: review?(cbiesinger)
Attachment #283740 - Flags: review+
(Assignee)

Comment 88

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

Comment 89

10 years ago
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

10 years ago
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

10 years ago
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

10 years ago
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.
Attachment #283739 - Flags: review?(kengert) → review-

Comment 93

10 years ago
(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

10 years ago
Created attachment 284241 [details] [diff] [review]
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

10 years ago
Created attachment 284242 [details] [diff] [review]
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.
(Assignee)

Comment 96

10 years ago
Created attachment 284405 [details] [diff] [review]
security/ code v1.5
Attachment #283739 - Attachment is obsolete: true
Attachment #284405 - Flags: superreview?(rrelyea)
Attachment #284405 - Flags: review?(kengert)
(Assignee)

Comment 97

10 years ago
Created attachment 284406 [details] [diff] [review]
uriloader/ code v2.3
Attachment #283740 - Attachment is obsolete: true
Attachment #284241 - Attachment is obsolete: true
Attachment #284242 - Attachment is obsolete: true
Attachment #284406 - Flags: superreview?(cbiesinger)
Attachment #284406 - Flags: review?(cbiesinger)
(Assignee)

Updated

10 years ago
Attachment #284405 - Attachment description: security/ code v1.4 → security/ code v1.5
(Assignee)

Comment 98

10 years ago
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

10 years ago
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.
Attachment #284405 - Flags: review?(kengert) → review+
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?
Attachment #284406 - Flags: superreview?(cbiesinger)
Attachment #284406 - Flags: superreview+
Attachment #284406 - Flags: review?(cbiesinger)
Attachment #284406 - Flags: review+

Comment 101

10 years ago
(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

10 years ago
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
Attachment #284405 - Flags: superreview?(rrelyea) → superreview+
(Assignee)

Comment 103

10 years ago
Created attachment 284619 [details] [diff] [review]
security/ code v1.6

This just fixes the bitrot that occurred.
Attachment #284405 - Attachment is obsolete: true
(Assignee)

Comment 104

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
(Assignee)

Comment 105

10 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
It was not this code, the test was at fault since it relies on external resources and failed again later (bug 399611)
(Assignee)

Comment 107

10 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Depends on: 402298
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.