win_rand.c calls RtlGenRandom instead of CryptGenRandom on modern windows

RESOLVED WONTFIX

Status

RESOLVED WONTFIX
10 years ago
10 years ago

People

(Reporter: perseus.usenet, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.11) Gecko/2009060215 Firefox/3.0.11 (.NET CLR 3.5.30729)
Build Identifier: 3.12.4 

(See URL reference for details about this issue). The patch for bug 501605 revealed a code path in win_rand.c, RNG_SystemRNG function, where first a check is done for the undocumented windows system function RtlGenRandom (through the placeholder function SystemFunction036) and if that fails (so if it's not available), the function falls back onto calling CryptGenRandom, if available. 

Microsoft strongly advices against the usage of RtlGenRandom (ref: http://msdn.microsoft.com/en-us/library/aa387694(VS.85).aspx ) as it's not a public function and subject to change and users are adviced to use CryptGenRandom instead. The problem however is that modern windows exposes RtlGenRandom through its placeholder function SystemFunction036 in every modern windows (Windows XP and up) and therefore win_rand.c's RNG_SystemRNG function will always call RtlGenRandom instead of CryptGenRandom. This is a risk because if Microsoft changes SystemFunction036 for whatever reason, e.g. changes the input parameter types or even refactors it to different functions, it will put NSS and with it its hosting applications (e.g. Firefox) at risk of crashing which is unnecessary. 

The fix is easy: instead of first checking for RtlGenRandom, the code should check for CryptGenRandom being present (and its types) and if not, fall back onto RtlGenRandom. 

Reproducible: Always

Steps to Reproduce:
See Details. 
Actual Results:  
Wrong call order of Windows CryptAPI functions. First a call to CryptGenRandom should be made, then a call to RtlGenRandom, IF CryptGenRandom isn't available. 

Expected Results:  
First a call to CryptGenRandom and if that's not available a call to RtlGenRandom
> This is a risk because if Microsoft changes SystemFunction036 for whatever reason,
It will not be likely to occur because CRT rand_s also depends on the RtlGenRandom function.
http://msdn.microsoft.com/en-us/library/sxtz2fa8%28VS.80%29.aspx
If Microsoft changes the SystemFunction036, all apps which links CRT will be broken either.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 2

10 years ago
(In reply to comment #1)
> > This is a risk because if Microsoft changes SystemFunction036 for whatever reason,
> It will not be likely to occur because CRT rand_s also depends on the
> RtlGenRandom function.
> http://msdn.microsoft.com/en-us/library/sxtz2fa8%28VS.80%29.aspx
> If Microsoft changes the SystemFunction036, all apps which links CRT will be
> broken either.

Checking rand_s.c in the CRT sourcecode shipped with the 6.1 SDK indeed shows RtlGenRandom reliance (albeit a safer method pointer obtain mechanism). If they patch MSVCRT* together with an updated AdvApi32, it might work, but on winxp where an installer could overwrite msvcrt* dlls it's not really something they'll risk to do indeed (but for vista and up they might). Their upcoming new CRT build in 2010 doesn't show a change in this though. 

That leaves us with future windows versions (e.g. windows 7 or even further away). I'll leave it to the maintainer if he wants to leave this reliability on an undocumented function in the code or not. Thanks for digging this up, Masatoshi Kimura, it gives solid background info for a choice made.
> If they patch MSVCRT* together with an updated AdvApi32, it might work,
They can't change statically linked CRT.
Also, apps can have a private CRT copy thanks to Side-by-Side assemblies. Some apps are even shipped with a customized CRT (like mozcrt). So patching CRT is more and more impractical.
(Reporter)

Comment 4

10 years ago
(In reply to comment #3)
> > If they patch MSVCRT* together with an updated AdvApi32, it might work,
> They can't change statically linked CRT.
> Also, apps can have a private CRT copy thanks to Side-by-Side assemblies. Some
> apps are even shipped with a customized CRT (like mozcrt). So patching CRT is
> more and more impractical.

Good points! So in practise it appears: RtlGenRandom might be marked undocumented and not recommended but in reality it's not a function which will change. I've mailed a dev inside Microsoft to see what's the story behind this, as the warning on RtlGenRandom's documentation is not really founded on a realistic scenario and causes confusion: what's the deal: will they change it or not. As you showed it's not likely they'll change it, RtlGenRandom then thus is a valid method to call, in current and future code.
(Reporter)

Comment 5

10 years ago
I got word back from Microsoft. The unofficial word is that it's not very likely that RtlGenRandom will change, also due to the reliance of their own rand_s function in the CRT. There's actually an advantage of using RtlGenRandom over CryptGenRandom which is that RtlGenRandom just does the RNG work and nothing else, while CryptGenRandom requires more overhead because it loads the whole CryptAPI code. 

I hope to get word back from the SDK people as well about the message on RtlGenRandom's docs, but that's not really a concern of Mozilla and co. In short, the code in win_rand.c can stay as is, with the reasoning as stated above. One small thing: rand_s.c, a sourcefile from the Win SDK CRT code base, contains a slightly different approach for loading the advapi32 dll as it uses code to unload the dll as well when done. On modern systems this isn't really an issue per se, but if memory is important this might be a thing to check. 

I'll close the bug report for now.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → WONTFIX

Comment 6

10 years ago
Frans, thanks for asking Microsoft about this issue.  Based on
our experience, we knew it is safe to use RtlGenRandom in spite
of the warnings in its MSDN page, which is why we didn't make
your suggested change.  But asking the source for an authoritative
answer is the ultimate way to settle this kind of issue.

In addition to what Masatoshi and you have written in this bug,
I'd like to add that RtlGenRandom seems to be one of those
undocumented Windows functions used by Microsoft apps that
Microsoft was asked by a court to disclose.

It would be nice to add a comment to win_rand.c summarizing
why it is safe to ignore the warnings in the RtlGenRandom
MSDN page, so that we don't need to repeat this exercise
when the next person notices this issue.
You need to log in before you can comment on or make changes to this bug.