crypto_box: fix XChaCha20Poly1305 key derivation [SECURITY]

[tarcieri]

Previous versions of this crate did not derive a uniformly random key when used with the XChaCha20Poly1305 AEAD.

The implementation has been updated to use the HChaCha20 function for this purpose, however this change makes the Curve25519XChaCha20Poly1305 construction implemented by versions earlier than this commit incompatible.

This was an unfortunate oversight in what is otherwise a non-standard construction with no other known implementations to test against. Perhaps libsodium implements this construction and we could try to ensure compatibility there going forward.

The suggestion for any users of the previous implementation is to migrate all ciphertexts to the new construction, as the original construction was broken. Please open an issue if this actually impacts you and you would like help addressing the problem.

This issue likely deserves a RUSTSEC advisory.

cc @alxiong

[codecov-commenter]

Codecov Report

Merging #295 (e622712) into master (2604d2a) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #295 +/- ##
==========================================
+ Coverage 84.43% 84.47% +0.04% 
==========================================
Files 31 31 Lines 1137 1140 +3 ==========================================
+ Hits 960 963 +3 
Misses 177 177 

Impacted Files	Coverage Δ
crypto_box/tests/lib.rs	100.00% <ø> (ø)
crypto_box/src/lib.rs	88.63% <100.00%> (+0.83%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2604d2a...e622712. Read the comment docs.

[tarcieri]

I was going to file a RUSTSEC advisory for this, but then I realized that this may not in fact be a security-critical issue.

Namely, the XChaCha20 construction performs a similar HChaCha20 derivation to derive the ChaCha20 key. In theory, this should produce a uniformly random ChaCha20 key.

The original NaCl crypto_box and its "Curve25519XSalsa20Poly1305" construction uses HSalsa20 twice however, so it is probably best to move forward with this construction as being closer to the XSalsa20-based equivalent, and perhaps there is a reason for the double-HSalsa20 cascade used in the original construction I'm not aware of.

FWIW, I have confirmed that we in theory implement the same construction as libsodium in crypto_box v0.6.0:

https://github.com/jedisct1/libsodium/blob/e1fa9cc/src/libsodium/crypto_box/curve25519xchacha20poly1305/box_curve25519xchacha20poly1305.c#L45-L51

I opened #303 to track adding test vectors to ensure that our implementation is compatible with libsodium's.

[tarcieri]

Previous versions of this crate did not derive a uniformly random key when used with the XChaCha20Poly1305 AEAD.

The implementation has been updated to use the HChaCha20 function for this purpose, however this change makes the Curve25519XChaCha20Poly1305 construction implemented by versions earlier than this commit incompatible.

This was an unfortunate oversight in what is otherwise a non-standard construction with no other known implementations to test against. Perhaps libsodium implements this construction and we could try to ensure compatibility there going forward.

The suggestion for any users of the previous implementation is to migrate all ciphertexts to the new construction, as the original construction was broken. Please open an issue if this actually impacts you and you would like help addressing the problem.

This issue likely deserves a RUSTSEC advisory.

cc @alxiong

[codecov-commenter]

Codecov Report

Merging #295 (e622712) into master (2604d2a) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #295 +/- ##
==========================================
+ Coverage 84.43% 84.47% +0.04% 
==========================================
Files 31 31 Lines 1137 1140 +3 ==========================================
+ Hits 960 963 +3 
Misses 177 177 

Impacted Files	Coverage Δ
crypto_box/tests/lib.rs	100.00% <ø> (ø)
crypto_box/src/lib.rs	88.63% <100.00%> (+0.83%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2604d2a...e622712. Read the comment docs.

[tarcieri]

I was going to file a RUSTSEC advisory for this, but then I realized that this may not in fact be a security-critical issue.

Namely, the XChaCha20 construction performs a similar HChaCha20 derivation to derive the ChaCha20 key. In theory, this should produce a uniformly random ChaCha20 key.

The original NaCl crypto_box and its "Curve25519XSalsa20Poly1305" construction uses HSalsa20 twice however, so it is probably best to move forward with this construction as being closer to the XSalsa20-based equivalent, and perhaps there is a reason for the double-HSalsa20 cascade used in the original construction I'm not aware of.

FWIW, I have confirmed that we in theory implement the same construction as libsodium in crypto_box v0.6.0:

https://github.com/jedisct1/libsodium/blob/e1fa9cc/src/libsodium/crypto_box/curve25519xchacha20poly1305/box_curve25519xchacha20poly1305.c#L45-L51

I opened #303 to track adding test vectors to ensure that our implementation is compatible with libsodium's.

[tarcieri]

Previous versions of this crate did not derive a uniformly random key when used with the XChaCha20Poly1305 AEAD.

The implementation has been updated to use the HChaCha20 function for this purpose, however this change makes the Curve25519XChaCha20Poly1305 construction implemented by versions earlier than this commit incompatible.

This was an unfortunate oversight in what is otherwise a non-standard construction with no other known implementations to test against. Perhaps libsodium implements this construction and we could try to ensure compatibility there going forward.

The suggestion for any users of the previous implementation is to migrate all ciphertexts to the new construction, as the original construction was broken. Please open an issue if this actually impacts you and you would like help addressing the problem.

This issue likely deserves a RUSTSEC advisory.

cc @alxiong

[codecov-commenter]

Codecov Report

Merging #295 (e622712) into master (2604d2a) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #295 +/- ##
==========================================
+ Coverage 84.43% 84.47% +0.04% 
==========================================
Files 31 31 Lines 1137 1140 +3 ==========================================
+ Hits 960 963 +3 
Misses 177 177 

Impacted Files	Coverage Δ
crypto_box/tests/lib.rs	100.00% <ø> (ø)
crypto_box/src/lib.rs	88.63% <100.00%> (+0.83%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2604d2a...e622712. Read the comment docs.

[tarcieri]

I was going to file a RUSTSEC advisory for this, but then I realized that this may not in fact be a security-critical issue.

Namely, the XChaCha20 construction performs a similar HChaCha20 derivation to derive the ChaCha20 key. In theory, this should produce a uniformly random ChaCha20 key.

The original NaCl crypto_box and its "Curve25519XSalsa20Poly1305" construction uses HSalsa20 twice however, so it is probably best to move forward with this construction as being closer to the XSalsa20-based equivalent, and perhaps there is a reason for the double-HSalsa20 cascade used in the original construction I'm not aware of.

FWIW, I have confirmed that we in theory implement the same construction as libsodium in crypto_box v0.6.0:

https://github.com/jedisct1/libsodium/blob/e1fa9cc/src/libsodium/crypto_box/curve25519xchacha20poly1305/box_curve25519xchacha20poly1305.c#L45-L51

I opened #303 to track adding test vectors to ensure that our implementation is compatible with libsodium's.

[tarcieri]

Previous versions of this crate did not derive a uniformly random key when used with the XChaCha20Poly1305 AEAD.

The implementation has been updated to use the HChaCha20 function for this purpose, however this change makes the Curve25519XChaCha20Poly1305 construction implemented by versions earlier than this commit incompatible.

This was an unfortunate oversight in what is otherwise a non-standard construction with no other known implementations to test against. Perhaps libsodium implements this construction and we could try to ensure compatibility there going forward.

The suggestion for any users of the previous implementation is to migrate all ciphertexts to the new construction, as the original construction was broken. Please open an issue if this actually impacts you and you would like help addressing the problem.

This issue likely deserves a RUSTSEC advisory.

cc @alxiong

[codecov-commenter]

Codecov Report

Merging #295 (e622712) into master (2604d2a) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #295 +/- ##
==========================================
+ Coverage 84.43% 84.47% +0.04% 
==========================================
Files 31 31 Lines 1137 1140 +3 ==========================================
+ Hits 960 963 +3 
Misses 177 177 

Impacted Files	Coverage Δ
crypto_box/tests/lib.rs	100.00% <ø> (ø)
crypto_box/src/lib.rs	88.63% <100.00%> (+0.83%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2604d2a...e622712. Read the comment docs.

[tarcieri]

I was going to file a RUSTSEC advisory for this, but then I realized that this may not in fact be a security-critical issue.

Namely, the XChaCha20 construction performs a similar HChaCha20 derivation to derive the ChaCha20 key. In theory, this should produce a uniformly random ChaCha20 key.

The original NaCl crypto_box and its "Curve25519XSalsa20Poly1305" construction uses HSalsa20 twice however, so it is probably best to move forward with this construction as being closer to the XSalsa20-based equivalent, and perhaps there is a reason for the double-HSalsa20 cascade used in the original construction I'm not aware of.

FWIW, I have confirmed that we in theory implement the same construction as libsodium in crypto_box v0.6.0:

https://github.com/jedisct1/libsodium/blob/e1fa9cc/src/libsodium/crypto_box/curve25519xchacha20poly1305/box_curve25519xchacha20poly1305.c#L45-L51

I opened #303 to track adding test vectors to ensure that our implementation is compatible with libsodium's.

[tarcieri]

Previous versions of this crate did not derive a uniformly random key when used with the XChaCha20Poly1305 AEAD.

The implementation has been updated to use the HChaCha20 function for this purpose, however this change makes the Curve25519XChaCha20Poly1305 construction implemented by versions earlier than this commit incompatible.

This was an unfortunate oversight in what is otherwise a non-standard construction with no other known implementations to test against. Perhaps libsodium implements this construction and we could try to ensure compatibility there going forward.

The suggestion for any users of the previous implementation is to migrate all ciphertexts to the new construction, as the original construction was broken. Please open an issue if this actually impacts you and you would like help addressing the problem.

This issue likely deserves a RUSTSEC advisory.

cc @alxiong

[codecov-commenter]

Codecov Report

Merging #295 (e622712) into master (2604d2a) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #295 +/- ##
==========================================
+ Coverage 84.43% 84.47% +0.04% 
==========================================
Files 31 31 Lines 1137 1140 +3 ==========================================
+ Hits 960 963 +3 
Misses 177 177 

Impacted Files	Coverage Δ
crypto_box/tests/lib.rs	100.00% <ø> (ø)
crypto_box/src/lib.rs	88.63% <100.00%> (+0.83%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2604d2a...e622712. Read the comment docs.

[tarcieri]

I was going to file a RUSTSEC advisory for this, but then I realized that this may not in fact be a security-critical issue.

Namely, the XChaCha20 construction performs a similar HChaCha20 derivation to derive the ChaCha20 key. In theory, this should produce a uniformly random ChaCha20 key.

The original NaCl crypto_box and its "Curve25519XSalsa20Poly1305" construction uses HSalsa20 twice however, so it is probably best to move forward with this construction as being closer to the XSalsa20-based equivalent, and perhaps there is a reason for the double-HSalsa20 cascade used in the original construction I'm not aware of.

FWIW, I have confirmed that we in theory implement the same construction as libsodium in crypto_box v0.6.0:

https://github.com/jedisct1/libsodium/blob/e1fa9cc/src/libsodium/crypto_box/curve25519xchacha20poly1305/box_curve25519xchacha20poly1305.c#L45-L51

I opened #303 to track adding test vectors to ensure that our implementation is compatible with libsodium's.

[tarcieri]

Previous versions of this crate did not derive a uniformly random key when used with the XChaCha20Poly1305 AEAD.

The implementation has been updated to use the HChaCha20 function for this purpose, however this change makes the Curve25519XChaCha20Poly1305 construction implemented by versions earlier than this commit incompatible.

This was an unfortunate oversight in what is otherwise a non-standard construction with no other known implementations to test against. Perhaps libsodium implements this construction and we could try to ensure compatibility there going forward.

The suggestion for any users of the previous implementation is to migrate all ciphertexts to the new construction, as the original construction was broken. Please open an issue if this actually impacts you and you would like help addressing the problem.

This issue likely deserves a RUSTSEC advisory.

cc @alxiong

[codecov-commenter]

Codecov Report

Merging #295 (e622712) into master (2604d2a) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #295 +/- ##
==========================================
+ Coverage 84.43% 84.47% +0.04% 
==========================================
Files 31 31 Lines 1137 1140 +3 ==========================================
+ Hits 960 963 +3 
Misses 177 177 

Impacted Files	Coverage Δ
crypto_box/tests/lib.rs	100.00% <ø> (ø)
crypto_box/src/lib.rs	88.63% <100.00%> (+0.83%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2604d2a...e622712. Read the comment docs.

[tarcieri]

I was going to file a RUSTSEC advisory for this, but then I realized that this may not in fact be a security-critical issue.

Namely, the XChaCha20 construction performs a similar HChaCha20 derivation to derive the ChaCha20 key. In theory, this should produce a uniformly random ChaCha20 key.

The original NaCl crypto_box and its "Curve25519XSalsa20Poly1305" construction uses HSalsa20 twice however, so it is probably best to move forward with this construction as being closer to the XSalsa20-based equivalent, and perhaps there is a reason for the double-HSalsa20 cascade used in the original construction I'm not aware of.

FWIW, I have confirmed that we in theory implement the same construction as libsodium in crypto_box v0.6.0:

https://github.com/jedisct1/libsodium/blob/e1fa9cc/src/libsodium/crypto_box/curve25519xchacha20poly1305/box_curve25519xchacha20poly1305.c#L45-L51

I opened #303 to track adding test vectors to ensure that our implementation is compatible with libsodium's.

[tarcieri]

Previous versions of this crate did not derive a uniformly random key when used with the XChaCha20Poly1305 AEAD.

The implementation has been updated to use the HChaCha20 function for this purpose, however this change makes the Curve25519XChaCha20Poly1305 construction implemented by versions earlier than this commit incompatible.

This was an unfortunate oversight in what is otherwise a non-standard construction with no other known implementations to test against. Perhaps libsodium implements this construction and we could try to ensure compatibility there going forward.

The suggestion for any users of the previous implementation is to migrate all ciphertexts to the new construction, as the original construction was broken. Please open an issue if this actually impacts you and you would like help addressing the problem.

This issue likely deserves a RUSTSEC advisory.

cc @alxiong

[codecov-commenter]

Codecov Report

Merging #295 (e622712) into master (2604d2a) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #295 +/- ##
==========================================
+ Coverage 84.43% 84.47% +0.04% 
==========================================
Files 31 31 Lines 1137 1140 +3 ==========================================
+ Hits 960 963 +3 
Misses 177 177 

Impacted Files	Coverage Δ
crypto_box/tests/lib.rs	100.00% <ø> (ø)
crypto_box/src/lib.rs	88.63% <100.00%> (+0.83%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2604d2a...e622712. Read the comment docs.

[tarcieri]

I was going to file a RUSTSEC advisory for this, but then I realized that this may not in fact be a security-critical issue.

Namely, the XChaCha20 construction performs a similar HChaCha20 derivation to derive the ChaCha20 key. In theory, this should produce a uniformly random ChaCha20 key.

The original NaCl crypto_box and its "Curve25519XSalsa20Poly1305" construction uses HSalsa20 twice however, so it is probably best to move forward with this construction as being closer to the XSalsa20-based equivalent, and perhaps there is a reason for the double-HSalsa20 cascade used in the original construction I'm not aware of.

FWIW, I have confirmed that we in theory implement the same construction as libsodium in crypto_box v0.6.0:

https://github.com/jedisct1/libsodium/blob/e1fa9cc/src/libsodium/crypto_box/curve25519xchacha20poly1305/box_curve25519xchacha20poly1305.c#L45-L51

I opened #303 to track adding test vectors to ensure that our implementation is compatible with libsodium's.

[tarcieri]

Previous versions of this crate did not derive a uniformly random key when used with the XChaCha20Poly1305 AEAD.

The implementation has been updated to use the HChaCha20 function for this purpose, however this change makes the Curve25519XChaCha20Poly1305 construction implemented by versions earlier than this commit incompatible.

This was an unfortunate oversight in what is otherwise a non-standard construction with no other known implementations to test against. Perhaps libsodium implements this construction and we could try to ensure compatibility there going forward.

The suggestion for any users of the previous implementation is to migrate all ciphertexts to the new construction, as the original construction was broken. Please open an issue if this actually impacts you and you would like help addressing the problem.

This issue likely deserves a RUSTSEC advisory.

cc @alxiong

[codecov-commenter]

Codecov Report

Merging #295 (e622712) into master (2604d2a) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #295 +/- ##
==========================================
+ Coverage 84.43% 84.47% +0.04% 
==========================================
Files 31 31 Lines 1137 1140 +3 ==========================================
+ Hits 960 963 +3 
Misses 177 177 

Impacted Files	Coverage Δ
crypto_box/tests/lib.rs	100.00% <ø> (ø)
crypto_box/src/lib.rs	88.63% <100.00%> (+0.83%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2604d2a...e622712. Read the comment docs.

[tarcieri]

I was going to file a RUSTSEC advisory for this, but then I realized that this may not in fact be a security-critical issue.

Namely, the XChaCha20 construction performs a similar HChaCha20 derivation to derive the ChaCha20 key. In theory, this should produce a uniformly random ChaCha20 key.

The original NaCl crypto_box and its "Curve25519XSalsa20Poly1305" construction uses HSalsa20 twice however, so it is probably best to move forward with this construction as being closer to the XSalsa20-based equivalent, and perhaps there is a reason for the double-HSalsa20 cascade used in the original construction I'm not aware of.

FWIW, I have confirmed that we in theory implement the same construction as libsodium in crypto_box v0.6.0:

https://github.com/jedisct1/libsodium/blob/e1fa9cc/src/libsodium/crypto_box/curve25519xchacha20poly1305/box_curve25519xchacha20poly1305.c#L45-L51

I opened #303 to track adding test vectors to ensure that our implementation is compatible with libsodium's.