x/crypto/ssh: doesn't always return x509.IncorrectPasswordError when it should

[sqweek]

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

$ go version
go version go1.21.0 linux/amd64

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'
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOOS='linux'
GOAMD64='v1'

What did you do?

I'm working with SSH private keys encrypted by passphrases, and thus using ssh.ParseRawPrivateKeyWithPassphrase during the decryption process. In some cases when an incorrect passphrase is provided, the function does not honour its interface.

What did you expect to see?

The documentation for the function says:

If the passphrase is wrong, it will return x509.IncorrectPasswordError.

What did you see instead?

With one specific passphrase/key combination, I instead saw this error:

asn1: structure error: length too large

This is a key I use every day with normal SSH tools so I know it is valid. Also if I provide the correct passphrase to the function then everything works, and other incorrect passhprases do result in an x509.IncorrectPasswordError as expected -- it's quite an obscure failure.

Unfortunately I cannot provide the key itself to reproduce, for obvious reasons. I spent some time tracing the decryption code path but was unable to get clarity myself. It may relate to the slightly antiquated/odd structure of my SSH key, which I am happy to share:

-----BEGIN RSA PRIVATE KEY-----
Proc-Type: 4,ENCRYPTED
DEK-Info: AES-128-CBC,«32 bytes redacted»

«24 lines * 64 bytes redacted»
-----END RSA PRIVATE KEY-----

No idea how I generated this one, it was a long time ago. The incorrect passphrase which generated the asn1 structure error was ooooo.

[cagedmantis]

cc @FiloSottile @rolandshoemaker @golang/security

[drakkan]

@sqweek I would like to take a look, but I need a reproducer. Can you please share more details about your key and/or create a new similar key that allow us to investigate this issue? I have tested the keys in our testdata package and I cannot replicate the reported issue. Thank you!

[sqweek]

IIRC this key started its life unencrypted and I added the passphrase later which may be related, but it also might rely on software versions which are many years old. I know that's not much but that's all the details I have sorry.

I do have time to experiment with code if you can offer any suggestions around what circumstance might trigger a "length too large" false positive.

[drakkan]

I think the error is generated here. ParsePKCS1PrivateKey calls asn1.Unmarshal which may return this error here. Can you please confirm?

[sqweek]

Yep I found a go debugger (delve) and your suspicion is correct. Specifically the second byte in the slice passed into asn1.parseTagAndLength has value 203 which means we fall into this branch:

                // Bottom 7 bits give the number of length bytes to follow.
                numBytes := int(b & 0x7f)

203 & 0x7f => 75 which means the code tries to interpret the next 75 bytes as the length of the coming field -- no surprises that "length too large" is the result :)

I don't think asn1.Unmarshal is at fault though; since I haven't provided the correct decryption key there's no way the input data can be correct so it seems like a case of garbage in garbage out. The big question is why has it been passed garbage...

If I trace the execution when using a different incorrect passphrase that does produce an x509.IncorrectPasswordError, it seems that ssh.ParseRawPrivateKeyWithPassphrase relies on x509.DecryptPEMBlock to produce this error code. Specifically x509.DecryptPEMBlock has several checks towards the end of the function which produce IncorrectPasswordError when an invalid padding is detected.

In the case where IncorrectPasswordError is returned, I see that the last byte of the "decrypted" data is a 0, which triggers the last == 0 check. In the "length too large" scenario, the last byte of the "decrypted" data is a 1, and the second-last byte is 152. According to the implementation of x509.DecryptPEMBlock, a single trailing 1 represents a valid padding for the block and no error is raised. The function comment for x509.DecryptPEMBlock does flag this scenario:

// ... Because
// of deficiencies in the format, it's not always possible to detect an
// incorrect password. In these cases no error will be returned but the
// decrypted DER bytes will be random noise.

And also:

// Deprecated: Legacy PEM encryption as specified in RFC 1423 is insecure by
// design. Since it does not authenticate the ciphertext, it is vulnerable to
// padding oracle attacks that can let an attacker recover the plaintext.

I would be very surprised if SSH keys do not have a mechanism to authenticate the ciphertext, so this suggests that the fault really lies with ssh.ParseRawPrivateKeyWithPassphrase for not doing any verification here and passing the buck entirely to x509.DecryptPEMBlock which [apparently] does not have the ability to perform a robust check.

Based on the mechanism here I suspect this can be reproduced with any key, simply by trying enough passphrases. I'll have a quick go at building a reproducer against a junk key using this hypothesis.

[sqweek]

Yep. Here's a key which can be used to reproduce: junk_key.txt

The correct key for this passphrase is abcde. A simple loop over passphrases derived from integers quickly reveals many failures:

package main

import (
        "golang.org/x/crypto/ssh"
        "crypto/x509"
        "os"
        "fmt"
)

func main() {
        pem, _ := os.ReadFile("junk_key.txt")
        for i := 0; i < 4096; i++ {
                _, err := ssh.ParseRawPrivateKeyWithPassphrase(pem, []byte(string(i)))
                if err != x509.IncorrectPasswordError {
                        fmt.Println(i, err)
                }
        }
}

463 asn1: structure error: length too large
726 asn1: structure error: length too large
1259 asn1: structure error: tags don't match (16 vs {class:2 tag:13 length:77 isCompound:false}) {optional:false explicit:false application:false private:false defaultValue:<nil> tag:<nil> stringType:0 timeType:0 set:false omitEmpty:false} pkcs1PrivateKey @2
1311 asn1: structure error: tags don't match (16 vs {class:1 tag:8 length:53 isCompound:true}) {optional:false explicit:false application:false private:false defaultValue:<nil> tag:<nil> stringType:0 timeType:0 set:false omitEmpty:false} pkcs1PrivateKey @2
1404 asn1: structure error: length too large
... «14 lines snipped»

[gopherbot]

Change https://go.dev/cl/538835 mentions this issue: ssh: try harder to detect incorrect passwords for legacy PEM encryption

[drakkan]

@sqweek do you have time to test the linked CL? Thank you!