Problem/Motivation

Per Drupal secure coding standards at http://drupal.org/node/845876

md5 and sha1 should not be used any place in Drupal core since 7.0, but are re-introduced in the DiffEngine for 8.x:

core/lib/Drupal/Component/Diff/DiffEngine.php
242:      return md5($line);

Since this is for non-cryptographic purposes, crc32b will do.

Steps to reproduce

Proposed resolution

Use crc32b

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#22 1884836-20.patch714 byteslendude
#21 1884836-20.patch0 byteslendude
#11 replace_md5_calls_in-1884836-11.patch714 bytescilefen
#11 interdiff.txt649 bytescilefen
#8 1884836-8.patch724 bytesmgifford
#2 1884836-1.patch679 bytespwolanin

Comments

pwolanin’s picture

pwolanin commented
StatusFileSize
new 1884836-1.patch679 bytes
pwolanin’s picture

pwolanin commented
Status: Active » Needs review
pwolanin’s picture

pwolanin commented
Assigned: Unassigned » pwolanin

alansaviolobo queued 2: 1884836-1.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2: 1884836-1.patch, failed testing.

mgifford’s picture

mgifford
he/him
Primary language English
commented
Assigned: pwolanin » Unassigned
Issue summary: View changes
Issue tags: +Security

core/lib/Drupal/Component/Diff/Engine/DiffEngine.php: return md5($line);
core/lib/Drupal/Core/Password/PhpassHashedPassword.php: $password = md5($password);

Also it's in a few vendor libraries too.

mgifford’s picture

mgifford
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 1884836-8.patch724 bytes

Decided to skip PhpassHashedPassword.php as there's logic there for sha256 too.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Yeah, that change there totally makes sense.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Debating the md5() has been done and whilst there is no need for a cryptographic hash here this is the only case of md5() outside of dealing with old passwords. However in other places we're using hash('crc32b', $value); where a hash is need for non-cryptographic purposes.

cilefen’s picture

cilefen commented
Title: replace md5 calls in DiffEngine with sha2 hashes » replace md5 calls in DiffEngine with crc32b hashes
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Security
StatusFileSize
new interdiff.txt649 bytes
new replace_md5_calls_in-1884836-11.patch714 bytes

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented

Reroll.

Is this still something we want to fix?

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
StatusFileSize
new 1884836-20.patch0 bytes

Now with the actual patch....

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
StatusFileSize
new 1884836-20.patch714 bytes

Sheesh, now with the actual patch that actually contains data....

quietone’s picture

quietone commented
Issue summary: View changes

The referenced doc page says to use \Drupal\Component\Utility\Crypt::hashBase64($data). Shouldn't that be used?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@quietone this is a non cryptographic hash so as per issue summary crc32b will do.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 8.9.x-dev » 9.3.x-dev

This will only happen in D9 now...

quietone’s picture

quietone commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Bug Smash Initiative

@alexpott, thanks. I wasn't sure.

OK. Then I guess this can go to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Eventually we should move to xxHash for non-cryptographic hashes - once the minimum version is PHP 8.1 - see https://php.watch/versions/8.1/xxHash

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed abc2310bb7 and pushed to 9.3.x. Thanks!

Whilst this is categorised as a bug I think it is okay to fix in 9.3.x only.

  • alexpott committed abc2310 on 9.3.x 
    Issue #1884836 by Lendude, cilefen, pwolanin, mgifford, alexpott:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.