Problem/Motivation

State what you believe is wrong or missing from the current standards.

Proposal:
mirror into the coding standards this standard of using only Drupal's hash helper functions when a hash is needed:

https://www.drupal.org/docs/7/security/writing-secure-code-0/use-of-hash...

The older functions are not significantly faster and offer less collision resistance, which is a key element of hash function quality. If there are (atypical and uncommon) cases where code actually needs a shorter hash string, it is better to truncate a sha-2 hash instead of using one of the deprecated functions:

http://crypto.stackexchange.com/questions/9435/is-truncating-a-sha512-ha...

compared to using sha-1, "truncating one of the SHA-2 functions to 160 bits is around 2^20 times stronger when it comes to collision resistance."

Note that 160 bits means taking 27 characters of the base64 encoded output. The absolute minimum substring length used should be 21 chars (126 bits) of base 64 output. Any use of a substring should be clearly justified in code comments.

Basically - it should be a coding standards violation and flagged automatically if people are using a different method to hash values.

related current core patch: #2569119: Use Crypt::hashBase64(), not hash('crc32b') or sha1 for placeholder tokens

Prior coding standard issue for Core that's too meandering: #2268875: [Policy, no patch] Using md5()/sha1()/crc32b in Drupal code

Benefits

If we adopted this change, the Drupal Project would benefit by ...

Three supporters required

  1. https://www.drupal.org/u/{userid} (yyyy-mm-dd they added support)
  2. https://www.drupal.org/u/{userid} (yyyy-mm-dd they added support)
  3. https://www.drupal.org/u/{userid} (yyyy-mm-dd they added support)

Proposed changes

Provide all proposed changes to the Drupal Coding standards. Give a link to each section that will be changed, and show the current text and proposed text as in the following layout:

1. {link to the documentation heading that is to change}

Current text

Add current text in blockquotes

Proposed text

Add proposed text in blockquotes

2. Repeat the above for each page or sub-page that needs to be changed.

Remaining tasks

  1. Create this issue in the Coding Standards queue, using the defined template
  2. Add supporters
  3. Create a Change Record
  4. Review by the Coding Standards Committee
  5. Coding Standards Committee takes action as required
  6. Discussed by the Core Committer Committee, if it impacts Drupal Core
  7. Final review by Coding Standards Committee
  8. Documentation updates
    1. Edit all pages
    2. Publish change record
    3. Remove 'Needs documentation edits' tag
  9. If applicable, create follow-up issues for PHPCS rules/sniffs changes

For a full explanation of these steps see the Coding Standards project page

Comments

pwolanin created an issue. See original summary.

pwolanin’s picture

Title: [Policy, no patch] Reflect secure coding policy in coding standard of using Crypt::hashBase64() » [Policy, no patch] Reflect secure coding policy in coding standard of only using Crypt::hashBase64() for hashes
Issue summary: View changes
greggles’s picture

Issue summary: View changes

(just cleaning up some language, don't mind me)

pwolanin’s picture

Issue summary: View changes
jthorson’s picture

Status: Active » Needs work
Issue tags: +Needs issue summary update

This was brought up at the last coding standards meeting. To help facilitate discussion, we'd like to request an update to the issue summary, with proposed wording and a suggested location for where it would be inserted into the existing coding standards documentation.

drunken monkey’s picture

Great idea! I see I also have some use of SHA-1 and even MD5 in my modules, not really justified in any way.
Having this as a coding standard should help make people aware of this problem.

(And we already have a standard for always using t() calls, so there cannot really be any discussion about whether coding standards are the right place for such a rule.)

lokesh jamadar’s picture

Assigned: Unassigned » lokesh jamadar
quietone’s picture

Assigned: lokesh jamadar » Unassigned

Removing assignment since this hasn't been worked on for over 2 years.

quietone’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

There has been no work here in 8 years, usually indicative of no interest in a change.

If there is interest then complete the issue summary thanks. Otherwise this issue may be closed after 3 months.

quietone’s picture

Title: [Policy, no patch] Reflect secure coding policy in coding standard of only using Crypt::hashBase64() for hashes » Reflect secure coding policy in coding standard of only using Crypt::hashBase64() for hashes
quietone’s picture

Status: Reviewed & tested by the community » Postponed (maintainer needs more info)
quietone’s picture

Status: Postponed (maintainer needs more info) » Closed (won't fix)

There is no interest in this for 9 years. I asked 1 year ago here and two weeks ago in the coding standards Slack channel. Only Jonathan105 replied, supporting the idea of closing this.

If you disagree, this can be re-opened. Or you can create a new issue and reference this one.

Thanks.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.