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.

Sign up for GitHub

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

Jump to bottom

crypto/rsa: mismatched keys no longer error #61077

Open
bazuker opened this issue Jun 29, 2023 · 5 comments
Open

crypto/rsa: mismatched keys no longer error #61077

bazuker opened this issue Jun 29, 2023 · 5 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@bazuker
Copy link

bazuker commented Jun 29, 2023 •
edited

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 seankhliao changed the title crypto/rsa Backwards compatibility issue with mismatched keys crypto/rsa: mismatched keys no longer error Jun 29, 2023
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 29, 2023
@seankhliao
Copy link
Member

cc @golang/security

@OffixialK

This comment was marked as spam.

Sign in to view
@Yawning
Copy link

Yawning commented Jun 30, 2023 •
edited

This one is easy to root-cause (5aa6313). The code has had bit of a refactor since the change that introduced the issue, I will refer to what is currently in tree.

go/src/crypto/rsa/rsa.go

Line 689 in 18e17e2

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

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.

go/src/crypto/rsa/rsa.go

Line 663 in 18e17e2

N = priv.Precomputed.n

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
Copy link
Contributor

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
Copy link

Yawning commented Jun 30, 2023

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"...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@FiloSottile @Yawning @bazuker @seankhliao @OffixialK