Validate improper TLS RSA certificates early-on #3496

yacovm · 2024-10-24T19:51:24Z

Why this should be merged

This commit makes validation of TLS certificates with either too big RSA keys, or the wrong exponent, fail as soon as the remote node presents its TLS certificate, in contrast to after the TLS handshake.

How this works

Activates the VerifyConnection in the tls.Config, which makes the TLS handshake code in the golang TLS stack invoke the function on the TLS certificate chain presented by the remote node.

How this was tested

Added a unit test to added code.

Added unit tests for existing code, because why not?

Need to be documented in RELEASES.md?

No.

network/peer/tls_config.go

marun · 2024-10-25T01:01:31Z

network/peer/tls_config_test.go

+			expectedErr: errors.New("no certificates sent by peer"),
+		},
+		{
+			description: "No TLS certs given",


Description is the same as the previous test case.

network/peer/tls_config_test.go

staking/parse.go

network/peer/upgrader_test.go

marun

Were this change deployed, would it be possible for the rsa certs of existing validators to fail this new validation? If so, what would be the impact?

yacovm · 2024-10-25T13:09:41Z

Were this change deployed, would it be possible for the rsa certs of existing validators to fail this new validation? If so, what would be the impact?

No, because all I did was moving the code that is called after the TLS handshake, during the TLS handshake when we parse the TLS key of the client but before we use that TLS key.

tsachiherman · 2024-10-25T13:45:33Z

network/peer/tls_config_test.go

+			input: func() tls.ConnectionState {
+				return tls.ConnectionState{}
+			},
+			expectedErr: errors.New("no certificates sent by peer"),


any particular reason you don't want to use the error object you've already been allocating ? ( ErrNoCertsSent )

ha, I rushed and forgot that, thanks

tsachiherman · 2024-10-25T13:47:41Z

network/peer/upgrader_test.go

+	"github.com/ava-labs/avalanchego/staking"
+)
+
+// 8192RSA.pem is used here because it's too expensive


I don't have a strong feeling here, but maybe consider naming the file 8192RSA_test.pem so it would be clear that it's being used for testing purposes only ?

I would say by putting a key in a file, implicitly it shouldn't be used in anything but tests ;-)

I changed accordingly though

tsachiherman · 2024-10-25T13:51:50Z

network/peer/upgrader_test.go

+			go func() {
+				defer wg.Done()
+				conn, err := listener.Accept()
+				require.NoError(t, err)


I suspect that this would generate a race error in case an error is returned, since the go testing framework wouldn't allow you failing from an inner go-routine.
to "solve" this issue, I'd suggest using an error channel instead of the waitgroup and test it's result on the main go-routine.

Why? Due to the waitgroup, all writes to memory the goroutine has made, would be synchronized by the main goroutine. I tested it with data race detector and no data races were observed in case of an error failure.

I don't think this causes a race condition, but the way that t.FailNow() works is by calling panic - which is normally caught by the testing harness managed when running a Test or in t.Run(...) but here that panic will escape the goroutine and cause the test to crash.

I doubt this is the only place in avalanchego where this could happen... so 🤷 on if we should fix it.

network/peer/upgrader_test.go

staking/parse_test.go

StephenButtolph

Should we consider fully validating the peer certificate prior to the handshake rather than just RSA keys?

We currently require:

TLS certifications are no larger than 2048 bytes.
A public key is provided
RSA modulus is eitther 2048 or 4096 bits
RSA modulus is not even
RSA exponent is 65537
ECDSA keys are on the P-256 curve.
Ed25519 keys are disallowed (will be allowed later)
DSA keys are disallowed

This PR introduces checks 2-5, but not 1, 6-8.

Essentially should we be trying to guarantee that x509 parsing -> custom verification can never produce a cert that isn't parsable by custom parsing?

StephenButtolph · 2024-10-27T19:15:13Z

network/peer/tls_config.go

+	switch rsaKey := pk.(type) {
+	case *rsa.PublicKey:
+		return staking.ValidateRSAPublicKeyIsWellFormed(rsaKey)


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, },

Added test + check in ValidateRSAPublicKeyIsWellFormed.

StephenButtolph · 2024-10-27T19:16:54Z

network/peer/tls_config_test.go

+			description: "Valid TLS cert",
+			input: func() tls.ConnectionState {
+				key, err := rsa.GenerateKey(rand.Reader, 2048)
+				require.NoError(t, err)


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

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

network/peer/tls_config_test.go

StephenButtolph · 2024-10-27T19:35:11Z

network/peer/upgrader_test.go

+			conn, err := tls.Dial("tcp", listener.Addr().String(), &clientConfig)
+			require.NoError(t, err)
+
+			require.NoError(t, conn.Handshake())


I'm a bit surprised that this passes, is this just because the client signature verification is performed last?

In TLS1.3 we first negotiate the encryption, and do the authentication last. The client is the last party to send its authentication, and the implementation doesn't wait for the acknowledgement from the server, i guess for performance reasons.

yacovm · 2024-10-27T19:59:51Z

Should we consider fully validating the peer certificate prior to the handshake rather than just RSA keys?

We currently require:

TLS certifications are no larger than 2048 bytes.

A public key is provided

RSA modulus is eitther 2048 or 4096 bits

RSA modulus is not even

RSA exponent is 65537

ECDSA keys are on the P-256 curve.

Ed25519 keys are disallowed (will be allowed later)

DSA keys are disallowed

This PR introduces checks 2-5, but not 1, 6-8.

Essentially should we be trying to guarantee that x509 parsing -> custom verification can never produce a cert that isn't parsable by custom parsing?

1. There is a check in the TLS code that the payload of each message is less than 16MB. When we invoke the method VerifyConnection, we have already read the certificate in its full size, so I'm not sure what is gained by restricting the certificate size after we have parsed it.
6. I can do that, however - can the rest of the codebase handle P384? If so, how can we be sure there aren't networks with P384 that would get locked out as a result? Do we just assume everybody use whatever is in staking/tls.go ?
7. Same question as above.
8. DSA is not available in TLS 1.3, so they cannot verify such a signature, hence the handshake would most likely fail at the cipher suite negotiation phase which is before the signature check.

I "optimistically" pushed a follow-up PR to address 6+7

yacovm · 2024-10-27T20:27:46Z

Essentially should we be trying to guarantee that x509 parsing -> custom verification can never produce a cert that isn't parsable by custom parsing?

In this PR I'm just addressing low hanging fruits that make sense to handle early-on to avoid nonsensical exotic configuration that shouldn't be used, such as an avalanchego node not using the RSA verification exponent that is built-in in Golang, or using an exceedingly large modulus of 8K bits.

Of course, the space of certificates valid by x509 is a much larger superset than what we need to accept.

This commit makes validation of TLS certificates with either too big RSA keys, or the wrong exponent, fail as soon as the remote node presents its TLS certificate, in contrast to after the TLS handshake. Signed-off-by: Yacov Manevich <[email protected]>

Signed-off-by: Yacov Manevich <[email protected]>

yacovm requested a review from StephenButtolph as a code owner October 24, 2024 19:51

yacovm force-pushed the TLS-RSA branch 6 times, most recently from bd1c956 to f4d5987 Compare October 24, 2024 22:38

yacovm self-assigned this Oct 24, 2024

yacovm added the enhancement New feature or request label Oct 24, 2024

marun reviewed Oct 25, 2024

View reviewed changes

yacovm force-pushed the TLS-RSA branch from f4d5987 to ff31d44 Compare October 25, 2024 13:04

tsachiherman reviewed Oct 25, 2024

View reviewed changes

yacovm force-pushed the TLS-RSA branch from ff31d44 to c83c579 Compare October 25, 2024 13:57

marun reviewed Oct 25, 2024

View reviewed changes

network/peer/upgrader_test.go Outdated Show resolved Hide resolved

marun reviewed Oct 25, 2024

View reviewed changes

staking/parse_test.go Show resolved Hide resolved

yacovm force-pushed the TLS-RSA branch from c83c579 to f42b816 Compare October 25, 2024 19:42

marun approved these changes Oct 25, 2024

View reviewed changes

StephenButtolph reviewed Oct 27, 2024

View reviewed changes

yacovm force-pushed the TLS-RSA branch 3 times, most recently from 2e73fd5 to 36ca8da Compare October 27, 2024 20:23

yacovm force-pushed the TLS-RSA branch from 36ca8da to 960b750 Compare October 27, 2024 20:30

Address code review comments

5d4212c

Signed-off-by: Yacov Manevich <[email protected]>

yacovm force-pushed the TLS-RSA branch from 960b750 to 5d4212c Compare October 27, 2024 20:38

yacovm closed this Oct 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate improper TLS RSA certificates early-on #3496

Validate improper TLS RSA certificates early-on #3496

yacovm commented Oct 24, 2024 •

edited

Loading

marun Oct 25, 2024

marun left a comment •

edited

Loading

yacovm commented Oct 25, 2024

tsachiherman Oct 25, 2024

yacovm Oct 25, 2024

tsachiherman Oct 25, 2024

yacovm Oct 25, 2024

yacovm Oct 25, 2024

tsachiherman Oct 25, 2024

yacovm Oct 25, 2024

StephenButtolph Oct 25, 2024

StephenButtolph left a comment •

edited

Loading

StephenButtolph Oct 27, 2024

yacovm Oct 27, 2024

StephenButtolph Oct 27, 2024

yacovm Oct 27, 2024

StephenButtolph Oct 27, 2024

yacovm Oct 27, 2024

yacovm commented Oct 27, 2024 •

edited

Loading

yacovm commented Oct 27, 2024

Validate improper TLS RSA certificates early-on #3496

Validate improper TLS RSA certificates early-on #3496

Conversation

yacovm commented Oct 24, 2024 • edited Loading

Why this should be merged

How this works

How this was tested

Need to be documented in RELEASES.md?

Choose a reason for hiding this comment

marun left a comment • edited Loading

Choose a reason for hiding this comment

yacovm commented Oct 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenButtolph left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yacovm commented Oct 27, 2024 • edited Loading

yacovm commented Oct 27, 2024

yacovm commented Oct 24, 2024 •

edited

Loading

marun left a comment •

edited

Loading

StephenButtolph left a comment •

edited

Loading

yacovm commented Oct 27, 2024 •

edited

Loading