Problem/Motivation

Use of a Broken or Risky Cryptographic Algorithm in modules/contrib/entity_browser/src/plugin/field/fieldwidget/entityreferencebrowserwidget.php (line 522)

'#name' => $this->fieldDefinition->getName() . '_remove_' . $entity->id() . '_' . $row_id . '_' . md5(json_encode($field_parents)),

See https://www.drupal.org/node/845876

the md5() and sha1() hash functions should never be used in any code

this can be a problem if, for example, Government entities require such audits - which would then require additional documentation to verify that they are indeed, not a security issue.

Proposed resolution

use \Drupal\Component\Utility\Crypt::hashBase64($data)

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

suresh kumara created an issue. See original summary.

alexmoreno’s picture

Agree, as per >D7 policy, weak cryptographic algorithms should not be used. See: https://www.drupal.org/node/845876

There are other files where this is an issue:

docroot/modules/contrib/entity_browser/src/Element/EntityBrowserElement.php 177
docroot/modules/contrib/entity_browser/src/Plugin/Field/FieldWidget/EntityReferenceBrowserWidget.php 562
docroot/modules/contrib/entity_browser/src/Plugin/Field/FieldWidget/EntityReferenceBrowserWidget.php 579
docroot/modules/contrib/entity_browser/src/Plugin/Field/FieldWidget/FileBrowserWidget.php 388
docroot/modules/contrib/entity_browser/src/Plugin/Field/FieldWidget/FileBrowserWidget.php 405

omkar06’s picture

Assigned: Unassigned » omkar06
omkar06’s picture

Version: 8.x-2.0 » 8.x-2.2
StatusFileSize
new6.74 KB

Replaced usage of sha1 and md5 with \Drupal\Component\Utility\Crypt::hashBase64($data)

omkar06’s picture

Status: Active » Needs review
omkar06’s picture

Version: 8.x-2.2 » 8.x-2.0
Assigned: omkar06 » Unassigned
omkar06’s picture

Patch for 8.2.2 version.

omkar06’s picture

Version: 8.x-2.0 » 8.x-2.x-dev
ducktape’s picture

Rerolled against dev.

alexmoreno’s picture

Status: Needs review » Reviewed & tested by the community

LGTM, tested and working in a platform with a few dozen sites running.

oknate’s picture

Title: Use of a Broken or Risky Cryptographic Algorithm in modules/contrib/entity_browser/src/plugin/field/fieldwidget/entityreferencebrowserwidget.php in (line 522) » the md5() and sha1() hash functions should never be used in any code
Issue summary: View changes
oknate’s picture

Issue summary: View changes
oknate’s picture

  • oknate committed 5531e0a on 8.x-2.x authored by omkar06
    Issue #3000587 by omkar06: the md5() and sha1() hash functions should...

  • oknate committed d8a857f on 8.x-1.x authored by omkar06
    Issue #3000587 by omkar06: the md5() and sha1() hash functions should...
oknate’s picture

Status: Reviewed & tested by the community » Fixed

This makes sense. The new hashing function is the recommended one, and it returns a consistent hash, and it won’t get flagged by government audits.

Status: Fixed » Closed (fixed)

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

guptahemant’s picture

hi @oknate

It seems the patch in this issue reverted the code which was added as a fix for this issue https://www.drupal.org/project/entity_browser/issues/3000587,

I.e
$this->fieldDefinition->getUniqueIdentifier()
got replaced by
$this->fieldDefinition->get('uuid')

Please check i think it would need to be reverted.

Thanks

oknate’s picture

Status: Closed (fixed) » Needs work

Thank you, guptahemant!

I have reverted this for now. We need a new patch that uses $this->fieldDefinition->getUniqueIdentifier() instead of $this->fieldDefinition->get('uuid')

mpp’s picture

Status: Needs work » Needs review
StatusFileSize
new6.2 KB
anybody’s picture

Status: Needs review » Reviewed & tested by the community

Well done @mpp, @oknate, would you also have a look?

mpp’s picture

Thanks for the feedback @Anybody.

joseph.olstad’s picture

there are other non cryptographic uses for sha1 , pretty strong language above.

https://www.drupal.org/project/media_unique

fernly’s picture

StatusFileSize
new6.17 KB

Reroll of patch 20 against version 8.x-2.8.

dave reid’s picture

Status: Reviewed & tested by the community » Needs work

Patch no longer applies and needs to be re-rolled or MR opened.

fernly’s picture

Status: Needs work » Needs review
StatusFileSize
new5.96 KB

Reroll of patch 24 against current 8.x-2.x-dev version (should work on 8.x-2.10).

anybody’s picture

Could someone turn this into a MR finally to get this fixed?

anybody’s picture

Thanks @joseph.olstad I'm willing to merge this once it's RTBC'd!

anybody’s picture

Status: Needs review » Reviewed & tested by the community

Okay it's used in a lot similar places in core, so I'm fine with that!
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Util...

anybody’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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