466745 - random number generator fails on windows ce

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Brad Lassey [:blassey] (use needinfo?)]

When navigating to any site over https in fennec on windows ce, we get a page load error and a pop up that reads:

SSL experienced a failure of its random number generator.

(Error code: ssl_error_generate_random_failure)

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Which one of you Fennec folks want to take this bug?

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

I suggest going and looking on the old WINCE branch to see how the PRNG 
was made to work there.  I no longer have a WinCE platform on which to try.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

It is failing here:

http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/freebl/prng_fips1861.c#506

506     if (rng->seedCount < MIN_SEED_COUNT) {
507         PZ_Unlock(rng->lock);
508         PORT_SetError(SEC_ERROR_NEED_RANDOM);
509         return SECFailure;
510     }


MIN_SEED_COUNT = 1024, rng->seedCount = 64

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

The PRNG needs to be "seeded" with an adequate amount of unpredictable stuff.
NSS presently requires 1K byte of it.  Depending on the source, 64-bytes is
probably not *nearly* enough.  I remember when I did a WinCE port of NSS about 
5 years ago (WINCE branch), I rewrote the seed/entropy gathering code for 
WinCE.  You can see that at either of these URLs
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=win_rand.c&branch=NSS_WINCE_ALPHA_BRANCH&root=/cvsroot&subdir=mozilla/security/nss/lib/freebl&command=DIFF_FRAMESET&rev1=1.4&rev2=1.4.2.1
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=win_rand.c&branch=&root=/cvsroot&subdir=mozilla/security/nss/lib/freebl&command=DIFF_FRAMESET&rev1=1.4&rev2=1.5

I guess that stuff is in the source you're using now, if your build defines
_WIN32_WCE, but maybe some things have changed, such as directory names, so 
that that code doesn't find the things it found back in 2002.

See also 
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=win_rand.c&branch=NSS_3_11_BRANCH&root=/cvsroot&subdir=mozilla/security/nss/lib/freebl&command=DIFF_FRAMESET&rev1=1.9.28.1&rev2=1.9.28.2
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=win_rand.c&branch=&root=/cvsroot&subdir=mozilla/security/nss/lib/freebl&command=DIFF_FRAMESET&rev1=1.9&rev2=1.10
which works around a bug in some versions of the MS CRT.

Wan-Teh wrote a patch to use the OS's native PRNG as a source of seed for 
NSS's PRNG.  You can see it at 
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=win_rand.c&branch=NSS_3_11_BRANCH&root=/cvsroot&subdir=mozilla/security/nss/lib/freebl&command=DIFF_FRAMESET&rev1=1.9&rev2=1.9.28.1
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=win_rand.c&branch=&root=/cvsroot&subdir=mozilla/security/nss/lib/freebl&command=DIFF_FRAMESET&rev1=1.10&rev2=1.11
If you can get it to build and run on WinCE, that's probably your best bet 
for seeding the PRNG.

I suspect that most of all of those patches are in the trunk code base you're
using.  You might have to tweak some ifdefs though.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: changes to use the same path as desktop

The root cause of this problem is a broken file io in nspr. I'll file a seperate bug for that.

However it seems like it would be better to use the same code path on windows ce as we do on windows desktop.  This patch does that as much as possible.

Also, we were seeding with the result of GetTickCount twice, which isn't very random.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: just the nss patch

Sorry, that previous patch had the changes to nspr and build as well

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 350136 [details] [diff] [review]
just the nss patch

Preliminary comment:

We claim that NSS can be built with gcc on windows desktops.
Now, I'm not sure, but I think this L"string" construct used in this patch 
is something that only works with Microsoft's compilers, not with gcc.  
If I'm right, then this patch would have the effect that NSS would no 
longer build with gcc on Windows desktops.  That would be bad.  
If I'm wrong, and you can point to something semi-authoritative that says 
that gcc supports it, please let me know.

>-    hModule = LoadLibrary("advapi32.dll");
>+    hModule = LoadLibraryW(L"advapi32.dll");

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Besides the forgoing comment, my only other concern with this patch is 
that I think it is going to get a LOT less entropy than the code it's
replacing did.  The code it's replacing examined files in 3 directories:
"\\Windows\\Temporary Internet Files", "\\Temp", "\\Windows"
(admittedly it used the wrong method to get those path names).
The patch would reduce that to only the content of one directory, 
which I gather is the localized directory name equivalent to \\Windows.  
I seriously doubt there's enough entropy there to be worthwhile.  

The crucial factor in this data gathering it to strive for unpredictability.
Reading in the contents of unchanging files is of no value whatsoever.  
It would be a serious vulnerability on WinCE if the PRNG seeing was not
sufficiently random.  We're looking for ~1k bit of entropy, and are reading
at least 1K byte to try to get it.  

If you have more ideas for gleaning entropy from the system, please share 
them here.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

may I suggest we get the entropy from some combination of CSIDL_BITBUCKET, CSIDL_FAVORITES, CSIDL_MYMUSIC, CSIDL_MYPICTURES, CSIDL_APPDATA, CSIDL_RECENT, CSIDL_DESKTOPDIRECTORY, CSIDL_MYVIDEO, and CSIDL_STARTUP.

CSIDL_BITBUCKET is the recycling bin, I think the rest are self explanatory.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Also, I think its important to do the same thing on windows ce as we do on desktop.  So I suggest we check those directories on desktop as well.  The trade off is the time we spend reading these in, so perhaps we stop at some threshold.

The last directory we read should be the system directory, because as you said it will be the most consistent of these directories.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

How about contents of the browser's cache directory?  That always struck me
as the best source of entropy in files.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

that does seem like a good place to find entropy, but I can't find a language independent way to get that directory.  If we hard code the path, my concern is that on a non-english device we'll be looking in the wrong directory and finding no files.

I think the recycling bin, my music and my pictures would all have a lot of entropy, assuming the user is using the device (I think that's an assumption for all of these though).  Recent documents should be good too.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v2

conditional on __GNUC__, we either use L"" or just "" for the wide char string literal.

Also, this reads in the files in the 10 folders I enumerated before for both windows desktop and windows ce.  Is there anyway to actually measure/test the amount of entropy we're finding here?

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

> Is there anyway to actually measure/test the amount of entropy we're finding 
> here?

A technique I used in the past, the last time I did significant work on the 
PRNG seeding, was to have the algorithm record all the bits that it would 
have fed into the PRNG (except for the contents of system files) into a file 
instead.  Execute it several times, producing several files of output.  
The compute the Hamming distance between those files, pairwise.  (Xor the 
contents of two of them and count the number of 1 bits in the result.) 
Average the results. This gives you an optimistic upper bound on the amount 
of entropy produced, on average, by the algorithm. 

I'll write more about considering the value of the file contents later.

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: hamming distances

I did what you suggested on both windows mobile (an emulator) and windows xp, generating 10 files back to back, comparing them all.  The results are attached.

There's obviously something wrong with the implementation for desktop, I'll debug that.  Do the seeds for windows ce look sufficient?

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Brad, thanks for doing that.  It was done long ago, but has not been 
repeated for a long time.  I am distressed by the results. 
They are unacceptably small (IMO) for Win XP, and seem absurdly high 
for WinCE.  (did you include the file contents in one but not the other?)

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

what I did was open a file in RNG_RNGInit, append *data to it in prng_RandomUpdate and close it in RNG_RNGShutdown.  I then wrote this test program:

int _tmain(int argc, _TCHAR* argv[])
{

        RNG_RNGInit();
        RNG_SystemInfoForRNG();

        RNG_RNGShutdown();
        return 0;
}

I suspect that on win XP we're not getting the "right" folders to look in.  If so, that should be easy to fix.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

> I'll write more about considering the value of the file contents later.

One of the techniques used on some platforms to obtain bits to feed into 
the PRNG is to chooses a subset of files in one or more directories and 
feed the contents of the selected files into the PRNG.   

In a system where the contents of the directory being sampled (both the set
of files and the contents of the respective files) are very dynamic, and 
change frequently, this technique can contribute real entropy.  But in a 
system where the contents of the directories being samples are essentially
static, such as in a directory of system executables and shared libraries,
it is pretty worthless.  In such systems, all the entropy actually comes
from the algorithm by which the files are selected, not from the file 
contents.  Each file effectively offers no more than one bit of entropy,
and the entropy really comes from the question of whether a certain file
is chosen or not, not from its contents.  The number of bits of entropy
in that scheme in no more than n, and *may* be no more than log2(n) where 
n is the number of files selected.  

So, I suspect that the number of bits of entropy gathered from directories
such as Favories, MY vidio/audio/pictures, and especially startup, is 
simply too low to be worth much.  That is why I recommended that file 
contents be omitted from the hamming distance computations above.  
When a system is resorting to file contents for entropy, it's really 
desparate for entropy.   

On all windows systems (XP, CE, and what all) I think our best bet is to 
get a buffer of randomized data from the system's own PRNG, if possible.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

So do you suggest we replace the ReadSystemFiles call in RNG_SystemInfoForRNG for windows with a call to CryptGenRandom?  Or just restrict the system files call to temp folders and the bit bucket and supplement it with CryptGenRandom?

I'll remove the system files and re-run the test.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Look at function RNG_SystemRNG.  It checks to see if the local system
possesses the shared library necessary for CryptGenRandom.  That DLL is
not present on all Windows systems.  So, NSS attempts to use it if present
but not if not.  

The use of files should probably be viewed as a fall-back, only as a 
desperate act when CryptGenRandom is not present.

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: more data

this file has hamming distances for seeds produced by:
windows xp w/ temp files
windows xp w/o temp files
windows ce emulator w/ temp files
windows ce emulator w/o temp files
windows ce device w/ temp files

the temp files are:
temp dir (returned by GetTempPath())
recycle bin (CSIDL_BITBUCKET)
recent docs (CSIDL_RECENT)
temp internet files (CSIDL_INTERNET_CACHE)
net neighbors (CSIDL_COMPUTERSNEARME)
internet history (CSIDL_HISTORY)

a couple notes:

I fixed a bug in the file reading code for windows desktop, and you can see a much larger distance between the seeds from the first run I posted.

windows xp w/o tmp files, and both emulator runs show about the same distances.  I assume the emulator isn't finding any randomized files.

On the windows ce device runs 1-7 happened in rapid succession.  There was more of a pause between 0 -> 1 and 7-> 8-> 9.  That pause seemed to effect the results.

The recycling bin's path is being returned as "" on both platforms, which I ignore.

Also, windows ce finds files in only 2 of the 5 directories that windows desktop does because the last 3 CSIDL values are defined on windows ce (and we skip the invalid recycling bin).

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 352514 [details]
more data

Nelson, do these numbers look good enough to you?

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Brad, none of the numbers below 1K look good enough to me. :(
I'd prefer to see numbers in the 2-4k range, at least

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Let me restate comment 23 in a way that doesn't sound like a mere whim.
There is a view that a PRNG should be seeded with an amount of truly random
entropy that exceeds the size of the largest key that will be generated 
from the output of that PRNG.  By that measure, most of the sets of numbers
gathered here don't contain enough seed entropy to generate a mere 1k bit
RSA key, much less a 2k bit key or 3k or 4k bit key.

Comment 25

[Brad Lassey [:blassey] (use needinfo?)]

Nelson, without the system files windows desktop doesn't meet that standard.  Perhaps this needs to be looked at more closely by someone familiar with the PRNG.

Comment 26

[Brendan Eich [:brendan]]

Seems like a Windows NSS bug, not just a CE bug as the OS field says. File a blocking bug and please cc: all here on it ("Clone This Bug" link at the bottom of this bug may do the right thing) -- I hope Nelson can recommend someone to take it.

/be

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

"Someone familiar with the PRNG".  That would be me.  

BTW, we're rewriting the PRNG (see bug 457045) and with it, the minimum 
number of acceptable entropy bits will be 384 bits.  Your set of numbers 
labeled "windows ce device, with temporary files" meets that criteria.  
However, some changes to the code for WinCE will undoubtedly be needed to
work with the new PRNG.

Comment 28

[Brad Lassey [:blassey] (use needinfo?)]

Nelson,  right now we crash on windows mobile as soon as we navigate off the first run page due to this, which is blocking our release.  Do you want to take the changes to how we read system files?  Another option is the CeGetRandomSeed function which returns a 64 bit number based on kernel activity.

http://msdn.microsoft.com/en-us/library/aa450794.aspx

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 352514 [details]
more data

The results shown in the table named 

>windows ce device, with temporary files:

are acceptable for Win CE.

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 350287 [details] [diff] [review]
patch v2

Produce a version of this patch without the NSS_L hack
and I think it is likely to pass review.

Comment 31

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch without NSS_L

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

Brad, please attach the source to your test program, the one with which
you produced the results in attachment 352514 [details].  I want to try to find tune
the set of temporary files searched on Windows desktop so that it produces
adequate but not excessive results.

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Brad's latest patch, converted to CVS diff, NSS coding style, removed Win16 code

This patch is Brad's patch converted to a CVS diff.
I also changed the patch to use NSS's indentation style, (indent 4, tab=8),
and I removed all the old Win16 code, because that old code would not 
compile with Brad's patch applied, and we don't support Win16 any more.

Brad, 
please confirm that this patch faithfully incorporates your latest patch.

I would give this patch r+, but since I have contributed to it, I will ask
another NSS owner/peer to review it.

Comment 34

[Nelson Bolyard (seldom reads bugmail)]

But before I ask for review on my version of this patch, I want to see if
the set of directories being used can be reduced and still produce 
acceptable results.

Comment 35

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: instrements to record seed data to file

Comment 36

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: produces hamming distance between a set of files

you can use this to compare two files with "hamming_dist file1 file2" or for a set of files with "hamming_dist -p <pattern>."  The format of pattern is the format used by FindFirstFile, documented here:

http://msdn.microsoft.com/en-us/library/aa364418%28VS.85%29.aspx

Comment 37

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Another variant, stops at size limit

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Some people in browser land have a benchmark that measures start-up time,
and they're very sensitive to changes that increase that time.  I think
we should ask them to test one or both of these last two patches and see
if they are going to object strenuously to either one.  But I don't know
who to ask.  Maybe one of the folks CC'ed on this bug knows who to ask?

Comment 39

[Brad Lassey [:blassey] (use needinfo?)]

Here are the results from a try server run:

Windows:
      twinopen: 257.42
      ts: 1156.53
      tp: 431.02
      tp_memset: 62.5MB
Linux:
      twinopen: 195.74
      ts: 1183.68
      tp: 420.05
      tp_RSS: 79.4MB
OS X:
      twinopen: 499.63
      ts: 1177.95
      tp: 298.55
      tp_RSS: 300.4MB

Talos didn't pick up my baseline build, but the Windows Ts numbers on the try server for the last 3 days range from 1153.37 - 1206.21, so it looks like this doesn't adversely effect Ts on Windows.

Comment 40

[Wan-Teh Chang]

Brad,

Nelson's comment 20 is right on.  Please make sure that
RNG_SystemRNG can call CryptGenRandom successfully on
Windows Mobile.  You'll need to port the code to Windows
Mobile because the DLL's name is coredll.dll and I suspect
the function name CryptAcquireContextA won't work (note
the "A" suffix).  The MSDN page for CryptGenRandom also
mentions CeGenRandom, which you should look into.  I
guess we should try to use CeGenRandom first, and then
CryptGenRandom.  If CeGenRandom always exists, then we
don't need the CryptGenRandom fallback:
http://msdn.microsoft.com/en-us/library/aa923614.aspx
http://msdn.microsoft.com/en-us/library/aa912301.aspx

Your comment 28 mentioned CeGetRandomSeed.  I think it
is not as good as CeGenRandom.

Comment 41

[Nelson Bolyard (seldom reads bugmail)]

Bob, this is the other bug that is trying to modify the PRNG seeding 
concurrently with your bug 457045

Comment 42

[Robert Relyea]

I agree with wtc's comments.  The new code is more dependent on RNG_SystemRNG than the old code was.

I suspect it will be easier to merge my changes into this patch than the other way around. I'm glad to see this patch is so close. It may be best to land the patch as is and then add the CeGenRandom.

We were hoping for FIPS feature complete, which requires the PRNG to land. I think the CeGenRandom can go in before FIPS code complete (hopefully next month).

bob

Comment 43

[Robert Relyea]

Comment on attachment 358769 [details] [diff] [review]
Brad's latest patch, converted to CVS diff, NSS coding style, removed Win16 code

r+

I'd like to see the CeGetRandom put in, but I believe this should work right now.

bob

Comment 44

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP patch - system rng

This patch successfully gets pointers to the three dynamically loaded functions.  However, when calling pCryptAcquireContext, I get the following error and crash:

Virtual Memory overlap! CopyRegions failed to reserve memory in proc 'xulrunner.exe' (0x804580A0) for module 'rsaenh.dll' @ 0x1F8B0000!

My reading of the msdn docs is that you should attempt to load Crypt* methods and use CeGenRandom as a fallback.  Given this error though, perhaps we should just jump to using CeGenRandom directly.

Comment 45

[Wan-Teh Chang]

Comment on attachment 359773 [details] [diff] [review]
WIP patch - system rng

For RNG_SystemRNG, WinMobile is different enough from Windows
Desktop that I think it's better to have separate implementations
of the entire function, rather than using ifdefs inside the
function.

Since Fennec is already linked with coredll.dll, you can use
those Crypt* and CeGenRandom functions directly, rather than
doing LoadLibraryW and GetProcAddress.

Replace the pRtlGenRandom code by CeGenRandom for WinMobile.
If you know CeGenRandom always exists, skip the Crypt* code
for WinMobile.

Comment 46

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: RNG_SystemRNG uses CeGenRandom directly

Comment 47

[Nelson Bolyard (seldom reads bugmail)]

Attachment: combined patch

This patch combines the patch that Bob Relyea previously gave r+ with 
Brad's latest patch to use CeGenRandom directly.  

Brad, if this is the patch you want, I'm willing to give it r+ now.

Comment 48

[Brad Lassey [:blassey] (use needinfo?)]

this looks right to me.  

At some point I'd like to sort out why the Crypt* APIs cause a crash, as the msdn docs lead me to believe they will give a "better" random number. But that is more of an enhancement and can be done in a follow up bug.

Comment 49

[Wan-Teh Chang]

Comment on attachment 359791 [details] [diff] [review]
RNG_SystemRNG uses CeGenRandom directly

Brad: it's better to move the #ifdef WINCE to the outside
of the RNG_SystemRNG function.  The only code the two
implementations share is the single line:
    size_t bytes = 0;

This discussion thread has some info on the relation between
CeGenRandom and CryptGenRandom:
http://www.pocketpcjunkies.com/Uwe/Forum.aspx/wince-pb/4913/What-seeds-CeGenRandom

Also, page 13 of WinCE's FIPS Security Policy:
http://csrc.nist.gov/groups/STM/cmvp/documents/140-1/140sp/140sp825.pdf
shows that CeGenRandom is exactly the function to be use in
RNG_SystemRNG -- for generating the seeding material for
a RNG.

Comment 50

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 359806 [details] [diff] [review]
combined patch

r=nelson
I plan to commit this patch.  Further refinements can be done in subsequent patches, perhaps in subsequent bugs.

Comment 51

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 359791 [details] [diff] [review]
RNG_SystemRNG uses CeGenRandom directly

This patch is ok with me, as part of the larger patch.

Comment 52

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: updated based on wtc's comments

(In reply to comment #49)
> (From update of attachment 359791 [details] [diff] [review])
> Brad: it's better to move the #ifdef WINCE to the outside
> of the RNG_SystemRNG function.  The only code the two
> implementations share is the single line:
>     size_t bytes = 0;

Done. Moved it into the same #ifdef/#else block as RNG_FileForRNG
 
> This discussion thread has some info on the relation between
> CeGenRandom and CryptGenRandom:
> http://www.pocketpcjunkies.com/Uwe/Forum.aspx/wince-pb/4913/What-seeds-CeGenRandom
> 
> Also, page 13 of WinCE's FIPS Security Policy:
> http://csrc.nist.gov/groups/STM/cmvp/documents/140-1/140sp/140sp825.pdf
> shows that CeGenRandom is exactly the function to be use in
> RNG_SystemRNG -- for generating the seeding material for
> a RNG.

Thanks, looks like there is no need for a follow up bug then.

Comment 53

[Nelson Bolyard (seldom reads bugmail)]

Attachment: combined patch including Brad's latest change (checked in)

I'm going to test this, and if it passes my test, I plan to commit it today.

Comment 54

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 359814 [details] [diff] [review]
updated based on wtc's comments

Brad,
Question: in what header file, if any, is CeGenRandom declared?
Is that file included in this version of win_rand.c?

Comment 55

[Wan-Teh Chang]

The MSDN page for CeGenRandom
(http://msdn.microsoft.com/en-us/library/aa912301.aspx)
says it's declared in winbase.h.  So we should already
have included that header (via windows.h).

Comment 56

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 360136 [details] [diff] [review]
combined patch including Brad's latest change (checked in)

Committed on trunk.
Checking in config.mk;  new revision: 1.22; previous revision: 1.21
Checking in win_rand.c; new revision: 1.16; previous revision: 1.15

Comment 57

[Brad Lassey [:blassey] (use needinfo?)]

Nelson, can you push to mozilla-central as well?

Comment 58

[Wan-Teh Chang]

Attachment: Cite the WinCE FIPS Security Policy for CeGenRandom

Remove an unnecessary memset call.  Add a comment
that cites official Microsoft documnentation of
the appropriateness of using CeGenRandom.

Nelson, please review this patch.

Brad:
- Please verify (in the debugger or using printf statements)
  that the CeGenRandom call succeeds.  This is the most
  important call in our entropy collection code.
- Please ask Kai Engert to push a new NSS snapshot to
  mozilla-central.  Nelson doesn't do that.

Comment 59

[Brad Lassey [:blassey] (use needinfo?)]

CeGenRandom is returning 1024 bytes of data.

Comment 60

[Brad Lassey [:blassey] (use needinfo?)]

Kaie, can you push these changes to mozilla-central?

Comment 61

[Nelson Bolyard (seldom reads bugmail)]

Brad,
The NSS team's policy is that we only support NSS builds made from sources
that match tags that the NSS team has put into its own CVS tree.  
We regard all other trees (than our CVS repository) to be downstream trees.
That includes Mozilla-Central. We do not maintain downstream trees.

Because Mozilla desires that the NSS team will support the versions of NSS
that it uses, Mozilla generally does not "push" individual patches for NSS
to its Hg repositories, but rather pushes snapshots that correspond to tags
in NSS's CVS repository.  

In keeping with those facts, your request is equivalent to asking for a 
tag to be generated in the NSS CVS tree, for the purpose of being the base
for such a snapshot.  We just did a snapshot in the last week or two, IIRC.
More changes are coming soon.  I guess it's up to Kai to decide how 
frequently he wants to make and push new tags/snapshots.

Comment 62

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 360180 [details] [diff] [review]
Cite the WinCE FIPS Security Policy for CeGenRandom

r=nelson for Wan-Teh's change.

Comment 63

[Wan-Teh Chang]

Comment on attachment 360180 [details] [diff] [review]
Cite the WinCE FIPS Security Policy for CeGenRandom

I checked in this patch on the NSS trunk (3.12.3).

Checking in win_rand.c;
/cvsroot/mozilla/security/nss/lib/freebl/win_rand.c,v  <--  win_rand.c
new revision: 1.17; previous revision: 1.16
done

Comment 64

[Nelson Bolyard (seldom reads bugmail)]

Note that the old code for WinCE, which was just replaced by Brad's patch,
did a RECURSIVE walk through all subdirectories of the directories named
at the top level.  The new patch does not, but only walks through the 
direct contents of the named directories.  This has significant impact on
the entropy gained from some directories, such as the directory of 
"Temporary Internet Files" (CSIDL_INTERNET_CACHE) and history (CSIDL_HISTORY)
because those directories generally contain only one member, a subdirectory,
in which all the interesting files are found.  For example, the directory
found via CSIDL_INTERNET_CACHE contains just one subdirectory named 
Content.IE5 (or similar, depending on version of IE).  

So, this recent patch actually reduces the amount of entropy gathered from 
those directories compared to the presious version of the code for WinCE.

I will leave it up to Brad to determine if the present situation is 
satisfactory for WinCE.  Brad, if you're happy with the code as it now is
in the CVS trunk, please mark this bug resolved/fixed.
I will pursue this for Windows desktop in bug 475090.

Comment 65

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: recurses into directories

(In reply to comment #64)
> Note that the old code for WinCE, which was just replaced by Brad's patch,
> did a RECURSIVE walk through all subdirectories of the directories named
> at the top level.  The new patch does not, but only walks through the 
> direct contents of the named directories.  This has significant impact on

No reason not to recurse into the subdirectories, this patch will add that back.

> the entropy gained from some directories, such as the directory of 
> "Temporary Internet Files" (CSIDL_INTERNET_CACHE) and history (CSIDL_HISTORY)
> because those directories generally contain only one member, a subdirectory,
> in which all the interesting files are found.  For example, the directory
> found via CSIDL_INTERNET_CACHE contains just one subdirectory named 
> Content.IE5 (or similar, depending on version of IE).  

Note that the CSIDL_INTERNET_CACHE and CSIDL_HISTORY aren't defined on windows ce.  Fenenc uses the temp directory for its cache, which as it turns out is an excellent source of entropy once the browser has been run a bit.

> 
> So, this recent patch actually reduces the amount of entropy gathered from 
> those directories compared to the presious version of the code for WinCE.
> 
> I will leave it up to Brad to determine if the present situation is 
> satisfactory for WinCE.  Brad, if you're happy with the code as it now is
> in the CVS trunk, please mark this bug resolved/fixed.
> I will pursue this for Windows desktop in bug 475090.

My impression is that with the inclusion of the recursion patch we will be better off than we were before in terms of entropy.  With that reviewed and committed, I'll close the bug.

Comment 66

[Wan-Teh Chang]

Attachment: recurses into directories v2

I edited Brad's patch (attachment 360254 [details] [diff] [review]) and checked it
in on the NSS trunk (NSS 3.12.3).

Checking in win_rand.c;
/cvsroot/mozilla/security/nss/lib/freebl/win_rand.c,v  <--  win_rand.c
new revision: 1.18; previous revision: 1.17
done

Comment 67

[Wan-Teh Chang]

Marked the bug fixed.

Comment 68

[Nelson Bolyard (seldom reads bugmail)]

uh, not yet.   The old code that recursed into subdirectories had some 
features to limit the amount of data it took in from that recursion,
without which it can conceivably take a LONG time to complete.  
Brad's new patch (now committed) doesn't employ any such limit.  
We need such limits, IMO.

Comment 69

[Slavomir Katuscak]

Wan-Teh, Tinderbox on goride machine fails on building with this patch, can you please look there ?

Comment 70

[Wan-Teh Chang]

Comment on attachment 360655 [details] [diff] [review]
recurses into directories   v2

Sorry that I broke the build.  This patch caused shlibsign
to crash while signing softokn3.dll.  I backed out this patch
(but kept the "shlobj.h" to <shlobj.h> change).

Checking in win_rand.c;
/cvsroot/mozilla/security/nss/lib/freebl/win_rand.c,v  <--  win_rand.c
new revision: 1.19; previous revision: 1.18
done

shlibsign crashed after running for a long time, so I
guess it was an infinite recursion.

Comment 71

[Kai Engert (:KaiE:)]

(In reply to comment #60)
> Kaie, can you push these changes to mozilla-central?

If I understand correctly, this bug is not yet fixed, so it's not yet helpful to push a new tag.

Comment 72

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: honors return value of func to stop recursion

Comment 73

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 360783 [details] [diff] [review]
honors return value of func to stop recursion

The real problem with this patch (and with its predecessor) is that it will recurse with values of '.' and '..'.  That's why it went into an apparently infinite loop.  The code must take care not to recurse with those two values.

Comment 74

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: returns early for path's ending in "\.", "/." or ".."

Also, this patch changes the spacing of this method to match file style

Comment 75

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 360798 [details] [diff] [review]
returns early for path's ending in "\.", "/." or ".."


>+    do {
>+      // pass the full pathname to the callback
>+      _snwprintf(szFileName,_MAX_PATH, L"%s\\%s", szSysDir, fdData.cFileName);
>+      WideCharToMultiByte(CP_ACP, 0, szFileName, -1, 
>+                          narrowFileName,_MAX_PATH,0,0);
>+      if (fdData.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
>+        EnumSystemFilesInFolder(func, szFileName);
>+      else
>+        iStatus = (*func)(narrowFileName)?0:FindNextFileW(lFindHandle, &fdData);
>+    } while (iStatus != 0);

In the code path that calls EnumSystemFilesInFolder, iStatus is left 
uninitialized when it reaches the final while statement.  Also, in 
that code path, nothing calls FindNextFileW, so if iStatus happens to 
be non-zero, the value of fdData.cFileName will never change.

Comment 76

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 360136 [details] [diff] [review]
combined patch including Brad's latest change (checked in)

This patch removed a bunch of code that bore comments saying it was Win16 code. But the ifdef's for that code were typically based on _WIN32 not being defined.  I'm worried that OS/2 may have also dependend on that code, as well as Win16. :(

Comment 77

[Wan-Teh Chang]

Comment on attachment 360136 [details] [diff] [review]
combined patch including Brad's latest change (checked in)

OS/2 uses os2_rand.c instead.  win_rand.c is only used for
WIN32 and WIN16.

Comment 78

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch to test

I am about to test this patch.  Will request review if/when it passes.

Wan-Teh, That's very good news about OS/2!

Comment 79

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 360834 [details] [diff] [review]
patch to test

Wan-Teh, please review this small patch.

Comment 80

[Wan-Teh Chang]

Comment on attachment 360834 [details] [diff] [review]
patch to test

r=wtc.

>+typedef PRInt32 (* handler)(const char *);

The type name "handler" should be capitalized, otherwise it
looks like a variable name.

>+    int                 iSuccess;

This variable could use a better name.  It actually means
whether we should continue in the do-while loop.

>+    if (szSysDir[pathLen - 1] == L'.' &&
>+       (szSysDir[pathLen - 2] == L'\\' || 
>+        szSysDir[pathLen - 2] == L'/' || 
>+        szSysDir[pathLen - 2] == L'.' ))
>+        return;

Please add a comment to say we want to skip "." and "..".
You shouldn't need to check for L'/' because we construct
the pathname using L"%s\\%s".

>     do {
> 	// pass the full pathname to the callback
> 	_snwprintf(szFileName,_MAX_PATH, L"%s\\%s", szSysDir, fdData.cFileName);
> 	WideCharToMultiByte(CP_ACP, 0, szFileName, -1,
> 			    narrowFileName, _MAX_PATH, NULL, NULL);

The WideCharToMultiByte call should be moved to the else
block of the if statement you added, because narrowFileName
is only used in the else block.

Comment 81

[Peter Weilbacher]

Because Nelson pointed me to this bug, I tested NSS trunk on OS/2. It still works as well as before. (At least when built within mozilla-central which is what we mainly care about.)

Comment 82

[Wan-Teh Chang]

Comment on attachment 360834 [details] [diff] [review]
patch to test

Two more suggested changes:

1. Use the new "handler" type in:

160 static BOOL
161 EnumSystemFiles(PRInt32 (*func)(const char *))

2. Skip "." and ".." inside the do-while loop here:
      
>@@ -149,13 +162,18 @@ EnumSystemFilesInFolder(PRInt32 (*func)(
>     do {
> 	// pass the full pathname to the callback
> 	_snwprintf(szFileName,_MAX_PATH, L"%s\\%s", szSysDir, fdData.cFileName);

At the beginning of this do while loop, you can do something
like:

      do {
          // skip . and ..
          if (wcscmp(fdData.cFileName, L".") == 0 ||
              wcscmp(fdData.cFileName, L"..") == 0) {
              iSuccess = 1;
              continue;
          }

This avoids the issue with L'/'.

By the way, we should check the return value of the
WideCharToMultiByte call for failure.

Comment 83

[Brad Lassey [:blassey] (use needinfo?)]

nelson, are you going to commit this patch?

kaie, when this change has landed in cvs I'd like to get it tagged and pushed to hg.

Comment 84

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch, incorporate WTC's latest suggestions, and other cleanup

The changes Wan-Teh suggested produced an infinite loop, but I liked the
idea so I took it and made it work in this patch.  Wan-Teh, please review.

Comment 85

[Wan-Teh Chang]

Comment on attachment 361352 [details] [diff] [review]
patch, incorporate WTC's latest suggestions, and other cleanup

r=wtc.

>+    PRUint32            pathLen = wcslen(szSysDir);

It seems that 'pathLen' is not used and can be removed.

Comment 86

[Wan-Teh Chang]

Nelson, after you check this in, I can create the
NSS_3_12_3_BETA3 CVS tag and push it to mozilla-central.

Comment 87

[Nelson Bolyard (seldom reads bugmail)]

Thanks, Wan-Teh.  I removed pathLen and committed it.
Checking in win_rand.c; new revision: 1.20; previous revision: 1.19

Comment 88

[Nelson Bolyard (seldom reads bugmail)]

I consider this bug resolved now on the CVS trunk.
Reopen if you disagree.

Comment 90

[Wan-Teh Chang]

I pushed NSS_3_12_3_BETA3 (tagged this morning) to
mozilla-central in changeset 8334582740d7:
http://hg.mozilla.org/mozilla-central/rev/8334582740d7

Comment 91

[Brad Lassey [:blassey] (use needinfo?)]

This patch needs to be pushed to mozilla-191 as well.  Its still not entirely clear to me if this needs to be done by an NSS peer or not.  If this is the case, could one of the peers do that?  If not, please comment on the bug and I'll push the change.

Comment 92

[Wan-Teh Chang]

We can't push an NSS Beta to mozilla-191 because Firefox
3.1 is close to being final.  We need to make sure Firefox
3.1 final ships with an NSS final release.  If we are
committed to releasing NSS 3.12.3 RTM soon for Firefox 3.1,
then we can push a Beta to mozilla-191 first.

Comment 93

[Nelson Bolyard (seldom reads bugmail)]

The agreement is that the Hg repositories will contain snapshots of NSS
that match CVS tags created by the NSS team for this purpose (e.g. RTM tags
or Beta tags, etc.).  It's not entirely clear whose job it is to actually
push those fixes into the Hg repositories.  The only peers of the NSS 
team who use Hg at all appear to be Wan-Teh and Kai.  Personally, I'd 
prefer that the NSS team not be responsible for updating downstream 
repositories.  In any case, a new tag needs to be produced, and right 
now, we're working on fixing some vulnerabilities.  When we've completed
that work (soon) would be a good time to make another such tag.

Comment 94

[Brad Lassey [:blassey] (use needinfo?)]

Just for planning purposes, what is the time frame to have something that's worthy of being tagged and pushed to 1.9.1?

Comment 95

[Kai Engert (:KaiE:)]

I would like to remind the NSS team, until bug 474606 is resolved, the Firefox 3.1 drivers probably won't accept a newer NSS snapshot for Mozilla 1.9.1.

Comment 96

[Nelson Bolyard (seldom reads bugmail)]

In reply to Kai's comment 95,  there will soon be some vulnerability fixes
and I'm pretty sure drivers will want them regardless of bug 474606.

Comment 97

[Kai Engert (:KaiE:)]

As this is an NSS bug, I've filed bug 481968 to track landing a more recent NSS snapshot into mozilla-1.9.1 (equiv. Firefox 3.1)


(In reply to comment #96)
> In reply to Kai's comment 95,  there will soon be some vulnerability fixes
> and I'm pretty sure drivers will want them regardless of bug 474606.

Let's make sure that the Firefox 3.1 drivers are aware of this conflict (if Firefox wants security fixes, they must accept the regression bug 474606, should a workaround not yet be ready).

Comment 98

[Mike Beltzner [:beltzner, not reading bugmail]]

I've added a dependency on bug 481968 here. As I understand it from a quick reading, for this to land on mozilla-1.9.1 it will need the NSS Beta. If there's another way (creating an NSS tag with just the fix required by this bug) please let us know, and remove the dependency.

Comment 99

[[e-mail]]

(In reply to comment #68)
> uh, not yet.   The old code that recursed into subdirectories had some 
> features to limit the amount of data it took in from that recursion,
> without which it can conceivably take a LONG time to complete.  
> Brad's new patch (now committed) doesn't employ any such limit.  
> We need such limits, IMO.
Are there any such limits yet? From time to time I was running in to a slow script warning in the login manager code which turned out to be caused by trying to get the pk11tokendb service, which needed to initialise NSS, which was enumerating my IE cache. Clearing my IE cache resolved my problem.

Comment 100

[Nelson Bolyard (seldom reads bugmail)]

Nail, yes, the code now limits the depth into the directories into which
it will descend looking for files.  Look for "maxDepth" in the code.

Perhaps we also need a limit on the number of files and/or immediate 
subdirectories of a single directory through which we will search.

Comment 101

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Is there some special significance to depth here?  Why not just limit on the total number of files processed and then abort the DFS/BFS search when we hit that limit?

Comment 102

[Robert Sayre]

(In reply to comment #100)
> Nail, yes, the code now limits the depth into the directories into which
> it will descend looking for files.  Look for "maxDepth" in the code.
> 
> Perhaps we also need a limit on the number of files and/or immediate 
> subdirectories of a single directory through which we will search.

What are the cryptographic properties of these files?

Comment 103

[[e-mail]]

(In reply to comment #100)
> Nail, yes, the code now limits the depth into the directories into which
> it will descend looking for files.  Look for "maxDepth" in the code.
All Temporary Internet Files are stored at the same depth:
%USERPROFILE%\Local Settings\Temporary Internet Files\Content.IE5\<salt>\<file>

For some reason I now see the scan happening four times (as recorded by printf()s I've added to EnumSystemFiles).

Comment 104

[Nelson Bolyard (seldom reads bugmail)]

Would be nice to see the stacks.  I wonder if NSS is being repeatedly
initialized and uninitialized.

Comment 105

[[e-mail]]

(In reply to comment #104)
> Would be nice to see the stacks.
Seems to be caused by this block of code from pkcs11.c:

    rv = RNG_RNGInit();         /* initialize random number generator */
    if (rv != SECSuccess) {
	crv = CKR_DEVICE_ERROR;
	return crv;
    }
    rv = BL_Init();             /* initialize freebl engine */
    if (rv != SECSuccess) {
	crv = CKR_DEVICE_ERROR;
	return crv;
    }
    RNG_SystemInfoForRNG();

RNG_RNGInit() already calls RNG_SystemInfoForRNG() (via PR_CallOnce no less)!

>freebl3!RNG_SystemInfoForRNG(void)+0x223 [security\nss\lib\freebl\win_rand.c @ 358]
>freebl3!rng_init(void)+0x14c [security\nss\lib\freebl\drbg.c @ 420]
>nspr4!PR_CallOnce(struct PRCallOnceType * once = 0x0b938cf8, <function> * func = 0x0b8e73a0)+0x35 [nsprpub\pr\src\misc\prinit.c @ 808]
>freebl3!RNG_RNGInit(void)+0x13 [security\nss\lib\freebl\drbg.c @ 462]
>softokn3!RNG_RNGInit(void)+0x25 [security\nss\lib\freebl\loader.c @ 835]
>softokn3!nsc_CommonInitialize(void * pReserved = 0x0012d7d4, int isFIPS = 0)+0x57 [security\nss\lib\softoken\pkcs11.c @ 2582]

>freebl3!RNG_SystemInfoForRNG(void)+0x223 [security\nss\lib\freebl\win_rand.c @ 358]
>softokn3!RNG_SystemInfoForRNG(void)+0x22 [security\nss\lib\freebl\loader.c @ 1570]
>softokn3!nsc_CommonInitialize(void * pReserved = 0x0012d7d4, int isFIPS = 0)+0x91 [security\nss\lib\softoken\pkcs11.c @ 2602]

Comment 106

[Nelson Bolyard (seldom reads bugmail)]

Neil, please file a new bug about this issue.  
This bug is too big, and really is fixed, I believe.

(Oh, and sorry for mistyping your name in comment 100.  
You can call me Nailson, if you wish. :)

Comment 107

[skierpage]

(In reply to comment #105)
> RNG_RNGInit() already calls RNG_SystemInfoForRNG() (via PR_CallOnce no less)!

BTW, I believe neil filed that as new bug 489811.