On Windows (lib/freebl/win_rand.c), ReadSystemFiles says "now read 10 files", but it actually reads 21 files in the system directory (C:\WINDOWS\system32).
Created attachment 242555 [details] [diff] [review] Patch that illustrates the off-by-11 problem There are 2481 files in my C:\WINDOWS\system32 directory. ReadSystemFiles intends to read one out of every 248 (= 2481/10) files. However, it has two mistakes. 1. It initializes filesToRead to 10, so it also reads the very first 10 files in the directory listing. (Note that we skip files that we don't have the permission to read, so it's really the first 10 readable files.) 2. It has an off-by-one error when it reads one file out of every 248 files. The following debug output shows the two mistakes. The 2481 files are numbered from 0 to 2480. We show the files that are read and the total number of files read. Before this patch is applied: 2 11 12 16 17 18 19 20 21 22 23 248 496 744 992 1240 1488 1736 1984 2232 2480 *********** TotalRead = 21 ******** After this patch is applied: 247 495 743 991 1239 1487 1735 1984 2231 2479 *********** TotalRead = 10 ********
Created attachment 242560 [details] [diff] [review] Patch that illustrates the off-by-11 problem, v2 This patch is better.
Wan-Teh, yes, it's true, the code reads the first 10 readable files, then 10 files spread throughout the directory. Is that a bug? Perhaps the only "fix" needed is in the comment.
That's why the severity of this bug is "trivial" (cosmetic problem like misspelled words or misaligned text) and I said my patch is mainly intended to point out where the off-by-21 error comes from. I am not picking nits in our source code. I had to document how we derive the seed for our deterministic RNG. So I studied the source code and traced it in the debuggers on several platforms. The documentation is the "Security of key generation method" subsection under http://wiki.mozilla.org/VE_07KeyMgmt#Key_Generation I found bug 355297 (including the bug fixed by attachment 242022 [details] [diff] [review]), bug 356884, bug 356886, bug 356595, and this bug in the code review and debuggers. I documented our implementation faithfully, so any oddity of our implementation, such as our passing the pseudo handle returned by the GetCurrentProcess function to RNG_RandomUpdate and our reading "at least 10 files" in C:\WINDOWS\system32, shows up in the documentation.
Created attachment 242707 [details] [diff] [review] Add a comment to describe what the code actually does I added the comment that Nelson suggested on the trunk (NSS 3.12) only. Checking in win_rand.c; /cvsroot/mozilla/security/nss/lib/freebl/win_rand.c,v <-- win_rand.c new revision: 1.13; previous revision: 1.12 done
Created attachment 242709 [details] [diff] [review] Proposed patch This patch allows the code to read n files spread throughout the system directory. In this patch I set n=20 so that the code reads as many files as it did before. You can change n back to the original 10 or any number you like. I won't waste anyone's time for a code review, so I'll mark the bug WONTFIX.
Wan-Teh, I disagree with the characterization that the code's behavior is due to a programming error. It's doing what I intended it to do. I agree that the comment was wrong. Is that a programming error?
Created attachment 242711 [details] [diff] [review] Proposed patch v2 I fixed the comment. I didn't know that's the intention. Sorry. I'll let you decide if "10 or 11 files spread throughout the system directory" is an off-by-one error. I don't want to be a pest. You can make that "10 files spread throughout the system directory" by taking just the change to the ReadFiles function from attachment 242709 [details] [diff] [review].
Comment fix checked in on NSS trunk (3.12). Checking in win_rand.c; /cvsroot/mozilla/security/nss/lib/freebl/win_rand.c,v <-- win_rand.c new revision: 1.14; previous revision: 1.13 done