RFC: Improve std.random.unpredictableSeed by yshui · Pull Request #5230 · dlang/phobos

yshui · 2017-03-03T02:20:34Z

Implemented according to suggestions from Cédric Picard (see 1 2)

I only have linux machine to test with, so I opened this pull request to abuse the build bot.

Shoot if you spot any problems.

wilzbach · 2017-03-03T02:34:49Z

so I opened this pull request to abuse the build bot.

That's what it's for.

For Linux >= 3.17 there's the awesome [getrandom`](http://man7.org/linux/man-pages/man2/getrandom.2.html) system call which should be used. For more details please see this intro article

Also you might be very interested in my related work at mir.random which also exposes a genRandom{,Blocking} API.

wilzbach · 2017-03-03T02:36:32Z

std/random.d

+        {
+            bool ret = CryptAcquireContext(&hProv, null, null, PROV_RSA_FULL, 0);
+            if (!ret)
+                throw new Exception("CryptAcquireContext");


it's possible to recover from this:

if (GetLastError() == NTE_BAD_KEYSET) { // Attempt to create default container if (!CryptAcquireContextA(&hProvider, null, null, PROV_RSA_FULL, CRYPT_NEWKEYSET | CRYPT_SILENT)) return 1;

wilzbach · 2017-03-03T02:37:10Z

std/random.d

+        uint rand;
+        if (!initialized)
+        {
+            bool ret = CryptAcquireContext(&hProv, null, null, PROV_RSA_FULL, 0);


CryptAcquireContextW(&hProv, null, null, PROV_RSA_FULL, CRYPT_VERIFYCONTEXT | CRYPT_SILENT))

-> https://msdn.microsoft.com/en-us/library/windows/desktop/aa379942(v=vs.85).aspx

wilzbach · 2017-03-03T02:42:21Z

It took me a very long time (and many runs on AppVeyor) to figure out that the Wincrypt headers in druntime aren't correct for x64. Hence I highly recommend to have use the overrides I use in Mir - it's tested ;-)

// the wincrypt headers in druntime are broken for x64!
private alias ULONG_PTR = size_t; // uint in druntime
private alias BOOL = bool;
private alias DWORD = size_t; // uint in druntime
private alias LPCWSTR = wchar*;
private alias PBYTE = ubyte*;
private alias HCRYPTPROV = ULONG_PTR;
private alias LPCSTR = const(char)*;

private extern(Windows) BOOL CryptGenRandom(HCRYPTPROV, DWORD, PBYTE) @nogc @safe nothrow;
private extern(Windows) BOOL CryptAcquireContextA(HCRYPTPROV*, LPCSTR, LPCSTR, DWORD, DWORD) @nogc nothrow;
private extern(Windows) BOOL CryptAcquireContextW(HCRYPTPROV*, LPCWSTR, LPCWSTR, DWORD, DWORD) @nogc nothrow;
private extern(Windows) BOOL CryptReleaseContext(HCRYPTPROV, ULONG_PTR) @nogc nothrow;

private __gshared ULONG_PTR hProvider;

yshui · 2017-03-03T02:52:40Z

@wilzbach What's the correct way to detect Linux version in D?

yshui · 2017-03-03T02:54:00Z

@wilzbach Maybe you should push changes to druntime?

wilzbach · 2017-03-03T03:02:47Z

@wilzbach What's the correct way to detect Linux version in D?

There's no "correct" way, but the system call uname is a good option.

-> Another friendly redirect to my work: https://github.com/libmir/mir-random/pull/13/files
Copied here for convenience:

import core.sys.posix.sys.utsname : utsname;

// druntime's version isn't annotated with attributes
private extern(C) int uname(utsname* __name) @nogc nothrow;

private bool hasGetRandom() @nogc @trusted nothrow
{
    import core.stdc.string : strtok;
    import core.stdc.stdlib : atoi;

    utsname uts;
    uname(&uts);
    char* p = uts.release.ptr;

    // poor man's version check
    auto token = strtok(p, ".");
    int major = atoi(token);
    if (major  > 3)
        return true;

    if (major == 3)
    {
        token = strtok(p, ".");
        if (atoi(token) >= 17)
            return true;
    }

    return false;
}

(this is done without the D standard library, s.t. it can be used in -betterC)

@wilzbach Maybe you should push changes to druntime?

I haven't used Windows for the last five years, so I don't know much about the Windows bindings in druntime.

jacob-carlborg · 2017-03-09T14:36:34Z

std/random.d

+else version(NetBSD)
+{
+    version = ARC4_RANDOM;
+}


macOS also has the arc4random function. Not sure what's best to use.

[1] https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man3/arc4random.3.html

In recent version of OpenBSD/NetBSD, arc4random is supported by ChaCha20. On macOS it's ARC4.

OTH macOS's /dev/random uses Yarrow, so I guess that's a better choice?

JackStouffer · 2017-03-09T14:44:22Z

std/random.d

+            return rand;
+        }
+    }
+    else


