File System Security Issue in Custom Auth Article #5846
Conversation
I hope to address this security concern: If `$token->nonce` is set to [ANY USER INPUT] and later we run `file_put_contents($token->nonce, time())` are we allowing hackers to destroy any www-writable file in the system?
I did notice that `$nonce` is run through `base64_decode($nonce)` later in the article, implying nonce needs to be a Base64 string. Could this Regex be updated to only accept a Base64 string `[a-zA-Z+/]+={0,2}` for the nonce?
At the same time, Base64 allows `/` characters, so `file_put_contents()` would fail in those cases, so even this change, while secure, seems flawed. Replace [+/] with [-_]?
|
What about not using the nonce itself as the filename but passing it to a hash function or normalise it another way first? |
|
👍 using a hash function |
|
And force Base64 string in regex or not? We still have a |
|
👍 good catch |
|
👍 to enforce a base64 string (specs tell that the nonce should be "16 bytes long and is passed along as a base64 encoded value.") and use a hash function for the filename. Can you still make these changes, @mattjanssen? Otherwise, we'll take this PR over (while trying to keep your credits). |
|
@wouterj You're totally welcome to take charge on this PR. Don't worry about credit :) But thank you for the consideration. I'll keep watch to make sure the original issue is ultimately addressed, too. |
|
Alright, see #6234 for the finished PR. |
…ed) (mattjanssen, WouterJ) This PR was merged into the 2.3 branch. Discussion ---------- File System Security Issue in Custom Auth Article (finished) Finishes #5846 Original description: > | Q | A > | ------------- | --- > | Doc fix? | yes > | New docs? | no > | Applies to | all > | Fixed tickets | #5845 > > I hope to address this security concern: If `$token->nonce` is set to [ANY USER INPUT] and later we run `file_put_contents($token->nonce, time())` are we allowing hackers to destroy any www-writable file in the system? > > I did notice that `$nonce` is run through `base64_decode($nonce)` later in the article, implying nonce needs to be a Base64 string. Could this Regex be updated to only accept a Base64 string `[a-zA-Z+/]+={0,2}` for the nonce? > > At the same time, Base64 allows `/` characters, so `file_put_contents()` would fail in those cases, so even this change, while secure, seems flawed. Replace [+/] with [-_]? Commits ------- 673fd71 Hash nonce when using as file name 5f125f3 File System Security Issue in Custom Auth Article
I hope to address this security concern: If
$token->nonceis set to [ANY USER INPUT] and later we runfile_put_contents($token->nonce, time())are we allowing hackers to destroy any www-writable file in the system?I did notice that
$nonceis run throughbase64_decode($nonce)later in the article, implying nonce needs to be a Base64 string. Could this Regex be updated to only accept a Base64 string[a-zA-Z+/]+={0,2}for the nonce?At the same time, Base64 allows
/characters, sofile_put_contents()would fail in those cases, so even this change, while secure, seems flawed. Replace [+/] with [-_]?