Skip to content

Commit

Permalink
slight adjustment to hashing
Browse files Browse the repository at this point in the history
  • Loading branch information
taylorotwell committed Jul 27, 2020
1 parent 3c13945 commit c9ce261
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 2 deletions.
2 changes: 1 addition & 1 deletion src/Illuminate/Cookie/Middleware/EncryptCookies.php
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ protected function decrypt(Request $request)
$value = $this->decryptCookie($key, $cookie);

$request->cookies->set(
$key, strpos($value, sha1($key.'v2').'|') !== 0 ? null : substr($value, 41)
$key, strpos($value, hash_hmac('sha1', $key.'v2', $this->encrypter->getKey()).'|') !== 0 ? null : substr($value, 41)
);
} catch (DecryptException $e) {
$request->cookies->set($key, null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,7 @@ protected function prepareCookiesForRequest()
}

return collect($this->defaultCookies)->map(function ($value, $key) {
return encrypt(sha1($key.'v2').'|'.$value, false);
return encrypt(hash_hmac('sha1', $key.'v2', base64_decode(substr(config('app.key'), 7))).'|'.$value, false);
})->merge($this->unencryptedCookies)->all();
}

Expand Down

3 comments on commit c9ce261

@Tofandel
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this added encryption step adds to the security of cookies except size?

Also it's missing a fallback to the previous encryption so all previously encrypted cookies are lost on upgrade..

Personally reverting to the old logic in my middleware, I see 0 benefit to this

@GrahamCampbell
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this provides additional security, not related to output size.

@Tofandel
Copy link
Contributor

@Tofandel Tofandel commented on c9ce261 Sep 25, 2020

Choose a reason for hiding this comment

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

Does it? The value is already encrypted with a salt.. This doesn't add much security but does increase the size of the cookie quite a lot (150% approx)

I guess maybe in the sense that it prevents you from using any laravel encrypted string in a cookie so you can't just swap the name of a cookie maybe it does a bit

But seriously the authentication cookie is now 2Kb (2092Chars) which is half of the default limit this is way too much something needs to be done about this
With session and XSRF token the total cookie size of laravel is 3Kb this is way too close to the limit and prevents us from adding other encrypted cookies

Please sign in to comment.