Reject unexpected JWT algorithms, and refuse a guessable signing key

server/config/default.go

@@ -1,5 +1,7 @@
 package config
 
+import "log"
+
 var defaultConfig = &Config{
 	Proto: "http",
 	Host:  "",
@@ -35,7 +37,14 @@ var defaultConfig = &Config{
 
 func checkDefaultValue() {
 	if instance.JWT.SecretKey == "" {
-		instance.JWT.SecretKey = generateRandomSecretKey()
+		key, err := generateSecretKey()
+		if err != nil {
+			// Every session on the device is signed with this. Running with a
+			// key we could not generate properly is not an option.
+			log.Fatalf("failed to generate a jwt secret key: %s", err)
+		}
+
+		instance.JWT.SecretKey = key
 		instance.JWT.RevokeTokensOnLogout = true
 	}
 

server/config/jwt.go

@@ -4,25 +4,42 @@ import (
 	"crypto/rand"
 	"encoding/base64"
 	"fmt"
-	"time"
+	"io"
+	"log"
 )
 
-// RegenerateSecretKey regenerate secret key when logout
+// RegenerateSecretKey regenerate secret key when logout.
+//
+// A rotation that cannot produce a key leaves the working one in place. The
+// outstanding tokens stay valid, which is the lesser of the two failures: the
+// alternative is signing with something guessable.
 func RegenerateSecretKey() {
-	if instance.JWT.RevokeTokensOnLogout {
-		instance.JWT.SecretKey = generateRandomSecretKey()
+	if !instance.JWT.RevokeTokensOnLogout {
+		return
 	}
+
+	key, err := generateSecretKey()
+	if err != nil {
+		log.Printf("keeping the current secret key: %s", err)
+		return
+	}
+
+	instance.JWT.SecretKey = key
 }
 
-// Generate random string for secret key.
-func generateRandomSecretKey() string {
+// secretKeyReader is the entropy source. Tests replace it to exercise the
+// failure path.
+var secretKeyReader io.Reader = rand.Reader
+
+func generateSecretKey() (string, error) {
 	b := make([]byte, 64)
-	_, err := rand.Read(b)
-	if err != nil {
-		currentTime := time.Now().UnixNano()
-		timeString := fmt.Sprintf("%d", currentTime)
-		return fmt.Sprintf("%064s", timeString)
+
+	if _, err := io.ReadFull(secretKeyReader, b); err != nil {
+		// There is no safe fallback. Anything derived from the clock is a key
+		// an attacker can search, because they know roughly when the device
+		// booted, so this has to fail instead.
+		return "", fmt.Errorf("failed to read random bytes for the secret key: %w", err)
 	}
 
-	return base64.URLEncoding.EncodeToString(b)
+	return base64.URLEncoding.EncodeToString(b), nil
 }

server/config/jwt_test.go

@@ -0,0 +1,75 @@
+package config
+
+import (
+	"errors"
+	"strings"
+	"testing"
+)
+
+type failingReader struct{}
+
+func (failingReader) Read([]byte) (int, error) {
+	return 0, errors.New("no entropy")
+}
+
+// Falling back to anything derived from the clock would hand an attacker a
+// signing key they can search: the device boots, and the key is the nanosecond
+// it happened to boot at.
+func TestGenerateSecretKeyFailsRatherThanReturningAPredictableValue(t *testing.T) {
+	original := secretKeyReader
+	t.Cleanup(func() { secretKeyReader = original })
+	secretKeyReader = failingReader{}
+
+	key, err := generateSecretKey()
+
+	if err == nil {
+		t.Fatalf("expected an error, got key %q", key)
+	}
+	if key != "" {
+		t.Fatalf("expected no key on failure, got %q", key)
+	}
+}
+
+func TestGenerateSecretKeyReturnsADifferentKeyEachTime(t *testing.T) {
+	// No override: this exercises the real crypto/rand source.
+	first, err := generateSecretKey()
+	if err != nil {
+		t.Fatalf("failed to generate key: %s", err)
+	}
+
+	second, err := generateSecretKey()
+	if err != nil {
+		t.Fatalf("failed to generate key: %s", err)
+	}
+
+	if first == second {
+		t.Fatal("two generated keys are identical")
+	}
+	if strings.TrimSpace(first) == "" {
+		t.Fatal("generated key is blank")
+	}
+}
+
+// A failed rotation must leave the working key in place rather than installing
+// something guessable.
+func TestRegenerateSecretKeyKeepsTheOldKeyWhenGenerationFails(t *testing.T) {
+	originalReader := secretKeyReader
+	originalRevoke := instance.JWT.RevokeTokensOnLogout
+	originalKey := instance.JWT.SecretKey
+
+	t.Cleanup(func() {
+		secretKeyReader = originalReader
+		instance.JWT.RevokeTokensOnLogout = originalRevoke
+		instance.JWT.SecretKey = originalKey
+	})
+
+	instance.JWT.RevokeTokensOnLogout = true
+	instance.JWT.SecretKey = "the-key-in-use"
+	secretKeyReader = failingReader{}
+
+	RegenerateSecretKey()
+
+	if instance.JWT.SecretKey != "the-key-in-use" {
+		t.Fatalf("secret key changed to %q after a failed rotation", instance.JWT.SecretKey)
+	}
+}

server/config/picoclaw_internal.go

@@ -34,7 +34,11 @@ func GetPicoclawInternalToken() (string, error) {
 		return "", err
 	}
 
-	token := generateRandomSecretKey()
+	token, err := generateSecretKey()
+	if err != nil {
+		return "", err
+	}
+
 	if err := os.MkdirAll(filepath.Dir(picoclawInternalTokenFile), 0o755); err != nil {
 		return "", err
 	}

server/middleware/jwt.go

@@ -90,9 +90,12 @@ func GenerateJWT(username string) (string, error) {
 func ParseJWT(jwtToken string) (*Token, error) {
 	conf := config.GetInstance()
 
+	// Pinned to the one algorithm GenerateJWT issues. Left open, the parser
+	// takes whichever the token's own header names, which is the opening every
+	// algorithm-confusion attack needs.
 	t, err := jwt.ParseWithClaims(jwtToken, &Token{}, func(token *jwt.Token) (interface{}, error) {
 		return []byte(conf.JWT.SecretKey), nil
-	})
+	}, jwt.WithValidMethods([]string{jwt.SigningMethodHS256.Alg()}))
 	if err != nil {
 		log.Debugf("parse jwt error: %s", err)
 		return nil, err

server/middleware/jwt_alg_test.go

@@ -0,0 +1,63 @@
+package middleware
+
+import (
+	"testing"
+	"time"
+
+	"NanoKVM-Server/config"
+
+	"github.com/golang-jwt/jwt/v5"
+)
+
+// signWith mints a token for the device's own secret using a chosen algorithm,
+// which is what an attacker holding a leaked or guessed secret would do.
+func signWith(t *testing.T, method jwt.SigningMethod) string {
+	t.Helper()
+
+	claims := Token{
+		Username: "admin",
+		RegisteredClaims: jwt.RegisteredClaims{
+			ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour)),
+		},
+	}
+
+	signed, err := jwt.NewWithClaims(method, claims).
+		SignedString([]byte(config.GetInstance().JWT.SecretKey))
+	if err != nil {
+		t.Fatalf("failed to sign token: %s", err)
+	}
+
+	return signed
+}
+
+func TestParseJWTAcceptsHS256(t *testing.T) {
+	if _, err := ParseJWT(signWith(t, jwt.SigningMethodHS256)); err != nil {
+		t.Fatalf("HS256 token should be accepted: %s", err)
+	}
+}
+
+// The parser must accept only the algorithm the server issues. Taking whatever
+// the token's own header asks for is how algorithm-confusion attacks start.
+func TestParseJWTRejectsOtherHMACAlgorithms(t *testing.T) {
+	for _, method := range []jwt.SigningMethod{jwt.SigningMethodHS384, jwt.SigningMethodHS512} {
+		if _, err := ParseJWT(signWith(t, method)); err == nil {
+			t.Fatalf("%s token should be rejected", method.Alg())
+		}
+	}
+}
+
+func TestParseJWTRejectsUnsignedToken(t *testing.T) {
+	unsigned, err := jwt.NewWithClaims(jwt.SigningMethodNone, Token{
+		Username: "admin",
+		RegisteredClaims: jwt.RegisteredClaims{
+			ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour)),
+		},
+	}).SignedString(jwt.UnsafeAllowNoneSignatureType)
+	if err != nil {
+		t.Fatalf("failed to build unsigned token: %s", err)
+	}
+
+	if _, err := ParseJWT(unsigned); err == nil {
+		t.Fatal("a token with alg=none should be rejected")
+	}
+}