[REF] Use random_bytes instead of uniqid/rand for random hex strings #32205
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
Using random_bytes is both faster and more secure than md5(uniqid(rand(), TRUE)). It is possibly also easier to read, in the sense that it is more obvious that it returns hexadecimal encoded random bytes. I did not find an instance where guessing the random identifier would result in a security vulnerability. So this change does not have direct security impact as far as I know. It's more of a best practice thing and I hope people copy paste the new, secure way of generating random bytes when creating identifiers for security-sensitive stuff, instead of copying the old, insecure way. In some test files the lengths of the random strings are one character longer. E.g. I replaced `substr(sha1(rand()), 0, 7)` with `bin2hex(random_bytes(4))`. The length did not seem very important here, so I don't think this matters. I haven't tested all changed code. I rely on unit tests, and that the code generates a random hex string of a certain length before and after I replaced it. I also looked into the SQL statements that use MD5(RAND()). These should be replaced by HEX(RANDOM_BYTES()), but this is only available starting in MariaDB 10.10, and we require 10.2.
|
Woot. This looks pretty good to me. I read 85-95% of the diffs, and I spot-checked (copied/executed) several of the earlier ones (which affect runtime code) -- and confirmed the resulting outputs were similar In all the cases where I saw elongated outputs, it seemed to be generating a test-identifier (not runtime code). In fact, a huge proportion of updates were test-related. So as long as those tests pass, they're good. (Aside: Thank goodness for |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Using random_bytes is both faster and more secure than md5(uniqid(rand(), TRUE)). It is possibly also easier to read, in the sense that it is more obvious that it returns hexadecimal encoded random bytes.
I did not find an instance where guessing the random identifier would result in a security vulnerability. So this change does not have direct security impact as far as I know. It's more of a best practice thing and I hope people copy paste the new, secure way of generating random bytes when creating identifiers for security-sensitive stuff, instead of copying the old, insecure way.
In some test files the lengths of the random strings are one character longer. E.g. I replaced
substr(sha1(rand()), 0, 7)withbin2hex(random_bytes(4)). The length did not seem very important here, so I don't think this matters.I haven't tested all changed code. I rely on unit tests, and that the code generates a random hex string of a certain length before and after I replaced it.
I also looked into the SQL statements that use MD5(RAND()). These should be replaced by HEX(RANDOM_BYTES()), but this is only available starting in MariaDB 10.10, and we require 10.2.