crypto/rsa: mismatched keys no longer error

bazuker commented 1 year ago

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

$ go version
go version go1.20.5 darwin/arm64

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
GO111MODULE="on"
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/bazuker/Library/Caches/go-build"
GOENV="/Users/bazuker/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/bazuker/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/bazuker/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.20.5"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/f5/66bkpbwx1xv9bvgppprg46000000gn/T/go-build1842695220=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

rsa.SignPKCS1v15 does not return an error in go1.20.5 when public key and private key are mismatched.

The test below passes in go.1.19.7 because the error is returned as expected. In go1.20.5 no error is returned and hence the test fails.

package main

import (
    "crypto"
    "crypto/rand"
    "crypto/rsa"
    "crypto/x509"
    "encoding/pem"
    "testing"

    "github.com/stretchr/testify/require"
)

// TestPKSignError demonstrates that an error will be returned if public key and private keys are mismatched.
func TestPKSignError(t *testing.T) {
    var err error
    // Parse the private key.
    privatePem, _ := pem.Decode([]byte(RSAPrivateKey))
    require.NotNil(t, privatePem)

    var parsedKey interface{}
    parsedKey, err = x509.ParsePKCS1PrivateKey(privatePem.Bytes)
    require.NoError(t, err)

    pk, ok := parsedKey.(*rsa.PrivateKey)
    require.True(t, ok)

    // Parse the public key.
    pubPem, _ := pem.Decode([]byte(RSAPublicKey))
    require.NotNil(t, pubPem)
    parsedKey, err = x509.ParsePKCS1PublicKey(pubPem.Bytes)
    require.NoError(t, err)

    var pubKey *rsa.PublicKey
    pubKey, ok = parsedKey.(*rsa.PublicKey)
    require.True(t, ok)

    // Assigned the mismatched public key to the private key.
    pk.PublicKey = *pubKey

    // Initialize hash and write the payload.
    hash := crypto.SHA256
    hasher := hash.New()

    payload := "stuff"
    _, _ = hasher.Write([]byte(payload))
    hashed := hasher.Sum(nil)

    // Try to sign using our private key.
    _, err = rsa.SignPKCS1v15(rand.Reader, pk, hash, hashed)
    // Returns "rsa: internal error" as expected in Go 1.19.7
    // Returns no error in Go 1.20.5
    require.Error(t, err)
}

const (
    // These keys are mismatched.
    RSAPublicKey = `-----BEGIN RSA PUBLIC KEY-----
MIIBCgKCAQEAv8iKQm3R7GguNGuXiyHCEz1VnvSWiIs+pphyOUiOac4lcerE9EnS
PG5Cq8vt/oYYHoT4/H2BqpDjtLjzND7U1iombdnufezjtOe42AgAyrGRNJUKe8uT
h+fEOIEeqbBz1nJ73DVL5QFlpf7OHIZfOFsq0R1elB5vvsLyF0Z8Z+/lrMlKtIK/
Zjd4xB1Rpg1MuijAaJet93JbyDlXZ6eVnKbEaJ0qetZJ95lpiX7hp2kpHCR7ch/K
UAO8tGk0qiwZ/M6r2HmX1SB8JXnAEaoor+mofdtx9lzNJZraQgRXTtBHMiaTQM1S
VzsokC6WE8GpzxK8Xw/g9Df/Yk9gm4wZwwIDAQAB
-----END RSA PUBLIC KEY-----`
    RSAPrivateKey = `-----BEGIN RSA PRIVATE KEY-----
MIIEpAIBAAKCAQEAsHGAauw+xcT8+2E4h9jyADw4il1gaFtQtjbfkJUtRoAFvOuo
Cy/aOY1j6PQ7GnibIhPa1/KSW2CYazopZ6Oj1zHjyy1YDFdSYB6JEufX+64HXsup
rtJvwPJvQN7GqBWcoHsXxgVAOrsnZ/IeiYOkZ5ornOl2uV/J/CzobIDKyuWr3Dsf
HWm1Wm1o36moXm+/SFy5Qvy5KwrWNdxi2Jn9M3OCPatLfjKUf9bWk2UJQLvMKZ7N
c22/yydB60v9vODTOBfwkSaR0lZsbLJQNYeaUd9GbrEY1I1lEsSoyELNdTXq0reb
j4XyWEmNndA1S//r2aJAqiToYAXmGVI2NOzs1wIDAQABAoIBAQCNpgEnVZOrZ9KE
6O7eOG9HexEVG5ObE7v/HJxsUSZw07RHj5RvrrHtxDGyQef0/1/jgrcM6GNQ0oJq
it3Uow2UZCvw3+0wsyWhnsICmHfPSE4Ib05A2BX5e2ePV4l6RSdhupBCW9LNk5Q2
Ia0cTr+8oItkxBSZC2p3dYH+IYMsUNIUvgbwd1J4gPJKCeSV+NH8wymQJXgvZqcV
QHtWm+x02dM6kMA64vNj7BYs6bTBVSedQzd9+oHbioVP6pAjLhh7WI44fGHTn/BE
qRgmYMA4xNPJg+dzDgYQVLj3pJqTSyjnGtb3vsZoSmJPDATOb4o3NkQ8KRM/DUac
Fgi4AhjZAoGBAMvEc0ax8oVJZgJEyvfAnoToE9TTDPVMWE2mZWaGDqlNxx5ODo8E
5LGNhy1zuvOtiRNIPhn2hs3rR+iKnd8jmn2dSS21HJkRaAhct+0MVdvqweTchh2k
bD3CFc3YEAGebWbg/PC6weoA6h4Oix9U3SW9QAl1eH9emXnEAyt9sT0jAoGBAN2s
ASVH+AmBrY+qtPfcaGzwFJ+KxBf79Ku4dehN72VQCp6V37k67MPz2K8C3qF6kXy2
uIdNJmUDAYbSQt5+gJ9aLRhOYp+g1U3CidWhJjqCl1trRdmQ68GxY+R85wf11mpy
VNKXRikckRZnK34q2NTNOw3ekXd6/u09KrNlN669AoGAXyEDwElrM5akrQJ4z1l5
qArA12cAcbSGtRmt1UNYrOnGv/spCNP8AHhWV33kFcc6a2oas/xHyvLAy2uLcJUq
luJLO6+F/mAF9YFzzJMpslXS14msg0Iz1lE55LOuJVNVN+Zpr+lAhoKOyiF4CdSQ
ugG0V7Yj3zLG6/X6lN9FU4kCgYEAh+QaD7C+7ZUBwUD1D72ehqnm+qcm700WAO9j
2LVuPL2ExRM7w2HMI5QpEaDAul1ZMwsQtGEnWGUvWmcrdxo133p4ip4C97ixCqpn
tP7FYLkN8I0ilO2ymVsV0cyAFPEwMLFGLpNt/2Xzy7gTgZTiuBHYUfhPVN+hx+3n
b3JtYEECgYAslLANqX5ZlVPlMlYX/cfLcT1546zvX+rcvmtjau2V2tevoPYdRooZ
/Z5bWfq4yv6QyTDyJYbF9XbD/X9p3cH76aoa3pLhubvcO3Re9S6K4rH4A75gDJyP
+DRAQU2lssNTQUFW26HnUc2rOIH+JfS1OVLHkoL+7o69YzAl9MNJPg==
-----END RSA PRIVATE KEY-----`
)

