Skip to content

Add Windows support - #39

Merged
avsm merged 7 commits into
mirage:masterfrom
avsm:win-port
Mar 15, 2020
Merged

avsm merged 7 commits into
mirage:masterfrom
avsm:win-port

Conversation

@avsm

@avsm avsm commented Mar 15, 2020

Copy link
Copy Markdown
Member

Need this to release conduit (which needs Windows support as well). It also fixes a bug in the C stubs, and adds GH Actions CI so that we can test Windows in CI as well.

@avsm
avsm requested a review from hannesm March 15, 2020 00:11

@hannesm hannesm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about this code path. Why did you choose BCryptGenRandom with BCRYPT_USE_SYSTEM_PREFERRED_RNG? According to what I found on the Internet, this may use the rather weak DUAL_EC generator? And is only available since Windows 8 (fine with me).

but more concerning, looking into other people's code (here libsodium):

/* `RtlGenRandom` is used over `CryptGenRandom` on Microsoft Windows based systems:
 *  - `CryptGenRandom` requires pulling in `CryptoAPI` which causes unnecessary
 *     memory overhead if this API is not being used for other purposes
 *  - `RtlGenRandom` is thus called directly instead. A detailed explanation
 *     can be found here: https://blogs.msdn.microsoft.com/michael_howard/2005/01/14/cryptographically-secure-random-number-on-windows-without-using-cryptoapi/
 *
 * In spite of the disclaimer on the `RtlGenRandom` documentation page that was
 * written back in the Windows XP days, this function is here to stay. The CRT
 * function `rand_s()` directly depends on it, so touching it would break many
 * applications released since Windows XP.
 *
 * Also note that Rust, Firefox and BoringSSL (thus, Google Chrome and everything
 * based on Chromium) also depend on it, and that libsodium allows the RNG to be
 * replaced without patching nor recompiling the library.
 */

from that, I'd suggest to use RtlGenRandom. What do you think?

Comment thread rng/unix/mc_getrandom_stubs.c Outdated
@avsm

avsm commented Mar 15, 2020

Copy link
Copy Markdown
Member Author

I'm in two minds about which one to use. It's a tossup between a complex linkage (for rtlgenrandom and advapi32.dll) and unclear semantics (for BCryptGenRandom, see here for the variations).

I picked the Microsoft-recommended version, but I'll give the RtlGenRandom one a shot now to see how that diff looks. I'm only testing on Windows 10 / Server 2016.

@hannesm

hannesm commented Mar 15, 2020

Copy link
Copy Markdown
Member

@avsm thanks for your work and comments. I'm not a windows expert. I suspect if we check for a recent enough windows version, we can as well avoid using DUAL_EC_DRBG, from https://docs.microsoft.com/en-us/windows/win32/seccng/cng-algorithm-identifiers?redirectedfrom=MSDN

Windows 8: Beginning with Windows 8, the EC RNG algorithm supports FIPS 186-3. Keys less than or equal to 1024 bits adhere to FIPS 186-2 and keys greater than 1024 to FIPS 186-3.

Windows 10: Beginning with Windows 10, the dual elliptic curve random number generator algorithm has been removed. Existing uses of this algorithm will continue to work; however, the random number generator is based on the AES counter mode specified in the NIST SP 800-90 standard. New code should use BCRYPT_RNG_ALGORITHM, and it is recommended that existing code be changed to use BCRYPT_RNG_ALGORITHM. 

-> I'd be happy with a code comment (why this is fine) and a test (if achievable) that at least windows 10 is used

@avsm

avsm commented Mar 15, 2020

Copy link
Copy Markdown
Member Author

Decision with @hannesm: we will use bcrypt as its more modern, and Windows 10 is our base of support anyway. I'll push a README and code comment note to this effect.

avsm and others added 2 commits March 15, 2020 17:06
Co-Authored-By: Hannes Mehnert <hannes@mehnert.org>
Move the doc for `initialize` to the function itself.
@avsm

avsm commented Mar 15, 2020

Copy link
Copy Markdown
Member Author

This should be ready for merge now. I rearranged the doc slightly in 2260884 @hannesm, if you want to glance over that.

@hannesm

hannesm commented Mar 15, 2020

Copy link
Copy Markdown
Member

ok, that looks good. could you add a dune-configurator {build} dependency to mirage-crypto-rng.opam, please?

@avsm

avsm commented Mar 15, 2020

Copy link
Copy Markdown
Member Author

Good catch! Done and will merge when CI passes

@avsm
avsm merged commit 77df68f into mirage:master Mar 15, 2020
@avsm
avsm deleted the win-port branch March 15, 2020 18:11
@hannesm hannesm mentioned this pull request Mar 15, 2020
avsm added a commit to avsm/opam-repository that referenced this pull request Mar 15, 2020
…mirage-crypto-entropy (0.6.1)

CHANGES:

* Add Windows 10+ support (mirage/mirage-crypto#39 @avsm, review by @hannesm @dinosaure)
* Fix unix stubs to be allocating in case of exception (mirage/mirage-crypto#39 @avsm
  review by @hannesm @dinosaure)
* Add GitHub Actions CI (mirage/mirage-crypto#39 @avsm)
* Rebuild source if the `MIRAGE_CRYPTO_ACCELERATE` environment
  variable changes value (mirage/mirage-crypto#40 @avsm)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants