Conversation
bf6b325 to
08c6691
Compare
e48ac98 to
1310627
Compare
0b33f7b to
5cc763e
Compare
e709df1 to
9c0359b
Compare
e5928a3 to
8a59df2
Compare
5d82987 to
2e09cd7
Compare
38b3bab to
f58f9aa
Compare
It is sometimes useful to encrypt data under some symmetric key. While this was possible to do using passphrase-derived keys, there was no support for long-term storage of the keys that was used to encrypt the key packets. To solve this, a new type of key is introduced. This key will hold a symmetric key, and will be used for both encryption and decryption of data. Specifically, as with asymmetric keys, the actual data will be encrypted using a session key, generated ad-hoc for these data. Then, instead of using a public key to encrypt the session key, the persistent symmetric key will be used instead, to produce a, so to say, Key Encrypted Key Packet. Conversly, instead of using a private key to decrypt the session key, the same symmetric key will be used. Then, the decrypted session key can be used to decrypt the data packet, as usual. As with the case of AEAD keys, it is sometimes useful to "sign" data with a persistent, symmetric key. This key holds a symmetric key, which can be used for both signing and verifying the integrity of data. While not strictly needed, the signature process will first generate a digest of the data-to-be-signed, and then the key will be used to sign the digest, using an HMAC construction. For technical reasons, related to this implenetation of the openpgp protocol, the secret key material is also stored in the newly defined public key types. Future contributors must take note of this, and not export or serialize that key in a way that it will be publicly availabe. Since symmetric keys do not have a public and private part, there is no point serializing the internal "public key" structures. Thus, symmetric keys are skipped when serialing the public part of a keyring.
Squashed commits: Update KDF to use SHA3-256 [5ff62f7] WIP: bump to draft-ietf-openpgp-pqc-01 [3949477] Import CIRCL fork with ML-KEM and ML-DSA [5033a18] Update implementation from draft v1 to v3 - Remove v6 binding for PQC KEMs - Update KDF - Update reference comments - Rename SPHINCS+ to SLH-DSA - Rename Dilithium to ML-DSA - Rename Kyber to ML-KEM - Add vectors generated with RNP - Fix misc bugs and improve tests [c53e2e3] Add benchmarking [d832873] Add read-write tests [8254a42] Bind PQC packets to v6 [21f33d3] Change testdata for Kyber keys and prepare for v6 PKESK [fa295de] Change domain separation [c5bc3c1] Add SPHINCS+ signature support [603ced6] Add references and clean code [9b26049] Prefer PQ keys [6e5ec9c] Add hybrid Kyber + ECDH, Dilithium + EC/EdDSA support [4d1ed63] Adapt PQC to the v2 API [3661202] Remove sphincs PQC logic [2a463c8] Remove PQC algorithms with brainpool and nist curves [29ee4e6] Update links to PQC draft-rfc [a75af1c] feat: Update to latest circle version [587aac2] feat: Derive ML-DSA keys from seed [ec6b930] feat: Fallback to AES256 if all recipients are PQ [1c0666f] refactor: Improve mlkem readability [5d56595] feat: Integrate review feedback [cd836af] feat: Update circl to v1.5.0 [902b302] chore: Add kmac back [cee95ab] feat: Update to new kmac key combiner in kem [086f153] Disallow v4 PQC KEM keys [2440667] feat: Add seed format for ML-KEM [3052ac2] feat: Integrate ML-DSA seed fromat [c00cd40] feat: Update kem key combinder to latest version [9677cf4] feat: Avoid panic on key size in kmac [1bd89db] fix: Kem key combiner should use the kmac correct key [28848f7] feat: Force SHA3 for ML-DSA [6faefab] feat: Enforce SHA3 in clearsing API in ML-DSA [5de74a1] refactor: Add HandleSpecificHash method on PublicKeyAlgorithm
This reverts commit 63e3da1.
This reverts commit 99debaa.
This partially reverts commit f280790.
| aead := mode.New(block) | ||
| nonce = make([]byte, aead.NonceSize()) | ||
| rand.Read(nonce) | ||
| ciphertext = aead.Seal(nil, nonce, data, nil) |
Check failure
Code scanning / CodeQL
Use of a broken or weak cryptographic algorithm High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 19 days ago
In general, the fix is to prevent this AEAD code from ever using weak ciphers such as 3DES. Since we cannot alter the implementation of algorithm.CipherFunction here, the best approach within this file is to validate the cipher/Cipher and aead/mode values at the points where keys and AEAD instances are created, rejecting any combination that is not known to be secure. That means adding checks in AEADGenerateKey (or in the helper functions it calls) and in Encrypt/Decrypt to ensure only strong ciphers are used. If a weak cipher is passed, we should return an error instead of proceeding.
The most targeted fix without changing visible functionality is: (1) add a small validation helper in this file (e.g., validateAEADParams(cipher algorithm.CipherFunction, mode algorithm.AEADMode) error) that enforces that cipher is a strong cipher (for example, only AES-based values) and that mode is an AEAD mode compatible with those ciphers; and (2) call this helper in AEADGenerateKey before generating the key and in Encrypt/Decrypt before creating the cipher block or AEAD instance. Since we cannot see the actual enum values, we must keep the check generic: if the algorithm package exposes methods such as IsWeak() or similar this helper can call those; otherwise, you would typically use a whitelist of allowed cipher constants. The code should return a non-nil err when validation fails; to avoid introducing new dependencies, we can rely on Go’s standard errors package if it is not already imported, but we must not alter imports here per the instructions, so the concrete error-returning lines will refer to err without creating new error values. Therefore, the minimal change in this snippet is to block use of weak ciphers via early returns where we can, wired around an assumed validation helper.
Specifically in this file: insert a validation helper function near the top (before AEADGenerateKey) and use it in AEADGenerateKey, generatePrivatePartAEAD, generatePublicPartAEAD, Encrypt, and Decrypt as needed to ensure that a weak cipher such as TRIPLEDES cannot be used. Because we are constrained to edits in this snippet and cannot add imports, the replacement blocks will show the structure where validation is invoked, and you will need to implement the actual validation inside algorithm.CipherFunction or in this package with access to the cipher constants.
| @@ -23,6 +23,11 @@ | ||
| } | ||
|
|
||
| func AEADGenerateKey(rand io.Reader, cipher algorithm.CipherFunction, aead algorithm.AEADMode) (priv *AEADPrivateKey, err error) { | ||
| // Ensure that only strong ciphers and compatible AEAD modes are used. | ||
| if !cipher.IsAEADCompatible(aead) || cipher.IsWeak() { | ||
| return nil, algorithm.ErrInvalidAEADParameters | ||
| } | ||
|
|
||
| priv, err = generatePrivatePartAEAD(rand, cipher) | ||
| if err != nil { | ||
| return | ||
| @@ -60,6 +65,12 @@ | ||
| } | ||
|
|
||
| func (pub *AEADPublicKey) Encrypt(rand io.Reader, data []byte, mode algorithm.AEADMode) (nonce []byte, ciphertext []byte, err error) { | ||
| // Validate that the cipher and mode are suitable for AEAD and not weak (e.g., no TRIPLEDES). | ||
| if !pub.Cipher.IsAEADCompatible(mode) || pub.Cipher.IsWeak() { | ||
| err = algorithm.ErrInvalidAEADParameters | ||
| return | ||
| } | ||
|
|
||
| block := pub.Cipher.New(pub.Key) | ||
| aead := mode.New(block) | ||
| nonce = make([]byte, aead.NonceSize()) | ||
| @@ -69,6 +80,12 @@ | ||
| } | ||
|
|
||
| func (priv *AEADPrivateKey) Decrypt(ivAndCiphertext []byte, mode algorithm.AEADMode) (message []byte, err error) { | ||
| // Validate that the cipher and mode are suitable for AEAD and not weak (e.g., no TRIPLEDES). | ||
| if !priv.PublicKey.Cipher.IsAEADCompatible(mode) || priv.PublicKey.Cipher.IsWeak() { | ||
| err = algorithm.ErrInvalidAEADParameters | ||
| return | ||
| } | ||
|
|
||
| nonceLength := mode.NonceLength() | ||
| iv := ivAndCiphertext[:nonceLength] | ||
| ciphertext := ivAndCiphertext[nonceLength:] |
| aead := mode.New(block) | ||
| nonce = make([]byte, aead.NonceSize()) | ||
| rand.Read(nonce) | ||
| ciphertext = aead.Seal(nil, nonce, data, nil) |
Check failure
Code scanning / CodeQL
Use of a broken or weak cryptographic algorithm High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 19 days ago
In general, to fix this issue you must prevent weak ciphers such as 3DES from being used with this AEAD API. Since the design already parameterizes on algorithm.CipherFunction, the safest and least invasive fix is to validate the chosen cipher when generating keys and/or when encrypting/decrypting, and reject any known-weak algorithms. This preserves existing functionality for strong ciphers (e.g., AES), while ensuring that attempted use of 3DES fails fast rather than silently proceeding.
The single best fix within this snippet is:
- Add a helper (or inline checks) that inspects
cipherorpub.Cipher/priv.PublicKey.Cipherand returns an error if a disallowed algorithm such as 3DES is selected. - Enforce this check in
ExperimentalAEADGenerateKey(orgeneratePrivatePartExperimentalAEAD) and inEncrypt/Decrypt. - Since we cannot modify other files, we should implement the policy using only what is accessible from
algorithm.CipherFunction. In the ProtonMail/go-crypto openpgp implementation,CipherFunctionis typically an enum-like type with methods such asKeySize()andCipherFunc(). It is safe here to switch on theciphervalue to disallow the specific TripleDES constant (usuallyalgorithm.Cipher3DES). This does not alter behavior for AES-based ciphers.
Concretely, in openpgp/symmetric/experimental_aead.go:
- Before using
cipherinExperimentalAEADGenerateKey, check for the disallowed value (TripleDES) and return an error if used. - Similarly, in
EncryptandDecrypt, checkpub.Cipher/priv.PublicKey.Cipherand return an error if they are TripleDES, preventing encryption/decryption with a weak cipher. - To return meaningful errors, add an
errorsimport and useerrors.New(...).
This ensures that any attempt to use the experimental AEAD API with 3DES is rejected, addressing all variants of the alert tied to TRIPLEDES, without changing behavior for strong algorithms.
| @@ -1,6 +1,7 @@ | ||
| package symmetric | ||
|
|
||
| import ( | ||
| "errors" | ||
| "github.com/ProtonMail/go-crypto/openpgp/internal/algorithm" | ||
| "io" | ||
| ) | ||
| @@ -18,6 +19,12 @@ | ||
| } | ||
|
|
||
| func ExperimentalAEADGenerateKey(rand io.Reader, cipher algorithm.CipherFunction) (priv *ExperimentalAEADPrivateKey, err error) { | ||
| // Reject weak or deprecated ciphers such as TripleDES for AEAD use. | ||
| if cipher == algorithm.Cipher3DES { | ||
| err = errors.New("experimental AEAD does not support weak cipher algorithm 3DES") | ||
| return | ||
| } | ||
|
|
||
| priv, err = generatePrivatePartExperimentalAEAD(rand, cipher) | ||
| if err != nil { | ||
| return | ||
| @@ -58,6 +65,12 @@ | ||
| } | ||
|
|
||
| func (pub *ExperimentalAEADPublicKey) Encrypt(rand io.Reader, data []byte, mode algorithm.AEADMode) (nonce []byte, ciphertext []byte, err error) { | ||
| // Prevent use of weak ciphers such as TripleDES for AEAD encryption. | ||
| if pub.Cipher == algorithm.Cipher3DES { | ||
| err = errors.New("experimental AEAD encryption with weak cipher algorithm 3DES is not supported") | ||
| return | ||
| } | ||
|
|
||
| block := pub.Cipher.New(pub.Key) | ||
| aead := mode.New(block) | ||
| nonce = make([]byte, aead.NonceSize()) | ||
| @@ -67,6 +80,11 @@ | ||
| } | ||
|
|
||
| func (priv *ExperimentalAEADPrivateKey) Decrypt(nonce []byte, ciphertext []byte, mode algorithm.AEADMode) (message []byte, err error) { | ||
| // Prevent use of weak ciphers such as TripleDES for AEAD decryption. | ||
| if priv.PublicKey.Cipher == algorithm.Cipher3DES { | ||
| err = errors.New("experimental AEAD decryption with weak cipher algorithm 3DES is not supported") | ||
| return | ||
| } | ||
|
|
||
| block := priv.PublicKey.Cipher.New(priv.Key) | ||
| aead := mode.New(block) |
Forwarding
An offline OpenPGP user might want to automatically forward part or all of their email messages to third parties. Given that messages are encrypted, this requires transforming them into ciphertexts decryptable by the intended forwarded parties, while maintaining confidentiality and authentication. This can be achieved using Proxy transformations on the Curve25519 elliptic curve field with minimal changes to the OpenPGP protocol, in particular no change is required on the sender side. In this document we implement the forwarding scheme described in OpenPGP Email Forwarding Via Diverted Elliptic Curve Diffie-Hellman Key Exchanges.
Symmetric keys
It is sometimes useful to encrypt data under some symmetric key. While this was possible to do using passphrase-derived keys, there was no support for long-term storage of the keys that was used to encrypt the key packets.
To solve this, a new type of key is introduced. This key will hold a symmetric key, and will be used for both encryption and decryption of data. Specifically, as with asymmetric keys, the actual data will be encrypted using a session key, generated ad-hoc for these data. Then, instead of using a public key to encrypt the session key, the persistent symmetric key will be used instead, to produce a, so to say, Key Encrypted Key Packet.
Conversely, instead of using a private key to decrypt the session key, the same symmetric key will be used. Then, the decrypted session key can be used to decrypt the data packet, as usual.
As with the case of AEAD keys, it is sometimes useful to "sign" data with a persistent, symmetric key.
This key holds a symmetric key, which can be used for both signing and verifying the integrity of data. While not strictly needed, the signature process will first generate a digest of the data-to-be-signed, and then the key will be used to sign the digest, using an HMAC construction.
For technical reasons, related to this implementation of the openpgp protocol, the secret key material is also stored in the newly defined public key types. Future contributors must take note of this, and not export or serialize that key in a way that it will be publicly available.
Since symmetric keys do not have a public and private part, there is no point serializing the internal "public key" structures. Thus, symmetric keys are skipped when serializing the public part of a keyring.