Skip to content

Commit

Permalink
No v6 ECC keys with legacy OIDs (#234)
Browse files Browse the repository at this point in the history
This PR addresses a compliance issue with RFC 9580, which mandates:

"Implementations MUST NOT accept or generate version 6 key material 
using the deprecated OIDs."

Currently, the implementation allows generation of v6 keys with deprecated OIDs, 
which violates the specification. This MR resolves the issue by introducing an error during 
key generation and parsing when deprecated OIDs are detected.
  • Loading branch information
lubux authored Oct 1, 2024
1 parent 77090fe commit f6ad483
Show file tree
Hide file tree
Showing 6 changed files with 86 additions and 9 deletions.
6 changes: 4 additions & 2 deletions openpgp/internal/ecc/curve_info.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import (
"github.com/ProtonMail/go-crypto/openpgp/internal/encoding"
)

const Curve25519GenName = "Curve25519"

type CurveInfo struct {
GenName string
Oid *encoding.OID
Expand Down Expand Up @@ -43,7 +45,7 @@ var Curves = []CurveInfo{
},
{
// Curve25519
GenName: "Curve25519",
GenName: Curve25519GenName,
Oid: encoding.NewOID([]byte{0x2B, 0x06, 0x01, 0x04, 0x01, 0x97, 0x55, 0x01, 0x05, 0x01}),
Curve: NewCurve25519(),
},
Expand All @@ -55,7 +57,7 @@ var Curves = []CurveInfo{
},
{
// Ed25519
GenName: "Curve25519",
GenName: Curve25519GenName,
Oid: encoding.NewOID([]byte{0x2B, 0x06, 0x01, 0x04, 0x01, 0xDA, 0x47, 0x0F, 0x01}),
Curve: NewEd25519(),
},
Expand Down
12 changes: 9 additions & 3 deletions openpgp/key_generation.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ func NewEntity(name, comment, email string, config *packet.Config) (*Entity, err
}
primary := packet.NewSignerPrivateKey(creationTime, primaryPrivRaw)
if config.V6() {
primary.UpgradeToV6()
if err := primary.UpgradeToV6(); err != nil {
return nil, err
}
}

e := &Entity{
Expand Down Expand Up @@ -187,7 +189,9 @@ func (e *Entity) AddSigningSubkey(config *packet.Config) error {
sub := packet.NewSignerPrivateKey(creationTime, subPrivRaw)
sub.IsSubkey = true
if config.V6() {
sub.UpgradeToV6()
if err := sub.UpgradeToV6(); err != nil {
return err
}
}

subkey := Subkey{
Expand Down Expand Up @@ -232,7 +236,9 @@ func (e *Entity) addEncryptionSubkey(config *packet.Config, creationTime time.Ti
sub := packet.NewDecrypterPrivateKey(creationTime, subPrivRaw)
sub.IsSubkey = true
if config.V6() {
sub.UpgradeToV6()
if err := sub.UpgradeToV6(); err != nil {
return err
}
}

subkey := Subkey{
Expand Down
30 changes: 29 additions & 1 deletion openpgp/packet/public_key.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,10 @@ func (pk *PublicKey) UpgradeToV5() {

// UpgradeToV6 updates the version of the key to v6, and updates all necessary
// fields.
func (pk *PublicKey) UpgradeToV6() {
func (pk *PublicKey) UpgradeToV6() error {
pk.Version = 6
pk.setFingerprintAndKeyId()
return pk.checkV6Compatibility()
}

// signingKey provides a convenient abstraction over signature verification
Expand Down Expand Up @@ -313,6 +314,23 @@ func (pk *PublicKey) setFingerprintAndKeyId() {
}
}

func (pk *PublicKey) checkV6Compatibility() error {
// Implementations MUST NOT accept or generate version 6 key material using the deprecated OIDs.
switch pk.PubKeyAlgo {
case PubKeyAlgoECDH:
curveInfo := ecc.FindByOid(pk.oid)
if curveInfo == nil {
return errors.UnsupportedError(fmt.Sprintf("unknown oid: %x", pk.oid))
}
if curveInfo.GenName == ecc.Curve25519GenName {
return errors.StructuralError("cannot generate v6 key with deprecated OID: Curve25519Legacy")
}
case PubKeyAlgoEdDSA:
return errors.StructuralError("cannot generate v6 key with deprecated algorithm: EdDSALegacy")
}
return nil
}

// parseRSA parses RSA public key material from the given Reader. See RFC 4880,
// section 5.5.2.
func (pk *PublicKey) parseRSA(r io.Reader) (err error) {
Expand Down Expand Up @@ -437,6 +455,11 @@ func (pk *PublicKey) parseECDH(r io.Reader) (err error) {
return errors.UnsupportedError(fmt.Sprintf("unknown oid: %x", pk.oid))
}

if pk.Version == 6 && curveInfo.GenName == ecc.Curve25519GenName {
// Implementations MUST NOT accept or generate version 6 key material using the deprecated OIDs.
return errors.StructuralError("cannot read v6 key with deprecated OID: Curve25519Legacy")
}

pk.p = new(encoding.MPI)
if _, err = pk.p.ReadFrom(r); err != nil {
return
Expand Down Expand Up @@ -474,6 +497,11 @@ func (pk *PublicKey) parseECDH(r io.Reader) (err error) {
}

func (pk *PublicKey) parseEdDSA(r io.Reader) (err error) {
if pk.Version == 6 {
// Implementations MUST NOT accept or generate version 6 key material using the deprecated OIDs.
return errors.StructuralError("cannot generate v6 key with deprecated algorithm: EdDSALegacy")
}

pk.oid = new(encoding.OID)
if _, err = pk.oid.ReadFrom(r); err != nil {
return
Expand Down
12 changes: 9 additions & 3 deletions openpgp/v2/key_generation.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,9 @@ func newEntity(uid *userIdData, config *packet.Config) (*Entity, error) {
}
primary := packet.NewSignerPrivateKey(creationTime, primaryPrivRaw)
if config.V6() {
primary.UpgradeToV6()
if err := primary.UpgradeToV6(); err != nil {
return nil, err
}
}

keyProperties := selectKeyProperties(creationTime, config, primary)
Expand Down Expand Up @@ -259,7 +261,9 @@ func (e *Entity) AddSigningSubkey(config *packet.Config) error {
sub.IsSubkey = true
// Every subkey for a v6 primary key MUST be a v6 subkey.
if e.PrimaryKey.Version == 6 {
sub.UpgradeToV6()
if err := sub.UpgradeToV6(); err != nil {
return err
}
}

subkey := Subkey{
Expand Down Expand Up @@ -308,7 +312,9 @@ func (e *Entity) addEncryptionSubkey(config *packet.Config, creationTime time.Ti
sub.IsSubkey = true
// Every subkey for a v6 primary key MUST be a v6 subkey.
if e.PrimaryKey.Version == 6 {
sub.UpgradeToV6()
if err := sub.UpgradeToV6(); err != nil {
return err
}
}

subkey := Subkey{
Expand Down
17 changes: 17 additions & 0 deletions openpgp/v2/keys_test_data.go
Original file line number Diff line number Diff line change
Expand Up @@ -536,3 +536,20 @@ VppQxdtxPvAA/34snHBX7Twnip1nMt7P4e2hDiw/hwQ7oqioOvc6jMkP
=Z8YJ
-----END PGP PRIVATE KEY BLOCK-----
`

const LegacyECCKeyV6 = `-----BEGIN PGP PRIVATE KEY BLOCK-----
xVoGZtnEsRYAAAAtCSsGAQQB2kcPAQEHQJ8u3Y3xhq7vDoYgyIjhniUavG9lhqqn
eoQLj08i1UyCAAEAnZMljJWA74axuVNrL6uhyBLGCwypSeWCXBRqKzR4yjzCswYf
FgoAAABBBQJm2cSxAhsDAh4JCAsJCAcKDQwLBRUKCQgLAhYCIiEG6/BxiEOhwOXw
t2eAShAq9G0keONgsN0KdQSyvNnuD3YAAAAAFEcgEheAMaYvmoAP4UKwK0hOPtFC
wnuf7v+O9tpko93/9GQA9Am6sr0XTsa8RDRc49lm5k2WYwGx5yk2JgsFLX26RJAA
/j0NorcLtTCV/uejDw26ZsWg2kF69ovLed4wF26iCaAHx18GZtnEsRIAAAAyCisG
AQQBl1UBBQEBB0DavdPV3M/mAO5/IkYVdHu006uVFO3eIuZ2ffJPoe5KGQMBCAcA
AP9V2OXFg/QewXMGMNol6S1DUbKhGuyKEK0hrmzUz224qMKZBhgWCAAAACwFAmbZ
xLECGwwiIQbr8HGIQ6HA5fC3Z4BKECr0bSR442Cw3Qp1BLK82e4PdgAAAAoJEOvw
cYhDocDl37EQls/BpUt572AJX8ZBqbsycgD/QfCjSRcPHelHFHkaAHMzXscDBaUS
jiCm2KiM+33wOYYA/2Hu4xmHg7wN2prRcB4+2qkUIdEzMyFs5TYcOU7Hst0N
=pm0Y
-----END PGP PRIVATE KEY BLOCK-----
`
18 changes: 18 additions & 0 deletions openpgp/v2/keys_v6_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,3 +224,21 @@ func TestKeyGenerationHighSecurityLevel(t *testing.T) {
}

}

func TestKeyGenLegacyECC(t *testing.T) {
c := &packet.Config{
V6Keys: true,
Algorithm: packet.PubKeyAlgoEdDSA,
}
_, err := NewEntity("V6 Key Owner", "V6 Key", "[email protected]", c)
if err == nil {
t.Fatal("should not be able to generate v6 keys with legacy OIDs")
}
}

func TestReadLegacyECC(t *testing.T) {
_, err := ReadArmoredKeyRing(strings.NewReader(LegacyECCKeyV6))
if err == nil {
t.Fatal("should not be able to read v6 legacy ECC key")
}
}

0 comments on commit f6ad483

Please sign in to comment.