addition of v6 and PQC #35

falko-strenzke · 2023-06-13T11:58:43Z

No description provided.

github-advanced-security

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

src/tests/rnp_tests.h

src/lib/CMakeLists.txt

github-advanced-security

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

include/repgp/repgp_def.h

ni4 · 2023-06-28T13:57:25Z

include/repgp/repgp_def.h

+#define PGP_MAX_FINGERPRINT_HEX_SIZE (PGP_MAX_FINGERPRINT_SIZE * 2) + 1
+
+/* SEIPDv2 salt length */
+#define PGP_SEIPDV2_SALT_LEN 32


Please move it under the define.

I don't really understand: move what exactly where?

@falko-strenzke As PGP_SEIPDV2_SALT_LEN is used only with ENABLE_CRYPTO_REFRESH, it should be under the #ifdef ENABLE_CRYPTO_REFRESH, so code without ENABLE_CRYPTO_REFRESH doesn't have unused defines.

addressed in upcoming commit

ni4

@falko-strenzke
Please see my comments and feel free to discuss those if I missed something. Will make another round a bit later, more focusing on stream-parse/stream-write as those has quite a load of changes.

include/repgp/repgp_def.h

ni4 · 2023-06-28T14:57:40Z

src/librepgp/stream-sig.cpp

