Skip to content

Commit

Permalink
minor #6234 File System Security Issue in Custom Auth Article (finish…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
xabbuh committed Feb 7, 2016
2 parents ff938a4 + 673fd71 commit 557f32d
Showing 1 changed file with 6 additions and 3 deletions.
9 changes: 6 additions & 3 deletions cookbook/security/custom_authentication_provider.rst
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ set an authenticated token in the security context if successful.
{
$request = $event->getRequest();
$wsseRegex = '/UsernameToken Username="([^"]+)", PasswordDigest="([^"]+)", Nonce="([^"]+)", Created="([^"]+)"/';
$wsseRegex = '/UsernameToken Username="([^"]+)", PasswordDigest="([^"]+)", Nonce="([a-zA-Z0-9+/]+={0,2})", Created="([^"]+)"/';
if (!$request->headers->has('x-wsse') || 1 !== preg_match($wsseRegex, $request->headers->get('x-wsse'), $matches)) {
return;
}
Expand Down Expand Up @@ -256,14 +256,17 @@ the ``PasswordDigest`` header value matches with the user's password.
// Validate that the nonce is *not* used in the last 5 minutes
// if it has, this could be a replay attack
if (file_exists($this->cacheDir.'/'.$nonce) && file_get_contents($this->cacheDir.'/'.$nonce) + 300 > time()) {
if (
file_exists($this->cacheDir.'/'.md5($nonce))
&& file_get_contents($this->cacheDir.'/'.md5($nonce)) + 300 > time()
) {
throw new NonceExpiredException('Previously used nonce detected');
}
// If cache directory does not exist we create it
if (!is_dir($this->cacheDir)) {
mkdir($this->cacheDir, 0777, true);
}
file_put_contents($this->cacheDir.'/'.$nonce, time());
file_put_contents($this->cacheDir.'/'.md5($nonce), time());
// Validate Secret
$expected = base64_encode(sha1(base64_decode($nonce).$created.$secret, true));
Expand Down

0 comments on commit 557f32d

Please sign in to comment.