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.

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

feat: WIP NoiseIK handshake based on libsodium #2450

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

test: testing if auto_reconnect_test fails

b1bf782
Select commit
Loading
Failed to load commit list.
Draft

feat: WIP NoiseIK handshake based on libsodium #2450

test: testing if auto_reconnect_test fails
b1bf782
Select commit
Loading
Failed to load commit list.
Mergeable / Mergeable failed Feb 16, 2025 in 1s

Semantic release conventions must be followed.

Status: FAIL

 Here are some stats of the run:
 3 validations were ran.
 2 PASSED
 1 FAILED

Details

✔️ Validator: TITLE

  • ✔️ All the requisite validations passed for 'or' option
    Input : feat: WIP NoiseIK handshake based on libsodium
    Settings : {"or":[{"must_include":{"regex":"^(feat|docs|chore|cleanup|fix|refactor|test|style|perf)(\\([^) ]+\\))?:\\ .+$","message":"Semantic release conventions must be followed."}},{"must_include":{"regex":"^Bump [^ ]* from [^ ]* to [^ ]*$","message":"Dependabot PRs are exempt from semantic release conventions."}}]}

❌ Validator: COMMIT

  • ❌ Semantic release conventions must be followed.
    Input : Started implementing an XAEAD function based on XChaCha20-Poly1305/libsodium in crypto core, which is to be used during the Noise-based handshake.,Added XAEAD functions, added HMAC-SHA512 functions, added HKDF for Noise in crypto_core. Added noise_handshake struct to net_crypto.h. Started Noise-based handshake implementation.,Added HMAC+HKDF functions to crypto_core.h, fixed wrong ). Fixed minor issues in crypto_core.c. Started handshake_init in net_crypto.c, added Noise functions MixKey(), MixHash(), EncryptAndHash(), DecryptAndHash().,Continued Noise handshake implementation.,Adapted create_crypto_handshake() for Noise Initiator and Responder, cf. Noise Spec WriteMessage(). Started to adapt handle_crypto_handshake() for Noise, cf. Noise Spec ReadMessage().,Fixed noise_handshake_init() which lead to minor adaptions in noise_create_crypto_handshake(). Adapted handle?crypto_handshake() for Noise - initiator and responder cases.,Merged noise_create_crypto_handshake() into create_crypto_handshake() and noise_handle_crypto_handshake() into handle_crypto_handshake() for backwards compatibility.,Adapted rest of necessary functions for Noise-based handshake.,Fixed SegFault because of unallocated noise_handshake memory. Added debugging statements.,Added debug output. Fixed noise_handshake_init() for Initiator case.,Added debugging code to log behavior, fixed bug in . Currently decryption #2 fails on the handshake packet from INITIATOR to RESPONDER. Adapted encryption/decryption functions, but not fixed yet.,NoiseIK handshake and NoiseIK transport encryption working. One CRYPTO_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.,Cleaned up logging/debugging code. Switched to Tox logger.,Changed buffer of log message from 1024 to 4096 bytes to be able to log whole handshake packet.,Cleaned up comments/TODOs.,fix/cleanup: Changed noise_handshake in Crypto_Connection and New_Connection structs from being dynamically allocated to statically allocated to fix memory leaks. Minor cleanup in regard to debug logging.,cleanup: reduced log amount by removing packet debug logging.,debug: added debug output to identify UAF issue root cause.,Changed noise_handshake to dynamic alloc again in Crypto_Connection.,fix: fixed ASAN memleaks after noise_handshake calloc in create_crypto_connection() via correct free in wipe_crypto_connection().,Improvements and added debugging due to test timeouts. Buggy, but most tests working.,Fix: Fixed issues when two peers initiate a handshake at the same time via new_crypto_connection().,Fix: Added further debug logging and fixed issue in special case of initiator to responder change case (relevant for reconnect test).,cleanup: small cleanup of debug/logging code.,change: moved noise_handshake_init() to handle_cookie_response for INITIATOR case. memzero noise_handshake before every init. Further minor adaptions, e.g. Crypto Conn Status changes. Minor logging/debug adaptions.,fix: fixed current behavior in handle_crypto_hs(). fixed wrong memzero of n_c.noise_handshake.,cleanup: removed unnecessary code.,Implemented different handshake behavior. Currently fails at create_send_handshake() because of a nullptr.,fix/cleanup: fixed connection status bug in accept_crypto_connection() - status needs to be set before create_send_handshake(). Cleaned up/adapted debug logging.,cleanup: Removed unused crypto_hmac512_verify(). Moved generic noise_* functions to crypto_core. Use function based on libsodium bin2hex() to print bytes for debugging (instead of custom func).,fix: fix compiler error due to usage of public/private.,cleanup/doc: Cleaned up Noise related code in crypto_core. Added Noise functions to end of crypto_core files. Added documentation and comments.,fix: Fixed typos in documentation.,cleanup/doc: cleaned up comments and added documentation to net_crypto. Adapted logging with bytes2string().,merge: merged current master. Resolved conflicts in .,fix: missed one curly bracket.,fix: fix double free after merge.,fix/cleanup: Changed NULL to nullptr, free() to mem_delete(), calloc() to mem_alloc(). Renamed Noise_Handshake struct.,fix: removed dependency for bin2hex_toupper() from create_common.h. Added generic bytes_to_string() function to util.c (based on id_to_string() from Messenger.c).,fix: minor fixes for CI.,fix: minor fixes for CI.,fix: minor fixes for CI.,fix: minor fixes for CI.,fix/cleanup: changed crypto_hkdf() to two outputs because output3 is currently not needed (maybe necessary in the future for PSKs).,fix: minor fixes for CI.,fix: changed noise_mix_key() to use crypto_scalarmult_curve25519() instead enrypt_precompute() - otherwise HSalsa20 is performed before HKDF.,cleanup: removed unnecessary memcpys and related parameters from handle_crypto_handshake(). Memzero/free of noise_handshake after crypto connection is established.,feat: Encrypt cookies in Noise handshake packets and authenticate via XAEAD.,cleanup: removed unnecessary TODOs.,cleanup: moved Noise Split (symmetric key derivation).,fix/cleanup: minor changes for CI checks.,fix: fix for cimple and MSAN uninitialized value.,fix: MSAN uninitialized value.,fix: for TSAN.,test: testing changed connection_kill() behavior.,fix: fixed handle_crypto_handshake() and calls because public_key (=remote static) is necessary for friend_connection.c->handle_new_connections(),fix: fixed call to handle_crypto_handshake().,add handshake compatiblilty option,cleanup: minor cleanup,fix: fix for ubsan.,fix: fix for ubsan.,fix: fix for ubsan.,fix: fixed noise backwards compatibility option.,feat: backwards compatibility to non-Noise handshake.,feat: backwards compatibility to non-Noise handshake.,fix merge issue,Restyled by astyle,fix: removed unused OtherCookie from RESPONDER handshake packet.,Fix: fixed wrong usages of CRYPTO_PUBLIC_KEY_SIZE to CRYPTO_SECRET_KEY_SIZE.,feat: backwards compatibility to non-Noise handshake.,fix: fixed uninitialized variable.,fix: fixed uninitialized variable.,fix: fixed reference to nullptr.,debug: added debug output.,fix: possible fix for wrong non-Noise handshake.,feat: backwards compatibility to non-Noise handshake. Successfully tested with minitox and current toktok/toxic master branch (+goldroom c-toxcore master).,fix: fix for backwards compatibility to non-Noise handshake.,cleanup: stated explicit lengths for Noise function parameters (where possible) in crypto_core.,fix: fixed merge issue found by coverage-linux check.,cleanup/fix: changed parameter ordering of crypto_hkdf() to make tokstyle happy.,fix: fixes for CI.,fix: added missing non_null()/nullable() statements to .,feat: Implemented Noise_IK_25519_ChaChaPoly_SHA512 instead of Noise_IK_25519_XChaChaPoly_SHA512 to be compliant with Noise specification/framework. Therefore added functions for IETF ChaCha20-Poly1305 construction to crypto_core (this uses 12 byte nonces). Adapted the handshake functions to use ChaCha20-Poly1305 instead of XChaCha20-Poly1305. NoiseIK based crypto connections still use XChaCha20-Poly1305 for transport message encryption.,fix: fix param for asan/tsan/ubsan.,fix: changed Noise protocol name to Noise_IK_25519_ChaChaPoly_SHA512.,test: trying test vectors from noise-c,Fixed crypto_hkdf() in crypto_core.c and verified with test vectors. Added NoiseIK test vectors test file, currently hacked into crypto_test.c for compilation and easy local troubleshooting.,Added further testing steps. Everything correct (incl. final handshake hash value) besides ciphertext4_transport1_initiator_print.,Adapted return type of aead/xaead encrypt/decrypt functions and removed unnecessary unsigned long long. Added and verified all test vectors values.,feat: Added verification of Noise_IK_25519_ChaChaPoly_SHA512 test vectors to crypto_test.c. Deleted intermediary test files. Added final version of crypto_hkdf() to crypto_core. Added prologue/prologue_length parameters to noise_handshake_init() (necessary in general, and in special for verification of test vectors). Added parameters to calls in net_crypto.,fix: removed logger.h from crypto_core.h and removed logging parameter for noise_handshake_init(),fix: removed logging param and added newline to crypto_core.c,fix: fixes for CI,fix: fixes for CI,fix: fixes for CI,fix: fixes for CI,fix: fixed bug introduced in crypto_test with CI fixes,fix: fixed terminating null character, noiseIK test vectors still verify correctly.,fix: fixes for CI,feat: Implemented enabling/disabling of backwards compatiblity to non-Noise handshakes.,fix: fix two tests after adding Tox option to disable backwards compatibility to non-Noise handshake.,cleanup: code cleanup and documentation of net_crypto.c,cleanup: minor cleanup of comments/documentation,cleanup: code cleanup and documentation of net_crypto.c,docs: Added doc and TODOs.,refactor: Implemented Noise_IK_25519_ChaChaPoly_BLAKE2s instead of Noise_IK_25519_ChaChaPoly_SHA512. Verified with test vectors from Noise-C. Adapted HMAC, HKDF and noise_mix_hash() functions to use Blake2b instead of SHA512 in crypto_core.,fix: added static,fix: fixed include, not sure where that came from,fix: fixes after master merge,fix: formatting after merge, ASAN caught memory leak in crypto_test.c,feat: started implementation of (possible) new cookie functionality.,fix: fixes for CI.,feat: implementation of (possible) new cookie functionality.,feat: implementation of (possible) new cookie mechanism.,feat: implementation of possible new cookie mechanism. Currently broken because both peers are Noise-responder.,fix: added debug output and fixed NoiseIK cookie implementation 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).,fix: fixed typo,Revert "fix: fixed typo"

This reverts commit 5ac12e7.,Revert "fix: added debug output and fixed NoiseIK cookie implementation 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.,Revert "feat: implementation of possible new cookie mechanism. Currently broken because both peers are Noise-responder."

This reverts commit 44e58f7.,Revert "feat: implementation of (possible) new cookie mechanism."

This reverts commit 3440302.,Revert "feat: implementation of (possible) new cookie functionality."

This reverts commit fdb0163.,Revert "fix: fixes for CI."

This reverts commit b3d73e2.,Revert "feat: started implementation of (possible) new cookie functionality."

This reverts commit fa59b7b.,refactor: minor refactoring and documentation (based on reverted cookie commits).,chore: added Noise compatibility option again after master merge.,fix: cannot have const Net_Crypto in handle_packet_crypto_hs() here (was added in master).,chore: cleanup and documentation of crypto_core.,refactor: removed base nonces from NoiseIK handshake packets to use (only) a counter instead. Due to separate encryption/decryption keys they were unnecessary overhead.,fix: fixed friend_connection after New_Connection refactoring.,chore: cleanup and documentation.,fix: fix for CI,cleanup: minor cleanup,cleanup: minor cleanup,fix: fixed two issues because of noise_handshake memzero in backwards compatiblity case. Moved ephemeral key pair generation to create_crypto_handshake(). Fixed setting of recv_nonce in NoiseIK handshake.,cleanup: moved crypto_memzero() out of noise_handshake_init() to cases where it's actually necessary.,chore: adapted logging. Need to test connection_kill() in L2054 and behavior of non-Noise handle_new_connection_handshake().,fix: fixed issues in real-world backwards-compatible handshake behavior.,fix: moved creation of new ephemeral key pair after noise_handshake_init() to avoid multiple calls.,chore: cleanup of documentation/comments,chore: removed unused SHA512 functions,fix: changed constant names to all upper case,chore: cleanup documentation and logging,fix: fixed logging for CI,fix: possible fix for failing auto_reconnect_test in bazel-asan,fix: fixes for ccplint CI check,test: added further debug logging for real-world tests. Removed connection_kill() from handle_data_packet_core() and also from handle_packet_crypto_hs(). Using conn->peer_id_public_key also in Noise for testing/debugging purposes.,fix: double declaration,fix: possible fix for CI,fix: possible fix for asan,test: testing if auto_reconnect_test fails
Settings : {"do":"commit","message":{"regex":"^((feat|docs|chore|cleanup|fix|refactor|test|style|perf)(\\([^) ]+\\))?:\\ .+|Bump [^ ]* from [^ ]* to [^ ]*)","message":"Semantic release conventions must be followed."}}

✔️ Validator: DESCRIPTION

  • ✔️ description must exclude '\[ \]'
    Input : ## 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

Settings : {"must_exclude":{"regex":"\\\\[ \\\\]","message":"There are incomplete TODO task(s) unchecked."}}

View more details on Mergeable