From 98c94c50c95c0f040d4976560548a5307f0fa4ed Mon Sep 17 00:00:00 2001 From: Aron Wussler Date: Fri, 30 Aug 2019 16:51:02 +0200 Subject: [PATCH 01/11] Add Notation data support in subpackets --- openpgp/keys_test.go | 42 ++++++++++++++++++++++++++++++++++ openpgp/keys_test_data.go | 32 ++++++++++++++++++++++++++ openpgp/packet/notation.go | 45 +++++++++++++++++++++++++++++++++++++ openpgp/packet/signature.go | 31 +++++++++++++++++++++++++ 4 files changed, 150 insertions(+) create mode 100644 openpgp/packet/notation.go diff --git a/openpgp/keys_test.go b/openpgp/keys_test.go index 35595e6b..eff8fb1c 100644 --- a/openpgp/keys_test.go +++ b/openpgp/keys_test.go @@ -960,6 +960,48 @@ func TestNewEntityPrivateSerialization(t *testing.T) { } } +func TestNotationPacket(t *testing.T) { + keys, err := ReadArmoredKeyRing(bytes.NewBufferString(keyWithNotation)) + if err != nil { + t.Fatal(err) + } + + if len(keys) != 1 { + t.Errorf("Failed to accept key, %d", len(keys)) + } + + identity := keys[0].Identities["Testt "] + + if numSigs, numExpected := len(identity.Signatures), 1; numSigs != numExpected { + t.Fatalf("got %d signatures, expected %d", numSigs, numExpected) + } + + notations := identity.Signatures[0].Notations + if numSigs, numExpected := len(notations), 2; numSigs != numExpected { + t.Fatalf("got %d Data Notation subpackets, expected %d", numSigs, numExpected) + } + + if notations[0].IsHumanReadable() != true { + t.Fatalf("got false, expected true") + } + + if notations[0].GetName() != "test@example.com" { + t.Fatalf("got %s, expected test@example.com", notations[0].GetName()) + } + + if notations[0].GetStringValue() != "2" { + t.Fatalf("got %s, expected 2", notations[0].GetName()) + } + + if notations[1].GetName() != "test@example.com" { + t.Fatalf("got %s, expected test@example.com", notations[0].GetName()) + } + + if notations[1].GetStringValue() != "3" { + t.Fatalf("got %s, expected 3", notations[0].GetName()) + } +} + func TestEntityPrivateSerialization(t *testing.T) { keys, err := ReadArmoredKeyRing(bytes.NewBufferString(armoredPrivateKeyBlock)) if err != nil { diff --git a/openpgp/keys_test_data.go b/openpgp/keys_test_data.go index 4bcfd5fd..4b07446c 100644 --- a/openpgp/keys_test_data.go +++ b/openpgp/keys_test_data.go @@ -518,3 +518,35 @@ XLCBln+wdewpU4ChEffMUDRBfqfQco/YsMqWV7bHJHAO0eC/DMKCjyU90xdH7R/d QgqsfguR1PqPuJxpXV4bSr6CGAAAAA== =MSvh -----END PGP PRIVATE KEY BLOCK-----` + +const keyWithNotation = `-----BEGIN PGP PUBLIC KEY BLOCK----- + +mQENBFzQOToBCADd0Pwh8edZ6gR3x49L1PaBPtiAQUr1QDUDWeNes8co5MTFl5hG +lHzptt+VD0JGucuIkvi34f5z2ZbInAV/xYDX3kSYefy6LB8XJD527I/o9bqY1P7T +PjtTZ4emcqNGkGhV2hNGV+hFcTevUS9Ty4vGg6P7X6RjfjeTrClHelJT8+9IiH+4 +0h4X/Y1hwoijRWanYnZjuAUIrOXnG76iknXQRGc8th8iI0oIZfKQomfF0K5lXFhH +SU8Yvmik3vCTLHC6Ce0GVRCTIcU0/Xi2MK/Yrg9bGzSblHxomLU0NT6pee+2UjqR +BZXOAPLY66Lsh1oqxQ6ihVnOmbraU9glAGm1ABEBAAG0EFRlc3R0IDx0ZXN0QGV4 +YT6JAYoEEwEIAHQCGwMFCQPCZwAFCwkIBwIGFQoJCAsCBBYCAwECHgECF4AWIQQZ +jHIqS6wzbp2qrkRXnQGzq+FUDgUCXNA5VBoUgAAAAAAQAAF0ZXN0QGV4YW1wbGUu +Y29tMhoUgAAAAAAQAAF0ZXN0QGV4YW1wbGUuY29tMwAKCRBXnQGzq+FUDoLiCACn +ls1iy0hT59Xt3o3tmmxe1jLzkbQEprR6MMfZamtex5/BHViu2HPAu5i13mXyBRnJ +4Zvd/HUxJukP3tdQyJIlZFe8XwloMoRAA37KOZ5QGyKH8Jxq3LcAcQOOkFtWgr+Z +JbjUKF1IuqCsK6SYB8f7SVKgpZk/kqG3HE3gk72ONnqdvwOa9cIhAuZScdgZ+PLC +6W/0+IrnQIasvKeEWeK4u6/NYT35HUsUE/9Z6WKF+qxJnp5Pi2Q5cio6bFlGDNQb ++MiuiEb3Mzb3ev2PVg7WELBRXOg8QlCxrghqfi1SH791mmiyGK+GIQgnjRwMejTh +dNsnHYag/KAewag55AQvuQENBFzQOToBCADJD+auK+Opo1q3ZLxODMyw5//XHJH4 +0vQPNawyBiOdBuneWHF3jfDwGa+lOftUx1abSwsq+Qs955THgLVSiJvivHWVy8pN +tPv0XLa9rMj2wh/OmckbcgzSMeJJIz09bTj095ONPGYW2D4AcpkOc+b5bkqV6r+N +yk9nopPJNCNqYYJtecTClDT5haRKBP5XjXRVsIXva/nHZGXKQLX8iWG2D5DOJNDP +ZkAEoIPg+7J85Q3u2iSFPnLPzKHlMAoQW8d9RAEYyJ6WqiILUIDShhvXg+RIkzri +wY/WkvhB/Kpj0r1SRbNhWRpmOWCR+0a2uHaLz9X0KTP7WMqQbmIdpRgZABEBAAGJ +ATwEGAEIACYWIQQZjHIqS6wzbp2qrkRXnQGzq+FUDgUCXNA5OgIbDAUJA8JnAAAK +CRBXnQGzq+FUDgI6B/9Far0CUR6rWvUiviBY4P5oe44I9P9P7ilWmum1cIQWxMyF +0sc5tRcVLpMomURlrDz0TR5GNs+nuGAHTRBfN7VO0Y+R/LyEd1Rf80ONObXOqzMp +vF9CdW3a7W4WicZwnGgUOImTICazR2VmR+RREdZshqrOCaOnuKmN3QwGH1zzFwJA +sTbLoNMdBv8SEARaRVOWPM1HwJ701mMYF48FqhHd5uinH/ZCeBhqrBfhmXa68FWx +xuyJz6ttl5Fp4nsB3waQdgPGZJ9NUrGfopLUZ44xDuJjBONd7rbYOh71TWbHd8wG +V+HOQJQxXJkVRYa3QrFUehiMzTeqqMdgC6ZqJy7+ +=et/d +-----END PGP PUBLIC KEY BLOCK-----` diff --git a/openpgp/packet/notation.go b/openpgp/packet/notation.go new file mode 100644 index 00000000..967b00fb --- /dev/null +++ b/openpgp/packet/notation.go @@ -0,0 +1,45 @@ +package packet + +// Notation type represents a Notation Data subpacket +// see https://tools.ietf.org/html/rfc4880#section-5.2.3.16 +type Notation struct { + flags []byte + name string + value []byte + critical bool +} + +func (not *Notation) IsHumanReadable() (bool) { + return not.flags[0] & 0x80 == 0x80 +} + +func (not *Notation) GetName() (string) { + return not.name +} + +func (not *Notation) GetBinaryValue() ([]byte) { + return not.value +} + +func (not *Notation) GetStringValue() (string) { + return string(not.value) +} + +func (not *Notation) IsCritical() (bool) { + return not.critical +} + +func (not *Notation) getData() ([]byte) { + nameLen := len(not.name) + valueLen := len(not.value) + nameData := []byte(not.name) + + data := not.flags + data[4] = byte(nameLen >> 8) + data[5] = byte(nameLen) + data[6] = byte(valueLen >> 8) + data[7] = byte(valueLen) + + data = append(data, nameData...) + return append(data, not.value...) +} diff --git a/openpgp/packet/signature.go b/openpgp/packet/signature.go index 73c705f6..f6a9241b 100644 --- a/openpgp/packet/signature.go +++ b/openpgp/packet/signature.go @@ -71,6 +71,7 @@ type Signature struct { IssuerFingerprint []byte SignerUserId *string IsPrimaryId *bool + Notations []Notation // TrustLevel and TrustAmount can be set by the signer to assert that // the key is not only valid but also trustworthy at the specified @@ -248,6 +249,7 @@ const ( keyExpirationSubpacket signatureSubpacketType = 9 prefSymmetricAlgosSubpacket signatureSubpacketType = 11 issuerSubpacket signatureSubpacketType = 16 + notationDataSubpacket signatureSubpacketType = 20 prefHashAlgosSubpacket signatureSubpacketType = 21 prefCompressionSubpacket signatureSubpacketType = 22 primaryUserIdSubpacket signatureSubpacketType = 25 @@ -367,6 +369,24 @@ func parseSignatureSubpacket(sig *Signature, subpacket []byte, isHashed bool) (r } sig.IssuerKeyId = new(uint64) *sig.IssuerKeyId = binary.BigEndian.Uint64(subpacket) + case notationDataSubpacket: + // Notation data, section 5.2.3.16 + nameLength := uint32(subpacket[4])<<8 | uint32(subpacket[5]) + valueLength := uint32(subpacket[6])<<8 | uint32(subpacket[7]) + + if len(subpacket) != (int(nameLength) + int(valueLength) + 8) { + err = errors.StructuralError("notation data subpacket with bad length") + return + } + + notation := Notation{ + flags: subpacket[0:4], + name: string(subpacket[8: (nameLength + 8)]), + value: subpacket[(nameLength + 8) : (valueLength + nameLength + 8)], + critical: isCritical, + } + + sig.Notations = append(sig.Notations, notation) case prefHashAlgosSubpacket: // Preferred hash algorithms, section 5.2.3.8 if !isHashed { @@ -938,6 +958,17 @@ func (sig *Signature) buildSubpackets(issuer PublicKey) (subpackets []outputSubp subpackets = append(subpackets, outputSubpacket{true, keyFlagsSubpacket, false, []byte{flags}}) } + for _, notation := range sig.Notations { + subpackets = append( + subpackets, + outputSubpacket{ + true, + notationDataSubpacket, + notation.IsCritical(), + notation.getData(), + }) + } + // The following subpackets may only appear in self-signatures. var features = byte(0x00) From 4744f3dacd6cc68b77788f1517a26ca6560a28e9 Mon Sep 17 00:00:00 2001 From: Aron Wussler Date: Tue, 6 Dec 2022 18:07:15 +0100 Subject: [PATCH 02/11] Test serialization --- openpgp/keys_test.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/openpgp/keys_test.go b/openpgp/keys_test.go index eff8fb1c..42813022 100644 --- a/openpgp/keys_test.go +++ b/openpgp/keys_test.go @@ -966,6 +966,23 @@ func TestNotationPacket(t *testing.T) { t.Fatal(err) } + assertNotationPackets(t, keys) + + serializedEntity := bytes.NewBuffer(nil) + err = keys[0].Serialize(serializedEntity) + if err != nil { + t.Fatal(err) + } + + keys, err = ReadKeyRing(serializedEntity) + if err != nil { + t.Fatal(err) + } + + assertNotationPackets(t, keys) +} + +func assertNotationPackets(t *testing.T, keys EntityList) { if len(keys) != 1 { t.Errorf("Failed to accept key, %d", len(keys)) } From d1152870321f27290623de6273b8d31a6972d187 Mon Sep 17 00:00:00 2001 From: Aron Wussler Date: Tue, 27 Dec 2022 13:09:15 +0100 Subject: [PATCH 03/11] Make notation structure fields exportable --- openpgp/keys_test.go | 18 ++++++++-------- openpgp/packet/notation.go | 42 ++++++++++++------------------------- openpgp/packet/signature.go | 10 ++++----- 3 files changed, 27 insertions(+), 43 deletions(-) diff --git a/openpgp/keys_test.go b/openpgp/keys_test.go index 42813022..186df735 100644 --- a/openpgp/keys_test.go +++ b/openpgp/keys_test.go @@ -998,24 +998,24 @@ func assertNotationPackets(t *testing.T, keys EntityList) { t.Fatalf("got %d Data Notation subpackets, expected %d", numSigs, numExpected) } - if notations[0].IsHumanReadable() != true { + if notations[0].HumanReadable != true { t.Fatalf("got false, expected true") } - if notations[0].GetName() != "test@example.com" { - t.Fatalf("got %s, expected test@example.com", notations[0].GetName()) + if notations[0].Name != "test@example.com" { + t.Fatalf("got %s, expected test@example.com", notations[0].Name) } - if notations[0].GetStringValue() != "2" { - t.Fatalf("got %s, expected 2", notations[0].GetName()) + if string(notations[0].Value) != "2" { + t.Fatalf("got %s, expected 2", string(notations[0].Value)) } - if notations[1].GetName() != "test@example.com" { - t.Fatalf("got %s, expected test@example.com", notations[0].GetName()) + if notations[1].Name != "test@example.com" { + t.Fatalf("got %s, expected test@example.com", notations[1].Name) } - if notations[1].GetStringValue() != "3" { - t.Fatalf("got %s, expected 3", notations[0].GetName()) + if string(notations[1].Value) != "3" { + t.Fatalf("got %s, expected 3", string(notations[1].Value)) } } diff --git a/openpgp/packet/notation.go b/openpgp/packet/notation.go index 967b00fb..5c215c2e 100644 --- a/openpgp/packet/notation.go +++ b/openpgp/packet/notation.go @@ -3,43 +3,27 @@ package packet // Notation type represents a Notation Data subpacket // see https://tools.ietf.org/html/rfc4880#section-5.2.3.16 type Notation struct { - flags []byte - name string - value []byte - critical bool + Name string + Value []byte + Critical bool + HumanReadable bool } -func (not *Notation) IsHumanReadable() (bool) { - return not.flags[0] & 0x80 == 0x80 -} - -func (not *Notation) GetName() (string) { - return not.name -} - -func (not *Notation) GetBinaryValue() ([]byte) { - return not.value -} - -func (not *Notation) GetStringValue() (string) { - return string(not.value) -} - -func (not *Notation) IsCritical() (bool) { - return not.critical -} +func (not *Notation) getData() []byte { + nameData := []byte(not.Name) + nameLen := len(nameData) + valueLen := len(not.Value) -func (not *Notation) getData() ([]byte) { - nameLen := len(not.name) - valueLen := len(not.value) - nameData := []byte(not.name) + data := make([]byte, 8 + nameLen + valueLen) + if not.HumanReadable { + data[0] = 0x80 + } - data := not.flags data[4] = byte(nameLen >> 8) data[5] = byte(nameLen) data[6] = byte(valueLen >> 8) data[7] = byte(valueLen) data = append(data, nameData...) - return append(data, not.value...) + return append(data, not.Value...) } diff --git a/openpgp/packet/signature.go b/openpgp/packet/signature.go index f6a9241b..23540111 100644 --- a/openpgp/packet/signature.go +++ b/openpgp/packet/signature.go @@ -380,10 +380,10 @@ func parseSignatureSubpacket(sig *Signature, subpacket []byte, isHashed bool) (r } notation := Notation{ - flags: subpacket[0:4], - name: string(subpacket[8: (nameLength + 8)]), - value: subpacket[(nameLength + 8) : (valueLength + nameLength + 8)], - critical: isCritical, + HumanReadable: (subpacket[0] & 0x80) == 0x80, + Name: string(subpacket[8: (nameLength + 8)]), + Value: subpacket[(nameLength + 8) : (valueLength + nameLength + 8)], + Critical: isCritical, } sig.Notations = append(sig.Notations, notation) @@ -964,7 +964,7 @@ func (sig *Signature) buildSubpackets(issuer PublicKey) (subpackets []outputSubp outputSubpacket{ true, notationDataSubpacket, - notation.IsCritical(), + notation.Critical, notation.getData(), }) } From 08684b70d64713df2a8b1ef2c2c6bc7a73dea92b Mon Sep 17 00:00:00 2001 From: Daniel Huigens Date: Fri, 27 Jan 2023 16:29:45 +0100 Subject: [PATCH 04/11] Rename Critical to IsCritical and HumanReadable to IsHumanReadable --- openpgp/keys_test.go | 2 +- openpgp/packet/notation.go | 6 +++--- openpgp/packet/signature.go | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/openpgp/keys_test.go b/openpgp/keys_test.go index 186df735..357f7610 100644 --- a/openpgp/keys_test.go +++ b/openpgp/keys_test.go @@ -998,7 +998,7 @@ func assertNotationPackets(t *testing.T, keys EntityList) { t.Fatalf("got %d Data Notation subpackets, expected %d", numSigs, numExpected) } - if notations[0].HumanReadable != true { + if notations[0].IsHumanReadable != true { t.Fatalf("got false, expected true") } diff --git a/openpgp/packet/notation.go b/openpgp/packet/notation.go index 5c215c2e..f4bd97b2 100644 --- a/openpgp/packet/notation.go +++ b/openpgp/packet/notation.go @@ -5,8 +5,8 @@ package packet type Notation struct { Name string Value []byte - Critical bool - HumanReadable bool + IsCritical bool + IsHumanReadable bool } func (not *Notation) getData() []byte { @@ -15,7 +15,7 @@ func (not *Notation) getData() []byte { valueLen := len(not.Value) data := make([]byte, 8 + nameLen + valueLen) - if not.HumanReadable { + if not.IsHumanReadable { data[0] = 0x80 } diff --git a/openpgp/packet/signature.go b/openpgp/packet/signature.go index 23540111..8cd89026 100644 --- a/openpgp/packet/signature.go +++ b/openpgp/packet/signature.go @@ -380,10 +380,10 @@ func parseSignatureSubpacket(sig *Signature, subpacket []byte, isHashed bool) (r } notation := Notation{ - HumanReadable: (subpacket[0] & 0x80) == 0x80, + IsHumanReadable: (subpacket[0] & 0x80) == 0x80, Name: string(subpacket[8: (nameLength + 8)]), Value: subpacket[(nameLength + 8) : (valueLength + nameLength + 8)], - Critical: isCritical, + IsCritical: isCritical, } sig.Notations = append(sig.Notations, notation) @@ -964,7 +964,7 @@ func (sig *Signature) buildSubpackets(issuer PublicKey) (subpackets []outputSubp outputSubpacket{ true, notationDataSubpacket, - notation.Critical, + notation.IsCritical, notation.getData(), }) } From 73a6b8f68014bc9882a7d6dd1626c8eba8fe5de0 Mon Sep 17 00:00:00 2001 From: Daniel Huigens Date: Fri, 27 Jan 2023 17:17:05 +0100 Subject: [PATCH 05/11] Add config.KnownNotations and check critical notations --- openpgp/packet/config.go | 11 +++++++ openpgp/read.go | 21 +++++++++--- openpgp/read_test.go | 57 +++++++++++++++++++++++++++++++++ openpgp/read_write_test_data.go | 33 +++++++++++++++++++ 4 files changed, 118 insertions(+), 4 deletions(-) diff --git a/openpgp/packet/config.go b/openpgp/packet/config.go index f9208158..4e2b27d1 100644 --- a/openpgp/packet/config.go +++ b/openpgp/packet/config.go @@ -94,6 +94,10 @@ type Config struct { // might be no other way than to tolerate the missing MDC. Setting this flag, allows this // mode of operation. It should be considered a measure of last resort. InsecureAllowUnauthenticatedMessages bool + // KnownNotations is a map of Notation Data names to bools, which controls + // the notation names that are allowed to be present in critical Notation Data + // signature subpackets. + KnownNotations map[string]bool } func (c *Config) Random() io.Reader { @@ -202,3 +206,10 @@ func (c *Config) AllowUnauthenticatedMessages() bool { } return c.InsecureAllowUnauthenticatedMessages } + +func (c *Config) KnownNotation(notationName string) bool { + if c == nil { + return false + } + return c.KnownNotations[notationName] +} diff --git a/openpgp/read.go b/openpgp/read.go index 34d324c0..130de839 100644 --- a/openpgp/read.go +++ b/openpgp/read.go @@ -515,12 +515,15 @@ func CheckArmoredDetachedSignature(keyring KeyRing, signed, signature io.Reader, } // checkSignatureDetails returns an error if: +// - The signature (or one of the binding signatures mentioned below) +// has a unknown critical notation data subpacket // - The primary key of the signing entity is revoked // The signature was signed by a subkey and: // - The signing subkey is revoked // - The primary identity is revoked // - The signature is expired -// - The primary key of the signing entity is expired according to the primary identity binding signature +// - The primary key of the signing entity is expired according to the +// primary identity binding signature // The signature was signed by a subkey and: // - The signing subkey is expired according to the subkey binding signature // - The signing subkey binding signature is expired @@ -534,20 +537,30 @@ func CheckArmoredDetachedSignature(keyring KeyRing, signed, signature io.Reader, func checkSignatureDetails(key *Key, signature *packet.Signature, config *packet.Config) error { now := config.Now() primaryIdentity := key.Entity.PrimaryIdentity() + signedBySubKey := key.PublicKey != key.Entity.PrimaryKey sigsToCheck := []*packet.Signature{ signature, primaryIdentity.SelfSignature } + if signedBySubKey { + sigsToCheck = append(sigsToCheck, key.SelfSignature, key.SelfSignature.EmbeddedSignature) + } + for _, sig := range sigsToCheck { + for _, not := range sig.Notations { + if not.IsCritical && !config.KnownNotation(not.Name) { + return errors.SignatureError("unknown critical notation: " + not.Name) + } + } + } if key.Entity.Revoked(now) || // primary key is revoked - (key.PublicKey != key.Entity.PrimaryKey && key.Revoked(now)) || // subkey is revoked + (signedBySubKey && key.Revoked(now)) || // subkey is revoked primaryIdentity.Revoked(now) { // primary identity is revoked return errors.ErrKeyRevoked } if key.Entity.PrimaryKey.KeyExpired(primaryIdentity.SelfSignature, now) { // primary key is expired return errors.ErrKeyExpired } - if key.PublicKey != key.Entity.PrimaryKey { + if signedBySubKey { if key.PublicKey.KeyExpired(key.SelfSignature, now) { // subkey is expired return errors.ErrKeyExpired } - sigsToCheck = append(sigsToCheck, key.SelfSignature, key.SelfSignature.EmbeddedSignature) } for _, sig := range sigsToCheck { if sig.SigExpired(now) { // any of the relevant signatures are expired diff --git a/openpgp/read_test.go b/openpgp/read_test.go index c9d1f334..10c92b78 100644 --- a/openpgp/read_test.go +++ b/openpgp/read_test.go @@ -435,6 +435,63 @@ func TestDetachedSignatureExpiredCrossSig(t *testing.T) { } } +func TestSignatureUnknownNotation(t *testing.T) { + el, err := ReadArmoredKeyRing(bytes.NewBufferString(criticalNotationSigner)) + if err != nil { + t.Error(err) + } + raw, err := armor.Decode(strings.NewReader(signedMessageWithCriticalNotation)) + if err != nil { + t.Error(err) + return + } + md, err := ReadMessage(raw.Body, el, nil, nil) + if err != nil { + t.Error(err) + return + } + _, err = ioutil.ReadAll(md.UnverifiedBody) + if err != nil { + t.Error(err) + return + } + const expectedErr string = "openpgp: invalid signature: unknown critical notation: test@example.com" + if md.SignatureError == nil || md.SignatureError.Error() != expectedErr { + t.Errorf("Expected error '%s', but got error '%s'", expectedErr, md.SignatureError) + } +} + +func TestSignatureKnownNotation(t *testing.T) { + el, err := ReadArmoredKeyRing(bytes.NewBufferString(criticalNotationSigner)) + if err != nil { + t.Error(err) + } + raw, err := armor.Decode(strings.NewReader(signedMessageWithCriticalNotation)) + if err != nil { + t.Error(err) + return + } + config := &packet.Config{ + KnownNotations: map[string]bool{ + "test@example.com": true, + }, + } + md, err := ReadMessage(raw.Body, el, nil, config) + if err != nil { + t.Error(err) + return + } + _, err = ioutil.ReadAll(md.UnverifiedBody) + if err != nil { + t.Error(err) + return + } + if md.SignatureError != nil { + t.Error(md.SignatureError) + return + } +} + func TestReadingArmoredPrivateKey(t *testing.T) { el, err := ReadArmoredKeyRing(bytes.NewBufferString(armoredPrivateKeyBlock)) if err != nil { diff --git a/openpgp/read_write_test_data.go b/openpgp/read_write_test_data.go index d65a3215..a5224424 100644 --- a/openpgp/read_write_test_data.go +++ b/openpgp/read_write_test_data.go @@ -239,3 +239,36 @@ ITEG9mMgp3TGS9ZzSifMZ8UGtHdp9QdBg8NEVPFzDOMGxpc/Bftav7RRRuPiAER+ =aQkm -----END PGP SIGNATURE----- ` + +const signedMessageWithCriticalNotation = `-----BEGIN PGP MESSAGE----- + +owGbwMvMwMH4oOW7S46CznTG09xJDDE3Wl1KUotLuDousDAwcjBYiSmyXL+48d6x +U1PSGUxcj8IUszKBVMpMaWAAAgEGZpAeh9SKxNyCnFS95PzcytRiBi5OAZjyXXzM +f8WYLqv7TXP61Sa4rqT12CI3xaN73YS2pt089f96odCKaEPnWJ3iSGmzJaW/ug10 +2Zo8Wj2k4s7t8wt4H3HtTu+y5UZfV3VOO+l//sdE/o+Lsub8FZH7/eOq7OnbNp4n +vwjE8mqJXetNMfj8r2SCyvkEnlVRYR+/mnge+ib56FdJ8uKtqSxyvgA= +=fRXs +-----END PGP MESSAGE-----` + +const criticalNotationSigner = `-----BEGIN PGP PUBLIC KEY BLOCK----- + +mI0EUmEvTgEEANyWtQQMOybQ9JltDqmaX0WnNPJeLILIM36sw6zL0nfTQ5zXSS3+ +fIF6P29lJFxpblWk02PSID5zX/DYU9/zjM2xPO8Oa4xo0cVTOTLj++Ri5mtr//f5 +GLsIXxFrBJhD/ghFsL3Op0GXOeLJ9A5bsOn8th7x6JucNKuaRB6bQbSPABEBAAG0 +JFRlc3QgTWNUZXN0aW5ndG9uIDx0ZXN0QGV4YW1wbGUuY29tPoi5BBMBAgAjBQJS +YS9OAhsvBwsJCAcDAgEGFQgCCQoLBBYCAwECHgECF4AACgkQSmNhOk1uQJQwDAP6 +AgrTyqkRlJVqz2pb46TfbDM2TDF7o9CBnBzIGoxBhlRwpqALz7z2kxBDmwpQa+ki +Bq3jZN/UosY9y8bhwMAlnrDY9jP1gdCo+H0sD48CdXybblNwaYpwqC8VSpDdTndf +9j2wE/weihGp/DAdy/2kyBCaiOY1sjhUfJ1GogF49rC4jQRSYS9OAQQA6R/PtBFa +JaT4jq10yqASk4sqwVMsc6HcifM5lSdxzExFP74naUMMyEsKHP53QxTF0Grqusag +Qg/ZtgT0CN1HUM152y7ACOdp1giKjpMzOTQClqCoclyvWOFB+L/SwGEIJf7LSCEr +woBuJifJc8xAVr0XX0JthoW+uP91eTQ3XpsAEQEAAYkBPQQYAQIACQUCUmEvTgIb +LgCoCRBKY2E6TW5AlJ0gBBkBAgAGBQJSYS9OAAoJEOCE90RsICyXuqIEANmmiRCA +SF7YK7PvFkieJNwzeK0V3F2lGX+uu6Y3Q/Zxdtwc4xR+me/CSBmsURyXTO29OWhP +GLszPH9zSJU9BdDi6v0yNprmFPX/1Ng0Abn/sCkwetvjxC1YIvTLFwtUL/7v6NS2 +bZpsUxRTg9+cSrMWWSNjiY9qUKajm1tuzPDZXAUEAMNmAN3xXN/Kjyvj2OK2ck0X +W748sl/tc3qiKPMJ+0AkMF7Pjhmh9nxqE9+QCEl7qinFqqBLjuzgUhBU4QlwX1GD +AtNTq6ihLMD5v1d82ZC7tNatdlDMGWnIdvEMCv2GZcuIqDQ9rXWs49e7tq1NncLY +hz3tYjKhoFTKEIq3y3Pp +=h/aX +-----END PGP PUBLIC KEY BLOCK-----` From 943deefc6b6976dbd7b2627c64f1ed3d96a66481 Mon Sep 17 00:00:00 2001 From: marinthiercelin Date: Mon, 30 Jan 2023 18:42:52 +0100 Subject: [PATCH 06/11] Fix serialization of signature notation data (#144) --- openpgp/packet/notation.go | 14 ++++++------ openpgp/packet/notation_test.go | 38 +++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 7 deletions(-) create mode 100644 openpgp/packet/notation_test.go diff --git a/openpgp/packet/notation.go b/openpgp/packet/notation.go index f4bd97b2..2e72c2fb 100644 --- a/openpgp/packet/notation.go +++ b/openpgp/packet/notation.go @@ -3,9 +3,9 @@ package packet // Notation type represents a Notation Data subpacket // see https://tools.ietf.org/html/rfc4880#section-5.2.3.16 type Notation struct { - Name string - Value []byte - IsCritical bool + Name string + Value []byte + IsCritical bool IsHumanReadable bool } @@ -14,7 +14,7 @@ func (not *Notation) getData() []byte { nameLen := len(nameData) valueLen := len(not.Value) - data := make([]byte, 8 + nameLen + valueLen) + data := make([]byte, 8+nameLen+valueLen) if not.IsHumanReadable { data[0] = 0x80 } @@ -23,7 +23,7 @@ func (not *Notation) getData() []byte { data[5] = byte(nameLen) data[6] = byte(valueLen >> 8) data[7] = byte(valueLen) - - data = append(data, nameData...) - return append(data, not.Value...) + copy(data[8:8+nameLen], nameData) + copy(data[8+nameLen:], not.Value) + return data } diff --git a/openpgp/packet/notation_test.go b/openpgp/packet/notation_test.go new file mode 100644 index 00000000..9b12daf9 --- /dev/null +++ b/openpgp/packet/notation_test.go @@ -0,0 +1,38 @@ +package packet + +import ( + "bytes" + "testing" +) + +func TestNotationGetData(t *testing.T) { + notation := Notation{ + Name: "test@proton.me", + Value: []byte("test-value"), + IsCritical: true, + IsHumanReadable: true, + } + expected := []byte{0x80, 0, 0, 0, 0, 14, 0, 10} + expected = append(expected, []byte(notation.Name)...) + expected = append(expected, []byte(notation.Value)...) + data := notation.getData() + if !bytes.Equal(expected, data) { + t.Fatalf("Expected %s, got %s", expected, data) + } +} + +func TestNotationGetDataNotHumanReadable(t *testing.T) { + notation := Notation{ + Name: "test@proton.me", + Value: []byte("test-value"), + IsCritical: true, + IsHumanReadable: false, + } + expected := []byte{0, 0, 0, 0, 0, 14, 0, 10} + expected = append(expected, []byte(notation.Name)...) + expected = append(expected, []byte(notation.Value)...) + data := notation.getData() + if !bytes.Equal(expected, data) { + t.Fatalf("Expected %s, got %s", expected, data) + } +} From e58fc9f66422438fb0fb5251db29a00a89cd50d9 Mon Sep 17 00:00:00 2001 From: Daniel Huigens Date: Mon, 30 Jan 2023 18:45:21 +0100 Subject: [PATCH 07/11] Rename `not` to `notation` --- openpgp/packet/notation.go | 10 +++++----- openpgp/read.go | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/openpgp/packet/notation.go b/openpgp/packet/notation.go index 2e72c2fb..2c3e3f50 100644 --- a/openpgp/packet/notation.go +++ b/openpgp/packet/notation.go @@ -9,13 +9,13 @@ type Notation struct { IsHumanReadable bool } -func (not *Notation) getData() []byte { - nameData := []byte(not.Name) +func (notation *Notation) getData() []byte { + nameData := []byte(notation.Name) nameLen := len(nameData) - valueLen := len(not.Value) + valueLen := len(notation.Value) data := make([]byte, 8+nameLen+valueLen) - if not.IsHumanReadable { + if notation.IsHumanReadable { data[0] = 0x80 } @@ -24,6 +24,6 @@ func (not *Notation) getData() []byte { data[6] = byte(valueLen >> 8) data[7] = byte(valueLen) copy(data[8:8+nameLen], nameData) - copy(data[8+nameLen:], not.Value) + copy(data[8+nameLen:], notation.Value) return data } diff --git a/openpgp/read.go b/openpgp/read.go index 130de839..537aa4c1 100644 --- a/openpgp/read.go +++ b/openpgp/read.go @@ -543,9 +543,9 @@ func checkSignatureDetails(key *Key, signature *packet.Signature, config *packet sigsToCheck = append(sigsToCheck, key.SelfSignature, key.SelfSignature.EmbeddedSignature) } for _, sig := range sigsToCheck { - for _, not := range sig.Notations { - if not.IsCritical && !config.KnownNotation(not.Name) { - return errors.SignatureError("unknown critical notation: " + not.Name) + for _, notation := range sig.Notations { + if notation.IsCritical && !config.KnownNotation(notation.Name) { + return errors.SignatureError("unknown critical notation: " + notation.Name) } } } From 84b8fbad64743e73f31a2fca301b6a9d7217e6b7 Mon Sep 17 00:00:00 2001 From: Daniel Huigens Date: Mon, 30 Jan 2023 19:16:45 +0100 Subject: [PATCH 08/11] Test re-serialization of notation data subpackets --- openpgp/keys_test.go | 16 ++++++++----- openpgp/keys_test_data.go | 48 ++++++++++++++------------------------- 2 files changed, 27 insertions(+), 37 deletions(-) diff --git a/openpgp/keys_test.go b/openpgp/keys_test.go index 357f7610..124d55b8 100644 --- a/openpgp/keys_test.go +++ b/openpgp/keys_test.go @@ -969,7 +969,7 @@ func TestNotationPacket(t *testing.T) { assertNotationPackets(t, keys) serializedEntity := bytes.NewBuffer(nil) - err = keys[0].Serialize(serializedEntity) + err = keys[0].SerializePrivate(serializedEntity, nil) if err != nil { t.Fatal(err) } @@ -987,7 +987,7 @@ func assertNotationPackets(t *testing.T, keys EntityList) { t.Errorf("Failed to accept key, %d", len(keys)) } - identity := keys[0].Identities["Testt "] + identity := keys[0].Identities["Test "] if numSigs, numExpected := len(identity.Signatures), 1; numSigs != numExpected { t.Fatalf("got %d signatures, expected %d", numSigs, numExpected) @@ -1002,19 +1002,23 @@ func assertNotationPackets(t *testing.T, keys EntityList) { t.Fatalf("got false, expected true") } - if notations[0].Name != "test@example.com" { + if notations[0].Name != "text@example.com" { t.Fatalf("got %s, expected test@example.com", notations[0].Name) } - if string(notations[0].Value) != "2" { + if string(notations[0].Value) != "test" { t.Fatalf("got %s, expected 2", string(notations[0].Value)) } - if notations[1].Name != "test@example.com" { + if notations[1].IsHumanReadable != false { + t.Fatalf("got true, expected false") + } + + if notations[1].Name != "binary@example.com" { t.Fatalf("got %s, expected test@example.com", notations[1].Name) } - if string(notations[1].Value) != "3" { + if !bytes.Equal(notations[1].Value, []byte{0, 1, 2, 3}) { t.Fatalf("got %s, expected 3", string(notations[1].Value)) } } diff --git a/openpgp/keys_test_data.go b/openpgp/keys_test_data.go index 4b07446c..108fd096 100644 --- a/openpgp/keys_test_data.go +++ b/openpgp/keys_test_data.go @@ -519,34 +519,20 @@ QgqsfguR1PqPuJxpXV4bSr6CGAAAAA== =MSvh -----END PGP PRIVATE KEY BLOCK-----` -const keyWithNotation = `-----BEGIN PGP PUBLIC KEY BLOCK----- - -mQENBFzQOToBCADd0Pwh8edZ6gR3x49L1PaBPtiAQUr1QDUDWeNes8co5MTFl5hG -lHzptt+VD0JGucuIkvi34f5z2ZbInAV/xYDX3kSYefy6LB8XJD527I/o9bqY1P7T -PjtTZ4emcqNGkGhV2hNGV+hFcTevUS9Ty4vGg6P7X6RjfjeTrClHelJT8+9IiH+4 -0h4X/Y1hwoijRWanYnZjuAUIrOXnG76iknXQRGc8th8iI0oIZfKQomfF0K5lXFhH -SU8Yvmik3vCTLHC6Ce0GVRCTIcU0/Xi2MK/Yrg9bGzSblHxomLU0NT6pee+2UjqR -BZXOAPLY66Lsh1oqxQ6ihVnOmbraU9glAGm1ABEBAAG0EFRlc3R0IDx0ZXN0QGV4 -YT6JAYoEEwEIAHQCGwMFCQPCZwAFCwkIBwIGFQoJCAsCBBYCAwECHgECF4AWIQQZ -jHIqS6wzbp2qrkRXnQGzq+FUDgUCXNA5VBoUgAAAAAAQAAF0ZXN0QGV4YW1wbGUu -Y29tMhoUgAAAAAAQAAF0ZXN0QGV4YW1wbGUuY29tMwAKCRBXnQGzq+FUDoLiCACn -ls1iy0hT59Xt3o3tmmxe1jLzkbQEprR6MMfZamtex5/BHViu2HPAu5i13mXyBRnJ -4Zvd/HUxJukP3tdQyJIlZFe8XwloMoRAA37KOZ5QGyKH8Jxq3LcAcQOOkFtWgr+Z -JbjUKF1IuqCsK6SYB8f7SVKgpZk/kqG3HE3gk72ONnqdvwOa9cIhAuZScdgZ+PLC -6W/0+IrnQIasvKeEWeK4u6/NYT35HUsUE/9Z6WKF+qxJnp5Pi2Q5cio6bFlGDNQb -+MiuiEb3Mzb3ev2PVg7WELBRXOg8QlCxrghqfi1SH791mmiyGK+GIQgnjRwMejTh -dNsnHYag/KAewag55AQvuQENBFzQOToBCADJD+auK+Opo1q3ZLxODMyw5//XHJH4 -0vQPNawyBiOdBuneWHF3jfDwGa+lOftUx1abSwsq+Qs955THgLVSiJvivHWVy8pN -tPv0XLa9rMj2wh/OmckbcgzSMeJJIz09bTj095ONPGYW2D4AcpkOc+b5bkqV6r+N -yk9nopPJNCNqYYJtecTClDT5haRKBP5XjXRVsIXva/nHZGXKQLX8iWG2D5DOJNDP -ZkAEoIPg+7J85Q3u2iSFPnLPzKHlMAoQW8d9RAEYyJ6WqiILUIDShhvXg+RIkzri -wY/WkvhB/Kpj0r1SRbNhWRpmOWCR+0a2uHaLz9X0KTP7WMqQbmIdpRgZABEBAAGJ -ATwEGAEIACYWIQQZjHIqS6wzbp2qrkRXnQGzq+FUDgUCXNA5OgIbDAUJA8JnAAAK -CRBXnQGzq+FUDgI6B/9Far0CUR6rWvUiviBY4P5oe44I9P9P7ilWmum1cIQWxMyF -0sc5tRcVLpMomURlrDz0TR5GNs+nuGAHTRBfN7VO0Y+R/LyEd1Rf80ONObXOqzMp -vF9CdW3a7W4WicZwnGgUOImTICazR2VmR+RREdZshqrOCaOnuKmN3QwGH1zzFwJA -sTbLoNMdBv8SEARaRVOWPM1HwJ701mMYF48FqhHd5uinH/ZCeBhqrBfhmXa68FWx -xuyJz6ttl5Fp4nsB3waQdgPGZJ9NUrGfopLUZ44xDuJjBONd7rbYOh71TWbHd8wG -V+HOQJQxXJkVRYa3QrFUehiMzTeqqMdgC6ZqJy7+ -=et/d ------END PGP PUBLIC KEY BLOCK-----` +const keyWithNotation = `-----BEGIN PGP PRIVATE KEY BLOCK----- + +xVgEY9gIshYJKwYBBAHaRw8BAQdAF25fSM8OpFlXZhop4Qpqo5ywGZ4jgWlR +ppjhIKDthREAAQC+LFpzFcMJYcjxGKzBGHN0Px2jU4d04YSRnFAik+lVVQ6u +zRdUZXN0IDx0ZXN0QGV4YW1wbGUuY29tPsLACgQQFgoAfAUCY9gIsgQLCQcI +CRD/utJOCym8pR0UgAAAAAAQAAR0ZXh0QGV4YW1wbGUuY29tdGVzdB8UAAAA +AAASAARiaW5hcnlAZXhhbXBsZS5jb20AAQIDAxUICgQWAAIBAhkBAhsDAh4B +FiEEEMCQTUVGKgCX5rDQ/7rSTgspvKUAAPl5AP9Npz90LxzrB97Qr2DrGwfG +wuYn4FSYwtuPfZHHeoIabwD/QEbvpQJ/NBb9EAZuow4Rirlt1yv19mmnF+j5 +8yUzhQjHXQRj2AiyEgorBgEEAZdVAQUBAQdARXAo30DmKcyUg6co7OUm0RNT +z9iqFbDBzA8A47JEt1MDAQgHAAD/XKK3lBm0SqMR558HLWdBrNG6NqKuqb5X +joCML987ZNgRD8J4BBgWCAAqBQJj2AiyCRD/utJOCym8pQIbDBYhBBDAkE1F +RioAl+aw0P+60k4LKbylAADRxgEAg7UfBDiDPp5LHcW9D+SgFHk6+GyEU4ev +VppQxdtxPvAA/34snHBX7Twnip1nMt7P4e2hDiw/hwQ7oqioOvc6jMkP +=Z8YJ +-----END PGP PRIVATE KEY BLOCK----- +` From f59228586effc8b7079701ae5506d226da6118fa Mon Sep 17 00:00:00 2001 From: Daniel Huigens Date: Tue, 31 Jan 2023 13:47:17 +0100 Subject: [PATCH 09/11] Check that notation data subpackets have at least 8 bytes --- openpgp/packet/signature.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/openpgp/packet/signature.go b/openpgp/packet/signature.go index 8cd89026..e5e32967 100644 --- a/openpgp/packet/signature.go +++ b/openpgp/packet/signature.go @@ -371,10 +371,14 @@ func parseSignatureSubpacket(sig *Signature, subpacket []byte, isHashed bool) (r *sig.IssuerKeyId = binary.BigEndian.Uint64(subpacket) case notationDataSubpacket: // Notation data, section 5.2.3.16 + if len(subpacket) < 8 { + err = errors.StructuralError("notation data subpacket with bad length") + return + } + nameLength := uint32(subpacket[4])<<8 | uint32(subpacket[5]) valueLength := uint32(subpacket[6])<<8 | uint32(subpacket[7]) - - if len(subpacket) != (int(nameLength) + int(valueLength) + 8) { + if len(subpacket) != int(nameLength) + int(valueLength) + 8 { err = errors.StructuralError("notation data subpacket with bad length") return } From 713b962201e372e48c6b0b4c69e9f70687eb4e67 Mon Sep 17 00:00:00 2001 From: Daniel Huigens Date: Tue, 31 Jan 2023 13:47:50 +0100 Subject: [PATCH 10/11] Ignore unhashed notation data subpackets --- openpgp/packet/signature.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/openpgp/packet/signature.go b/openpgp/packet/signature.go index e5e32967..ffc1daa4 100644 --- a/openpgp/packet/signature.go +++ b/openpgp/packet/signature.go @@ -371,6 +371,9 @@ func parseSignatureSubpacket(sig *Signature, subpacket []byte, isHashed bool) (r *sig.IssuerKeyId = binary.BigEndian.Uint64(subpacket) case notationDataSubpacket: // Notation data, section 5.2.3.16 + if !isHashed { + return + } if len(subpacket) < 8 { err = errors.StructuralError("notation data subpacket with bad length") return From 657886dc6b94de096009dc0b5886256c5e8f1f78 Mon Sep 17 00:00:00 2001 From: Daniel Huigens Date: Tue, 31 Jan 2023 13:49:47 +0100 Subject: [PATCH 11/11] Make sig.Notations an array of pointers to Notations --- openpgp/packet/signature.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openpgp/packet/signature.go b/openpgp/packet/signature.go index ffc1daa4..df313c75 100644 --- a/openpgp/packet/signature.go +++ b/openpgp/packet/signature.go @@ -71,7 +71,7 @@ type Signature struct { IssuerFingerprint []byte SignerUserId *string IsPrimaryId *bool - Notations []Notation + Notations []*Notation // TrustLevel and TrustAmount can be set by the signer to assert that // the key is not only valid but also trustworthy at the specified @@ -393,7 +393,7 @@ func parseSignatureSubpacket(sig *Signature, subpacket []byte, isHashed bool) (r IsCritical: isCritical, } - sig.Notations = append(sig.Notations, notation) + sig.Notations = append(sig.Notations, ¬ation) case prefHashAlgosSubpacket: // Preferred hash algorithms, section 5.2.3.8 if !isHashed {