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

Fixes for pqc and crypto refresh #2142

Merged
merged 8 commits into from
Feb 13, 2024

Conversation

TJ-91
Copy link
Contributor

@TJ-91 TJ-91 commented Nov 8, 2023

This includes the following fixes for the Crypto Refresh and PQC code:

  • KMAC256::fixedInfo() now returns the correct value
  • the v3 PKESK algorithm identifier byte is now included in the correct place (for X25519 and PQC)

Thanks to @teythoon for spotting the issues.


Further, I fixed a compile error (wrong variable used) for the Crypto Refresh / PQC code and included a copy constructor for the composite algorithms such that it will now build with MSVC/Clang.

A last change has been made to the salg parsing in the PKESK code. The variable was undefined (up to a certain point) and I initially set it to salg = PGP_SA_UNKNOWN;. In the Crypto Refresh code it is checked whether or not this is AES since v3 PKESK with the new algorithms don't encrypt the session key and requires AES for that case. This part of the logic is a bit messy currently since the salg value is defined early when it's not encrypted (can directly be parsed when the PKESKv3 material is parsed), but for other cases it's only available after decryption in encrypted_try_key().

Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (9e5e13a) 77.26% compared to head (c551af4) 77.25%.

Files Patch % Lines
src/librepgp/stream-sig.cpp 85.71% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2142      +/-   ##
==========================================
- Coverage   77.26%   77.25%   -0.02%     
==========================================
  Files         194      194              
  Lines       37746    37751       +5     
==========================================
- Hits        29165    29163       -2     
- Misses       8581     8588       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TJ-91
Copy link
Contributor Author

TJ-91 commented Nov 16, 2023

For v6 it's mandated to have direct-key signatures.
This is what I implemented in 432e453:

  • Add a direct-key signature only for v6 packets in pgp_generate_primary_key() (via new pgp_key_t::add_direct_sig() funciton).
  • Leave the behaviour for old pgp versions as-is.
  • Split rnp_selfsig_cert_info_t::populate() s.t. sig and uid can be populated independently. For direct-key signatures, only the signature part is relevant.
  • For subkey / uid certification signatures the "preference subpackets" are not duplicated in other signatures

As far as I can see this implements the correct behaviour ("MUST") but not all of the Crypto Refresh's additional and optional subpackets etc.

@TJ-91 TJ-91 force-pushed the fixes-for-pqc-and-crypto-refresh branch from 432e453 to c551af4 Compare February 9, 2024 13:33
@TJ-91
Copy link
Contributor Author

TJ-91 commented Feb 9, 2024

I updated the branch (rebase on main) and fixed a typo (RNP_EXPERIMENTAL_CRYPTO_PQC to RNP_EXPERIMENTAL_PQC)

Copy link
Contributor

@ni4 ni4 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! We may safely ignore codecov failure as changed lines just throw exeption on malloc failure.

@antonsviridenko antonsviridenko merged commit 9fd1893 into rnpgp:main Feb 13, 2024
122 of 123 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants