Skip to content

Fix bug in second hash calculation #141

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Mar 3, 2023
Merged

Fix bug in second hash calculation #141

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
fc6b7af
Fix bug in second hash calculation
ischasny Mar 2, 2023
211c6ea
Review feedback
ischasny Mar 2, 2023
d52a261
Add dhash test
ischasny Mar 2, 2023
97b536b
Fix go.mod
ischasny Mar 2, 2023
f71ac58
Update store/dhash/dhash.go
ischasny Mar 3, 2023
8397bed
Update store/dhash/dhash_test.go
ischasny Mar 3, 2023
3c83f4e
Update store/dhash/dhash_test.go
ischasny Mar 3, 2023
b6de4a3
Update store/dhash/dhash_test.go
ischasny Mar 3, 2023
bf7b779
Update store/dhash/dhash_test.go
ischasny Mar 3, 2023
f46ab32
Update store/dhash/dhash_test.go
ischasny Mar 3, 2023
db78586
Update store/dhash/dhash_test.go
ischasny Mar 3, 2023
b96a4f7
Update store/dhash/dhash_test.go
ischasny Mar 3, 2023
3318d7d
Update store/dhash/dhash_test.go
ischasny Mar 3, 2023
5fbbb28
Clean up
ischasny Mar 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 21 additions & 18 deletions store/dhash/dhash.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,15 @@ import (
const (
// nonceLen defines length of the nonce to use for AESGCM encryption
nonceLen = 12
// keysize defines the size of multihash key
keysize = 32
)

var (
// secondHashPrefix is a prefix that a mulithash is prepended with when calculating a second hash
secondHashPrefix = []byte("CR_DOUBLEHASH\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00")
// deriveKeyPrefix is a prefix that a multihash is prepended with when deriving an encryption key
deriveKeyPrefix = []byte("CR_ENCRYPTIONKEY\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00")
// noncePrefix is a prefix that a multihash is prepended with when calculating a nonce
noncePrefix = []byte("CR_NONCE\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00")
)

// SecondSHA returns SHA256 over the payload
Expand All @@ -26,17 +33,13 @@ func SHA256(payload, dest []byte) []byte {

// SecondMultihash calculates SHA256 over the multihash and wraps it into another multihash with DBL_SHA256 codec
func SecondMultihash(mh multihash.Multihash) (multihash.Multihash, error) {
prefix := []byte("CR_DOUBLEHASH")
mh, err := multihash.Sum(append(prefix, mh...), multihash.DBL_SHA2_256, keysize)
if err != nil {
return nil, err
}
return mh, nil
digest := SHA256(append(secondHashPrefix, mh...), nil)
return multihash.Encode(digest, multihash.DBL_SHA2_256)
}

// deriveKey derives encryptioin key from the passphrase using SHA256
func deriveKey(passphrase []byte) []byte {
return SHA256(append([]byte("AESGCM"), passphrase...), nil)
return SHA256(append(deriveKeyPrefix, passphrase...), nil)
}

// DecryptAES decrypts AES payload using the nonce and the passphrase
Expand Down Expand Up @@ -64,7 +67,7 @@ func EncryptAES(payload, passphrase []byte) ([]byte, []byte, error) {
// Create initialization vector (nonse) to be used during encryption
// Nonce is derived from the mulithash (passpharase) so that encrypted payloads
// for the same multihash can be compared to each other without having to decrypt
nonce := SHA256(passphrase, nil)[:nonceLen]
nonce := SHA256(append(noncePrefix, passphrase...), nil)[:nonceLen]
Copy link

@Jorropo Jorropo Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@Jorropo Jorropo Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Create cypher and seal the data
block, err := aes.NewCipher(derivedKey)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand All @@ -83,13 +86,13 @@ func EncryptAES(payload, passphrase []byte) ([]byte, []byte, error) {
}

// DecryptValueKey decrypts the value key using the passphrase
func DecryptValueKey(valKey, passphrase []byte) ([]byte, error) {
return DecryptAES(valKey[:nonceLen], valKey[nonceLen:], passphrase)
func DecryptValueKey(valKey, mh multihash.Multihash) ([]byte, error) {
return DecryptAES(valKey[:nonceLen], valKey[nonceLen:], mh)
}

// EncryptValueKey encrypts raw value key using the passpharse
func EncryptValueKey(valKey, passphrase []byte) ([]byte, error) {
nonce, encValKey, err := EncryptAES(valKey, passphrase)
func EncryptValueKey(valKey, mh multihash.Multihash) ([]byte, error) {
nonce, encValKey, err := EncryptAES(valKey, mh)
if err != nil {
return nil, err
}
Expand All @@ -98,13 +101,13 @@ func EncryptValueKey(valKey, passphrase []byte) ([]byte, error) {
}

// DecryptMetadata decrypts metdata using the provided passphrase
func DecryptMetadata(encMetadata, passphrase []byte) ([]byte, error) {
return DecryptAES(encMetadata[:nonceLen], encMetadata[nonceLen:], passphrase)
func DecryptMetadata(encMetadata, valueKey []byte) ([]byte, error) {
return DecryptAES(encMetadata[:nonceLen], encMetadata[nonceLen:], valueKey)
}

// EncryptMetadata encrypts metadata using the provided passphrase
func EncryptMetadata(metadata, passphrase []byte) ([]byte, error) {
nonce, encValKey, err := EncryptAES(metadata, passphrase)
func EncryptMetadata(metadata, valueKey []byte) ([]byte, error) {
nonce, encValKey, err := EncryptAES(metadata, valueKey)
if err != nil {
return nil, err
}
Expand Down