nodejs / nodejs/node

Equal hashes on non-equal JS strings are dangerous to the ecosystem

Open
#60,267 4 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

security
Dominant language
JavaScript
Stars
122k
Forks
37.3k
Avg merge
4d 2h
Merged PRs (30d)
283

Description

Instead, anything in crypto module accepting JS strings should verify that the strings are well-formed before converting them to Buffers, and throw

In before: the argument that those strings would be treated as equal in other contexts is not valid

E.g., using bn.js@4:

import BN from 'bn.js' // 4
import { hash } from 'node:crypto'
const [a, b] = JSON.parse(payload) // user input
console.log('Hashes equal?', hash('sha256', a) === hash('sha256', b))
console.log('Strings equal?', a === b)
console.log('bn.js values:', (new BN(a)).toNumber(), (new BN(b)).toNumber())

Output:

Hashes equal? true
Strings equal? false
bn.js values: 55229 57036

If anything anywhere uses string hashes for any purpose like k/nonce calculation (ref: GHSA-vjh7-7g9h-fjfh), e.g. in curve cryptography or in a block/stream cipher, this can lead to private key/data exposure

Also obviously it allows to do things like this:

import { hash } from 'node:crypto'
const [a, b, c, d] = JSON.parse(payload) // user input
const sha256 = (x) => hash('sha256', x)
console.log('a === b?', sha256(a) === sha256(b))
console.log('c === d?', sha256(c) === sha256(d))
console.log('a + c === b + d?', sha256(a + c) === sha256(b + d))
a === b? true
c === d? true
a + c === b + d? false

As a solution, enforce usage of String.prototype.isWellFormed on all string input and throw


Obviously, also applicable to sign/verify and other APIs:

import { generateKeyPairSync, createSign, createVerify } from 'node:crypto'
const { privateKey, publicKey } = generateKeyPairSync('rsa', { modulusLength: 2048 })

const [a, b] = JSON.parse(payload) // user input

const sign = createSign('SHA256')
sign.update(a)
sign.end()
const signature = sign.sign(privateKey)

const verify = createVerify('SHA256')
verify.update(b)
verify.end()

console.log('types:', typeof a, typeof b)
console.log('verified?', verify.verify(publicKey, signature))
console.log('a === b?', a === b)
types: string string
verified? true
a === b? false

WebCrypto is not affected as it doesn't accept strings

Node.js accepts JS strings but operates on byte arrays, and js-string-to-byte-array transform is not injective
There should be a safeguard against transform collisions

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by inventorying the crypto module entry points that accept JavaScript strings, including hash, sign, and verify, and trace their string-to-byte-array conversion. Compare the examples with String.prototype.isWellFormed behavior; done requires a defined safeguard across the relevant APIs and tests showing ill-formed strings are not treated as equivalent.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js
Domain
backend, cryptography, security
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.