[Security] [Core] String utils refactor #14028
Conversation
|
👍 looks great to me |
ircmaxell
changed the title
|
|
||
| // Extend the known string to avoid uninitialized string offsets | ||
| $knownString .= $userInput; | ||
| if ($userLen != $knownLen) { |
There was a problem hiding this comment.
We also prefer strong comparison !==.
|
Closing in favor of #14024, which includes these changes (it's much easier for everyone to work on one PR). |
|
This makes me sad. |
|
@hhamon thanks for the review. If this ever gets re-opened I'll handle the issues. But seeing as it's closed, there's no point. |
|
@ircmaxell Sorry about this. I thought the third PR was really just about getting the changes from your PR and #13984, and then adding on top of it, but I read too fast (just by looking at the commit list.) I apologize for the mistake and I'm reopening this PR. I'm no security expert and I trust you and @sarciszewski on those matters, so I will let you let me know which one(s) should be merged. |
…rcmaxell) This PR was submitted for the 2.7 branch but it was merged into the 2.3 branch instead (closes #14028). Discussion ---------- [Security] [Core] String utils refactor | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | This supersedes #13984 (it includes it, but also includes additional refactoring). Since length information is leaked in any case, preventing unnecessary duplication of secrets is important. Since casting will *always* make a copy, we only cast if absolutely necessary. Additionally, appending will create a new copy of the secret, so we avoid doing that, but instead only iterate over the minimum of the two strings. Commits ------- 45cfb44 Change behavior to mirror hash_equals() returning early if there is a length mismatch 8269589 CS fixing bdea4ba Prevent modifying secrets as much as possible 76b36d3 Update StringUtils.php 7221efc Whitespace 56ed71c Update StringUtils.php
|
I've merged this PR in the 2.3 branch (I've fixed the CS in 2c67400). Thank you @ircmaxell and sorry for the confusion yesterday. |
|
👍 glad to see this resolved in days rather than months :) |
This supersedes #13984 (it includes it, but also includes additional refactoring).
Since length information is leaked in any case, preventing unnecessary duplication of secrets is important. Since casting will always make a copy, we only cast if absolutely necessary. Additionally, appending will create a new copy of the secret, so we avoid doing that, but instead only iterate over the minimum of the two strings.