-
Notifications
You must be signed in to change notification settings - Fork 7.9k
random: Move the CSPRNG implementation into a separate C file #10668
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
Conversation
The CSPRNG is a delicate and security relevant piece of code and having it in the giant random.c makes it much harder to verify changes to it. Split it into a separate file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just nitpicking on already existing issues.
But looks good to me otherwise if the CI doesn't complain.
# include <sanitizer/msan_interface.h> | ||
#endif | ||
|
||
PHPAPI int php_random_bytes(void *bytes, size_t size, bool should_throw) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PHPAPI int php_random_bytes(void *bytes, size_t size, bool should_throw) | |
PHPAPI zend_result php_random_bytes(void *bytes, size_t size, bool should_throw) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've noticed that myself, having followed the discussion on the ML. However I've intentionally not included this change in this PR to make it immediately obvious that it's a simple moving around when diffing the files. I consider this important for "trustworthiness" / to keep a clear "audit log".
Will make the change in a dedicated follow-up commit / PR.
return SUCCESS; | ||
} | ||
|
||
PHPAPI int php_random_int(zend_long min, zend_long max, zend_long *result, bool should_throw) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PHPAPI int php_random_int(zend_long min, zend_long max, zend_long *result, bool should_throw) | |
PHPAPI zend_result php_random_int(zend_long min, zend_long max, zend_long *result, bool should_throw) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. That's making sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LCTM
Thanks, y'all. |
The CSPRNG is a delicate and security relevant piece of code and having it in the giant random.c makes it much harder to verify changes to it. Split it into a separate file.
Requested @devnexen to double check that I didn't accidentally break the conditional compilation for some platform. Compared to random.c I've dropped the following headers:
Thus I'm reasonably sure the result is correct.