golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.88k stars 17.65k forks source link

x/crypto/openpgp: tag byte does not have MSB set but gpg decrypts it #29082

Closed fmpwizard closed 3 years ago

fmpwizard commented 5 years ago

What version of Go are you using (go version)?

$ go version
go version go1.11 linux/amd64

commit has for openpgp

eb0de9b17e854e9b1ccd9963efafc79862359959  (Nov 27th , latest as of today, Dec 1st 2018)

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/diego/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/diego/work/golang"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build336610440=/tmp/go-build -gno-record-gcc-switches"

What did you do?

We use this function to decrypt files we get from different companies, only files from one company fail, all others decrypt fine.

func PGPDecrypt(file io.Reader) ([]byte, error) {

    secretKeyring := "/path/to/gpg/secring.gpg"
    passphrase := 'pgpPassphrase' // of course not haardoded on prod :)

    // init some vars
    var entityList openpgp.EntityList

    // Open the private key file
    keyringFileBuffer, err := os.Open(secretKeyring)
    if err != nil {
        return nil, errors.New("os.Open(secretKeyring): " + err.Error())
    }
    defer keyringFileBuffer.Close()
    entityList, err = openpgp.ReadKeyRing(keyringFileBuffer)
    if err != nil {
        return nil, errors.New("openpgp.ReadKeyRing(keyringFileBuffer): " + err.Error())
    }

    // Get the passphrase and read the private key.
    // Have not touched the encrypted string yet
    passphraseByte := []byte(passphrase)
    // Support a user with multiple private keys
    for _, entity := range entityList {
        entity.PrivateKey.Decrypt(passphraseByte)
        for _, subkey := range entity.Subkeys {
            subkey.PrivateKey.Decrypt(passphraseByte)
        }
    }

    md, err := openpgp.ReadMessage(file, entityList, nil, nil)
        // =========== this is where I get the error:
        // tag byte does not have MSB set
    if err != nil {
        return nil, errors.New("openpgp.ReadMessage(file, entityList, nil, nil): " + err.Error())
    }
    ret, err := ioutil.ReadAll(md.UnverifiedBody)
    if err != nil {
        return nil, errors.New("ioutil.ReadAll(md.UnverifiedBody): " + err.Error())
    }

    return ret, nil
}

What did you expect to see?

No error and a decrypted zip file, which is what the command line gpg does

What did you see instead?

the error:

tag byte does not have MSB set

More details.

I brought this up on the mailing list https://groups.google.com/d/topic/golang-nuts/-bBXt-0nVT4/discussion

but I'll paste more details here:

Part of the verbose output from gpg when it decrypts it is:

gpg --decrypt-files -vvv xyz.zip.pgp
gpg: using character set `utf-8'
gpg: armor: BEGIN PGP MESSAGE
gpg: armor header: Version: EldoS PGPBlackbox (.NET edition)
:pubkey enc packet: version 3, algo 1, keyid AFF537579B90F252
    data: [4095 bits]
....
gpg: public key encrypted data: good DEK
:pubkey enc packet: version 3, algo 1, keyid <removed from paste>
    data: [4095 bits]
gpg: public key is <removed from paste>
:encrypted data packet:
    length: unknown
    mdc_method: 2

....
:literal data packet:
    mode b (62), created 1542720445, name="xyz.zip",
    raw data: unknown length
gpg: original file name='xyz.zip'
gpg: decryption okay
jmataa commented 5 years ago

On the Helm project: (https://github.com/helm/helm/issues/2843) I think they @technosophos found a potential cause. There is a new keyring format for gnupg 2.1: https://gnupg.org/faq/whats-new-in-2.1.html#keybox

jmataa commented 5 years ago

Following up here, I was getting this error due to using ReadKeyRing when I should have been using ReadArmoredKeyRing. The key format has changed though, so it may be that as well.

fmpwizard commented 5 years ago

I don't think @jmataa and I have the same issue, while the error is the same, from my sample code, the error you get would be at:

entityList, err = openpgp.ReadKeyRing(keyringFileBuffer)

but in my case it is at

d, err := openpgp.ReadMessage(file, entityList, nil, nil)

And note that using the same code and key I can decrypt many other files just fine, but this one vendor sends me these files that cause issues. But I'm still getting into the details of the pgp format so maybe I'm missing something

fmpwizard commented 5 years ago

@FiloSottile I found the problem!

a keyring can be in two formats, binary or armored, and we have two functions to read each, ReadKeyRing and ReadArmoredKeyRing

The pgp file we get from this one company, is also armored, where all the previous files from other companies we have been getting were in binary format.

But, we don't have a ReadArmoredMessage function.

Right now I create a bufio.Reader and Peek into the file to see if it starts with -- (could prob make it read more to see if I get a full -----BEGIN PGP MESSAGE-----) , and if I do, I call openpgp/armor.Decode and then call openpgp.ReadMessage

This works for our use case and maybe you would want to close this ticket, but, would you be ok accepting a CL that:

  1. Adds a ReadArmoredMessage and a bit of code comment to ReadMessage pointing out you may want to look at the *Armored* version? or
  2. Only add a bit of code comment to ReadMessage or some other better solution ?

Thanks.

arnisoph commented 4 years ago

It also took some hours (!) for me to figure out, I have to export the secret key to the old format..

https://gist.github.com/stuart-warren/93750a142d3de4e8fdd2

ArcticSnowman commented 3 years ago

Why not have ReadKeyRing handle both binary or armored key rings at the same time? Really should not be up to application developers to have to test the raw keyring.

alifarooq0 commented 3 years ago

Why not have ReadKeyRing handle both binary or armored key rings at the same time? Really should not be up to application developers to have to test the raw keyring.

+1

FiloSottile commented 3 years ago

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.