Replace sha1 and md5 hashing with sha256 algorithm #8609

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

jfsimon commented Jul 30, 2013

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #8583

@stof stof commented on an outdated diff Jul 30, 2013

...fony/Component/Translation/Dumper/XliffFileDumper.php
@@ -41,7 +41,7 @@ protected function format(MessageCatalogue $messages, $domain)
foreach ($messages->all($domain) as $source => $target) {
$translation = $dom->createElement('trans-unit');
- $translation->setAttribute('id', md5($source));
+ $translation->setAttribute('id', hash('sha256', $source));
@stof

stof Jul 30, 2013

Member

This is a BC break as it will generate a different id for the messages

Member

stof commented Jul 30, 2013

And this is breaking the tests

Contributor

jfsimon commented Jul 30, 2013

@stof fixed, thanks :)

Contributor

jfsimon commented Jul 30, 2013

@pwolanin I replaced each sha1 and md5 encryption with sha256, except for XLIFF id calculation (which uses sha1 to keep BC) and security hashes using sha1 + md5 concatenation. Is it okay for you?

Sounds like a good start - let me look at the security hashes part. That actually sounds liek somehting important to upgrade?

@fabpot fabpot and 1 other commented on an outdated diff Aug 2, 2013

...ty/Http/EntryPoint/DigestAuthenticationEntryPoint.php
@@ -41,7 +41,7 @@ public function __construct($realmName, $key, $nonceValiditySeconds = 300, Logge
public function start(Request $request, AuthenticationException $authException = null)
{
$expiryTime = microtime(true) + $this->nonceValiditySeconds * 1000;
- $signatureValue = md5($expiryTime.':'.$this->key);
+ $signatureValue = hash('sha256', $expiryTime.':'.$this->key);
@fabpot

fabpot Aug 2, 2013

Owner

This one must be reverted as using md5 is part of the Digest scheme, so we cannot control what to use.

@jfsimon

jfsimon Aug 2, 2013

Contributor

Ooops! Reverted.

@fabpot fabpot commented on an outdated diff Aug 2, 2013

...curity/Http/Firewall/DigestAuthenticationListener.php
@@ -193,15 +193,15 @@ public function validateAndDecode($entryPointKey, $expectedRealm)
$this->nonceExpiryTime = $nonceTokens[0];
- if (md5($this->nonceExpiryTime.':'.$entryPointKey) !== $nonceTokens[1]) {
+ if (hash('sha256', $this->nonceExpiryTime.':'.$entryPointKey) !== $nonceTokens[1]) {
@fabpot

fabpot Aug 2, 2013

Owner

same as above

@fabpot fabpot and 1 other commented on an outdated diff Aug 2, 2013

...curity/Http/Firewall/DigestAuthenticationListener.php
@@ -200,8 +200,8 @@ public function validateAndDecode($entryPointKey, $expectedRealm)
public function calculateServerDigest($password, $httpMethod)
{
@fabpot

fabpot Aug 2, 2013

Owner

l the changes in this file should also be reverted, no?

@pwolanin

pwolanin Aug 2, 2013

Yes, if this is also digest auth - that has to use the hash in the spec

@pwolanin pwolanin commented on an outdated diff Aug 6, 2013

src/Symfony/Component/DomCrawler/Field/FileFormField.php
@@ -63,7 +63,7 @@ public function setValue($value)
$name = $info['basename'];
// copy to a tmp location
- $tmp = sys_get_temp_dir().'/'.sha1(uniqid(mt_rand(), true));
+ $tmp = sys_get_temp_dir().'/'.hash('sha256', uniqid(mt_rand(), true));
@pwolanin

pwolanin Aug 6, 2013

These seems a bit silly - you don't need to hash the uniqid, it's already supposed to be unique!

@pwolanin pwolanin commented on an outdated diff Aug 6, 2013

...m/Extension/Csrf/CsrfProvider/DefaultCsrfProvider.php
@@ -45,7 +45,7 @@ public function __construct($secret)
*/
public function generateCsrfToken($intention)
{
- return sha1($this->secret.$intention.$this->getSessionId());
+ return hash('sha256', $this->secret.$intention.$this->getSessionId());
@pwolanin

pwolanin Aug 6, 2013

this should be a HMAC, not just a hash

@pwolanin pwolanin commented on an outdated diff Aug 6, 2013

...tension/Csrf/CsrfProvider/DefaultCsrfProviderTest.php
@@ -57,7 +57,7 @@ public function testGenerateCsrfTokenOnUnstartedSession()
$token = $this->provider->generateCsrfToken('foo');
- $this->assertEquals(sha1('SECRET'.'foo'.session_id()), $token);
+ $this->assertEquals(hash('sha256', 'SECRET'.'foo'.session_id()), $token);
@pwolanin

pwolanin Aug 6, 2013

should be HMAC

@pwolanin pwolanin commented on an outdated diff Aug 6, 2013

...tension/Csrf/CsrfProvider/DefaultCsrfProviderTest.php
@@ -65,7 +65,7 @@ public function testIsCsrfTokenValidSucceeds()
{
session_start();
- $token = sha1('SECRET'.'foo'.session_id());
+ $token = hash('sha256', 'SECRET'.'foo'.session_id());

@pwolanin pwolanin commented on an outdated diff Aug 6, 2013

...tension/Csrf/CsrfProvider/DefaultCsrfProviderTest.php
@@ -74,7 +74,7 @@ public function testIsCsrfTokenValidFails()
{
session_start();
- $token = sha1('SECRET'.'bar'.session_id());
+ $token = hash('sha256', 'SECRET'.'bar'.session_id());

@pwolanin pwolanin commented on an outdated diff Aug 6, 2013

...tension/Csrf/CsrfProvider/SessionCsrfProviderTest.php
@@ -48,7 +48,7 @@ public function testGenerateCsrfToken()
$token = $this->provider->generateCsrfToken('foo');
- $this->assertEquals(sha1('SECRET'.'foo'.'ABCDEF'), $token);
+ $this->assertEquals(hash('sha256', 'SECRET'.'foo'.'ABCDEF'), $token);

@pwolanin pwolanin commented on an outdated diff Aug 6, 2013

...onent/Security/Tests/Http/Firewall/DigestDataTest.php
- $response = md5(
- md5($username.':'.$realm.':'.$password).':'.$nonce.':'.$nc.':'.$cnonce.':'.$qop.':'.md5($method.':'.$uri)
+ $response = hash('sha256',
@pwolanin

pwolanin Aug 6, 2013

Looks like this is also part of the digest auth code?

Owner

fabpot commented Aug 9, 2013

ping @jfsimon

Contributor

jfsimon commented Aug 31, 2013

@pwolanin thank you for your help.
@fabpot sorry for the delay.

fabpot closed this in cade045 Aug 31, 2013

@fabpot fabpot added a commit that referenced this pull request May 15, 2014

@fabpot fabpot bug #10904 [HttpKernel] Replace sha1 with sha256 in recently added te…
…sts (jakzal)

This PR was merged into the 2.4 branch.

Discussion
----------

[HttpKernel] Replace sha1 with sha256 in recently added tests

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | no
| Fixed tickets | -
| License       | MIT

Tests were introduced in #10896 and are broken starting with 2.4, since the hashing algorithm has changed (introduced in #8609).

Commits
-------

255544f [HttpKernel] Replace sha1 with sha256 in recently added tests.
be3c0dc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment