Skip to content

replace uniqid() with random_bytes() to create identifiers #57856

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
Jul 29, 2024

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Jul 28, 2024

Q A
Branch? 7.2
Bug fix? no
New feature? no
Deprecations? no
Issues Fix #57588
License MIT

@xabbuh xabbuh requested a review from OskarStark as a code owner July 28, 2024 11:20
@carsonbot carsonbot added this to the 7.2 milestone Jul 28, 2024
@OskarStark OskarStark changed the title replace uniqid() with random_bytes() to create identifiers replace uniqid() with random_bytes() to create identifiers Jul 29, 2024
@fabpot
Copy link
Member

fabpot commented Jul 29, 2024

Thank you @xabbuh.

@fabpot fabpot merged commit 7368685 into symfony:7.2 Jul 29, 2024
9 of 10 checks passed
@xabbuh xabbuh deleted the issue-57588 branch July 29, 2024 08:09
@@ -521,7 +521,7 @@ public function add(string $body, array $headers, int $delayInMs = 0): string

try {
if ($delayInMs > 0) { // the delay is <= 0 for queued messages
$id = uniqid('', true);
$id = bin2hex(random_bytes(4));
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that random_bytes(4) only has 32 bits of entropy, which is less than the previous call to uniqid() with $more_entropy set to true. This should use at least random_bytes(8) for 64 bits of entropy and ideally random_bytes(16) for 128 bits (the latter would result in a slightly longer output of 32 characters instead of 23 for uniqid).

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe we should use the initial suggestion from the issue and use a UUID instead (see #57872)

nicolas-grekas added a commit that referenced this pull request Aug 12, 2024
…() (xabbuh)

This PR was merged into the 7.2 branch.

Discussion
----------

[Messenger][Notifier] use more entropy with random_bytes()

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | #57856 (comment)
| License       | MIT

Commits
-------

8152a02 use more entropy with random_bytes()
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.

Ban uniqid() from codebase
5 participants