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
| Comment | File | Size | Author |
|---|---|---|---|
| #26 | 3000587_26.patch | 5.96 KB | fernly |
| #24 | 3000587_24.patch | 6.17 KB | fernly |
| #20 | 3000587_20.patch | 6.2 KB | mpp |
| #9 | entity_browser-replace_crypto-30005870-8.patch | 6.73 KB | ducktape |
| #7 | entity_browser-8.2.2--replace_risky_crypto_algorithm_usage-3000587-7.patch | 6.72 KB | omkar06 |
Issue fork entity_browser-3000587
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
Comment #2
alexmoreno commentedAgree, 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
Comment #3
omkar06 commentedComment #4
omkar06 commentedReplaced usage of sha1 and md5 with \Drupal\Component\Utility\Crypt::hashBase64($data)
Comment #5
omkar06 commentedComment #6
omkar06 commentedComment #7
omkar06 commentedPatch for 8.2.2 version.
Comment #8
omkar06 commentedComment #9
ducktape commentedRerolled against dev.
Comment #10
alexmoreno commentedLGTM, tested and working in a platform with a few dozen sites running.
Comment #11
oknateComment #12
oknateComment #13
oknateComment #16
oknateThis 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.
Comment #18
guptahemant commentedhi @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
Comment #19
oknateThank you, guptahemant!
I have reverted this for now. We need a new patch that uses
$this->fieldDefinition->getUniqueIdentifier()instead of$this->fieldDefinition->get('uuid')Comment #20
mpp commentedComment #21
anybodyWell done @mpp, @oknate, would you also have a look?
Comment #22
mpp commentedThanks for the feedback @Anybody.
Comment #23
joseph.olstadthere are other non cryptographic uses for sha1 , pretty strong language above.
https://www.drupal.org/project/media_unique
Comment #24
fernly commentedReroll of patch 20 against version 8.x-2.8.
Comment #25
dave reidPatch no longer applies and needs to be re-rolled or MR opened.
Comment #26
fernly commentedReroll of patch 24 against current 8.x-2.x-dev version (should work on 8.x-2.10).
Comment #27
anybodyCould someone turn this into a MR finally to get this fixed?
Comment #29
anybodyThanks @joseph.olstad I'm willing to merge this once it's RTBC'd!
Comment #30
anybodyOkay 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...
Comment #31
anybody