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

Validate improper TLS RSA certificates early-on #3496

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 100 additions & 0 deletions network/peer/8192RSA_test.pem
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
-----BEGIN -----
MIISRAIBADANBgkqhkiG9w0BAQEFAASCEi4wghIqAgEAAoIEAQDBND9cycCvR+iY
uKrG58jvz5VcU+V4ff9AQ4jUXAUCcUTtxz/82BCSBS0SJZLPJq3HHMHFZ8Qnjrfo
yMYACZ5wnZvpyUdOMVqAka6k1IYLkD/+sQYN92c6GZIOxNciBrox804jRTGl9Frg
5AX4cmmGBXyhJVm0waxl2PAqdayPJ/cf+CSWn3XyzkE/irBHFLFny9BnH5h111lO
4AMpl8yojVWPqPVuj/S5JJ7z9Mnut6nMHEumy184+C952NhGMmlYhX4OTW/S/Ylp
b+vURMk44/iMCJ2XanSD85lEZOJKIq05e9vNdcIn23F//e//oD4zMpAfiwWOkcNP
tva8OLpAZcREbqHn/2skTkl25hLOtUubChF/Zfj5xAnbcD18iyIug/KryyQsA0HK
e2xzdBOkj5axRzOTIgg8bwz+eEoGLM0kmSxd5TQXA130bP7zhAyZ2Bb4pJsPC03/
h8yc4lBhsagc1s2+tj/tdRk/fAwadwp2V1+hPI70dM13jt7xeNh6yyqNcP13Ky1Y
ajfYYsXSkSVOsovJmUyO7EqeHlP5bxUirljSG+RXqcbvdzvEaFTfM5iGMaHxn7IC
i8pJPUUCDseqsT0VFXD5UYc/G4rxSFPrecEQN14wHcMyUzO4r8CLjidBUJn6HQcQ
RiIP+uFSF70UAz5XvdmyqLeXS/ZhsMqLBH1qvsnd3cvGEStDjsujZf/u4VhOKe8f
SV9U2c7YNu22ShX1BnRzuZgDavnNr/Be+eyxwZ8z1XtGTqOMvL18kPMo9RGxaoKL
if+0yahFBGkY8TsmbB578Vxn0EYFx6neNw6H1HHpRS5osraG4iJTpBfgwVLyRLBC
aLB6mnsTie3bjUbIJCaFeOVPNrlyzNbuG22SJYCrOXIXzBNjh+GHNT//ubP/DWoz
wkVAHhooz315KMFiOyKfrlaoVa9IOFBEFJ2VmedA+j3VkJSfocBBGrdDGT0XijXX
ZCUSwn5EY7Ks5/gb3jGcpts028mTwkaLLRqL5e40ZF5FLmJHih7sbl6YyrUbg/Lv
ihe49CXO4DqWyCgq5+Zk1nq2eWalgsHpGchdMOPPkJHY/8HdAiBt/4trD12PstF3
qy8rmzTh8CH+RzWTcnb5A0bwgVcTZErEbKcE938pfsFgenHQmAs2ryRIMmMnu2ck
GlrJzowYVmWFBPIIoT5BsytWYLluErs9p8FjhHhTA1z9AE56cyu5qwz46i4TAysF
TJ+LITvQLeVt1lYooXNfUXsdxRbBqKXb2kzkACWNhtVeofLsqFIxRM4z9F7X9r0h
skXG+42WZRrLK/3hW5NtQ2BX3WvEjBBlGwR4n6IcCJeX6plL0exiYMUuPqwogJBv
o3h/gCD5AgMBAAECggQASQ1EWAVBAgWigPxyNjs10tceloZyYZjihp4Cgqk4i6/g
bDfGjgf0XAHxBMeINyNc2ciZy9ZsaLih+TbRBvqcGeC+LyuX9ozat3peGpzxAjZM
vDSbIXTGZ0V74HG1FnyMso5YoSVsnF9EbXxKdaJtG+u/L/87aAlC8k+Qn71WvdpS
qpfc3cb1hhVOvoPmGzpLyf9akWN09jmy3wv8piFrlN+71lIAWwm7crXSFFQedlCj
tzWLtUl4e8X7zYqcXA57nqj6/NVyzshmyKM0/FH187jfJbOsQrBR1gKplR7AIV/z
N6UJeypne0KSK98MfA9O9XTM4eBi/YFH5EA+EvUwF2FjUKy0M1B0ZonjZT2hJt+N
8tVfwFgCSA5D2+EYnprNFeF2RFbPGoUwvyrj2tOtCa/xPp65dYyMqK0ksKMy+hq+
hnQUPnyHsZvoTp9X1yO60ADQzrsOliWkHFZwm3FHC2ltM1pU+SNYEKUSItr4iJky
L4Th98k6FFyFxAsVaSBUWjmvoUNz0zdUMfYXn43ZVsDi5lrEWDnKpM/bduXowoup
5i8eDnPVZwAe5DSlOKJqVOrhZPwnS4EigavxlLfB/AEypevWOL6etOaKyOXVJ149
vO+QfF0zE+ZtA/5JtC9gEmRxm1Sqo9ON9C1Qe9JUmAG50HNZgzuZsN/yaxah1lWl
0It7akaxJgeEl747FIsi8Q7+/6hrhKapEFrJjH/1KNy2yZc9S6e7N+hQkAO5iN0G
B3R/bVe+DFqK0iXas4Eac9/mnfqDNbPkxFX7OZkAFqDhbj+4j8JsVTTmCiPXiZQc
FvzHsF3WL+Gp+/iui80530Wds9hQ/z7rwIDsMRX/+TxzhkIHTl3BVZ1IbvWcf6Nh
Ieyp7Umr9+oaJr5sHuWwh+aa5vkIz+2l+wcDE9gfkxclWSxKhumb+SYH404JdORs
ZPm86sBmF1ePcOQ+T6oNVTPnM8pDWyO4E0bYsv/qso/ZEd0tIQiwQOoTyJ1kLe5X
5TdXYmV3oby2bp7FM3Tb1AX2e7LFOboxMImU9z+X9+orXy6IEky7QfE3w2SGzgmv
JrZ18vtpAVgFefqqj3PnmJlc8q6YbU31wHwnyiSZDSe0TEPr4hOPUOe/Qn7KJVy6
fs+K6PWdcIDNZx9jgnTWPhkWYM0pGznAAy+V8PlCfQMc57I47HhnFzR4YGWWfox0
K3TI7yUJJpa17TSTKUwsuFNTTahxC3439u1wj6xBdDf+vR0tq6svCmzBcJ15vS4r
yrm8J00OtPnlA1zoYlfWwDvNIFvQ5xgxNEcpnD2jUoe08FaYcZ6Xxz+92OhGNYPm
67d7P55mST60fa8fNWM82BW+tV5oo5+1n/uphjxQcQKCAgEA8EYIqh+ij5bL1+af
zf8BgB/ovdwKOma6MzHop1+fW+wRr92u7mYhkaI1bA+/X9FrC844Lb7oJ0w8E3gg
WMyw9DGHEqk+/FCiEfb/U8D9h0/zV8YTPlV1/sZmmwQgItoiaIZP6LOiNTevhjL6
JNDM+Uh8Wd15AT9RA2vdPWdWR0DvEbRXPx2RkFpHoBDiI6+CuJ+vgpBbSSU7PTZ5
+C8PTuFMh0V7fp16rufs8HNPt/CE/5z4KhVIArfgh6yF8VzW7Fz+RHL6Ao3n5khj
0/14nblKBEfeNpDcCMDRKPYxPi2iTvvMmk8ZZg1XsbdJQZbpUBfp66kDRlGuamlG
92Nqjl2+fPOJhmKQYls1oFL1Gp0RIQeYmJvVRwQlazo2a94QJN5A8MkhxXjLjHGz
IKKrwbJ4sRAS0GPs3hruRggDGoJpXMKWXa98cUp9KOYVvBnNoQLA6V8WDfqRCV58
BY/ogpPGeJcoDE6h//SPL4vp2r2rGy9GGPnCbNZGfYuCOHlJvwcddsuM86ApSxOs
rsGMHMz85xtINlhIQgTyr13+EcZEkfB0j9Yv1mdlpTs5tuDDu09TGUEXAFUiD2Rs
WepKpT7rX+KoIuMjMwuUaLdgXP6OPvsPHqLc3oFKkMY4H0yHOq2bDse80jrEg++U
JbhKDABA0xUSmSCHxGNMTvD4x28CggIBAM3Zh2AwWiLsHCHqPY5/NWenLENeWDhs
Omirl5OkTRGYxrzvELDDnMmyh0QfbdkUn6aoXird8sPGHvjYIgKVxiK/q8Hbz1x+
a9lqfLV2DBSQZGq3OvpiLubPcHCnJm3+MXL7foer49HatAK7qimcq8SKEMGXx2Fy
pzIHtoBm7JXZqrhr6o6K6GCpPm57YrkqEAz6bu+oCgHzmm4QN5aVcFr3/JxSZbdV
hccRBP87BkzyQrfgDqra2oxvKNV1KL7ByhdQxWq76/YkCI+U59U2rJM6UhRNPizG
/VGrhAahSuZkFkWhwKvg9QxBjixljRwOrFsJW5lmkzX26hnsyv0czE4O+nxtP4Ug
BMQ/QdrwQmjMNp95rYXhbCeirjuc3O3G58uHAgjx+UIopE4SNmRgqQb0tXM2DEN9
8Ppx1aklcyBt+17qh2ToTkZuW6KK39eYXZi8ZtYl0C908vJ7RUQGlL+wwlvVswCv
KuQOXWl4Kv1oVKf8k/0lqtZh7LQwTGUPspndGyBxuN5f9UFvOxLpy5k6yA/J7Idp
k/0nzQei++UltQ//LiyMSb/JpA7GAEE68HoOhDzsablNoLaiIPrUfo7SLFUw7n/H
m2CiSF21KBEPuq5waXpe8aeYtbCcXjCZI34Swkt892mws01XY4AiJ5xUizv+gcJD
hADngf/XcioXAoICAQDH89BEG12CFyD+RCubF2sdP/DFB3fvkAvGjPMrTpVkvvkd
HOP1+0JWWuIQUq6VQ8bMpUn1L9ks0vFv1lk87OMZ5Jmeuv/yo/ur7ZwgDAwwbiV5
VxoulppCcsNyn6VKu7NEvvmDEvKbTQMiMAwhVS4vCdaKRpfrpNB7g2kzL2sKkwwg
9K5ilO3NboQKveIjhmzHzgQWKKH/Jh+9Wjd4hVk88JtqOzWBcfZl1hZFKAEgduWH
fw66nsk1keYlojo5WWR2gREMz44lUAi7iGSjR134C/l/xHs1d6nVEvk9GFx0fS+E
gWGMzOS7G8Ft4LTzA26YO75sYlOaUmFOptvrBm3nmjXq8BTzo9S6NWNUT5UwF6Po
k9S2s4BywA2PxXsCm2Nd+yOZ/he/qT3jW7+RGi7LXAW6fEDb8TxuvYSq/QHwLrUV
/814m5B5C19LCObviZ2pL4xw6bOF4I6QeHPHgTIicG4LbudiDpIcWl5KWCo94fei
AN5Z7IeTYWJ6Gf49lxn7AiXP9acQG6ohk3byW5mJYkHY5chbiW5gmpOHwzWrfw8T
UEMAbGOVDqj1L2thOH1KxMHH03Ybzb0xiAXvcd261Li2K/52QgXJ9goEdw6XdTPV
T8MOYMRj2r696mdMDLjA6TaPv0LwxP1DOr5UAaCFijRoNTIsAnlZwrT/QOQXuwKC
AgEAxqtiF3izFa9Q+36KSIQHc/GJK7/bXyE9QhYR5aGV7BzJ+kC0mBVCtfuCx0Ga
D//ykbM/pxmsmjwVWk+mi14n6xOX3jKaMAenaR94Gt5CjHpLIB+VYV/vKj4co+z+
jvvcl7+X/7Lq3ne4ckbS1PRrZvVldKJbAHbaXNPK1KQBRCLevL0SlN4FpnzRT2nv
/wtUkGIHPW+tsPJ+IimurLuvw2xBtlFj8AwvX8/SRc6epxbNQ4+QOF+evBjwjQtU
9r4roFMJJZkXA+kFBiZNlZ798d5Ap21hS3AFvpPNiWST2EXSpQOW44vqlRiT8c9U
4DZdLEOczzGLdHLIv5qk0qK/n7qfEAWUX5RmZU0z7u0g+unU8hdKXMMSUjKU+93J
8AafYfP8B8wZqDt3UA4NxtTvbVIx6W7JaT4cnGnPLz+AnFTpXVL2t3HpUdpiwD5O
CVL5SlbS3W2DProdW9+TGzNKzrL28hEOgOOOfqpKh2c9/nJ5+eMwpQp8lgnOnJ1c
rdD3q74U1zxKkvyDxNJobjmMkWeE/JACozJHbPXD0NIBUMgStsyusLn414vxtXxt
dIdA3lwyTmZRJ1F/gaR6Nftt5cN8m//svxBTqnEVbLNRZx4KKx88/aiyi/E7sadI
1JiIA75xHNAQLUYn1sY3tsu/9QY3lwBsFaR5uzG0aspxWaMCggIBAM10S3/g8Zbi
DGX70XlgNWBQMtlKhQWK64I3VdP79pJ8LZu2mPnOmwV6oa+MAW2pKWJ96ZuARj1k
jOSzi5022qfBQOx2zD1jDOWZa2FMz7gyJWiLKSBG3b0UVWa+2OxxIkIK2pEWYURX
qvbVy/6Kh34xupzHYAvyYraac2NKIpZHmxVxYZjazm7Ot6bn7oXG09D11oXHq4/r
u9hGCkOVD9pKu7it/8IQMyNNCm8Sw8ShYLrA6PYtGOqV6ByuUp7EgcJzbPNGHlXX
p7fwIsxInWhZXqFz8ARji+za4G65vr+tjQzBMGL6V/QWNzM8CRgXQJwjr50bk79t
U64t/I/bHTnQiEUMqkdE4ly1A3QxttUaCN6s63Rj+Pfy7cJZZOehSFn0YAH16BYH
NUrjQxhKy3U5UdWGxp9V8wZ0YW9ThGJM8g/n3PZjhQK/LilwUkDk4eGwMspyh09z
3Azsfvl+nsiNBu3ft3dLggX729cWKkGg1Kdv7OTcmxLTipkYESXH708SsGlJhqSq
YW9sl2Ankve2apwrLTLQUE06o4KF5F9KP2MC3uXiwYAmoHuyYMnZ9fcfH2P1zbQL
czHlpnSg1/TDbJfAU9E9LauPuha63KrqFaPNOpjDFNyotPxbe+Oy35B0UX5ZVamN
Jk2UczPy2rNK8CZ6iIuChYZ5f3gujYHb
-----END -----
34 changes: 34 additions & 0 deletions network/peer/tls_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,18 @@
package peer

import (
"crypto/rsa"
"crypto/tls"
"errors"
"io"

"github.com/ava-labs/avalanchego/staking"
)

var (
ErrNoCertsSent = errors.New("no certificates sent by peer")
ErrEmptyCert = errors.New("certificate sent by peer is empty")
ErrEmptyPublicKey = errors.New("no public key sent by peer")
)

// TLSConfig returns the TLS config that will allow secure connections to other
Expand All @@ -26,5 +36,29 @@ func TLSConfig(cert tls.Certificate, keyLogWriter io.Writer) *tls.Config {
InsecureSkipVerify: true, //#nosec G402
MinVersion: tls.VersionTLS13,
KeyLogWriter: keyLogWriter,
VerifyConnection: ValidateRSACertificate,
}
}

// ValidateRSACertificate validates TLS certificates
// with RSA public keys in the leaf of the certificate chain of the given connection state.
func ValidateRSACertificate(cs tls.ConnectionState) error {
if len(cs.PeerCertificates) == 0 {
return ErrNoCertsSent
}

if cs.PeerCertificates[0] == nil {
return ErrEmptyCert
}

pk := cs.PeerCertificates[0].PublicKey

switch rsaKey := pk.(type) {
case *rsa.PublicKey:
return staking.ValidateRSAPublicKeyIsWellFormed(rsaKey)
Comment on lines +56 to +58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely me just being overly defensive, but should we attempt to handle the case that rsaKey is nil here?

Test would look like:

{
	description: "nil RSA key",
	input: func() tls.ConnectionState {
		key, err := rsa.GenerateKey(rand.Reader, 2048)
		require.NoError(t, err)

		x509CertWithNilPK := makeRSACertAndKey(t, key)
		x509CertWithNilPK.cert.PublicKey = (*rsa.PublicKey)(nil)
		return tls.ConnectionState{PeerCertificates: []*x509.Certificate{&x509CertWithNilPK.cert}}
	},
	expectedErr: peer.ErrEmptyPublicKey,
},

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test + check in ValidateRSAPublicKeyIsWellFormed.

case nil:
return ErrEmptyPublicKey
default:
return nil
}
}
94 changes: 94 additions & 0 deletions network/peer/tls_config_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
// Copyright (C) 2019-2024, Ava Labs, Inc. All rights reserved.
// See the file LICENSE for licensing terms.

package peer_test
StephenButtolph marked this conversation as resolved.
Show resolved Hide resolved

import (
"crypto/ecdsa"
"crypto/elliptic"
"crypto/rand"
"crypto/rsa"
"crypto/tls"
"crypto/x509"
"testing"

"github.com/stretchr/testify/require"

"github.com/ava-labs/avalanchego/network/peer"
"github.com/ava-labs/avalanchego/staking"
)

func TestValidateRSACertificate(t *testing.T) {
for _, testCase := range []struct {
marun marked this conversation as resolved.
Show resolved Hide resolved
description string
input func(t *testing.T) tls.ConnectionState
expectedErr error
}{
{
description: "Valid TLS cert",
input: func(t *testing.T) tls.ConnectionState {
key, err := rsa.GenerateKey(rand.Reader, 2048)
require.NoError(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these functions are referencing the wrong t. These would call operations on the TestValidateRSACertificate t rather than the inner t.Run(...) t.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes you're right. I moved them because of code review comments and didn't notice it.

x509Cert := makeRSACertAndKey(t, key)
return tls.ConnectionState{PeerCertificates: []*x509.Certificate{&x509Cert.cert}}
},
},
{
description: "No TLS certs given",
input: func(*testing.T) tls.ConnectionState {
return tls.ConnectionState{}
},
expectedErr: peer.ErrNoCertsSent,
},
{
description: "Empty certificate given by peer",
input: func(*testing.T) tls.ConnectionState {
return tls.ConnectionState{PeerCertificates: []*x509.Certificate{nil}}
},
expectedErr: peer.ErrEmptyCert,
},
{
description: "nil RSA key",
input: func(t *testing.T) tls.ConnectionState {
key, err := rsa.GenerateKey(rand.Reader, 2048)
require.NoError(t, err)

x509CertWithNilPK := makeRSACertAndKey(t, key)
x509CertWithNilPK.cert.PublicKey = (*rsa.PublicKey)(nil)
return tls.ConnectionState{PeerCertificates: []*x509.Certificate{&x509CertWithNilPK.cert}}
},
expectedErr: staking.ErrInvalidRSAPublicKey,
},
{
description: "No public key in the cert given",
input: func(t *testing.T) tls.ConnectionState {
key, err := rsa.GenerateKey(rand.Reader, 2048)
require.NoError(t, err)

x509CertWithNoPK := makeRSACertAndKey(t, key)
x509CertWithNoPK.cert.PublicKey = nil
return tls.ConnectionState{PeerCertificates: []*x509.Certificate{&x509CertWithNoPK.cert}}
},
expectedErr: peer.ErrEmptyPublicKey,
},
{
description: "EC cert",
input: func(t *testing.T) tls.ConnectionState {
ecKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
require.NoError(t, err)

basicCert := basicCert()
certBytes, err := x509.CreateCertificate(rand.Reader, basicCert, basicCert, &ecKey.PublicKey, ecKey)
require.NoError(t, err)

ecCert, err := x509.ParseCertificate(certBytes)
require.NoError(t, err)
return tls.ConnectionState{PeerCertificates: []*x509.Certificate{ecCert}}
},
},
} {
t.Run(testCase.description, func(t *testing.T) {
require.Equal(t, testCase.expectedErr, peer.ValidateRSACertificate(testCase.input(t)))
})
}
}
Loading