WIP: Use ProcessPrng on Windows #415
Conversation
josephlr
force-pushed
the
windows
branch
2 times, most recently
from
5495418
to
5223c3e
Compare
josephlr
force-pushed
the
windows
branch
7 times, most recently
from
1d62b41
to
03abb05
Compare
src/windows.rs
Outdated
| let code = unsafe { NonZeroU32::new_unchecked(code) }; | ||
| return Err(Error::from(code)); | ||
| } | ||
| let dll = unsafe { LoadLibraryA(b"bcryptprimitives.dll\0".as_ptr()) }; |
There was a problem hiding this comment.
We should use LoadLibraryEx with LOAD_LIBRARY_SEARCH_SYSTEM32 instead (but see the documentation about that flag and compatibility) or LOAD_WITH_ALTERED_SEARCH_PATH with an absolute path to the System32 folder.
Google Chrome doesn't security bug reports about this issue to be valid, but not everybody agrees with Google Chrome. Regardless, I can tell you from past experience that this will trigger security vulnerability bug reports to every application that uses getrandom, if we don't restrict the search path to System32. These bug reports are difficult for many projects to deal with. It's worth avoiding.
There was a problem hiding this comment.
If it's in \KnownDlls then it doesn't particularly matter because it will indeed prioritize the system directory for those dlls. However bcryptprimitives is a bit more awkward because it's a late addition, iirc the earliest versions of Windows 10 did not have it as a known dll.
|
@briansmith the above are really good point w.r.t. sandboxing. I think that it would be good to have general documentation along the lines of "before starting a sandbox, you should first successfully call
I think that this won't cause issues in some sandboxes provided that |
josephlr
force-pushed
the
windows
branch
7 times, most recently
from
6825ae5
to
4bfd348
Compare
|
@newpavlov and @briansmith this is now ready for review! |
Use
ProcessPrngon Windows 10 and up, and useRtlGenRandomon older legacy Windows versions. Don't useBCryptGenRandomdue to stability issues.