Skip to content
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

Fix: sanity check /dev/urandom #4777

Merged

Conversation

ysbaddaden
Copy link
Contributor

Sanity checks /dev/urandom to be character device, not a tempered file, a socket or whatever. Also makes sure that FD_CLOEXEC is set.

closes #4752

@@ -12,8 +12,9 @@ module Crystal::System::Random

if sys_getrandom(Bytes.new(16)) >= 0
@@getrandom_available = true
else
elsif File.stat("/dev/urandom").chardev?
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use #stat on the opened file descriptor, to avoid race conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Will change.

@ysbaddaden ysbaddaden force-pushed the fix-add-urandom-chardev-check branch from a609119 to 97d78fc Compare August 11, 2017 09:32
Sanity checks /dev/urandom to be character device, not a tempered
file, a socket or whatever. Also makes sure that FD_CLOEXEC is set.

closes crystal-lang#4752
@ysbaddaden ysbaddaden force-pushed the fix-add-urandom-chardev-check branch from 97d78fc to 28c8746 Compare August 11, 2017 09:32
@ysbaddaden ysbaddaden changed the title Fix: sanitfy check /dev/urandom Fix: sanity check /dev/urandom Aug 11, 2017
@ysbaddaden
Copy link
Contributor Author

Done.

@RX14
Copy link
Contributor

RX14 commented Aug 11, 2017

I think this is fine but it would be nice to do something about the duplicated code, especially as it is security related and reflecting any changes in all places is an absolute must.

@ysbaddaden
Copy link
Contributor Author

I'm merging as is. If someone has an epiphany to avoid the duplication in an simple way, please open a pull request :-)

@ysbaddaden ysbaddaden merged commit a75e3c4 into crystal-lang:master Aug 11, 2017
@ysbaddaden ysbaddaden added this to the Next milestone Aug 11, 2017
@ysbaddaden ysbaddaden deleted the fix-add-urandom-chardev-check branch June 27, 2018 08:08
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.

System::Random when using /dev/urandom doesn't seem to do any sanity checking
3 participants