What did you expect to see?

I expect this test to pass in go.1.20.5.

What did you see instead?

The test fails in go1.20.5.

seankhliao commented 1 year ago

cc @golang/security

OffixialK commented 1 year ago

go.1.20.5@

Yawning commented 1 year ago

This one is easy to root-cause (https://github.com/golang/go/commit/5aa6313e587d7fa7caba9a76d23dd4d246cb6d1f). The code has had bit of a refactor since the change that introduced the issue, I will refer to what is currently in tree.

https://github.com/golang/go/blob/18e17e2cb12837ea2c8582ecdb0cc780f49a1aac/src/crypto/rsa/rsa.go#L689

This used to call check := encrypt(&priv.PublicKey, m). While the new operation is the equivalent RSA encryption, N is now pulled from the PrivateKey if precomputed values are present.

https://github.com/golang/go/blob/18e17e2cb12837ea2c8582ecdb0cc780f49a1aac/src/crypto/rsa/rsa.go#L663

The fix to restore previous behavior is to unconditionally de-serialize priv.N in the check branch, for example:

        enn, err := bigmod.NewModulusFromBig(priv.N)
        if err != nil {
            return nil, ErrDecryption
        }

        c1 := bigmod.NewNat().ExpShort(m, uint(priv.E), enn)

Edit: One could conceivably also splatter checks all over the place where the precomputed values are used to try to detect "user did something unwise and there is now a mismatch", but I'm not sure how much that is worth it.

FiloSottile commented 1 year ago

The check is there to ensure that if we do the CRT wrong due to a bug or a fault, we don't leak the private key. It's actually safer and more correct to do it against what we used to produce the signature. The consistency check was a side effect, and I'm not sure it was an intentional one. (For sure there was no test or dedicated error message.)

NewModulusFromBig is a slow operation, adding it to every signature would be significant.

We can probably find a quicker way to check the PublicKey is not mismatched but 1) how can that happen? and 2) how bad is it? PrivateKeys can be corrupted in a number of ways we don't catch, the assumption is always that the private key is trusted (for better or worse).

Yawning commented 1 year ago

The check is there to ensure that if we do the CRT wrong due to a bug or a fault, we don't leak the private key. It's actually safer and more correct to do it against what we used to produce the signature. The consistency check was a side effect, and I'm not sure it was an intentional one. (For sure there was no test or dedicated error message.)

Makes sense.

NewModulusFromBig is a slow operation, adding it to every signature would be significant.

We can probably find a quicker way to check the PublicKey is not mismatched but 1) how can that happen? and 2) how bad is it? PrivateKeys can be corrupted in a number of ways we don't catch, the assumption is always that the private key is trusted (for better or worse).

This sort of thing is why I tend to be a proponent of "make key types as opaque as I can get away with", but due to backward compatibility concerns that ship has long sailed here. FWIW, I personally also would not be overly concerned with this case, because this feels like something that lands firmly in "So don't do that then" territory.

PrivateKey.Validate correctly flags this inconsistency, so "surely if the user messes with the private key, they will call the dedicated validation routine"...

golang / go