+    unsigned splen;
+    size_t   splen_size;
+
+    if (version == PGP_V4) {


For better extensibility (adding v5 and, who knows v7 and so on later) I'd prefer this refactoring to something like the following:

/* Without crypto-refresh support it would stick always to 2 */ size_t splen = 0; size_t splen_size = 2; switch (version) { case PGP_V4: { break; #if defined(ENABLE_CRYPTO_REFRESH) case PGP_V6: splen_size = 4; break; #endif default: RNP_LOG("unsupported signature version: %d", (int) version); return RNP_ERROR_BAD_FORMAT; } if (!pkt.get_len(splen, splen_size * 8)) { RNP_LOG("failed to read hashed subpackets length"); return RNP_ERROR_BAD_FORMAT; }

Where pkt.get_len() would read to size_t some length according to second bits parameter.

This approach would use less code lines/constants/complexity.

I hope you agree with the proposed alternative - I tried to make it more readable with switch-case. Implementing pkt.get_len() in this generality as you suggest is a bit troublesome.

I think it doesn't make much sense to work on bits here since reading from the packet is done byte-by-byte and it is unclear what happens with the remaining bits if reading e.g. 5 bits length.

size_t has no guaranteed max size / byte width in memory and this variability introduces some complexity to correctly handle it.

All in all it might make sense to refactor the fixed-length big-endian get/read/write functions across the code base with a more general and parametrized approach. But I do think it's not a trivial change and it's best done separately.

Regarding pkt.get_len() : as it is only internal function, it may rely on real conditions, and allow input only of 16 or 32 bits (or has parameter bytes, instead of bits). Anyway, this may be addressed later on, once adding V5 support.

src/librepgp/stream-sig.cpp

src/librepgp/stream-packet.cpp

ni4 · 2023-06-28T16:07:12Z

src/librepgp/stream-parse.cpp

@@ -100,6 +102,7 @@ typedef struct pgp_source_encrypted_param_t {
    size_t                     aead_adlen{}; /* length of the additional data */
    pgp_symm_alg_t             salg;         /* data encryption algorithm */
    pgp_parse_handler_t *      handler{};    /* parsing handler with callbacks */
+    pgp_seipdv2_hdr_t          seipdv2_hdr;  /* SEIPDv2 encryption parameters */


This should be under the ifdef.

addressed in upcoming commit

src/librepgp/stream-sig.cpp

ni4 · 2023-06-28T16:09:29Z

src/librepgp/stream-write.cpp

@@ -55,6 +56,7 @@
 #include "defaults.h"
 #include <time.h>
 #include <algorithm>
+#include "v2_seipd.h"


This should be under the ifdef.

addressed in upcoming commit

ni4 · 2023-07-07T12:23:11Z

@falko-strenzke @TJ-91 Thanks for fixes! Please also re-apply clang-format as lint run is now broken. I'll take another look on PR, including changes in stream-parse.cpp/stream-write.cpp.

TJ-91 · 2023-07-11T10:50:00Z

FYI: I pushed some changes to align with the newest PQC draft version

ni4

Thanks for fixing previous comments. Please see the additional batch.
My main concern is to not accidentally change the old/default behaviour adding this code, with or without CRYPTO_REFRESH enabled.

ni4 · 2023-07-12T08:43:28Z

src/librepgp/stream-parse.cpp

@@ -48,6 +48,10 @@
 #include "crypto/signatures.h"
 #include "fingerprint.h"
 #include "pgp-key.h"
+#include "crypto/hkdf.hpp"


Please move it under the define.

fixed in upcoming commit

ni4 · 2023-07-12T08:53:52Z

src/lib/crypto/rng.h

 #endif

 namespace rnp {
 class RNG {
  private:
 #ifdef CRYPTO_BACKEND_BOTAN
-    struct botan_rng_struct *botan_rng;
+    struct botan_rng_struct *                     botan_rng;
+    std::unique_ptr<Botan::RandomNumberGenerator> botan_rng_obj;


Isn't this used only for V6/or PQC? Then it should be put under the corresponding define.

That's (currently) true, but will also be needed when you rewrite other crypto functionality to use the C++ interface of Botan. I put it under ifdef for now.

ni4 · 2023-07-12T09:20:40Z

src/librepgp/stream-parse.cpp

@@ -448,7 +485,9 @@ encrypted_start_aead_chunk(pgp_source_encrypted_param_t *param, size_t idx, bool
    size_t  nlen;

    /* set chunk index for additional data */
-    STORE64BE(param->aead_ad + param->aead_adlen - 8, idx);
+    if (param->auth_type == rnp::AuthType::AEADv1) {


I'd prefer to use switch (param->auth_type) here, leaving the old code for AEADv1, and adding new if crypto refresh is enabled. This would duplicate pgp_cipher_aead_set_ad call, but look a bit cleaner (and will not create add_data_seipd_v2 even if v1 is used).

Does that mean you also for instance want this code to be duplicated?

if (last) { uint64_t total = idx * param->chunklen; if (idx && param->chunkin) { total -= param->chunklen - param->chunkin; } if (!param->chunkin) { /* reset the crypto in case we had empty chunk before the last one */ pgp_cipher_aead_reset(&param->decrypt); } STORE64BE(param->aead_ad + param->aead_adlen, total); param->aead_adlen += 8; }

I personally disagree with an approach that leads to code duplication. Code duplication is in my opinion one of the clearest errors in code design, leading to less readability, higher maintenance efforts and more complexity. Also I don't see the benefit of leaving the v5 AEAD code untouched. I think it should suffice if it is clearly visible that in case of v5 AEAD, the code behaves the same.

If the goal is to have the code a bit cleaner than curently and especially not to create the add_data_seipd_v2 vector in case of v5 AEAD, then I think we can work towards that without introducing code duplication. I can work on a proposal for that if you agree.

@falko-strenzke Nope, duplicating such large code piece would be an overkill. Looks like I clicked on a wrong code line while adding a comment, causing confusion. I mean only that part which builds ad and calls pgp_cipher_aead_set_ad(). (so only duplication would be if (!pgp_cipher_aead_set_ad(&param->decrypt, add_data, add_data_len)) ...)

OK, I came up with a solution which only slightly needs to alter the parts of the original v5 AEAD code. This alteration is necessary in order to be able to handle everything in a single switch block. Coming soon in upcoming commit.

ni4 · 2023-07-12T09:56:37Z

src/librepgp/stream-parse.cpp

@@ -1877,6 +2087,39 @@ get_compressed_src_alg(pgp_source_t *src, uint8_t *alg)
    return true;
 }

+static bool
+parse_aead_chunk_size(uint8_t chunk_size_octet, size_t *chunk_size)


It would be cleaner to use size_t &chunk_size here as it is non-NULL.

fixed in upcoming commit

ni4 · 2023-07-12T10:02:22Z

src/librepgp/stream-sig.cpp

-        hash.add(hdr, 3);
-        hash.add(key.hashed_data, key.hashed_len);
-        return;
+    if (key.version <= PGP_V4) {


Please use switch here to not miss V5 later and better readability.

ni4 · 2023-07-12T12:18:26Z

src/rnp/rnp.cpp

@@ -60,6 +60,10 @@ static const char *usage =
  "  -V, --version        Print RNP version information.\n"
  "  -e, --encrypt        Encrypt data using the public key(s).\n"
  "    -r, --recipient    Specify recipient's key via uid/keyid/fingerprint.\n"
+#if defined(ENABLE_CRYPTO_REFRESH)
+  "    --v3-pkesk-only    Only create v3 PKESK (otherwise v6 will be created if "
+  "appropirate).\n"


Typo: should be 'appropriate'

ni4 · 2023-07-12T12:19:48Z

src/rnpkeys/tui.cpp

+#endif
+#if defined(ENABLE_PQC)
+          "\t(25) (Dilithium3 + Ed25519) + (Kyber768 + X25519)\n"
+          //"\t(26) (Dilithium5 + Ed448) + (Kyber1024 + X448)\n"


It's not good to have commented-out code here.

ni4 · 2023-07-12T12:21:14Z

src/rnpkeys/tui.cpp

+#if defined(ENABLE_CRYPTO_REFRESH)
+          "\t(21) EDDSA + ECDH (v6 key) \n"
+#endif
+          "\t(22) EDDSA + ECDH (v4 key) \n"


Mentioning v4 here is redundant in case of disabled crypto-refresh support.

ni4 · 2023-07-12T12:22:21Z

src/tests/CMakeLists.txt

@@ -109,6 +109,7 @@ add_executable(rnp_tests
  ffi-key.cpp
  file-utils.cpp
  generatekey.cpp
+  hkdf.cpp


This and ones below should not be compiled in if PQC/v6 are disabled.

Fixed in upcoming commit.

ni4 · 2023-07-12T12:24:47Z

src/tests/generatekey.cpp

@@ -1148,6 +1148,7 @@ TEST_F(rnp_tests, test_generated_key_sigs)
        pgp_signature_info_t     ssiginfo = {};

        memset(&desc, 0, sizeof(desc));
+        desc.pgp_version = PGP_V4;


This should be put under ifdef, so we make sure that without setting a version everything works as before.

correction of key and signature related code based on rnp feedback address some more points of PR reiew address some more points of PR reiew rename parse_v3 and update comments correction of key version handling according to rnp feedback changes to v6 key & sig remove redundant ifdef fix conditional compilation of Botan C++ RNG

addressed various feedback comments

…/eddsa abstraction for the following PQC integration

correction of dilithium based on rnp feedback

correction of kyber based on rnp feedback

conditionally compile RNP_ALGNAME_* macros address some more points of PR review (JRH) corrections of key-gen ui according to rnp feedback SHA3 hash binding for dilithium-exdsa-composite

…and pgp_key_material_t::curve()

manual correction of code formatting that still deviates

update copyright (2)

pqc draft: remove packet hash pqc draft: add ecc pubkey to hashed ecdh key share pqc draft: check AES for v3 PKESK

check SPHINCS+ Botan feature

fix Botan deprecation in kyber code fix build system remove comments reverse ifdefs and config.h inclusion in rnp.h fix false positive in CodeQL

…ster branch

fixed missing case in forget_secret_key_fields()

TJ-91 · 2024-04-24T07:55:50Z

@falko-strenzke We can close this right?

falko-strenzke · 2024-04-24T08:06:29Z

What was the history here again? We made a new branch by reordering and squashing the commits from this one, didn't we?

TJ-91 · 2024-04-24T08:09:46Z

Yes, I think so, and then we (you) transferred the commits to another repo (direct fork of RNP) to make a PR

falko-strenzke requested a review from ni4 June 13, 2023 11:58

github-advanced-security bot found potential problems Jun 13, 2023

View reviewed changes

falko-strenzke force-pushed the v6_pqc_2306 branch from 8342963 to 30b4688 Compare June 14, 2023 11:42

TJ-91 reviewed Jun 15, 2023

View reviewed changes

src/tests/rnp_tests.h Outdated Show resolved Hide resolved

TJ-91 reviewed Jun 15, 2023

View reviewed changes

src/lib/CMakeLists.txt Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Jun 15, 2023

View reviewed changes

ni4 reviewed Jun 28, 2023

View reviewed changes

include/repgp/repgp_def.h Show resolved Hide resolved

ni4 reviewed Jun 28, 2023

View reviewed changes

falko-strenzke force-pushed the v6_pqc_2306 branch from 9a81f4a to 40ca8f5 Compare June 29, 2023 11:22

ni4 suggested changes Jul 12, 2023

View reviewed changes

ni4 mentioned this pull request Jul 21, 2023

Valgrind Error when using AEAD Encryption via CLI #40

Closed

falko-strenzke force-pushed the v6_pqc_2306 branch 15 times, most recently from bfd274f to dcb7636 Compare July 26, 2023 12:06

TJ-91 force-pushed the v6_pqc_2306 branch from 87cae3d to 5a3a9e1 Compare July 28, 2023 14:12

TJ-91 force-pushed the v6_pqc_2306 branch 2 times, most recently from 72ecad3 to f953a39 Compare September 6, 2023 13:43

falko-strenzke mentioned this pull request Sep 6, 2023

addition of v6 and PQC rnpgp/rnp#2124

Closed

falko-strenzke force-pushed the v6_pqc_2306 branch from f953a39 to a7e3388 Compare September 7, 2023 06:26

TJ-91 and others added 18 commits September 11, 2023 12:54

added v6 PKESK and v2 SEIPD

744fe3e

addressed various feedback comments

add standalone v6 Ed25519 algorithm

e3b72cc

add standalone v6 X25519 encryption algorithm; add ecdh kem and ecdsa…

1e1757c

…/eddsa abstraction for the following PQC integration

add dilithium standalone functionality

012fd6a

correction of dilithium based on rnp feedback

add dilithium + ecdsa/eddsa composite algorithm

c9b1cf5

add kmac functionality

64ba6cd

add kyber standalone functionality

8be734b

correction of kyber based on rnp feedback

add kyber ecdh composite algorithms

eb8ba12

conditionally compile RNP_ALGNAME_* macros address some more points of PR review (JRH) corrections of key-gen ui according to rnp feedback SHA3 hash binding for dilithium-exdsa-composite

add check for PKESK v3 and XX25519 for AES; align pgp_key_t::curve() …

2ab6c04

…and pgp_key_material_t::curve()

correction of code formatting

4d0fc64

manual correction of code formatting that still deviates

update copyright

19bde74

update copyright (2)

clang format

ad0e455

pqc draft: update dom separation string

cfa5b9b

pqc draft: remove packet hash pqc draft: add ecc pubkey to hashed ecdh key share pqc draft: check AES for v3 PKESK

clang format

f17d80b

add SPHINCS+

bd8ab73

check SPHINCS+ Botan feature

several fixes

e42827a

fix Botan deprecation in kyber code fix build system remove comments reverse ifdefs and config.h inclusion in rnp.h fix false positive in CodeQL

replace fallthrough statements

89e91a3

falko-strenzke force-pushed the v6_pqc_2306 branch from a7e3388 to 89e91a3 Compare September 11, 2023 10:54

falko-strenzke added 2 commits September 18, 2023 09:08

fixed API string for KMAC-256 to be compatible with official Botan ma…

c804bad

…ster branch

various fixes for v6 key operations

737c71f

fixed missing case in forget_secret_key_fields()

falko-strenzke force-pushed the v6_pqc_2306 branch from 2baa4fc to 737c71f Compare September 25, 2023 07:40

falko-strenzke closed this Apr 24, 2024

addition of v6 and PQC #35

addition of v6 and PQC #35

Conversation

falko-strenzke commented Jun 13, 2023

github-advanced-security bot left a comment

Choose a reason for hiding this comment

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ni4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ni4 commented Jul 7, 2023

TJ-91 commented Jul 11, 2023

ni4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TJ-91 commented Apr 24, 2024

falko-strenzke commented Apr 24, 2024

TJ-91 commented Apr 24, 2024