Skip to content
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

File System Security Issue in Custom Auth Article #5845

Closed
mattjanssen opened this issue Oct 28, 2015 · 0 comments
Closed

File System Security Issue in Custom Auth Article #5845

mattjanssen opened this issue Oct 28, 2015 · 0 comments
Labels
hasPR A Pull Request has already been submitted for this issue. Security

Comments

@mattjanssen
Copy link
Contributor

https://github.com/symfony/symfony-docs/blob/master/cookbook/security/custom_authentication_provider.rst#the-listener

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). Could this Regex be updated to only accept a Base64 string [a-zA-Z+/]+={0,2} for the nonce?

@xabbuh xabbuh added Security hasPR A Pull Request has already been submitted for this issue. labels Oct 28, 2015
xabbuh added a commit that referenced this issue Feb 7, 2016
…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
@xabbuh xabbuh closed this as completed Feb 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hasPR A Pull Request has already been submitted for this issue. Security
Projects
None yet
Development

No branches or pull requests

2 participants