Do we even support any non-windows and non-posix OS's?

JackStouffer · 2017-03-09T14:45:51Z

std/random.d

-    static bool seeded;
-    static MinstdRand0 rand;
-    if (!seeded)
+    version(Windows)


space between version and (

JackStouffer · 2017-03-09T14:55:00Z

std/random.d

@@ -1281,17 +1294,79 @@ A single unsigned integer seed value, different on each successive call
 */
 @property uint unpredictableSeed() @trusted


this should be a template with trusted escapes so the attributes can be inferred

JackStouffer · 2017-03-09T14:56:13Z

std/random.d

+
+version(ARC4_RANDOM)
+{
+    extern(C) uint arc4random();


@nogc nothrow pure

I don't think this can be pure?

Implemented according to suggestions from Cédric Picard.

n8sh · 2017-10-04T21:50:48Z

std/random.d

+{
+    version = ARC4_RANDOM;
+}
+


I suggest also setting version = ARC4_RANDOM for version (FreeBSD) and version (OSX).

n8sh · 2017-10-09T19:51:10Z

On my machine I profiled all of the implementations except the one using CryptGenRandom:

The current unpredictableSeed takes 2.9 seconds for 100 million calls.
arc4random() took 3.2 seconds for 100 million calls. I ran this test under OS X whose arc4random uses AES rather than ChaCha20.
The implementation that reads from /dev/urandom on every call takes around a minute and ten seconds for 100 million calls.
The fallback implementation based on SHA256Digest takes nine minutes for 100 million calls.

I think the last two have the wrong speed/unpredictability tradeoff for a non-cryptographic API.

JackStouffer · 2017-11-17T19:08:42Z

Going to close this in a week if @yshui doesn't respond.

yshui · 2017-11-18T14:46:16Z

@JackStouffer I'm not sure where this pull request is going...

JackStouffer · 2017-11-20T15:27:57Z

@yshui

Does it matter how slow unpredictableSeed is (I suspect no, because you should really only be calling it once per run of the software)?
Does arc4random work on OSX and FreeBSD?
Add info to the docs about the crypto security of the seed on each platform
Add a warning that this function shouldn't be re-called for every new random number created and should really just be called once per run

Also, @wilzbach you seem to know about this area, any other comments on the implementation?

n8sh · 2017-11-20T16:36:56Z

Does arc4random work on OSX and FreeBSD?

OSX has arc4random with AES instead of ChaCha20.

FreeBSD has an arc4random that still uses RC4 and has the problem that forked processes see the same sequences when the PID wraps. It shouldn't be used anywhere a cryptographically secure PRNG is required, but for non-cryptographic uses it would still be a significant improvement over the current unpredictableSeed.

JackStouffer · 2018-01-04T17:41:37Z

Ping @yshui

quickfur · 2018-01-04T21:23:57Z

One general comment / question: reading the articles posted at the two given links in the original PR description, I come away with the understanding that what the article was advocating for was not to try to make unpredictableSeed cryptographically secure, but rather to point out that std.random was never intended to be used for cryptographic purposes in the first place, and therefore user code should not be relying on std.random for any cryptographic properties it may or may not have.

In that context, this PR seems a little misplaced. What's the value of going through all of this trouble to use /dev/urandom and/or its equivalent to get a supposedly cryptographically-secure seed value, only to use it to seed a PRNG that was never designed for cryptographic applications in the first place, and that is therefore almost certainly insecure?

yshui · 2018-01-08T11:21:36Z

@quickfur I agree. I have the impression that this issue has already been solved when the disclaimer is add to the std.random document.

I will close this PR.

yshui force-pushed the random branch 2 times, most recently from 582727d to d424605 Compare March 3, 2017 02:30

wilzbach reviewed Mar 3, 2017

View reviewed changes

yshui force-pushed the random branch from d424605 to 48f614b Compare March 3, 2017 02:39

wilzbach self-assigned this Mar 3, 2017

yshui force-pushed the random branch from 48f614b to bdd85ab Compare March 3, 2017 03:02

yshui force-pushed the random branch from bdd85ab to bc3f2be Compare March 8, 2017 21:28

jacob-carlborg reviewed Mar 9, 2017

View reviewed changes

JackStouffer reviewed Mar 9, 2017

View reviewed changes

std/random.d

return rand;

}

}

else

Copy link

Contributor

JackStouffer Mar 9, 2017

Choose a reason for hiding this comment

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

Do we even support any non-windows and non-posix OS's?

JackStouffer reviewed Mar 9, 2017

View reviewed changes

yshui force-pushed the random branch 9 times, most recently from 6414960 to 2d2e2af Compare March 9, 2017 22:14

Improve std.random.unpredictableSeed

c173aa2

Implemented according to suggestions from Cédric Picard.

yshui force-pushed the random branch from 2d2e2af to c173aa2 Compare March 9, 2017 22:24

JackStouffer added the Review:Needs Review label Apr 6, 2017

dlang-bot added the Merge:stalled label Jun 22, 2017

wilzbach mentioned this pull request Jul 8, 2017

Add initial version of a CODEOWNERS mapping #5573

Merged

n8sh reviewed Oct 4, 2017

View reviewed changes

std/random.d

{

version = ARC4_RANDOM;

}

Copy link

Member

n8sh Oct 4, 2017

Choose a reason for hiding this comment

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

I suggest also setting version = ARC4_RANDOM for version (FreeBSD) and version (OSX).

n8sh mentioned this pull request Oct 21, 2017

Issue 17912: Add function for creating a temporary file. #5788

Closed

JackStouffer removed the Merge:stalled label Nov 20, 2017

dlang-bot added Merge:Needs Rebase and removed Merge:Needs Rebase labels Jan 1, 2018

dlang-bot removed the Merge:Needs Rebase label Jan 5, 2018

yshui closed this Jan 8, 2018

n8sh mentioned this pull request Jan 9, 2018

Fix Issue 18217: Don't repeatedly call unpredictableSeed to initialize rndGen #6021

Merged

n8sh mentioned this pull request Mar 12, 2018

Partially Fix Issue 18596: use arc4random when available for unpredictableSeed #6267

Merged

n8sh mentioned this pull request Apr 2, 2018

Fix Issue 18595 & Issue 18596: add unpredictableSeed!UIntType, support non-arc4random entropy sources, & replace MindstdRand0 with SplitMix #6388

Closed

		@@ -1281,17 +1294,79 @@ A single unsigned integer seed value, different on each successive call
		*/
		@property uint unpredictableSeed() @trusted

Uh oh!

Conversation

yshui commented Mar 3, 2017

Uh oh!

wilzbach commented Mar 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Mar 3, 2017

Uh oh!

yshui commented Mar 3, 2017

Uh oh!

yshui commented Mar 3, 2017

Uh oh!

wilzbach commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n8sh commented Oct 9, 2017

Uh oh!

JackStouffer commented Nov 17, 2017

Uh oh!

yshui commented Nov 18, 2017

Uh oh!

JackStouffer commented Nov 20, 2017

Uh oh!

n8sh commented Nov 20, 2017

Uh oh!

JackStouffer commented Jan 4, 2018

Uh oh!

quickfur commented Jan 4, 2018

Uh oh!

yshui commented Jan 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

wilzbach commented Mar 3, 2017 •

edited

Loading