Fix bug in second hash calculation

[ischasny]

• Fix a bug in how second hash is calculated;
• Aligned magic values with the DHT spec;
• Fix nonce to be derived from passphrase || payload instead of just passphrase to avoid attacks on gcm with repeated nonces (example).

[codecov-commenter]

Codecov Report

Patch coverage has no change and project coverage change: -0.04 ⚠️

Comparison is base (9aef865) 60.14% compared to head (211c6ea) 60.11%.

❗ Current head 211c6ea differs from pull request most recent head d52a261. Consider uploading reports for the commit d52a261 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #141      +/-   ##
==========================================
- Coverage   60.14%   60.11%   -0.04%     
==========================================
  Files          20       20              
  Lines        3450     3452       +2     
==========================================
  Hits         2075     2075              
- Misses       1079     1081       +2     
  Partials      296      296              

Impacted Files	Coverage Δ
store/dhash/dhash.go	0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[ischasny]

CC @Jorropo @guillaumemichel

[Jorropo]

AFAIT this means that two different peer will use the same nonce (since it is just a salted hash of the multihash). Assuming this is true:

This is not OK, because then an attacker can use multiple different provider records from multiple different nodes using the same nonce to recover the derived key, which is the multihash. (the actual attack relies on cipher textes being different, here the cipher text should be the peer id)
If this what the spec says, the spec needs to be updated.

You can search: aes gcm duplicated nonce this is a well known attack.
Other symetric encryptions constructions supports using duplicated nonce with minimal issue (attackers can match duplicates without revealing their content, either based on blocks of the stream without feedback, or until the first duplicate is found if there is feedback).
We are not using this here tho, and this is unsafe.

cc @guillaumemichel we had brainstorming together around this, I was maybe not clear at the end but this was assuming the nonce was salted with the cipher text and we don't mind duplicates being matched. not related

[Jorropo]

Responding to:

We derive nonse from the passphrase so that we can deterministically produce the same result of hash(mh) -> EncPeerID given the same multihash / peerID.

You need to salt this by the peer id then:

nonce := SHA256(append(append(binary.AppendUvarint(noncePrefix, uint64(len(payload))), payload...)), passphrase...))

[ischasny]

Thanks for pointing this out - will push a fix in shortly

[Jorropo]

Btw I can't comment on untouched code, but you should really make:

func SHA256(dest []byte, payloads ...[]byte) []byte {
	h := sha256.New()
	for _, payload := range payloads {
		h.Write(payload)
	}
	return h.Sum(dest)
}

This will allow to rewrite:

nonce := SHA256(append(append(binary.AppendUint64(noncePrefix, uint64(len(payload))), payload...)), passphrase...))

into the much more readable:

nonce := SHA256(nil, noncePrefix, binary.Uvarint(uint64(len(payload))), payload, passphrase)

[guillaumemichel]

Concerning the payloadLen, the exact format isn't defined yet in ipfs/specs#373.

Would it make more sense to use a constant byte array (e.g make([]byte, 8)) or to use an unsigned varint. As we mostly encrypt peerids, that are quite short, we don't always need a big number for the payload len. A varint allows to use less bytes to describe the payload len, and the maximal payload len can also be very large if needed.

I think that I am more in favor of an unsigned varint format for the payload len, as it gives more agility. WDYT?

cc: @ischasny @Jorropo @masih

[ischasny]

we don't always need a big number for the payload len

These values are hashed over, so payloadLen isn't going to be a part of the encrypted value, i.e. see line 79: nonce := sha256Multiple(nil, noncePrefix, payloadLen, payload, passphrase)[:nonceLen].

Does that make sense?

[guillaumemichel]

Alright, all good! 👍🏻

[guillaumemichel]

The format for Encrypted PeerID/Metadata was recently updated in the DHT Spec. The format should be [encryption_varint, payload_len, nonce, encrypted_payload]. See ipfs/specs#373

The encryption varint for aes-gcm-256 is 0x8040 and the multicodec is 0x2000 (See multiformats/multicodec#314 (comment))

[ischasny]

Great point! As IPNI doesn't have anything stored after EncPeerID the encryption_varint and payload_len can be appended on retrieval.

[guillaumemichel]

Perfect, just wanted to make sure we are on the same page 😄