Skip to content

Conversation

sjaeckel
Copy link
Member

@sjaeckel sjaeckel commented Apr 5, 2018

I've re-factored the way mp_rand() gets its random values.

There are now 4 big different cases

  1. arc4random() for *BSD
  2. the Windows CSP
  3. getrandom() for linux with new glibc
  4. reading from /dev/urandom for linux with old glibc and all other platforms

Some of the code is based on rng_get_bytes() of libtomcrypt.

This fixes #103

@sjaeckel
Copy link
Member Author

sjaeckel commented Apr 5, 2018

@jnthn @timo can you please test this on @MoarVM where the arc4random() support originates from?

@sjaeckel
Copy link
Member Author

sjaeckel commented Apr 5, 2018

I've just tested this on Windows and it's horribly slow... it's a bit better if I change the hProv to be static and only do CryptAcquireContext() once...

Copy link
Collaborator

@mkj mkj left a comment

Choose a reason for hiding this comment

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

Looks ok, have tested that urandom works as expected.
Needs another level of nesting for that #ifdef

bn_mp_rand.c Outdated
}
#endif /* WIN32 */

#if !defined(MP_WIN_CSP) && defined(__linux__) && defined(__GLIBC_PREREQ) && __GLIBC_PREREQ(2, 25)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This fails to build on OSX (clang 9.0.0)

bn_mp_rand.c:72:78: error: function-like macro '__GLIBC_PREREQ' is not defined
#if !defined(MP_WIN_CSP) && defined(__linux__) && defined(__GLIBC_PREREQ) && __GLIBC_PREREQ(2, 25)

I guess it needs to be

+#if !defined(MP_WIN_CSP) && defined(__linux__) && defined(__GLIBC_PREREQ)
+#if __GLIBC_PREREQ(2, 25)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I've seen that as well on windows but I'm busy right now, will update later.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@sjaeckel
Copy link
Member Author

could someone please have a look&try? I'd like to prepare a new release asap

int ret;
do {
ret = getrandom(p, sizeof(*p), 0);
} while((ret == -1) && (errno == EINTR));
Copy link

@sebastianas sebastianas Apr 14, 2018

Choose a reason for hiding this comment

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

-   return ret;
+   if (ret == sizeof(*p))
+          return MP_OKAY;
+   return -1;

otherwise you do this and fallback to /dev/urandom because getrandom returns the number of random bytes.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

bn_mp_rand.c Outdated
fd = open(MP_DEV_URANDOM, O_RDONLY);
} while((fd == -1) && (errno == EINTR));
if (fd == -1) return -1;
r = read(fd, p, sizeof(*p));

Choose a reason for hiding this comment

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

this read() needs a do while loop which will retry for errno==EINTR just like open.

Copy link
Member Author

Choose a reason for hiding this comment

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

-   r = read(fd, p, sizeof(*p));
+   do {
+      r = read(fd, p, sizeof(*p));
+   } while((r == -1) && (errno == EINTR));

like that?

Choose a reason for hiding this comment

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

yes, I think this is good now.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link

@sebastianas sebastianas left a comment

Choose a reason for hiding this comment

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

just the two things from my point of view :)

@sjaeckel sjaeckel force-pushed the fix/103 branch 2 times, most recently from 8f6b2ba to 3d672de Compare April 14, 2018 12:34
@sjaeckel
Copy link
Member Author

@mkj can you please re-review?

@mkj
Copy link
Collaborator

mkj commented Apr 24, 2018

Sorry won't be able to look at this until Saturday

@sjaeckel
Copy link
Member Author

Perfectly fine!

@sjaeckel sjaeckel merged commit 71c5c8a into develop May 1, 2018
@sjaeckel sjaeckel deleted the fix/103 branch May 1, 2018 18:57
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.

MP_GEN_RANDOM() calling rand() might be insecure
3 participants