diff --git a/client/internal/certproof/collect.go b/client/internal/certproof/collect.go index d3f90fcf5..40dce1bc9 100644 --- a/client/internal/certproof/collect.go +++ b/client/internal/certproof/collect.go @@ -6,6 +6,7 @@ import ( "time" log "github.com/sirupsen/logrus" + "golang.zx2c4.com/wireguard/wgctrl/wgtypes" "github.com/netbirdio/netbird/shared/management/certposture" "github.com/netbirdio/netbird/shared/management/proto" @@ -31,7 +32,17 @@ func logNoChallenges(checks []*proto.Checks) { // CollectChallenges answers challenges already extracted from the posture checks, so a // caller that ships them across a process boundary reuses the same matching and signing. +// Only nonces of the size management issues are signed, for a peer key of the size of +// ours, so the keys behind the store never sign arbitrary caller-chosen data. func CollectChallenges(ctx context.Context, store Store, challenges []*proto.CertificateChallenge, peerKey []byte) []certposture.Proof { + if len(peerKey) != wgtypes.KeyLen { + log.Warnf("certificate posture: refusing to sign for a %d byte peer key", len(peerKey)) + return nil + } + challenges = wellFormed(challenges) + if len(challenges) == 0 { + return nil + } log.Debugf("certificate posture: answering %d certificate challenges from store %T", len(challenges), store) candidates, err := store.Candidates(ctx) @@ -94,11 +105,24 @@ func CollectChallenges(ctx context.Context, store Store, challenges []*proto.Cer func certificateChallenges(checks []*proto.Checks) []*proto.CertificateChallenge { var challenges []*proto.CertificateChallenge for _, check := range checks { - if challenge := check.GetCertificateChallenge(); challenge != nil && len(challenge.GetNonce()) > 0 { + if challenge := check.GetCertificateChallenge(); challenge != nil { challenges = append(challenges, challenge) } } - return challenges + return wellFormed(challenges) +} + +// wellFormed drops challenges whose nonce is not one management could have issued. +func wellFormed(challenges []*proto.CertificateChallenge) []*proto.CertificateChallenge { + var kept []*proto.CertificateChallenge + for _, challenge := range challenges { + if len(challenge.GetNonce()) != certposture.NonceSize { + log.Debugf("certificate posture: skipping challenge with a %d byte nonce", len(challenge.GetNonce())) + continue + } + kept = append(kept, challenge) + } + return kept } func prove(candidate Candidate, nonce, peerKey []byte) (certposture.Proof, error) { diff --git a/client/internal/certproof/collect_test.go b/client/internal/certproof/collect_test.go index 16768b974..53bfebe02 100644 --- a/client/internal/certproof/collect_test.go +++ b/client/internal/certproof/collect_test.go @@ -106,3 +106,31 @@ func writeFile(t *testing.T, dir, name, content string) { t.Helper() require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte(content), 0o600)) } + +func TestCollectChallenges_RefusesMalformedInput(t *testing.T) { + ca := certtest.NewCA(t, "corp-root") + dir := t.TempDir() + key := certtest.ECDSAKey(t) + writeFile(t, dir, "device.pem", certtest.CertPEM(ca.Issue(t, key, "device"))+certtest.KeyPEM(t, key)) + store := NewFileStore(dir) + nonce := certposture.NewChallenger([]byte("secret")).Nonce(peerKey, time.Now()) + + tests := []struct { + name string + nonce []byte + peerKey []byte + want int + }{ + {"issued nonce and peer key are signed", nonce, peerKey, 1}, + {"short nonce is not signed", nonce[:8], peerKey, 0}, + {"oversized nonce is not signed", append(append([]byte{}, nonce...), 0), peerKey, 0}, + {"short peer key is not signed", nonce, peerKey[:16], 0}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + challenges := []*proto.CertificateChallenge{{Nonce: tt.nonce, CaCertificates: []string{ca.PEM}}} + assert.Len(t, CollectChallenges(context.Background(), store, challenges, tt.peerKey), tt.want, + "the device key signs only what management could have issued") + }) + } +} diff --git a/client/internal/certproof/collector_test.go b/client/internal/certproof/collector_test.go index 73503e9c6..e2fe86c15 100644 --- a/client/internal/certproof/collector_test.go +++ b/client/internal/certproof/collector_test.go @@ -12,7 +12,9 @@ import ( "github.com/netbirdio/netbird/shared/management/proto" ) -var challengeChecks = []*proto.Checks{{CertificateChallenge: &proto.CertificateChallenge{Nonce: []byte("nonce")}}} +var challengeChecks = []*proto.Checks{{CertificateChallenge: &proto.CertificateChallenge{ + Nonce: certposture.NewChallenger([]byte("secret")).Nonce(peerKey, time.Now()), +}}} func TestCollector_SkipsChecksWithoutChallenges(t *testing.T) { var c Collector diff --git a/shared/management/certposture/challenge.go b/shared/management/certposture/challenge.go index 584d2f30e..ce56bea0a 100644 --- a/shared/management/certposture/challenge.go +++ b/shared/management/certposture/challenge.go @@ -14,6 +14,9 @@ const ( challengeDomain = "netbird-cert-challenge-v1" windowLen = 8 nonceLen = windowLen + sha256.Size + + // NonceSize is the length of every nonce a Challenger issues. + NonceSize = nonceLen ) var (