[client, management] Fix the edge cases left open in the certificate posture check (#8174)

client/internal/pkcs11/driver_linux.go

@@ -32,6 +32,7 @@ const (
 	// signature is 2 KiB.
 	maxAttributeLen = 1 << 20
 	maxSignatureLen = 1 << 16
+	maxFoundObjects = 1 << 16
 )
 
 type version struct {
@@ -103,6 +104,15 @@ const (
 	fnSign              = 43
 )
 
+// checkFunctionListVersion refuses a module whose function list is older than the layout
+// functionList mirrors, which would otherwise be read past its end.
+func checkFunctionListVersion(v version) error {
+	if v.major < 2 {
+		return fmt.Errorf("PKCS#11 function list version %d.%d is older than 2.0", v.major, v.minor)
+	}
+	return nil
+}
+
 // module holds the entry points of one loaded library, bound straight from its
 // CK_FUNCTION_LIST.
 type module struct {
@@ -152,6 +162,10 @@ func load(path string) (_ driver, err error) {
 		return nil, Error{Op: "C_GetFunctionList", Code: uint(rv)}
 	}
 
+	if err := checkFunctionListVersion(list.version); err != nil {
+		return nil, fmt.Errorf("%s: %w", path, err)
+	}
+
 	m := &module{}
 	for _, entry := range []struct {
 		fn    any
@@ -185,21 +199,43 @@ func load(path string) (_ driver, err error) {
 	return m, nil
 }
 
+// slotListAttempts bounds how often the slot list is asked for again after it grew between
+// the size query and the read.
+const slotListAttempts = 3
+
+// slotList returns the slots with a token present. A token plugged in between the size
+// query and the read makes the module answer CKR_BUFFER_TOO_SMALL, in which case the list
+// is asked for again.
+func (m *module) slotList() ([]ulong, error) {
+	for range slotListAttempts {
+		var count ulong
+		if rv := m.cGetSlotList(1, nil, &count); rv != rvOK {
+			return nil, Error{Op: "C_GetSlotList", Code: uint(rv)}
+		}
+		if count == 0 {
+			return nil, nil
+		}
+		slots := make([]ulong, count)
+		rv := m.cGetSlotList(1, &slots[0], &count)
+		if rv == rvBufferTooSmall {
+			continue
+		}
+		if rv != rvOK {
+			return nil, Error{Op: "C_GetSlotList", Code: uint(rv)}
+		}
+		return slots[:count], nil
+	}
+	return nil, Error{Op: "C_GetSlotList", Code: rvBufferTooSmall}
+}
+
 func (m *module) tokens() ([]Token, error) {
-	var count ulong
-	if rv := m.cGetSlotList(1, nil, &count); rv != rvOK {
-		return nil, Error{Op: "C_GetSlotList", Code: uint(rv)}
-	}
-	if count == 0 {
-		return nil, nil
-	}
-	slots := make([]ulong, count)
-	if rv := m.cGetSlotList(1, &slots[0], &count); rv != rvOK {
-		return nil, Error{Op: "C_GetSlotList", Code: uint(rv)}
+	slots, err := m.slotList()
+	if err != nil {
+		return nil, err
 	}
 
-	tokens := make([]Token, 0, count)
-	for _, slot := range slots[:count] {
+	tokens := make([]Token, 0, len(slots))
+	for _, slot := range slots {
 		var info tokenInfo
 		if rv := m.cGetTokenInfo(slot, &info); rv != rvOK {
 			continue
@@ -262,11 +298,16 @@ func (m *module) findObjects(session uint, template []Attribute) ([]Object, erro
 		if count > findBatch {
 			return nil, fmt.Errorf("C_FindObjects reported %d handles for a batch of %d", count, findBatch)
 		}
+		// Only an empty batch ends the search: a module may return fewer handles than
+		// asked for before it has returned them all.
+		if count == 0 {
+			return objects, nil
+		}
 		for _, handle := range batch[:count] {
 			objects = append(objects, Object(handle))
 		}
-		if count < findBatch {
-			return objects, nil
+		if len(objects) > maxFoundObjects {
+			return nil, fmt.Errorf("C_FindObjects returned more than %d objects", maxFoundObjects)
 		}
 	}
 }

client/internal/pkcs11/driver_linux_test.go

@@ -70,3 +70,119 @@ func loadFirst(t *testing.T, paths ...string) *Module {
 	t.Skip("p11-kit trust module not installed")
 	return nil
 }
+
+// TestSlotList_RetriesWhenTheListGrows: a token plugged in between the size query and the
+// fill makes the module answer CKR_BUFFER_TOO_SMALL; the list has to be asked for again.
+func TestSlotList_RetriesWhenTheListGrows(t *testing.T) {
+	calls := 0
+	m := &module{cGetSlotList: func(_ byte, slots *ulong, count *ulong) ulong {
+		calls++
+		switch {
+		case slots == nil && calls == 1:
+			*count = 1
+		case slots != nil && calls == 2:
+			*count = 2
+			return rvBufferTooSmall
+		case slots == nil:
+			*count = 2
+		default:
+			*count = 2
+			backing := unsafe.Slice(slots, 2)
+			backing[0], backing[1] = 7, 9
+		}
+		return rvOK
+	}}
+
+	slots, err := m.slotList()
+
+	require.NoError(t, err)
+	assert.Equal(t, []ulong{7, 9}, slots, "the list is read again at its new size")
+}
+
+func TestSlotList_GivesUpOnAListThatKeepsGrowing(t *testing.T) {
+	m := &module{cGetSlotList: func(_ byte, slots *ulong, count *ulong) ulong {
+		if slots == nil {
+			*count = 1
+			return rvOK
+		}
+		return rvBufferTooSmall
+	}}
+
+	_, err := m.slotList()
+
+	assert.Error(t, err, "a module that never settles on a size is an error, not a loop")
+}
+
+func TestCheckFunctionListVersion(t *testing.T) {
+	assert.NoError(t, checkFunctionListVersion(version{major: 2, minor: 40}))
+	assert.NoError(t, checkFunctionListVersion(version{major: 3, minor: 0}))
+	assert.Error(t, checkFunctionListVersion(version{major: 1, minor: 0}), "a pre-2.0 list has no entry points to index")
+	assert.Error(t, checkFunctionListVersion(version{}), "an empty version is not a function list")
+}
+
+// fakeFindModule answers C_FindObjects from batches, a handle per entry, and reports how
+// the search was torn down.
+func fakeFindModule(batches [][]ulong, finalized *int) *module {
+	next := 0
+	return &module{
+		cFindObjectsInit:  func(ulong, *attribute, ulong) ulong { return rvOK },
+		cFindObjectsFinal: func(ulong) ulong { *finalized++; return rvOK },
+		cFindObjects: func(_ ulong, objects *ulong, max ulong, count *ulong) ulong {
+			if next >= len(batches) {
+				*count = 0
+				return rvOK
+			}
+			batch := batches[next]
+			next++
+			copy(unsafe.Slice(objects, max), batch)
+			*count = ulong(len(batch))
+			return rvOK
+		},
+	}
+}
+
+var findTemplate = []Attribute{{Type: AttrClass, Value: ULong(ClassPrivateKey)}}
+
+// TestFindObjects_ReadsPastAShortBatch: PKCS#11 ends a search with an empty batch, not with
+// one shorter than asked for, so a module that hands out its handles in small batches must
+// still have all of them returned.
+func TestFindObjects_ReadsPastAShortBatch(t *testing.T) {
+	finalized := 0
+	m := fakeFindModule([][]ulong{{1, 2}, {3}, {4, 5, 6}}, &finalized)
+
+	objects, err := m.findObjects(1, findTemplate)
+
+	require.NoError(t, err)
+	assert.Equal(t, []Object{1, 2, 3, 4, 5, 6}, objects, "every batch is read until the module returns an empty one")
+	assert.Equal(t, 1, finalized, "the search is finished once")
+}
+
+func TestFindObjects_StopsAtAnEmptyBatch(t *testing.T) {
+	finalized := 0
+	m := fakeFindModule(nil, &finalized)
+
+	objects, err := m.findObjects(1, findTemplate)
+
+	require.NoError(t, err)
+	assert.Empty(t, objects, "a search that finds nothing returns nothing")
+	assert.Equal(t, 1, finalized)
+}
+
+// TestFindObjects_BoundsAModuleThatNeverEnds: a module that keeps returning handles must
+// not make the daemon allocate without limit.
+func TestFindObjects_BoundsAModuleThatNeverEnds(t *testing.T) {
+	finalized := 0
+	endless := &module{
+		cFindObjectsInit:  func(ulong, *attribute, ulong) ulong { return rvOK },
+		cFindObjectsFinal: func(ulong) ulong { finalized++; return rvOK },
+		cFindObjects: func(_ ulong, _ *ulong, _ ulong, count *ulong) ulong {
+			*count = findBatch
+			return rvOK
+		},
+	}
+
+	_, err := endless.findObjects(1, findTemplate)
+
+	assert.Error(t, err, "an endless search is cut off")
+	assert.Equal(t, 1, finalized, "and still finished")
+}

client/internal/pkcs11/pkcs11.go

@@ -50,6 +50,7 @@ const (
 	rvPINExpired          = 0xa3
 	rvPINLocked           = 0xa4
 	rvUserAlreadyLoggedIn = 0x100
+	rvBufferTooSmall      = 0x150
 	rvAlreadyInitialized  = 0x191
 )
 

client/internal/pkcs11/publickey.go

@@ -70,8 +70,10 @@ func rsaPublicKey(modulus, exponent []byte) (*rsa.PublicKey, error) {
 // uncompressed point wrapped in a DER OCTET STRING, which some modules hand out bare.
 func ecPublicKey(params, point []byte) (*ecdsa.PublicKey, error) {
 	var oid asn1.ObjectIdentifier
-	if _, err := asn1.Unmarshal(params, &oid); err != nil {
+	if rest, err := asn1.Unmarshal(params, &oid); err != nil {
 		return nil, fmt.Errorf("CKA_EC_PARAMS: %w", err)
+	} else if len(rest) != 0 {
+		return nil, errors.New("CKA_EC_PARAMS has trailing data")
 	}
 	curve, ok := curvesByOID[oid.String()]
 	if !ok {
@@ -80,8 +82,10 @@ func ecPublicKey(params, point []byte) (*ecdsa.PublicKey, error) {
 	size := (curve.Params().BitSize + 7) / 8
 	raw := point
 	if len(raw) != 1+2*size {
-		if _, err := asn1.Unmarshal(point, &raw); err != nil {
+		if rest, err := asn1.Unmarshal(point, &raw); err != nil {
 			return nil, fmt.Errorf("CKA_EC_POINT: %w", err)
+		} else if len(rest) != 0 {
+			return nil, errors.New("CKA_EC_POINT has trailing data")
 		}
 	}
 	if len(raw) != 1+2*size || raw[0] != 4 {

client/internal/pkcs11/publickey_test.go

@@ -6,6 +6,7 @@ import (
 	"crypto/rand"
 	"crypto/rsa"
 	"encoding/asn1"
+	"math"
 	"math/big"
 	"testing"
 
@@ -21,6 +22,8 @@ func TestECPublicKey(t *testing.T) {
 	}{
 		{"P-256", elliptic.P256(), asn1.ObjectIdentifier{1, 2, 840, 10045, 3, 1, 7}},
 		{"P-384", elliptic.P384(), asn1.ObjectIdentifier{1, 3, 132, 0, 34}},
+		// The wrapped P-521 point is 133 bytes, which needs the long form of the DER length.
+		{"P-521", elliptic.P521(), asn1.ObjectIdentifier{1, 3, 132, 0, 35}},
 	}
 	for _, tt := range curves {
 		t.Run(tt.name, func(t *testing.T) {
@@ -57,6 +60,13 @@ func TestECPublicKey_Rejections(t *testing.T) {
 	assert.Error(t, err, "a truncated point must be rejected")
 	_, err = ecPublicKey([]byte("junk"), point)
 	assert.Error(t, err, "malformed parameters must be rejected")
+
+	wrapped, err := asn1.Marshal(point)
+	require.NoError(t, err)
+	_, err = ecPublicKey(append(append([]byte{}, p256...), 0), point)
+	assert.Error(t, err, "bytes after the curve parameters must be rejected")
+	_, err = ecPublicKey(p256, append(append([]byte{}, wrapped...), 0))
+	assert.Error(t, err, "bytes after the wrapped point must be rejected")
 }
 
 func TestRSAPublicKey(t *testing.T) {
@@ -69,6 +79,12 @@ func TestRSAPublicKey(t *testing.T) {
 
 	_, err = rsaPublicKey(key.N.Bytes(), nil)
 	assert.Error(t, err, "a missing exponent must be rejected")
+	_, err = rsaPublicKey(key.N.Bytes(), big.NewInt(math.MaxInt32).Bytes())
+	assert.NoError(t, err, "the largest exponent an int holds is accepted")
+	_, err = rsaPublicKey(key.N.Bytes(), big.NewInt(math.MaxInt32+1).Bytes())
+	assert.Error(t, err, "an exponent past int32 must be rejected")
+	_, err = rsaPublicKey(key.N.Bytes(), []byte{0})
+	assert.Error(t, err, "a zero exponent must be rejected")
 }
 
 func TestULongRoundTrip(t *testing.T) {

client/internal/pkcs11/uri.go

@@ -29,7 +29,7 @@ type URI struct {
 // module-path must be absolute and a module-name must be a bare name. Unknown query
 // attributes are ignored, as the RFC asks.
 func ParseURI(raw string) (*URI, error) {
-	rest, ok := strings.CutPrefix(raw, "pkcs11:")
+	rest, ok := cutPrefixFold(raw, "pkcs11:")
 	if !ok {
 		return nil, errors.New("PKCS#11 URI does not start with the pkcs11 scheme")
 	}
@@ -72,11 +72,61 @@ func (u *URI) setQueryAttribute(name, value string) error {
 	case "pin-value":
 		u.pinValue = &value
 	case "pin-source":
-		u.pinSource = value
+		path, err := pinSourcePath(value)
+		if err != nil {
+			return err
+		}
+		u.pinSource = path
 	}
 	return nil
 }
 
+// pinSourcePath turns a pin-source value into the file to read. A plain path is used as
+// it is; a file URI must name a local absolute path, with no host or localhost (RFC 8089
+// section 2), so a PIN is never read from somewhere the URI only appears to point to.
+func pinSourcePath(value string) (string, error) {
+	if value == "" {
+		return "", errors.New("PKCS#11 URI pin-source is empty")
+	}
+	rest, isFile := cutPrefixFold(value, "file:")
+	if !isFile {
+		return value, nil
+	}
+	authorityAndPath, hasAuthority := strings.CutPrefix(rest, "//")
+	if !hasAuthority {
+		if !isAbsolutePath(rest) {
+			return "", fmt.Errorf("PKCS#11 URI pin-source %q must be an absolute path", value)
+		}
+		return rest, nil
+	}
+	if isAbsolutePath(authorityAndPath) {
+		return authorityAndPath, nil
+	}
+	host, path, _ := strings.Cut(authorityAndPath, "/")
+	if host != "" && !strings.EqualFold(host, "localhost") {
+		return "", fmt.Errorf("PKCS#11 URI pin-source host %q is not the local machine", host)
+	}
+	if path == "" {
+		return "", fmt.Errorf("PKCS#11 URI pin-source %q names no file", value)
+	}
+	return "/" + path, nil
+}
+
+// isAbsolutePath reports whether p is absolute in a file URI. A path starting with a slash
+// is absolute there on every platform, including Windows, where filepath.IsAbs wants a
+// drive letter.
+func isAbsolutePath(p string) bool {
+	return strings.HasPrefix(p, "/") || filepath.IsAbs(p)
+}
+
+// cutPrefixFold is strings.CutPrefix with the prefix matched without regard to case.
+func cutPrefixFold(s, prefix string) (string, bool) {
+	if len(s) < len(prefix) || !strings.EqualFold(s[:len(prefix)], prefix) {
+		return s, false
+	}
+	return s[len(prefix):], true
+}
+
 // Module is the library to load, DefaultModule when the URI names none.
 func (u *URI) Module() string {
 	if u.ModulePath == "" {
@@ -99,8 +149,7 @@ func (u *URI) PIN() ([]byte, error) {
 	if u.pinSource == "" {
 		return nil, nil
 	}
-	path := strings.TrimPrefix(strings.TrimPrefix(u.pinSource, "file://"), "file:")
-	pin, err := os.ReadFile(path)
+	pin, err := os.ReadFile(u.pinSource)
 	if err != nil {
 		return nil, fmt.Errorf("read PIN: %w", err)
 	}

client/internal/pkcs11/uri_test.go

@@ -84,6 +84,10 @@ func TestParseURI_Rejections(t *testing.T) {
 		"module-name with dotdot":  "pkcs11:token=a?module-name=..",
 		"empty module-name":        "pkcs11:token=a?module-name=",
 		"pin-value and pin-source": "pkcs11:token=a?pin-value=1&pin-source=file:/etc/pin",
+		// Without a value the URI would silently mean "no PIN" and skip the login.
+		"empty pin-source":    "pkcs11:token=a?pin-source=",
+		"remote pin-source":   "pkcs11:token=a?pin-source=file://example.com/etc/pin",
+		"relative pin-source": "pkcs11:token=a?pin-source=file:pin",
 	}
 	for name, raw := range tests {
 		_, err := ParseURI(raw)
@@ -91,6 +95,31 @@ func TestParseURI_Rejections(t *testing.T) {
 	}
 }
 
+// TestParseURI_SchemeIsCaseInsensitive: RFC 3986 section 3.1 makes schemes case-insensitive.
+func TestParseURI_SchemeIsCaseInsensitive(t *testing.T) {
+	for _, raw := range []string{"PKCS11:token=netbird", "Pkcs11:token=netbird"} {
+		uri, err := ParseURI(raw)
+		require.NoError(t, err, raw)
+		assert.Equal(t, "netbird", uri.Token, raw)
+	}
+}
+
+func TestURI_PINFromLocalhostFileURI(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("file URIs with a host name are written differently on Windows")
+	}
+	path := filepath.Join(t.TempDir(), "pin")
+	require.NoError(t, os.WriteFile(path, []byte("secret\n"), 0o600))
+
+	for _, host := range []string{"localhost", "LocalHost"} {
+		uri, err := ParseURI("pkcs11:token=netbird?pin-source=file://" + host + path)
+		require.NoError(t, err, host)
+		pin, err := uri.PIN()
+		require.NoError(t, err, host)
+		assert.Equal(t, []byte("secret"), pin, "RFC 8089 names the local machine as localhost")
+	}
+}
+
 func TestURI_PINFromFile(t *testing.T) {
 	path := filepath.Join(t.TempDir(), "pin")
 	require.NoError(t, os.WriteFile(path, []byte("secret\n"), 0o600))
@@ -117,3 +146,30 @@ func absModule(name string) string {
 	}
 	return "/lib/" + name
 }
+
+// TestPinSourcePath covers how a pin-source value becomes a file path, without touching the
+// file system, so the same cases run on every platform. A path starting with a slash is
+// absolute in a file URI everywhere, including Windows, where filepath.IsAbs would refuse it.
+func TestPinSourcePath(t *testing.T) {
+	tests := []struct {
+		value string
+		want  string
+	}{
+		{"/etc/netbird/pkcs11.pin", "/etc/netbird/pkcs11.pin"},
+		{"file:/etc/netbird/pkcs11.pin", "/etc/netbird/pkcs11.pin"},
+		{"file:///etc/netbird/pkcs11.pin", "/etc/netbird/pkcs11.pin"},
+		{"FILE:///etc/netbird/pkcs11.pin", "/etc/netbird/pkcs11.pin"},
+		{"file://localhost/etc/netbird/pkcs11.pin", "/etc/netbird/pkcs11.pin"},
+		{"file://LocalHost/etc/netbird/pkcs11.pin", "/etc/netbird/pkcs11.pin"},
+	}
+	for _, tt := range tests {
+		got, err := pinSourcePath(tt.value)
+		require.NoError(t, err, tt.value)
+		assert.Equal(t, tt.want, got, tt.value)
+	}
+
+	for _, value := range []string{"", "file:pin", "file://example.com/etc/pin", "file://localhost", "file://localhost/"} {
+		_, err := pinSourcePath(value)
+		assert.Error(t, err, "%q must be refused", value)
+	}
+}