From f573e8a26695278f9d71587390fbfe0d0933aa21 Mon Sep 17 00:00:00 2001 From: Jack Doan Date: Fri, 6 Feb 2026 13:26:51 -0600 Subject: [PATCH] Merge commit from fork Newly signed P256 based certificates will have their signature clamped to the low-s form. Update CHANGELOG.md --- CHANGELOG.md | 16 +++++- cert/ca_pool.go | 18 ++++++ cert/ca_pool_test.go | 47 ++++++++++++++-- cert/cert.go | 33 +++++++++++ cert/p256/p256.go | 122 +++++++++++++++++++++++++++++++++++++++++ cert/p256/p256_test.go | 28 ++++++++++ cert/sign.go | 9 +++ cert/sign_test.go | 46 ++++++++++++++++ go.mod | 1 + go.sum | 2 + 10 files changed, 317 insertions(+), 5 deletions(-) create mode 100644 cert/p256/p256.go create mode 100644 cert/p256/p256_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 330a7f78..2ef7551f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [1.10.3] - 2026-02-06 + +### Security + +- Fix an issue where blocklist bypass is possible when using curve P256 since the signature can have 2 valid representations. + Both fingerprint representations will be tested against the blocklist. + Any newly issued P256 based certificates will have their signature clamped to the low-s form. + Nebula will assert the low-s signature form when validating certificates in a future version. [GHSA-69x3-g4r3-p962](https://github.com/slackhq/nebula/security/advisories/GHSA-69x3-g4r3-p962) + +### Changed + +- Improve error reporting if nebula fails to start due to a tun device naming issue. (#1588) + ## [1.10.2] - 2026-01-21 ### Fixed @@ -775,7 +788,8 @@ created.) - Initial public release. -[Unreleased]: https://github.com/slackhq/nebula/compare/v1.10.2...HEAD +[Unreleased]: https://github.com/slackhq/nebula/compare/v1.10.3...HEAD +[1.10.3]: https://github.com/slackhq/nebula/releases/tag/v1.10.3 [1.10.2]: https://github.com/slackhq/nebula/releases/tag/v1.10.2 [1.10.1]: https://github.com/slackhq/nebula/releases/tag/v1.10.1 [1.10.0]: https://github.com/slackhq/nebula/releases/tag/v1.10.0 diff --git a/cert/ca_pool.go b/cert/ca_pool.go index 2bf480f2..e9903e1f 100644 --- a/cert/ca_pool.go +++ b/cert/ca_pool.go @@ -141,10 +141,23 @@ func (ncp *CAPool) VerifyCertificate(now time.Time, c Certificate) (*CachedCerti return nil, err } + // Pre nebula v1.10.3 could generate signatures in either high or low s form and validation + // of signatures allowed for either. Nebula v1.10.3 and beyond clamps signature generation to low-s form + // but validation still allows for either. Since a change in the signature bytes affects the fingerprint, we + // need to test both forms until such a time comes that we enforce low-s form on signature validation. + fp2, err := CalculateAlternateFingerprint(c) + if err != nil { + return nil, fmt.Errorf("could not calculate alternate fingerprint to verify: %w", err) + } + if fp2 != "" && ncp.IsBlocklisted(fp2) { + return nil, ErrBlockListed + } + cc := CachedCertificate{ Certificate: c, InvertedGroups: make(map[string]struct{}), Fingerprint: fp, + fingerprint2: fp2, signerFingerprint: signer.Fingerprint, } @@ -158,6 +171,11 @@ func (ncp *CAPool) VerifyCertificate(now time.Time, c Certificate) (*CachedCerti // VerifyCachedCertificate is the same as VerifyCertificate other than it operates on a pre-verified structure and // is a cheaper operation to perform as a result. func (ncp *CAPool) VerifyCachedCertificate(now time.Time, c *CachedCertificate) error { + // Check any available alternate fingerprint forms for this certificate, re P256 high-s/low-s + if c.fingerprint2 != "" && ncp.IsBlocklisted(c.fingerprint2) { + return ErrBlockListed + } + _, err := ncp.verify(c.Certificate, now, c.Fingerprint, c.signerFingerprint) return err } diff --git a/cert/ca_pool_test.go b/cert/ca_pool_test.go index b0fdd5fb..e872c7d4 100644 --- a/cert/ca_pool_test.go +++ b/cert/ca_pool_test.go @@ -5,6 +5,7 @@ import ( "testing" "time" + "github.com/slackhq/nebula/cert/p256" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -170,6 +171,15 @@ func TestCertificateV1_VerifyP256(t *testing.T) { _, err = caPool.VerifyCertificate(time.Now(), c) require.EqualError(t, err, "certificate is in the block list") + // Create a copy of the cert and swap to the alternate form for the signature + nc := c.Copy() + b, err := p256.Swap(c.Signature()) + require.NoError(t, err) + require.NoError(t, nc.(*certificateV1).setSignature(b)) + + _, err = caPool.VerifyCertificate(time.Now(), nc) + require.EqualError(t, err, "certificate is in the block list") + caPool.ResetCertBlocklist() _, err = caPool.VerifyCertificate(time.Now(), c) require.NoError(t, err) @@ -187,7 +197,7 @@ func TestCertificateV1_VerifyP256(t *testing.T) { require.NoError(t, err) caPool = NewCAPool() - b, err := caPool.AddCAFromPEM(caPem) + b, err = caPool.AddCAFromPEM(caPem) require.NoError(t, err) assert.Empty(t, b) @@ -196,7 +206,17 @@ func TestCertificateV1_VerifyP256(t *testing.T) { }) c, _, _, _ = NewTestCert(Version1, Curve_P256, ca, caKey, "test", time.Now(), time.Now().Add(5*time.Minute), nil, nil, []string{"test1"}) - _, err = caPool.VerifyCertificate(time.Now(), c) + cc, err := caPool.VerifyCertificate(time.Now(), c) + require.NoError(t, err) + + // Reset the blocklist and block the alternate form fingerprint + caPool.ResetCertBlocklist() + caPool.BlocklistFingerprint(cc.fingerprint2) + err = caPool.VerifyCachedCertificate(time.Now(), cc) + require.EqualError(t, err, "certificate is in the block list") + + caPool.ResetCertBlocklist() + err = caPool.VerifyCachedCertificate(time.Now(), cc) require.NoError(t, err) } @@ -394,6 +414,15 @@ func TestCertificateV2_VerifyP256(t *testing.T) { _, err = caPool.VerifyCertificate(time.Now(), c) require.EqualError(t, err, "certificate is in the block list") + // Create a copy of the cert and swap to the alternate form for the signature + nc := c.Copy() + b, err := p256.Swap(c.Signature()) + require.NoError(t, err) + require.NoError(t, nc.(*certificateV2).setSignature(b)) + + _, err = caPool.VerifyCertificate(time.Now(), nc) + require.EqualError(t, err, "certificate is in the block list") + caPool.ResetCertBlocklist() _, err = caPool.VerifyCertificate(time.Now(), c) require.NoError(t, err) @@ -411,7 +440,7 @@ func TestCertificateV2_VerifyP256(t *testing.T) { require.NoError(t, err) caPool = NewCAPool() - b, err := caPool.AddCAFromPEM(caPem) + b, err = caPool.AddCAFromPEM(caPem) require.NoError(t, err) assert.Empty(t, b) @@ -420,7 +449,17 @@ func TestCertificateV2_VerifyP256(t *testing.T) { }) c, _, _, _ = NewTestCert(Version2, Curve_P256, ca, caKey, "test", time.Now(), time.Now().Add(5*time.Minute), nil, nil, []string{"test1"}) - _, err = caPool.VerifyCertificate(time.Now(), c) + cc, err := caPool.VerifyCertificate(time.Now(), c) + require.NoError(t, err) + + // Reset the blocklist and block the alternate form fingerprint + caPool.ResetCertBlocklist() + caPool.BlocklistFingerprint(cc.fingerprint2) + err = caPool.VerifyCachedCertificate(time.Now(), cc) + require.EqualError(t, err, "certificate is in the block list") + + caPool.ResetCertBlocklist() + err = caPool.VerifyCachedCertificate(time.Now(), cc) require.NoError(t, err) } diff --git a/cert/cert.go b/cert/cert.go index 9d40e625..01d775e5 100644 --- a/cert/cert.go +++ b/cert/cert.go @@ -4,6 +4,8 @@ import ( "fmt" "net/netip" "time" + + "github.com/slackhq/nebula/cert/p256" ) type Version uint8 @@ -110,6 +112,9 @@ type CachedCertificate struct { InvertedGroups map[string]struct{} Fingerprint string signerFingerprint string + + // A place to store a 2nd fingerprint if the certificate could have one, such as with P256 + fingerprint2 string } func (cc *CachedCertificate) String() string { @@ -152,3 +157,31 @@ func Recombine(v Version, rawCertBytes, publicKey []byte, curve Curve) (Certific return c, nil } + +// CalculateAlternateFingerprint calculates a 2nd fingerprint representation for P256 certificates +// CAPool blocklist testing through `VerifyCertificate` and `VerifyCachedCertificate` automatically performs this step. +func CalculateAlternateFingerprint(c Certificate) (string, error) { + if c.Curve() != Curve_P256 { + return "", nil + } + + nc := c.Copy() + b, err := p256.Swap(nc.Signature()) + if err != nil { + return "", err + } + + switch v := nc.(type) { + case *certificateV1: + err = v.setSignature(b) + case *certificateV2: + err = v.setSignature(b) + default: + return "", ErrUnknownVersion + } + + if err != nil { + return "", err + } + return nc.Fingerprint() +} diff --git a/cert/p256/p256.go b/cert/p256/p256.go new file mode 100644 index 00000000..be0a2381 --- /dev/null +++ b/cert/p256/p256.go @@ -0,0 +1,122 @@ +package p256 + +import ( + "crypto/elliptic" + "errors" + "math/big" + + "filippo.io/bigmod" + + "golang.org/x/crypto/cryptobyte" + "golang.org/x/crypto/cryptobyte/asn1" +) + +var halfN = new(big.Int).Rsh(elliptic.P256().Params().N, 1) +var nMod *bigmod.Modulus + +func init() { + n, err := bigmod.NewModulus(elliptic.P256().Params().N.Bytes()) + if err != nil { + panic(err) + } + nMod = n +} + +func IsNormalized(sig []byte) (bool, error) { + r, s, err := parseSignature(sig) + if err != nil { + return false, err + } + return checkLowS(r, s), nil +} + +func checkLowS(_, s []byte) bool { + bigS := new(big.Int).SetBytes(s) + // Check if S <= (N/2), because we want to include the midpoint in the set of low-s + return bigS.Cmp(halfN) <= 0 +} + +func swap(r, s []byte) ([]byte, []byte, error) { + var err error + bigS, err := bigmod.NewNat().SetBytes(s, nMod) + if err != nil { + return nil, nil, err + } + sNormalized := nMod.Nat().Sub(bigS, nMod) + + return r, sNormalized.Bytes(nMod), nil +} + +func Normalize(sig []byte) ([]byte, error) { + r, s, err := parseSignature(sig) + if err != nil { + return nil, err + } + + if checkLowS(r, s) { + return sig, nil + } + + newR, newS, err := swap(r, s) + if err != nil { + return nil, err + } + + return encodeSignature(newR, newS) +} + +// Swap will change sig between its current form to the opposite high or low form. +func Swap(sig []byte) ([]byte, error) { + r, s, err := parseSignature(sig) + if err != nil { + return nil, err + } + + newR, newS, err := swap(r, s) + if err != nil { + return nil, err + } + + return encodeSignature(newR, newS) +} + +// parseSignature taken exactly from crypto/ecdsa/ecdsa.go +func parseSignature(sig []byte) (r, s []byte, err error) { + var inner cryptobyte.String + input := cryptobyte.String(sig) + if !input.ReadASN1(&inner, asn1.SEQUENCE) || + !input.Empty() || + !inner.ReadASN1Integer(&r) || + !inner.ReadASN1Integer(&s) || + !inner.Empty() { + return nil, nil, errors.New("invalid ASN.1") + } + return r, s, nil +} + +func encodeSignature(r, s []byte) ([]byte, error) { + var b cryptobyte.Builder + b.AddASN1(asn1.SEQUENCE, func(b *cryptobyte.Builder) { + addASN1IntBytes(b, r) + addASN1IntBytes(b, s) + }) + return b.Bytes() +} + +// addASN1IntBytes encodes in ASN.1 a positive integer represented as +// a big-endian byte slice with zero or more leading zeroes. +func addASN1IntBytes(b *cryptobyte.Builder, bytes []byte) { + for len(bytes) > 0 && bytes[0] == 0 { + bytes = bytes[1:] + } + if len(bytes) == 0 { + b.SetError(errors.New("invalid integer")) + return + } + b.AddASN1(asn1.INTEGER, func(c *cryptobyte.Builder) { + if bytes[0]&0x80 != 0 { + c.AddUint8(0) + } + c.AddBytes(bytes) + }) +} diff --git a/cert/p256/p256_test.go b/cert/p256/p256_test.go new file mode 100644 index 00000000..486a7242 --- /dev/null +++ b/cert/p256/p256_test.go @@ -0,0 +1,28 @@ +package p256 + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestFlipping(t *testing.T) { + priv, err1 := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err1) + + out, err := ecdsa.SignASN1(rand.Reader, priv, []byte("big chungus")) + require.NoError(t, err) + + r, s, err := parseSignature(out) + require.NoError(t, err) + + r, s1, err := swap(r, s) + require.NoError(t, err) + r, s2, err := swap(r, s1) + require.NoError(t, err) + require.Equal(t, s, s2) + require.NotEqual(t, s, s1) +} diff --git a/cert/sign.go b/cert/sign.go index 3eb08592..fbfffe4e 100644 --- a/cert/sign.go +++ b/cert/sign.go @@ -9,6 +9,8 @@ import ( "fmt" "net/netip" "time" + + "github.com/slackhq/nebula/cert/p256" ) // TBSCertificate represents a certificate intended to be signed. @@ -126,6 +128,13 @@ func (t *TBSCertificate) SignWith(signer Certificate, curve Curve, sp SignerLamb return nil, err } + if curve == Curve_P256 { + sig, err = p256.Normalize(sig) + if err != nil { + return nil, err + } + } + err = c.setSignature(sig) if err != nil { return nil, err diff --git a/cert/sign_test.go b/cert/sign_test.go index e6f43cdf..bf4c9c0d 100644 --- a/cert/sign_test.go +++ b/cert/sign_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/slackhq/nebula/cert/p256" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -89,3 +90,48 @@ func TestCertificateV1_SignP256(t *testing.T) { require.NoError(t, err) assert.NotNil(t, uc) } + +func TestCertificate_SignP256_AlwaysNormalized(t *testing.T) { + before := time.Now().Add(time.Second * -60).Round(time.Second) + after := time.Now().Add(time.Second * 60).Round(time.Second) + pubKey := []byte("01234567890abcedfghij1234567890ab1234567890abcedfghij1234567890ab") + + tbs := TBSCertificate{ + Version: Version1, + Name: "testing", + Networks: []netip.Prefix{ + mustParsePrefixUnmapped("10.1.1.1/24"), + mustParsePrefixUnmapped("10.1.1.2/16"), + }, + UnsafeNetworks: []netip.Prefix{ + mustParsePrefixUnmapped("9.1.1.2/24"), + mustParsePrefixUnmapped("9.1.1.3/16"), + }, + Groups: []string{"test-group1", "test-group2", "test-group3"}, + NotBefore: before, + NotAfter: after, + PublicKey: pubKey, + IsCA: true, + Curve: Curve_P256, + } + + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + pub := elliptic.Marshal(elliptic.P256(), priv.PublicKey.X, priv.PublicKey.Y) + rawPriv := priv.D.FillBytes(make([]byte, 32)) + + for i := 0; i < 1000; i++ { + if i&1 == 1 { + tbs.Version = Version1 + } else { + tbs.Version = Version2 + } + c, err := tbs.Sign(nil, Curve_P256, rawPriv) + require.NoError(t, err) + assert.NotNil(t, c) + assert.True(t, c.CheckSignature(pub)) + normie, err := p256.IsNormalized(c.Signature()) + require.NoError(t, err) + assert.True(t, normie) + } +} diff --git a/go.mod b/go.mod index 1c564d03..f302f928 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.25 require ( dario.cat/mergo v1.0.2 + filippo.io/bigmod v0.1.0 github.com/anmitsu/go-shlex v0.0.0-20200514113438-38f4b401e2be github.com/armon/go-radix v1.0.0 github.com/cyberdelia/go-metrics-graphite v0.0.0-20161219230853-39f87cc3b432 diff --git a/go.sum b/go.sum index c4613e01..f4b1074c 100644 --- a/go.sum +++ b/go.sum @@ -1,6 +1,8 @@ cloud.google.com/go v0.34.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw= dario.cat/mergo v1.0.2 h1:85+piFYR1tMbRrLcDwR18y4UKJ3aH1Tbzi24VRW1TK8= dario.cat/mergo v1.0.2/go.mod h1:E/hbnu0NxMFBjpMIE34DRGLWqDy0g5FuKDhCb31ngxA= +filippo.io/bigmod v0.1.0 h1:UNzDk7y9ADKST+axd9skUpBQeW7fG2KrTZyOE4uGQy8= +filippo.io/bigmod v0.1.0/go.mod h1:OjOXDNlClLblvXdwgFFOQFJEocLhhtai8vGLy0JCZlI= github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc= github.com/alecthomas/template v0.0.0-20190718012654-fb15b899a751/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc= github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0=