Skip to content
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.

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

feat: WIP NoiseIK handshake based on libsodium #2450

Draft
wants to merge 142 commits into
base: master
Choose a base branch
from

Conversation

goldroom
Copy link

@goldroom goldroom commented Dec 4, 2023

This PR adds a NoiseIK implementation for a new KCI-resistant crypto handshake. The implemented Noise protocol is: Noise_IK_25519_ChaChaPoly_SHA512 Noise_IK_25519_ChaChaPoly_BLAKE2b (update 11.12.2024 ). This resolves #426.

NoiseIK is working, tested and passes all auto/CI tests. There is also an auto test added which verifies the cryptographic correctness of Noise_IK_25519_ChaChaPoly_BLAKE2b (and also of former Noise_IK_25519_ChaChaPoly_SHA512) with test vectors provided by Noise-C.

Backwards compatibility for NoiseIK handshake to non-Noise handshake is implemented (though possibly still subject to change). This enables clients using NoiseIK-toxcore to communicate with non-NoiseIK-toxcore clients. Backwards compatibility can be disabled by setting noise_compatibility_enabled to false (default: true) via tox_options_set_noise_compatibility_enabled.
Enabled and disabled backwards compatibility was tested successfully with toxic and minitox clients.

Status 14.06.2024: Currently most failing CI tests/checks fail due to missing functions released with libsodium 1.0.19 (i.e. crypto_kdf_hkdf_*() which are necessary for NoiseIK; besides the linting stuff).
Side effect of using libsodium >=1.0.19: Slightly faster 25519 operations.
Update 11.12.2024: Implemented HKDF-BLAKE2b-512 in crypto_core based on libsodium BLAKE2b primitive functions (since it's not exposed on the high-level API9. Therefore only the linting CI checks are failing (and do not require a newer libsodium version). See update 11.12.2024 for more info.

Further testing, reviews, feedback etc. highly appreciated.

This PR also changes symmetric encryption during the Noise handshake to ChaCha20-Poly1305 and data transport phase (based on net_crypto/struct Crypto_Connection) to XChaCha20-Poly1305, both instead of XSalsa20-Poly1305.
This change is necessary because XSalsa20-Poly1305 from libsodium doesn't provide Authenticated Encryption with Associated Data (AEAD), but only Authenticated Encryption (AE). AEAD is a requirement for NoiseIK. Further, the Noise specification/framework only considers ChaCha20-Poly1305 and AES256-GCM as symmetric ciphers. This means, that existing security proofs and cryptographic test vectors only exist and verify with these two ciphers. By adopting ChaCha20-Poly1305 in Tox' NoiseIK handshakes, correctness can and is verified using existing test vectors.
By utilizing XChaCha20 during data transport phase (instead of ChaCha20) it is still possible to use random 24 byte (base) nonces as this is already the case with XSalsa20 (in net_crypto/toxcore in general) and further using the existing handling of out-of-order message functionality (same as with old/non-Noise XSalsa20-Poly1305 encryption).

Some necessary functions for NoiseIK were added to crypto_core:

Added functions to net_crypto:

  • Added struct Noise_Handshake to and adapted struct New_Connection in net_crypto.h
  • Adapted struct Crypto_Connection in net_crypto.c
  • noise_handshake_init() to initialize a Noise handshake state was moved to crypto_core for testing/crypto verification purposes
  • Besides that, only existing functions were adapted to implement NoiseIK and backwards compatibility to non-Noise handshakes.

Added option to enable/disable backwards compatibility to non-Noise handshakes/toxcore versions/clients:

  • bool noise_compatibility_enabled in tox.h: default is true (i.e. backwards compatibility is enabled) and can be disabled by setting to false via tox_options_set_noise_compatibility_enabled()

For backwards compatibility additionally these functions were adapted (all net_crypto):

  • create_crypto_handshake()
  • handle_crypto_handshake()
  • send_data_packet(): to support both XSalsa20-Poly1305 (i.e. encrypt_data_symmetric()) and XChaCha20-Poly1305 (i.e. encrypt_data_symmetric_xaead())
  • handle_data_packet(): to support both XSalsa20-Poly1305 (i.e. decrypt_data_symmetric()) and XChaCha20-Poly1305 (i.e. decrypt_data_symmetric_xaead())
  • create_send_handshake()
  • handle_packet_cookie_response()
  • handle_packet_crypto_hs()
  • handle_new_connection_handshake()
  • accept_crypto_connection()
  • new_crypto_connection()

Verification of Noise_IK_25519_ChaChaPoly_BLAKE2b and Noise_IK_25519_ChaChaPoly_SHA512 cryptographic correctness:

The most notable changes to the non-Noise handshake are:

  • Different handshake packets for both initiator (345 bytes) and responder (185 bytes), adapted for NoiseIK (non-Noise handshake packets are 385 bytes)
    • Cookies are encrypted and authenticated (via ChaCha20-Poly1305) in handshake packets (instead of being sent in the clear and authenticated via a SHA512 hash included in the encrypted part of the non-Noise handshake packet)
  • Different symmetric key derivation based on NoiseIK and therefore HKDF-SHA512 (during the handshake and after successful handshake for the symmetric keys used in the actual data transport/encryption phase)
  • ChaCha20-Poly1305 for handshake packet encryption and XChaCha20-Poly1305 data encryption after a successful handshake.
    • After a successful handshake: two symmetric keys, one for encrypting outgoing data packets and one for decrypting incoming data packets by still providing deniability.
  • No API changes (currently) and no changes to cookie phase (currently)
  • Code documentation
  • Side effect of using libsodium >=1.0.19: Slightly faster 25519 operations.

Security Audit / Cryptographic Analysis Report

  • Funding from NLnet foundation included a security audit (cf. https://nlnet.nl/NGI0/services/)
  • Pentester: Younes Talibi Alaoui (cryptographer with a PhD in cryptography from KU Leuven; https://github.com/YounesTal1) from Radically Open Security (ROS; https://www.radicallyopensecurity.com/; a Non-Profit Computer Security Consultancy)
    • The scope of the analysis was limited to checking the new Noise-based handshake for the Tox protocol, and informally
      to discuss the security guarantees provided with this new handshake. The analysis also included an inspection of the
      implementation's code.
    • Note that the analysis was constrained by a time limit of 10 person-days (including reporting).
  • Full Cryptographic Analysis Report:
    3report_ngir-tox.pdf

Changes/recommendations from the security analysis where incorporated:

  • Section 3.6.1: Exchange of an unnecessary cookie
    • Change: Removed unused/unnecessary "Other cookie" from Noise-responder handshake packet; reduced total packet size from 321 to 209 bytes
    • Commit: goldroom@190c038
  • Section 3.6.2: Variable being used instead of another one
    • Cleanup/fix: wrong usages of CRYPTO_PUBLIC_KEY_SIZE to CRYPTO_SECRET_KEY_SIZE
    • Commit: goldroom@2201b8c
  • Section 3.6.3: Missing step from the Noise handshake
    • Fix: need to call MixHash(prologue) in noise_handshake_init() even if the prologue is zero-length in Tox
    • Commit: Commit: 55b3ef5
  • Feat/change: Implemented Noise_IK_25519_ChaChaPoly_SHA512 instead of Noise_IK_25519_XChaChaPoly_SHA512 to be compliant with Noise specification/framework after discussion with ROS reviewer. This further reduced handshake packet size for initiator and responder (i.e. removed unnecessary random nonces from handshake packets).
  • => Lead to decreased handshake packet sizes; i.e. initiator 345 bytes, responder 185 bytes (non-Noise handshake packets are 385 bytes)
  • Remarks/suggestions from sections 3.2 to 3.4 are open/future work, i.e. to adapt the cookie phase for NoiseIK-based handshakes to remove unnecessary (cryptographic) information (as also suggested by ROS auditor). This was already clear to me beforehand, but wasn't included in the scope of this project. Anyway, this will be done soon.

NoiseIK provides formally-verified security properties (similar to WireGuard®; cf. https://noiseexplorer.com/):

  • Strong key agreement & authenticity
  • Key-compromise impersonation resistance
  • Unknown key-share attack resistance
  • Key secrecy
  • Forward secrecy
  • Session uniqueness
  • Identity hiding
  • Replay-attack prevention, while allowing for network packet reordering

11.06.2024: Update on the work performed in this pull request:

  • The implemented Noise protocol changed from Noise_IK_25519_XChaChaPoly_SHA512 to Noise_IK_25519_ChaChaPoly_SHA512; i.e. ChaCha20-Poly1305 is used during the handshake instead of XChaCha20. This means that a compliant Noise protocol with existing test vectors is used. The implementation was verified with test vectors from Noise-C [1] by adding a test_noiseik() function to auto_tests/crypto_test.c.
    • This verification lead to using libsodium-HKDF functions for correct values
    • During the transport phase the actual message/payload encryption happens with XChaCha20-Poly1305 to be able to still use the existing (base)nonce and handling of out-of-order message functionality
  • Changes from the security analysis from Radically Open Security (ROS) where incorporated:
    • Auditor: Younes Talibi Alaoui
    • A missing call to noise_mix_hash() with an empty/zero-length prologue was discovered
    • Noise protocol change after discussing cryptographic details with the auditor
    • Removed unused/unnecessary "Other cookie" from Noise-responder handshake packet
      • Removed unnecessary random nonces from handshake packets
      • => both leaded to decreased handshake packet sizes; i.e. initiator 345 bytes, responder 185 bytes (non-Noise handshake packets are 385 bytes)
  • Currently most failing CI tests/checks fail due to missing libsodium functions (i.e. crypto_kdf_hkdf_*() from 1.0.19 release which are necessary for NoiseIK) besides the linting stuff. (@iphydf )

Update 11.12.2024 (comment)

Open:

  • Adapt cookie phase for NoiseIK-based handshakes to remove unnecessary (cryptographic) information (as also suggested by ROS auditor)

Contact Information

Tobi/goldroom is available in Tox/TokTok development channel (360497DA684BCE2A500C1AF9B3A5CE949BBB9F6FB1F91589806FB04CA039E313) and in Matrix (@tobi_fh:matrix.org) and ready for any input, questions, remarks, discussions or complaints.

Resources:

See fore more information:


This project was funded through the NGI Assure Fund, a fund established by NLnet with financial support from the European Commission's Next Generation Internet programme, under the aegis of DG Communications Networks, Content and Technology under grant agreement No 957073.


This change is Reviewable

goldroom added 30 commits May 10, 2023 19:31
…bsodium in crypto core, which is to be used during the Noise-based handshake.
…ise in crypto_core. Added noise_handshake struct to net_crypto.h. Started Noise-based handshake implementation.
…r issues in crypto_core.c. Started handshake_init in net_crypto.c, added Noise functions MixKey(), MixHash(), EncryptAndHash(), DecryptAndHash().
…cf. Noise Spec WriteMessage(). Started to adapt handle_crypto_handshake() for Noise, cf. Noise Spec ReadMessage().
…reate_crypto_handshake(). Adapted handle?crypto_handshake() for Noise - initiator and responder cases.
… and noise_handle_crypto_handshake() into handle_crypto_handshake() for backwards compatibility.
…tion #2 fails on the handshake packet from INITIATOR to RESPONDER. Adapted encryption/decryption functions, but not fixed yet.
…O_NONCE_SIZE was missing in NOISE_HANDSHAKE_PACKET_LENGTH_INITIATOR, therefore 24 bytes were missing in actual sent handshake packet => fixed encryption/decryption during handshake. Changed encrypted_length/plaintext_length to unsigned long long to remove compiler warning. Added some logging/debugging code. Fixed a SegFault in send_temp_packet() due to my crappy logging/debugging code.
…nection structs from being dynamically allocated to statically allocated to fix memory leaks. Minor cleanup in regard to debug logging.
…o_connection() via correct free in wipe_crypto_connection().
…nitiator to responder change case (relevant for reconnect test).
…ITIATOR case. memzero noise_handshake before every init. Further minor adaptions, e.g. Crypto Conn Status changes. Minor logging/debug adaptions.
…) - status needs to be set _before_ create_send_handshake(). Cleaned up/adapted debug logging.
…* functions to crypto_core. Use function based on libsodium bin2hex() to print bytes for debugging (instead of custom func).
@goldroom
Copy link
Author

Implemented Noise_IK_25519_ChaChaPoly_BLAKE2b instead of Noise_IK_25519_ChaChaPoly_SHA512 (cf. 841ba31 - commit message with BLAKE2s is wrong). Verified cryptographic protocol correctness with test vectors from Noise-C.

Blake2b is faster than SHA512, optimized for 64-bit systems and at least as secure as the latest standard SHA-3 (cf. here https://www.blake2.net/ ). This also removes usage of libsodium HKDF-SHA512, therefore requires no new libsodium version for CI (which was added in libsodium v1.0.19, cf. https://github.com/jedisct1/libsodium/releases/tag/1.0.19-RELEASE ) and therefore most CI checks are passing now. HMAC-BLAKE2b-512 and HKDF-BLAKE2b-512 added based on BLAKE2b primitive functions of libsodium since these are not provided at the high level API (cf. jedisct1/libsodium#788 , jedisct1/libsodium#531 ).
BLAKE2b could be used as KDF directly without HKDF construction, but Noise is only specified for HKDF-BLAKE2b-512 and therefore also its security proofs (and available test vectors).

@iphydf iphydf modified the milestones: v0.2.21, v0.2.22 Jan 14, 2025
This reverts commit 5ac12e7.
…on issues. This lead to the discovery, that usage of IP_Port is currently broken implementation-wise and by design (NoiseIK-initiator doesn't know the actual IP_Port when the handshake packet is created)."

This reverts commit 7ec460f.
…tly broken because both peers are Noise-responder."

This reverts commit 44e58f7.
This reverts commit b3d73e2.
@goldroom
Copy link
Author

Reverted commits of possible new cookie mechanism/functionality. Aim is to cleanup this PR, get it merged and test it.
I will create a separate PR for new a new cookie mechanism.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature for the user, not a new feature for build script
Development

Successfully merging this pull request may close these issues.

Tox Handshake Vulnerable to KCI
7 participants