Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/crypto/openpgp: silently fail to read keyring when smartcard stub is present #9312

Closed
jvehent opened this issue Dec 14, 2014 · 5 comments
Closed
Assignees
Milestone

Comments

@jvehent
Copy link

@jvehent jvehent commented Dec 14, 2014

When a GPG secring contains a smartcard stub, the openpgp package fails to read the keyring correctly and does not return an error. A call to openpgp.ReadKeyRing() will returns the entities that precedes the stub, excluding the entity located right before the stub.

For example, if a secring contains 3 entities and the smartcard stub is in position 3, then openpgp.ReadKeyRing() will only return the first entity. The second entity and the stub are ignored. No error is returned.

Steps to reproduce:

1. Create a keyring with two regular 1024 bits RSA keys

$ mkdir /tmp/testgolangopenpgp
$ gpg --batch --no-default-keyring --keyring /tmp/testgolangopenpgp/pubring.gpg --secret-keyring /tmp/testgolangopenpgp/secring.gpg --gen-key << EOF
> Key-Type: 1
> Key-Length: 1024
> Subkey-Type: 1
> Subkey-Length: 1024
> Name-Real: test key 1
> Name-Email: testkey1@example.net
> Expire-Date: 12m
> EOF
gpg: keyring `/tmp/testgolangopenpgp/secring.gpg' created
gpg: keyring `/tmp/testgolangopenpgp/pubring.gpg' created
..+++++
...+++++
.....+++++
..+++++
gpg: key 4BACAA9F marked as ultimately trusted
$ gpg --batch --no-default-keyring --keyring /tmp/testgolangopenpgp/pubring.gpg --secret-keyring /tmp/testgolangopenpgp/secring.gpg --gen-key << EOF
> Key-Type: 1
> Key-Length: 1024
> Subkey-Type: 1
> Subkey-Length: 1024
> Name-Real: test key 2
> Name-Email: testkey2@example.net
> Expire-Date: 12m
> EOF
.+++++
.+++++
+++++
+++++
gpg: key 56CE5F62 marked as ultimately trusted

2. Import a smartcard stub from a yubikey into the keyring

$ gpg --no-default-keyring --keyring /tmp/testgolangopenpgp/pubring.gpg --secret-keyring /tmp/testgolangopenpgp/secring.gpg --allow-secret-key-import --import yubikeystub yubikeytest1_stub.asc 
gpg: key 4FB5544F: already in secret keyring
gpg: key 59BEDBFC: already in secret keyring
gpg: Total number processed: 2
gpg:       secret keys read: 2
gpg:  secret keys unchanged: 2

/tmp/testgolangopenpgp/secring.gpg
----------------------------------
sec   1024R/4BACAA9F 2014-12-14 [expires: 2015-12-09]
uid                  test key 1 <testkey1@example.net>
ssb   1024R/AEC9CD3A 2014-12-14

sec   1024R/56CE5F62 2014-12-14 [expires: 2015-12-09]
uid                  test key 2 <testkey2@example.net>
ssb   1024R/A1E03C73 2014-12-14

sec>  2048R/59BEDBFC 2014-12-14 [expires: 2015-12-09]
      Card serial no. = 0000 00000001
uid                  Yubikey Smartcard test1 <yubikeytest1@example.net>
ssb>  2048R/9B633300 2014-12-14
ssb>  2048R/A8DE1BB7 2014-12-14

3. Attempt to read the entities in the keyring. Only the first entity is returned.

$ go run readkeyring.go 
found 1 entities in keyring
reading entity with fingerprint 2FA49C2800342153CA439C90ADC366D34BACAA9F

source code:

package main

import (
    "code.google.com/p/go.crypto/openpgp"
    "encoding/hex"
    "fmt"
    "os"
    "strings"
)

func main() {
    secringFile, err := os.Open("/tmp/testgolangopenpgp/secring.gpg")
    if err != nil {
        panic(err)
    }
    defer secringFile.Close()
    keyring, err := openpgp.ReadKeyRing(secringFile)
    if err != nil {
        err = fmt.Errorf("Keyring access failed: '%v'", err)
        panic(err)
    }
    fmt.Printf("found %d entities in keyring\n", len(keyring))
    for _, entity := range keyring {
        fingerprint := strings.ToUpper(hex.EncodeToString(entity.PrimaryKey.Fingerprint[:]))
        fmt.Println("reading entity with fingerprint", fingerprint)
    }
}
@jvehent
Copy link
Author

@jvehent jvehent commented Dec 14, 2014

Ping @rsc & @agl who wrote the code according to git blame. Let me know if I can help test further!

@mikioh mikioh changed the title go.crypto/openpgp: silently fail to read keyring when smartcard stub is present openpgp: silently fail to read keyring when smartcard stub is present Jan 4, 2015
@jvehent
Copy link
Author

@jvehent jvehent commented Mar 9, 2015

Can I do anything to help with the resolution of this issue? It's blocking some of my users...

@agl agl self-assigned this Mar 9, 2015
@jvehent
Copy link
Author

@jvehent jvehent commented Mar 20, 2015

I took a peek at it, but this is my first dive into RFC4880 so some things are still unclear to me.
Looking at the yubikey stub in the secring, if I skip the first 272 bytes, I get directly to the S2K value, which is set to 0xff:

$ hexdump -s 272 -C 000015-005.secret_key
00000110  ff 00 65 00 47 4e 55 02  10 d2 76 00 01 24 01 02  |..e.GNU...v..$..|
00000120  00 00 00 00 00 00 01 00  00                       |.........|
00000129

0xff indicates an encrypted private key. In the code [1], (pk *PrivateKey) parse() tries to read the next byte to guess the encryption algorithm, but that next byte is set to 00 which doesn't map to any valid cipher.
Then, entering s2k.Parse(), the following 2 bytes are read (0x6500), and the second byte (0x00) is used in s2k.HashIdToHash(). But 0 is not a valid hash id, and the function returns error openpgp: unsupported feature: hash for S2K function: 0. The error is silenced is openpgp.ReadKeyRing [2].

I could do the analysis, but I'm afraid writing a patch is beyond my understand of the OpenPGP standard at this point...

[1] https://github.com/golang/crypto/blob/master/openpgp/packet/private_key.go#L67-L80
[2] https://github.com/golang/crypto/blob/master/openpgp/keys.go#L258

@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc removed the release-none label Apr 10, 2015
@rsc rsc changed the title openpgp: silently fail to read keyring when smartcard stub is present x/crypto/openpgp: silently fail to read keyring when smartcard stub is present Apr 14, 2015
@rsc rsc modified the milestones: Unreleased, Unplanned Apr 14, 2015
@rsc rsc removed the repo-crypto label Apr 14, 2015
@benburkert
Copy link
Contributor

@benburkert benburkert commented Dec 16, 2015

The hexdump from @jvehent is an example of a gnupg extension S2K type, which can be identified by the 65 (101 in decimal, rfc4480#section-3.7.1 denotes 100-110 as "Private/Experimental") followed by the 47 4e 55 (GNU in ASCII). The 02 byte after that signifies that the stub is for "gnu-divert-to-card" S2K (01 signifies a "gnu-dummy" S2K as in #13605).

The relavent gnupg code is here and here.

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Mar 29, 2021

Per the accepted #44226 proposal and due to lack of maintenance, the golang.org/x/crypto/openpgp package is now frozen and deprecated. No new changes will be accepted except for security fixes. The package will not be removed.

If this is a security issue, please email security@golang.org and we will assess it and provide a fix.

If you're looking for alternatives, consider the crypto/ed25519 package for simple signatures, golang.org/x/mod/sumdb/note for inline signatures, or filippo.io/age for encryption. You can read a summary of OpenPGP issues and alternatives here.

If you are required to interoperate with OpenPGP systems and need a maintained package, we suggest considering one of multiple community forks of golang.org/x/crypto/openpgp. We don't endorse any specific one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants