Skip to content

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

Merged
merged 1 commit into from
Feb 23, 2023

Conversation

TimWolla
Copy link
Member

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:

#include <math.h>
#include "Zend/zend_enum.h"
# include <sys/time.h>
#include "random_arginfo.h"

Thus I'm reasonably sure the result is correct.

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.
Copy link
Member

@Girgias Girgias left a 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)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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)

Copy link
Member Author

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)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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)

Copy link
Contributor

@zeriyoshi zeriyoshi left a 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.

Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

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

LCTM

@TimWolla
Copy link
Member Author

Thanks, y'all.

@TimWolla TimWolla merged commit b14dd85 into php:master Feb 23, 2023
@TimWolla TimWolla deleted the csprng-dedicated-file branch February 28, 2023 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants