From fd5f17ee729917fcb39f16421460212d917a0813 Mon Sep 17 00:00:00 2001 From: aviau Date: Tue, 12 Jun 2018 16:26:42 -0400 Subject: [PATCH] openpgp: read keys with revoked user ids The existing code was wrongly assuming that UserID packets must be immediately followed by a Signature packet. However, this is not true. See RFC4880 11.1: > Immediately following each User ID packet, there are zero or more > Signature packets. This change will ensure that Entities that are not immediately followed by a Signature packet are read without raising a StructuralError. Instead, UserID packets that are not immediately followed by a self signature will be ignored. Maximum backwards compatibility is retained because revoked UserIDs are not added to the Entity's identities. In a follow-up patch, we should probably add these UserIDs to the Entity's identities too, but not without making sure that the revocation is also available in the Entity's (or the Identity's) Revocations slice. This would require adding support for a new Signature Type, "Certification revocation signature", as defined in RFC 48880 5.2.1. Fixes golang/go#25850 Change-Id: Idde34b97429998f28e0c687171024e51ed959bf0 Reviewed-on: https://go-review.googlesource.com/118376 Reviewed-by: Filippo Valsorda Run-TryBot: Filippo Valsorda TryBot-Result: Gobot Gobot --- openpgp/keys.go | 14 ++++++---- openpgp/keys_test.go | 66 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 5 deletions(-) diff --git a/openpgp/keys.go b/openpgp/keys.go index aceb1b7..216b812 100644 --- a/openpgp/keys.go +++ b/openpgp/keys.go @@ -346,22 +346,25 @@ EachPacket: switch pkt := p.(type) { case *packet.UserId: + // Make a new Identity object, that we might wind up throwing away. + // We'll only add it if we get a valid self-signature over this + // userID. current = new(Identity) current.Name = pkt.Id current.UserId = pkt - e.Identities[pkt.Id] = current for { p, err = packets.Next() if err == io.EOF { - return nil, io.ErrUnexpectedEOF + break EachPacket } else if err != nil { return nil, err } sig, ok := p.(*packet.Signature) if !ok { - return nil, errors.StructuralError("user ID packet not followed by self-signature") + packets.Unread(p) + continue EachPacket } if (sig.SigType == packet.SigTypePositiveCert || sig.SigType == packet.SigTypeGenericCert) && sig.IssuerKeyId != nil && *sig.IssuerKeyId == e.PrimaryKey.KeyId { @@ -369,9 +372,10 @@ EachPacket: return nil, errors.StructuralError("user ID self-signature invalid: " + err.Error()) } current.SelfSignature = sig - break + e.Identities[pkt.Id] = current + } else { + current.Signatures = append(current.Signatures, sig) } - current.Signatures = append(current.Signatures, sig) } case *packet.Signature: if pkt.SigType == packet.SigTypeKeyRevocation { diff --git a/openpgp/keys_test.go b/openpgp/keys_test.go index fd1f662..40aa391 100644 --- a/openpgp/keys_test.go +++ b/openpgp/keys_test.go @@ -105,6 +105,33 @@ func TestGoodCrossSignature(t *testing.T) { } } +func TestRevokedUserID(t *testing.T) { + // This key contains 2 UIDs, one of which is revoked: + // [ultimate] (1) Golang Gopher + // [ revoked] (2) Golang Gopher + keys, err := ReadArmoredKeyRing(bytes.NewBufferString(revokedUserIDKey)) + if err != nil { + t.Fatal(err) + } + + if len(keys) != 1 { + t.Fatal("Failed to read key with a revoked user id") + } + + var identities []*Identity + for _, identity := range keys[0].Identities { + identities = append(identities, identity) + } + + if numIdentities, numExpected := len(identities), 1; numIdentities != numExpected { + t.Errorf("obtained %d identities, expected %d", numIdentities, numExpected) + } + + if identityName, expectedName := identities[0].Name, "Golang Gopher "; identityName != expectedName { + t.Errorf("obtained identity %s expected %s", identityName, expectedName) + } +} + // TestExternallyRevokableKey attempts to load and parse a key with a third party revocation permission. func TestExternallyRevocableKey(t *testing.T) { kring, err := ReadKeyRing(readerFromHex(subkeyUsageHex)) @@ -481,3 +508,42 @@ SqLHvbKh2dL/RXymC3+rjPvQf5cup9bPxNMa6WagdYBNAfzWGtkVISeaQW+cTEp/ MtgVijRGXR/lGLGETPg2X3Afwn9N9bLMBkBprKgbBqU7lpaoPupxT61bL70= =vtbN -----END PGP PUBLIC KEY BLOCK-----` + +const revokedUserIDKey = `-----BEGIN PGP PUBLIC KEY BLOCK----- + +mQENBFsgO5EBCADhREPmcjsPkXe1z7ctvyWL0S7oa9JaoGZ9oPDHFDlQxd0qlX2e +DZJZDg0qYvVixmaULIulApq1puEsaJCn3lHUbHlb4PYKwLEywYXM28JN91KtLsz/ +uaEX2KC5WqeP40utmzkNLq+oRX/xnRMgwbO7yUNVG2UlEa6eI+xOXO3YtLdmJMBW +ClQ066ZnOIzEo1JxnIwha1CDBMWLLfOLrg6l8InUqaXbtEBbnaIYO6fXVXELUjkx +nmk7t/QOk0tXCy8muH9UDqJkwDUESY2l79XwBAcx9riX8vY7vwC34pm22fAUVLCJ +x1SJx0J8bkeNp38jKM2Zd9SUQqSbfBopQ4pPABEBAAG0I0dvbGFuZyBHb3BoZXIg +PG5vLXJlcGx5QGdvbGFuZy5jb20+iQFUBBMBCgA+FiEE5Ik5JLcNx6l6rZfw1oFy +9I6cUoMFAlsgO5ECGwMFCQPCZwAFCwkIBwMFFQoJCAsFFgIDAQACHgECF4AACgkQ +1oFy9I6cUoMIkwf8DNPeD23i4jRwd/pylbvxwZintZl1fSwTJW1xcOa1emXaEtX2 +depuqhP04fjlRQGfsYAQh7X9jOJxAHjTmhqFBi5sD7QvKU00cPFYbJ/JTx0B41bl +aXnSbGhRPh63QtEZL7ACAs+shwvvojJqysx7kyVRu0EW2wqjXdHwR/SJO6nhNBa2 +DXzSiOU/SUA42mmG+5kjF8Aabq9wPwT9wjraHShEweNerNMmOqJExBOy3yFeyDpa +XwEZFzBfOKoxFNkIaVf5GSdIUGhFECkGvBMB935khftmgR8APxdU4BE7XrXexFJU +8RCuPXonm4WQOwTWR0vQg64pb2WKAzZ8HhwTGbQiR29sYW5nIEdvcGhlciA8cmV2 +b2tlZEBnb2xhbmcuY29tPokBNgQwAQoAIBYhBOSJOSS3Dcepeq2X8NaBcvSOnFKD +BQJbIDv3Ah0AAAoJENaBcvSOnFKDfWMIAKhI/Tvu3h8fSUxp/gSAcduT6bC1JttG +0lYQ5ilKB/58lBUA5CO3ZrKDKlzW3M8VEcvohVaqeTMKeoQd5rCZq8KxHn/KvN6N +s85REfXfniCKfAbnGgVXX3kDmZ1g63pkxrFu0fDZjVDXC6vy+I0sGyI/Inro0Pzb +tvn0QCsxjapKK15BtmSrpgHgzVqVg0cUp8vqZeKFxarYbYB2idtGRci4b9tObOK0 +BSTVFy26+I/mrFGaPrySYiy2Kz5NMEcRhjmTxJ8jSwEr2O2sUR0yjbgUAXbTxDVE +/jg5fQZ1ACvBRQnB7LvMHcInbzjyeTM3FazkkSYQD6b97+dkWwb1iWG5AQ0EWyA7 +kQEIALkg04REDZo1JgdYV4x8HJKFS4xAYWbIva1ZPqvDNmZRUbQZR2+gpJGEwn7z +VofGvnOYiGW56AS5j31SFf5kro1+1bZQ5iOONBng08OOo58/l1hRseIIVGB5TGSa +PCdChKKHreJI6hS3mShxH6hdfFtiZuB45rwoaArMMsYcjaezLwKeLc396cpUwwcZ +snLUNd1Xu5EWEF2OdFkZ2a1qYdxBvAYdQf4+1Nr+NRIx1u1NS9c8jp3PuMOkrQEi +bNtc1v6v0Jy52mKLG4y7mC/erIkvkQBYJdxPaP7LZVaPYc3/xskcyijrJ/5ufoD8 +K71/ShtsZUXSQn9jlRaYR0EbojMAEQEAAYkBPAQYAQoAJhYhBOSJOSS3Dcepeq2X +8NaBcvSOnFKDBQJbIDuRAhsMBQkDwmcAAAoJENaBcvSOnFKDkFMIAIt64bVZ8x7+ +TitH1bR4pgcNkaKmgKoZz6FXu80+SnbuEt2NnDyf1cLOSimSTILpwLIuv9Uft5Pb +OraQbYt3xi9yrqdKqGLv80bxqK0NuryNkvh9yyx5WoG1iKqMj9/FjGghuPrRaT4l +QinNAghGVkEy1+aXGFrG2DsOC1FFI51CC2WVTzZ5RwR2GpiNRfESsU1rZAUqf/2V +yJl9bD5R4SUNy8oQmhOxi+gbhD4Ao34e4W0ilibslI/uawvCiOwlu5NGd8zv5n+U +heiQvzkApQup5c+BhH5zFDFdKJ2CBByxw9+7QjMFI/wgLixKuE0Ob2kAokXf7RlB +7qTZOahrETw= +=IKnw +-----END PGP PUBLIC KEY BLOCK-----`