nsRandomGenerator doesn't guard against NSS shutdown

RESOLVED FIXED in mozilla37

Status

()

Product:
Core
Component:
Security: PSM
Type:
defect
Status:
RESOLVED FIXED
Opened:
5 years ago
Updated:
5 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

Version:
unspecified
Target:
mozilla37
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

patch
3.90 KB, patch
jcj
: review+
Details | Diff | Splinter Review 
nsRandomGenerator doesn't guard against NSS shutdown in GenerateRandomBytes. I believe it caused this crash:

11:56:35  WARNING -  PROCESS-CRASH | chrome://mochitests/content/browser/netwerk/test/browser/browser_child_resource.js | application crashed [@ KERNELBASE.dll + 0x89ae4]
11:56:35     INFO -  Crash dump filename: c:\users\cltbld~1.t-w\appdata\local\temp\tmpdwfnv8.mozrunner\minidumps\21aec483-9151-4e6f-af3e-ed800e2add39.dmp
11:56:35     INFO -  Operating system: Windows NT
11:56:35     INFO -                    6.2.9200
11:56:35     INFO -  CPU: x86
11:56:35     INFO -       GenuineIntel family 6 model 30 stepping 5
11:56:35     INFO -       8 CPUs
11:56:35     INFO -  Crash reason:  EXCEPTION_BREAKPOINT
11:56:35     INFO -  Crash address: 0x77949ae4
11:56:35     INFO -  Thread 0 (crashed)
11:56:35     INFO -   0  KERNELBASE.dll + 0x89ae4
11:56:35     INFO -      eip = 0x77949ae4   esp = 0x00fac838   ebp = 0x00fac848   ebx = 0x6f7d203c
11:56:35     INFO -      esi = 0x6f831ec6   edi = 0x6f7d2048   eax = 0x00000000   ecx = 0x6f83ff12
11:56:35     INFO -      edx = 0x00faac40   efl = 0x00000206
11:56:35     INFO -      Found by: given as instruction pointer in context
11:56:35     INFO -   1  nss3.dll!PK11_GetInternalSlot [pk11slot.c:d88622dd6622 : 1799 + 0x1c]
11:56:35     INFO -      eip = 0x6f5c5319   esp = 0x00fac850   ebp = 0x00fac860
11:56:35     INFO -      Found by: previous frame's frame pointer
11:56:35     INFO -   2  xul.dll!nsRandomGenerator::GenerateRandomBytes(unsigned int,unsigned char * *) [nsRandomGenerator.cpp:d88622dd6622 : 28 + 0x4]
11:56:35     INFO -      eip = 0x732b78bd   esp = 0x00fac868   ebp = 0x00fac878
11:56:35     INFO -      Found by: call frame info
11:56:35     INFO -   3  xul.dll!mozilla::net::WebSocketChannel::SetupRequest() [WebSocketChannel.cpp:d88622dd6622 : 2324 + 0x19]
11:56:35     INFO -      eip = 0x7194906d   esp = 0x00fac880   ebp = 0x00fac91c
11:56:35     INFO -      Found by: call frame info
11:56:35     INFO -   4  xul.dll!mozilla::net::WebSocketChannel::AsyncOpen(nsIURI *,nsACString_internal const &,nsIWebSocketListener *,nsISupports *) [WebSocketChannel.cpp:d88622dd6622 : 2926 + 0x6]
11:56:35     INFO -      eip = 0x71949a0a   esp = 0x00fac924   ebp = 0x00fac960
11:56:35     INFO -      Found by: call frame info
11:56:35     INFO -   5  xul.dll!NS_InvokeByIndex [xptcinvoke.cpp:d88622dd6622 : 70 + 0x2]
11:56:35     INFO -      eip = 0x7178f1cf   esp = 0x00fac968   ebp = 0x00fac98c
11:56:35     INFO -      Found by: call frame info
11:56:35     INFO -   6  xul.dll!CallMethodHelper::Invoke() [XPCWrappedNative.cpp:d88622dd6622 : 2396 + 0xe]
11:56:35     INFO -      eip = 0x71d204eb   esp = 0x00fac994   ebp = 0x00fac9b8
11:56:35     INFO -      Found by: call frame info

http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-beta-win32-debug/1419271168/mozilla-beta_win8-debug_test-mochitest-browser-chrome-2-bm111-tests1-windows-build31.txt.gz
Posted patch patch — — Splinter Review 
JC, if you're not already familiar with our NSS-shutdown-prevention mechanisms, this would be a good time to learn. Most of the background information is here: https://hg.mozilla.org/mozilla-central/annotate/912036eeb024/security/manager/ssl/src/nsNSSShutDown.h#l166
See also bugs with titles that mention NSS shutdown. Also, feel free to ask for clarification.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8544281 - Flags: review?(jjones)
Comment on attachment 8544281 [details] [diff] [review]
patch

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

LGTM. We all know that I comment too much, but I'd toss one in to nsRandomGenerator.cpp:23 that the purpose of nsRandomGenerator being a shutdown object is the shutdown prevention lock. But again, whatever convention says is best!
Attachment #8544281 - Flags: review?(jjones) → review+
Thanks, JC. I don't think a comment is necessary, so I didn't add one. The only reason anything would be a shutdown object is to prevent/handle NSS shutdown via a shutdown prevention lock.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=32b2894d0928
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5889fb4ad1e
https://hg.mozilla.org/mozilla-central/rev/f5889fb4ad1e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.