New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Linux syscall "getrandom". #180

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@wmark
Copy link

wmark commented Sep 25, 2014

Implements the syscall "getrandom", available in Linux 3.17, as new engine in a LIBC-agnostic way.

Please note that the engine is not available if the syscall is not implemented (ENOSYS returned).

@wmark

This comment has been minimized.

Copy link

wmark commented Sep 25, 2014

See also:

@wmark wmark force-pushed the Blitznote:linux-getrandom branch from b20880c to 66b0ae2 Sep 25, 2014

@@ -0,0 +1,150 @@
#include <openssl/opensslconf.h>

This comment has been minimized.

@mattcaswell

mattcaswell May 5, 2016

Member

From here you should probably enclose the below in OPENSSL_SYS_LINUX guards as it is Linux specific.

This comment has been minimized.

@richsalz

richsalz May 5, 2016

Contributor

This is way cool and important, but seems like a new feature. The RNG in OpenSSL needs work -- heck, we created a special mailing list to talk about it -- but nothing was done in time for 1.1 :(

This comment has been minimized.

@ghedo

ghedo May 5, 2016

Contributor

Well, maybe because nobody knew about the new mailing list. Well, I didn't know about it anyway :P

In any case this seems the wrong way to add getrandom() support. It should be added to RAND_poll() instead
https://github.com/openssl/openssl/blob/master/crypto/rand/rand_unix.c#L242

which is in dire need of a refactoring btw like what I proposed in (and previously in that big mailing list discussion that didn't go anywhere) #898 (comment)

but yes, it's probably too late at this point for 1.1.0.

RNG-related is also #512 which is not a new feature.

This comment has been minimized.

@richsalz

richsalz May 5, 2016

Contributor

And there are some RT tickets too , mainly about windows RNG.

This comment has been minimized.

@mattcaswell

mattcaswell May 5, 2016

Member

@ghedo, so is it your view that we should close this PR as unsuitable?

This comment has been minimized.

@ghedo

ghedo May 5, 2016

Contributor

Not that my opinion matters all that much, but yes. Adding getrandom() through a new engine means that only the people who enable the engine are going to benefit from this, but it would also bypass md_rand impacting performance significantly.

On the other hand adding this to RAND_poll() means that getrandom() is only used to seed the user-space PRNG and that we don't need a syscall every time random data is needed (the RNG on Linux is also quite susceptible to lock contention).

(The fact that md_rand is not exactly optimal and needs to be rewritten is a different problem).

This comment has been minimized.

@mattcaswell

mattcaswell May 5, 2016

Member

On the contrary, I think your opinion is greatly respected here! @richsalz do you agree with closing this?

This comment has been minimized.

@richsalz

richsalz May 5, 2016

Contributor

I too resepect @ghedo's views and think that's a better approach; close this.

@mattcaswell

This comment has been minimized.

Copy link
Member

mattcaswell commented May 5, 2016

Dusting this one off after quite a long time. Sorry!

In order for this PR to be acceptable it will need some significant update to bring it up to date with all of the changes in the codebase since this was written. Please rebase and update this PR if you are still interested in pursuing this. Unfortunately this cannot be integrated until after the 1.1.0 release though.

@mattcaswell mattcaswell added the feature label May 5, 2016

@mattcaswell mattcaswell added this to the Post 1.1.0 milestone May 5, 2016

@mattcaswell

This comment has been minimized.

Copy link
Member

mattcaswell commented May 5, 2016

Closing as per the discussion above.

@mattcaswell mattcaswell closed this May 5, 2016

christianpaquin added a commit to christianpaquin/openssl that referenced this pull request Dec 21, 2017

Squashed 'vendor/liboqs/' changes from 04d7eaa..ff3986a
ff3986a removed hard paths (openssl#193)
f62bb02 Enabled and documented building on ARM32 (Raspberry Pi). (openssl#179)
9dab6f6 Flags for configured algorithms generated in config.h (openssl#177)
2d5eb13 Covscan defect fix (openssl#189)
a5b239d Updated README (openssl#191)
d7a72e2 Add checks to verify length of input data for McBits (openssl#186)
cbee5ef Vsoftco issue160 (openssl#188)
581fbbb Initialize out-parameters to NULL (openssl#183)
0d8a354 Properly separate SIDH CLN16 from SIDH CLN16 compressed (openssl#181)
8bc8cd9 Added VisualStudio DLL build configurations (openssl#182)
fc522d6 Embed SIDH IQC REFERENCE parameters (openssl#180)
40ffb4e Updated Windows build (added sig, fixed warnings, 2017 update) (openssl#169)
a329060 Update README.md (openssl#178)
fcbd0f3 KEX memory benchmarks (openssl#171)
b9854b4 Arm compilation (openssl#170)
f3e24e1 Link to algorithm data sheets.
28cc05a Added datasheets for SIDH and Picnic. (openssl#166)

git-subtree-dir: vendor/liboqs
git-subtree-split: ff3986ab9585e521462fb28d24ed024328f609b9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment