Modify TLS parsing rules for Durango #2458

StephenButtolph · 2023-12-10T06:02:49Z

Why this should be merged

This PR removes the reliance on the crypto/x509 package for parsing TLS certs used outside of TLS connections.

pkg: github.com/ava-labs/avalanchego/staking
BenchmarkParse/ParseCertificate-12         	  412269	      2994 ns/op	 2664 B/op	      19 allocs/op
BenchmarkParse/ParseCertificatePermissive-12     1280276	     925.5 ns/op	  752 B/op	       5 allocs/op
PASS
coverage: 14.4% of statements in all
ok  	github.com/ava-labs/avalanchego/staking	4.627s

The result is faster (as less checks are performed) and more obviously correct (as we no longer need to perform additional verification outside of parsing).

How this works

Inlines x509.ParseCertificate and removes unnecessary checks.
Moves certificate validation into parsing.

How this was tested

Fuzzing parsing against 600+ certificates
Inspecting certificates on Fuji + Mainnet
Testing against prior proposervm blocks with non-standard certs
CI

StephenButtolph · 2023-12-10T06:07:48Z

staking/parse.go

+		namedCurve := elliptic.P256()
+		x, y := elliptic.Unmarshal(namedCurve, der)


TODO: Should we use a different parsing function? This aligns with the x509 package, but is deprecated.

The canonical entry point is x509.ParsePKIXPublicKey but you are correctly reimplementing that. I think this might be a rare enough case in which elliptic.Unmarshal is appropriate. Its behavior is very well specified, anyway, so you don't need to worry about it changing.

staking/parse_test.go

StephenButtolph · 2023-12-10T06:08:37Z

staking/verify.go

-	if err := ValidateCertificate(cert); err != nil {
-		return err
-	}
-


This is now done in certificate parsing (pre-durango) or not at all (post-durango).

ValidateCertificate is still called in connToIDAndCert post-durango, no? Or are you referring to something else?

func connToIDAndCert(conn *tls.Conn, invalidCerts prometheus.Counter, durangoTime time.Time) (ids.NodeID, net.Conn, *staking.Certificate, error) { if err := conn.Handshake(); err != nil { return ids.EmptyNodeID, nil, nil, err } state := conn.ConnectionState() if len(state.PeerCertificates) == 0 { return ids.EmptyNodeID, nil, nil, errNoCert } tlsCert := state.PeerCertificates[0] // Invariant: ParseCertificate is used rather than CertificateFromX509 to // ensure that signature verification can assume the certificate was // parseable according the staking package's parser. // // TODO: Remove pre-Durango parsing after v1.11.x has activated. var ( peerCert *staking.Certificate err error ) if time.Now().Before(durangoTime) { peerCert, err = staking.ParseCertificate(tlsCert.Raw) } else { peerCert, err = staking.ParseCertificatePermissive(tlsCert.Raw) } if err != nil { invalidCerts.Inc() return ids.EmptyNodeID, nil, nil, err } // We validate the certificate here to attempt to make the validity of the // peer certificate as clear as possible. Specifically, a node running a // prior version using an invalid certificate should not be able to report // healthy. if err := staking.ValidateCertificate(peerCert); err != nil { invalidCerts.Inc() return ids.EmptyNodeID, nil, nil, err } nodeID := ids.NodeIDFromCert(peerCert) return nodeID, conn, peerCert, nil }

I think it also still happens here:

avalanchego/node/node.go

Lines 112 to 114 in 7623ffd

if err := staking.ValidateCertificate(stakingCert); err != nil {

return nil, fmt.Errorf("invalid staking certificate: %w", err)

}

Good catch. We expect any certificates returned from a Parse function to have been validated by that parse function... So I removed the call from connToIDAndCert.

As for the call in node - I'm not exactly sure how we want to handle that in the future yet... We'll probably just re-parse it or something.

I think Parse sounds right

StephenButtolph · 2023-12-10T06:09:41Z

staking/parse.go

+}
+
+// Ref: https://github.com/golang/go/blob/go1.19.12/src/crypto/x509/parser.go#L215-L306
+func parsePublicKey(keyData *publicKeyInfo) (any, x509.SignatureAlgorithm, error) {


After Durango activates, we'll remove the SignatureAlgorithm from the cert - and this function.

staking/verify_test.go

vms/proposervm/state/block_state.go

vms/proposervm/block/parse_test.go

staking/asn1.go

network/network.go

staking/parse.go

StephenButtolph · 2023-12-11T23:11:07Z

staking/parse.go

+
+	input := cryptobyte.String(bytes)
+	// Consume the length and tag bytes.
+	if !input.ReadASN1(&input, cryptobyte_asn1.SEQUENCE) {


Technically - there is no reason to enforce the tags here. Should we switch to using ReadAnyASN1? It might be nice to keep the tags just to make it more clear what we are reading/skipping. Although it would be faster to just ignore the tag.

Behind the scenes, this just calls ReadAnyASN1 and compares the tag afterwards. If speed is a factor in the decision, I don't think that switch would actually make much of an impact: https://cs.opensource.google/go/x/crypto/+/refs/tags/v0.16.0:cryptobyte/asn1.go;l=594-605

As long as it doesn't change in the future, it doesn't matter really. Aesthetically, I think I like leaving the tags in, as it makes the parser more precise about what it is it's extracting, even if it doesn't check the contents of what it's ignoring.

patrick-ogrady · 2023-12-12T00:46:03Z

network/network.go

@@ -276,6 +276,12 @@ func NewNetwork(
 		IPSigner:             peer.NewIPSigner(config.MyIPPort, config.TLSKey),
 	}

+	// Invariant: We delay the activation of durango during the TLS handshake to


network/network.go

staking/parse.go

network/peer/upgrader.go

FiloSottile

Changes under staking/ LGTM.

FiloSottile · 2023-12-14T22:28:20Z

staking/parse.go

+
+	input := cryptobyte.String(bytes)
+	// Consume the length and tag bytes.
+	if !input.ReadASN1(&input, cryptobyte_asn1.SEQUENCE) {


As long as it doesn't change in the future, it doesn't matter really. Aesthetically, I think I like leaving the tags in, as it makes the parser more precise about what it is it's extracting, even if it doesn't check the contents of what it's ignoring.

staking/parse.go

FiloSottile · 2023-12-14T23:03:14Z

staking/parse.go

+		}
+		return pub, x509.SHA256WithRSA, nil
+	case oid.Equal(oidPublicKeyECDSA):
+		namedCurve := elliptic.P256()


I went back and forth on whether to recommend checking the AI parameters to make sure they say P-256, but ultimately there is no benefit in the "certificate is just a complex encoding for keys" model, so this LGTM.

FiloSottile · 2023-12-14T23:04:30Z

staking/parse.go

+		namedCurve := elliptic.P256()
+		x, y := elliptic.Unmarshal(namedCurve, der)


The canonical entry point is x509.ParsePKIXPublicKey but you are correctly reimplementing that. I think this might be a rare enough case in which elliptic.Unmarshal is appropriate. Its behavior is very well specified, anyway, so you don't need to worry about it changing.

FiloSottile · 2023-12-14T23:08:27Z

staking/parse.go

+		}
+		if pub.E <= 0 {
+			return nil, 0, ErrRSAPublicExponentNotPositive
+		}


I would restrict N and E further. I can't promise crypto/rsa will always accept E=1, or 256-bit keys, for example

Ideally, if your in-use certificates allow, you'd restrict N's bitsize to two or three options, and E to two or three values. If you need to be more flexible than that, I would check at least 3 <= E <= MaxInt32, and 2048 <= N.BitLen <= 8192.

Oh, and also I'd check that both N and E are odd. (You can do that with N.Bit(0))

N needs to be a product of two large primes, and E needs to be coprime with phi(N) which is even. Libraries can and do reject even values because some math tricks don't work on them.

Co-authored-by: Filippo Valsorda <github@bip.filippo.io> Signed-off-by: Stephen Buttolph <stephen@avalabs.org>

github-actions · 2024-01-14T00:00:38Z

This PR has become stale because it has been open for 30 days with no activity. Adding the lifecycle/frozen label will cause this PR to ignore lifecycle events.

StephenButtolph added 2 commits December 8, 2023 15:01

Add permissive TLS parsing for durango

0261da9

fix tests + move around verification logic

9498e69

StephenButtolph added the networking This involves networking label Dec 10, 2023

StephenButtolph added this to the v1.10.18 milestone Dec 10, 2023

StephenButtolph self-assigned this Dec 10, 2023

StephenButtolph requested review from abi87, danlaine and joshua-kim as code owners December 10, 2023 06:02

StephenButtolph marked this pull request as draft December 10, 2023 06:02

StephenButtolph added 2 commits December 10, 2023 01:13

nit

3efa223

nit

5771e0d

StephenButtolph commented Dec 10, 2023

View reviewed changes

StephenButtolph added 8 commits December 10, 2023 19:56

reduce diff

e7d6ea9

Remove fuzzing function - for now

3f1e63f

remove more checks

dcd1cce

nit

757094d

nit

a72ee89

nit

d986723

reduce diff

a250e81

nit

7f3a1c3

abi87 reviewed Dec 11, 2023

View reviewed changes

vms/proposervm/block/parse_test.go Outdated Show resolved Hide resolved

abi87 reviewed Dec 11, 2023

View reviewed changes

staking/asn1.go Show resolved Hide resolved

abi87 reviewed Dec 11, 2023

View reviewed changes

network/network.go Show resolved Hide resolved

abi87 reviewed Dec 11, 2023

View reviewed changes

staking/parse.go Show resolved Hide resolved

StephenButtolph added 5 commits December 11, 2023 11:58

Merge branch 'dev' into tls-durango

dca8f4f

Add benchmark

2adcee0

nit

c80c059

nolint

fb337ed

nit

37372d3

StephenButtolph commented Dec 11, 2023

View reviewed changes

StephenButtolph added the Durango durango fork label Dec 11, 2023

patrick-ogrady reviewed Dec 12, 2023

View reviewed changes

network/network.go Show resolved Hide resolved

patrick-ogrady reviewed Dec 12, 2023

View reviewed changes

staking/parse.go Outdated Show resolved Hide resolved

StephenButtolph added 2 commits December 11, 2023 20:01

nit

0683e65

remove unnecessary checks

8dd4072

patrick-ogrady reviewed Dec 12, 2023

View reviewed changes

network/peer/upgrader.go Show resolved Hide resolved

patrick-ogrady approved these changes Dec 12, 2023

View reviewed changes

dhrubabasu approved these changes Dec 12, 2023

View reviewed changes

abi87 approved these changes Dec 12, 2023

View reviewed changes

Merge branch 'dev' into tls-durango

973608b

FiloSottile reviewed Dec 14, 2023

View reviewed changes

StephenButtolph and others added 9 commits December 14, 2023 18:19

Update staking/parse.go

eefac5f

Co-authored-by: Filippo Valsorda <github@bip.filippo.io> Signed-off-by: Stephen Buttolph <stephen@avalabs.org>

nit

993d4ad

Restrict RSA keys

148b387

nit

9449692

nit

da2aaaf

nit

5fbf77b

nit

65de2aa

fix test

0d3c6e9

Merge branch 'dev' into tls-durango

fa8482f

github-actions bot added the lifecycle/stale label Jan 14, 2024

StephenButtolph added 2 commits January 18, 2024 11:15

merged

88ce17a

Merge branch 'dev' into tls-durango

6de9fed

StephenButtolph removed the lifecycle/stale label Jan 18, 2024

StephenButtolph added this pull request to the merge queue Jan 18, 2024

Merged via the queue into dev with commit f5884bb Jan 18, 2024

StephenButtolph deleted the tls-durango branch January 18, 2024 18:49

		namedCurve := elliptic.P256()
		x, y := elliptic.Unmarshal(namedCurve, der)

	if err := staking.ValidateCertificate(stakingCert); err != nil {
	return nil, fmt.Errorf("invalid staking certificate: %w", err)
	}

Modify TLS parsing rules for Durango #2458

Modify TLS parsing rules for Durango #2458

Uh oh!

Conversation

StephenButtolph commented Dec 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this should be merged

How this works

How this was tested

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

StephenButtolph Dec 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FiloSottile left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 14, 2024

Uh oh!

Uh oh!

StephenButtolph commented Dec 10, 2023 •

edited

Loading

StephenButtolph Dec 10, 2023 •

edited

Loading