From afaf038311228a626c7622728285df06229518e4 Mon Sep 17 00:00:00 2001 From: Lukas Burkhalter Date: Mon, 11 Nov 2024 11:36:52 +0100 Subject: [PATCH 1/4] feat: Validate argon2 s2k params on read --- openpgp/packet/private_key.go | 6 ++- openpgp/packet/symmetric_key_encrypted.go | 6 ++- openpgp/s2k/s2k.go | 50 +++++++++++++++++++---- 3 files changed, 50 insertions(+), 12 deletions(-) diff --git a/openpgp/packet/private_key.go b/openpgp/packet/private_key.go index f04e6c6b..6e5433d0 100644 --- a/openpgp/packet/private_key.go +++ b/openpgp/packet/private_key.go @@ -40,7 +40,7 @@ type PrivateKey struct { Encrypted bool // if true then the private key is unavailable until Decrypt has been called. encryptedData []byte cipher CipherFunction - s2k func(out, in []byte) + s2k func(out, in []byte) error aead AEADMode // only relevant if S2KAEAD is enabled // An *{rsa|dsa|elgamal|ecdh|ecdsa|ed25519|ed448}.PrivateKey or // crypto.Signer/crypto.Decrypter (Decryptor RSA only). @@ -629,7 +629,9 @@ func (pk *PrivateKey) Decrypt(passphrase []byte) error { } key := make([]byte, pk.cipher.KeySize()) - pk.s2k(key, passphrase) + if err := pk.s2k(key, passphrase); err != nil { + return err + } if pk.s2kType == S2KAEAD { key = pk.applyHKDF(key) } diff --git a/openpgp/packet/symmetric_key_encrypted.go b/openpgp/packet/symmetric_key_encrypted.go index f843c35b..c18a8b08 100644 --- a/openpgp/packet/symmetric_key_encrypted.go +++ b/openpgp/packet/symmetric_key_encrypted.go @@ -26,7 +26,7 @@ type SymmetricKeyEncrypted struct { Version int CipherFunc CipherFunction Mode AEADMode - s2k func(out, in []byte) + s2k func(out, in []byte) error iv []byte encryptedKey []byte // Contains also the authentication tag for AEAD } @@ -121,7 +121,9 @@ func (ske *SymmetricKeyEncrypted) parse(r io.Reader) error { // packet. func (ske *SymmetricKeyEncrypted) Decrypt(passphrase []byte) ([]byte, CipherFunction, error) { key := make([]byte, ske.CipherFunc.KeySize()) - ske.s2k(key, passphrase) + if err := ske.s2k(key, passphrase); err != nil { + return nil, ske.CipherFunc, err + } if len(ske.encryptedKey) == 0 { return key, ske.CipherFunc, nil } diff --git a/openpgp/s2k/s2k.go b/openpgp/s2k/s2k.go index 92511580..28950a9a 100644 --- a/openpgp/s2k/s2k.go +++ b/openpgp/s2k/s2k.go @@ -199,8 +199,8 @@ func Generate(rand io.Reader, c *Config) (*Params, error) { } params = &Params{ - mode: SaltedS2K, - hashId: hashId, + mode: SaltedS2K, + hashId: hashId, } } else { // Enforce IteratedSaltedS2K method otherwise hashId, ok := algorithm.HashToHashId(c.hash()) @@ -226,7 +226,7 @@ func Generate(rand io.Reader, c *Config) (*Params, error) { // and returns a function which performs that transform. If the S2K is a special // GNU extension that indicates that the private key is missing, then the error // returned is errors.ErrDummyPrivateKey. -func Parse(r io.Reader) (f func(out, in []byte), err error) { +func Parse(r io.Reader) (f func(out, in []byte) error, err error) { params, err := ParseIntoParams(r) if err != nil { return nil, err @@ -319,7 +319,7 @@ func (params *Params) salt() []byte { } } -func (params *Params) Function() (f func(out, in []byte), err error) { +func (params *Params) Function() (f func(out, in []byte) error, err error) { if params.Dummy() { return nil, errors.ErrDummyPrivateKey("dummy key found") } @@ -337,26 +337,33 @@ func (params *Params) Function() (f func(out, in []byte), err error) { switch params.mode { case SimpleS2K: - f := func(out, in []byte) { + f := func(out, in []byte) error { Simple(out, hashObj.New(), in) + return nil } return f, nil case SaltedS2K: - f := func(out, in []byte) { + f := func(out, in []byte) error { Salted(out, hashObj.New(), in, params.salt()) + return nil } return f, nil case IteratedSaltedS2K: - f := func(out, in []byte) { + f := func(out, in []byte) error { Iterated(out, hashObj.New(), in, params.salt(), decodeCount(params.countByte)) + return nil } return f, nil case Argon2S2K: - f := func(out, in []byte) { + f := func(out, in []byte) error { + if err := validateArgon2Params(params); err != nil { + return err + } Argon2(out, in, params.salt(), params.passes, params.parallelism, params.memoryExp) + return nil } return f, nil } @@ -412,3 +419,30 @@ func Serialize(w io.Writer, key []byte, rand io.Reader, passphrase []byte, c *Co f(key, passphrase) return nil } + +// validateArgon2Params checks that the argon2 parameters are valid according to RFC9580. +func validateArgon2Params(params *Params) error { + // The number of passes t and the degree of parallelism p MUST be non-zero. + if params.parallelism == 0 { + return errors.StructuralError("invalid argon2 params: parallelism is 0") + } + if params.passes == 0 { + return errors.StructuralError("invalid argon2 params: iterations is 0") + } + + // The encoded memory size MUST be a value from 3+ceil(log2(p)) to 31. + p := params.parallelism + ceiledLogarithm := byte(0) + for p > 1 { + p /= 2 + ceiledLogarithm += 1 + } + if byte(1)< 31 { + return errors.StructuralError("invalid argon2 params: memory is out of bounds") + } + + return nil +} From d1adff9bc12b5f74ffbff32738caac9c42229a75 Mon Sep 17 00:00:00 2001 From: Lukas Burkhalter Date: Mon, 11 Nov 2024 12:02:24 +0100 Subject: [PATCH 2/4] feat: Add test vectors to verify input params --- openpgp/s2k/s2k_test.go | 46 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/openpgp/s2k/s2k_test.go b/openpgp/s2k/s2k_test.go index 8a072057..030911bf 100644 --- a/openpgp/s2k/s2k_test.go +++ b/openpgp/s2k/s2k_test.go @@ -276,3 +276,49 @@ func testSerializeConfigOK(t *testing.T, c *Config) *Params { return params } + +func TestValidateArgon2Params(t *testing.T) { + tests := []struct { + params Params + wantErr bool + }{ + { + params: Params{parallelism: 4, passes: 3, memoryExp: 6}, + wantErr: false, + }, + { + params: Params{parallelism: 0, passes: 3, memoryExp: 6}, + wantErr: true, + }, + { + params: Params{parallelism: 4, passes: 0, memoryExp: 6}, + wantErr: true, + }, + { + params: Params{parallelism: 4, passes: 3, memoryExp: 4}, + wantErr: true, + }, + { + params: Params{parallelism: 4, passes: 3, memoryExp: 32}, + wantErr: true, + }, + { + params: Params{parallelism: 4, passes: 3, memoryExp: 5}, + wantErr: false, + }, + { + params: Params{parallelism: 4, passes: 3, memoryExp: 31}, + wantErr: false, + }, + } + + for _, tt := range tests { + err := validateArgon2Params(&tt.params) + if tt.wantErr && err == nil { + t.Errorf("validateArgon2Params: expected an error") + } + if !tt.wantErr && err != nil { + t.Error("validateArgon2Params: expected no error") + } + } +} From 42c5666f6a60ace0e9fc044e46a3939b37964cb0 Mon Sep 17 00:00:00 2001 From: Lukas Burkhalter Date: Mon, 11 Nov 2024 12:28:01 +0100 Subject: [PATCH 3/4] feat: Validate on parse --- openpgp/packet/private_key.go | 6 ++---- openpgp/packet/symmetric_key_encrypted.go | 6 ++---- openpgp/s2k/s2k.go | 22 +++++++++------------- 3 files changed, 13 insertions(+), 21 deletions(-) diff --git a/openpgp/packet/private_key.go b/openpgp/packet/private_key.go index 6e5433d0..f04e6c6b 100644 --- a/openpgp/packet/private_key.go +++ b/openpgp/packet/private_key.go @@ -40,7 +40,7 @@ type PrivateKey struct { Encrypted bool // if true then the private key is unavailable until Decrypt has been called. encryptedData []byte cipher CipherFunction - s2k func(out, in []byte) error + s2k func(out, in []byte) aead AEADMode // only relevant if S2KAEAD is enabled // An *{rsa|dsa|elgamal|ecdh|ecdsa|ed25519|ed448}.PrivateKey or // crypto.Signer/crypto.Decrypter (Decryptor RSA only). @@ -629,9 +629,7 @@ func (pk *PrivateKey) Decrypt(passphrase []byte) error { } key := make([]byte, pk.cipher.KeySize()) - if err := pk.s2k(key, passphrase); err != nil { - return err - } + pk.s2k(key, passphrase) if pk.s2kType == S2KAEAD { key = pk.applyHKDF(key) } diff --git a/openpgp/packet/symmetric_key_encrypted.go b/openpgp/packet/symmetric_key_encrypted.go index c18a8b08..f843c35b 100644 --- a/openpgp/packet/symmetric_key_encrypted.go +++ b/openpgp/packet/symmetric_key_encrypted.go @@ -26,7 +26,7 @@ type SymmetricKeyEncrypted struct { Version int CipherFunc CipherFunction Mode AEADMode - s2k func(out, in []byte) error + s2k func(out, in []byte) iv []byte encryptedKey []byte // Contains also the authentication tag for AEAD } @@ -121,9 +121,7 @@ func (ske *SymmetricKeyEncrypted) parse(r io.Reader) error { // packet. func (ske *SymmetricKeyEncrypted) Decrypt(passphrase []byte) ([]byte, CipherFunction, error) { key := make([]byte, ske.CipherFunc.KeySize()) - if err := ske.s2k(key, passphrase); err != nil { - return nil, ske.CipherFunc, err - } + ske.s2k(key, passphrase) if len(ske.encryptedKey) == 0 { return key, ske.CipherFunc, nil } diff --git a/openpgp/s2k/s2k.go b/openpgp/s2k/s2k.go index 28950a9a..5a5b31b5 100644 --- a/openpgp/s2k/s2k.go +++ b/openpgp/s2k/s2k.go @@ -226,7 +226,7 @@ func Generate(rand io.Reader, c *Config) (*Params, error) { // and returns a function which performs that transform. If the S2K is a special // GNU extension that indicates that the private key is missing, then the error // returned is errors.ErrDummyPrivateKey. -func Parse(r io.Reader) (f func(out, in []byte) error, err error) { +func Parse(r io.Reader) (f func(out, in []byte), err error) { params, err := ParseIntoParams(r) if err != nil { return nil, err @@ -283,6 +283,9 @@ func ParseIntoParams(r io.Reader) (params *Params, err error) { params.passes = buf[Argon2SaltSize] params.parallelism = buf[Argon2SaltSize+1] params.memoryExp = buf[Argon2SaltSize+2] + if err := validateArgon2Params(params); err != nil { + return nil, err + } return params, nil case GnuS2K: // This is a GNU extension. See @@ -319,7 +322,7 @@ func (params *Params) salt() []byte { } } -func (params *Params) Function() (f func(out, in []byte) error, err error) { +func (params *Params) Function() (f func(out, in []byte), err error) { if params.Dummy() { return nil, errors.ErrDummyPrivateKey("dummy key found") } @@ -337,33 +340,26 @@ func (params *Params) Function() (f func(out, in []byte) error, err error) { switch params.mode { case SimpleS2K: - f := func(out, in []byte) error { + f := func(out, in []byte) { Simple(out, hashObj.New(), in) - return nil } return f, nil case SaltedS2K: - f := func(out, in []byte) error { + f := func(out, in []byte) { Salted(out, hashObj.New(), in, params.salt()) - return nil } return f, nil case IteratedSaltedS2K: - f := func(out, in []byte) error { + f := func(out, in []byte) { Iterated(out, hashObj.New(), in, params.salt(), decodeCount(params.countByte)) - return nil } return f, nil case Argon2S2K: - f := func(out, in []byte) error { - if err := validateArgon2Params(params); err != nil { - return err - } + f := func(out, in []byte) { Argon2(out, in, params.salt(), params.passes, params.parallelism, params.memoryExp) - return nil } return f, nil } From cec76d0ab83e9a5925785fc1b753a37d5a5d96cf Mon Sep 17 00:00:00 2001 From: Lukas Burkhalter <10532077+lubux@users.noreply.github.com> Date: Mon, 11 Nov 2024 12:43:01 +0100 Subject: [PATCH 4/4] refactor: Get rid of the logarithm computation. Co-authored-by: Daniel Huigens --- openpgp/s2k/s2k.go | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/openpgp/s2k/s2k.go b/openpgp/s2k/s2k.go index 5a5b31b5..6871b84f 100644 --- a/openpgp/s2k/s2k.go +++ b/openpgp/s2k/s2k.go @@ -426,17 +426,9 @@ func validateArgon2Params(params *Params) error { return errors.StructuralError("invalid argon2 params: iterations is 0") } - // The encoded memory size MUST be a value from 3+ceil(log2(p)) to 31. - p := params.parallelism - ceiledLogarithm := byte(0) - for p > 1 { - p /= 2 - ceiledLogarithm += 1 - } - if byte(1)< 31 { + // The encoded memory size MUST be a value from 3+ceil(log2(p)) to 31, + // such that the decoded memory size m is a value from 8*p to 2^31. + if params.memoryExp > 31 || decodeMemory(params.memoryExp) < 8*uint32(params.parallelism) { return errors.StructuralError("invalid argon2 params: memory is out of bounds") }