jwe: parse and bound-check PBES2 p2c in int64 space; name the violated bound in the error (#2118)

decryptKeyPBES2 read p2c into float64, compared against the cap in
float64 space, then cast via int(countFlt) before passing to PBKDF2.
At the default MaxPBES2Count=1_000_000 every value is exact and the
arithmetic is sound. The risk lives at the cap-raising end: a caller
who sets WithMaxPBES2Count past 2^53 to support legitimately heavy
PBES2 traffic enables float-precision drift to influence which value
the cap accepts vs rejects, and the int() cast then converts the
rounded float, not the wire integer. int64 throughout removes the
precision class entirely.

The two error sites that reject p2c (NaN/Inf/non-integer; out-of-range)
both emitted the identical opaque "invalid 'p2c' value" string. The
caller couldn't tell which check rejected, the offending value, or
which option to call to widen/tighten. Reword each error site to
include the value, the bound that was violated, and the option name.

Bundled doc improvements:
- Decrypt godoc now mentions WithMaxPBES2Count / WithMinPBES2Count
  alongside the existing WithMaxDecompressBufferSize paragraph; the
  PBES2 amplification surface is now visible from go doc jwe.Decrypt.
- WithMinPBES2Count godoc adds the threat model (under-iteration
  attack on the recipient by a careless or malicious issuer) and
  notes the OWASP 2023 vs RFC 7518 §4.8.1.2 floor asymmetry.

Float-domain bounds use float64(1 << 63) instead of math.MaxInt64 to
keep the comparison platform-independent: 1<<63 is a 65-bit untyped
constant that converts cleanly to float64 (= 2^63, exact) without
going through the implementation's int width. The eventual int(count)
cast is safe because count is bound-checked against int64(maxCount)
where maxCount is platform-int-typed; on 32-bit, count is therefore
always within int range when the cast runs.

Author: lestrrat

jwe/decrypt.go

@@ -83,26 +83,48 @@ func decryptKeyPBES2(recipientKey []byte, alg string, key any, headers Headers,
 	if !ok {
 		return nil, fmt.Errorf(`jwe: decrypt key: missing %q field for PBES2`, CountKey)
 	}
-	var countFlt float64
+
+	// Parse p2c into int64 directly. Float64 cannot represent integers
+	// above 2^53 exactly; comparing a parsed value against a high
+	// MaxPBES2Count cap in float-space and then casting via int(...) lets
+	// out-of-range values silently round into the accepted range, which
+	// would defeat the cap when callers raise it past 2^53. int64 keeps
+	// the bound check exact.
+	var count int64
 	switch v := countV.(type) {
 	case float64:
-		countFlt = v
+		if math.IsNaN(v) || math.IsInf(v, 0) || math.Trunc(v) != v {
+			return nil, fmt.Errorf(`jwe: decrypt key: invalid 'p2c' value: not a positive integer (got %v)`, v)
+		}
+		// Reject values outside int64 range before casting; the cast
+		// of an out-of-range float to int is implementation-defined.
+		// Use explicit float-domain bounds (2^63 / -2^63) instead of
+		// math.MaxInt64 / MinInt64 so the comparison is independent
+		// of the platform's int width and the constants do not need
+		// implicit conversion.
+		const (
+			int64MaxAsFloat = float64(1 << 63) // 2^63, the smallest float > MaxInt64
+			int64MinAsFloat = -int64MaxAsFloat // -2^63, exact float = MinInt64
+		)
+		if v >= int64MaxAsFloat || v < int64MinAsFloat {
+			return nil, fmt.Errorf(`jwe: decrypt key: invalid 'p2c' value: not representable as int64 (got %v)`, v)
+		}
+		count = int64(v)
 	case stdjson.Number:
-		var err error
-		countFlt, err = v.Float64()
+		c, err := v.Int64()
 		if err != nil {
-			return nil, fmt.Errorf(`jwe: decrypt key: %q field is not a valid number: %w`, CountKey, err)
+			return nil, fmt.Errorf(`jwe: decrypt key: invalid 'p2c' value: %q is not a valid integer: %w`, v.String(), err)
 		}
+		count = c
 	default:
 		return nil, fmt.Errorf(`jwe: decrypt key: %q field is not a number`, CountKey)
 	}
 
-	if math.IsNaN(countFlt) || math.IsInf(countFlt, 0) || math.Trunc(countFlt) != countFlt {
-		return nil, fmt.Errorf("jwe: decrypt key: invalid 'p2c' value")
+	if count < int64(minCount) {
+		return nil, fmt.Errorf(`jwe: decrypt key: invalid 'p2c' value: %d is below WithMinPBES2Count=%d (RFC 7518 §4.8.1.2 floor; loosen via jwe.WithMinPBES2Count)`, count, minCount)
 	}
-
-	if countFlt > float64(maxCount) || countFlt < float64(minCount) {
-		return nil, fmt.Errorf("jwe: decrypt key: invalid 'p2c' value")
+	if count > int64(maxCount) {
+		return nil, fmt.Errorf(`jwe: decrypt key: invalid 'p2c' value: %d exceeds WithMaxPBES2Count=%d (DoS amplification cap; raise via jwe.WithMaxPBES2Count)`, count, maxCount)
 	}
 
 	saltBytes, err := base64.DecodeString(saltB64)
@@ -113,7 +135,7 @@ func decryptKeyPBES2(recipientKey []byte, alg string, key any, headers Headers,
 	salt := []byte(alg)
 	salt = append(salt, byte(0))
 	salt = append(salt, saltBytes...)
-	return jwebb.KeyDecryptPBES2(recipientKey, recipientKey, alg, password, salt, int(countFlt))
+	return jwebb.KeyDecryptPBES2(recipientKey, recipientKey, alg, password, salt, int(count))
 }
 
 func decryptKeyAESGCMKW(recipientKey []byte, alg string, key any, headers Headers) ([]byte, error) {

jwe/jwe.go

@@ -1097,6 +1097,17 @@ func (ec *encryptContext) EncryptMessage(payload []byte, cek []byte) ([]byte, er
 //
 //	jwe.Settings(jwe.WithMaxDecompressBufferSize(10*1024*1024)) // changes value globally
 //	jwe.Decrypt(..., jwe.WithMaxDecompressBufferSize(250*1024)) // changes just for this call
+//
+// PBES2 amplification: PBES2 algorithms (PBES2-HS256+A128KW, etc.)
+// derive the CEK via PBKDF2 with the iteration count taken from the
+// JWE's `p2c` header. An attacker-controlled iteration count multiplied
+// by `WithMaxRecipients` is the major CPU-amplification vector on the
+// decrypt side. Bound it via `WithMaxPBES2Count` (default 1,000,000)
+// and reject too-low counts via `WithMinPBES2Count` (default 1000;
+// RFC 7518 §4.8.1.2 floor — note OWASP 2023 recommends ≥600,000 for
+// production password-derived key material). Both options accept a
+// `Settings()` global or a per-call value the same way
+// `WithMaxDecompressBufferSize` does.
 func Decrypt(buf []byte, options ...DecryptOption) ([]byte, error) {
 	dc := decryptContextPool.Get()
 	defer decryptContextPool.Put(dc)

jwe/jwe_test.go

@@ -1306,6 +1306,8 @@ func TestPBES2RejectsNonIntegerCount(t *testing.T) {
 		_, err := jwe.Decrypt(tampered, jwe.WithKey(jwa.PBES2_HS256_A128KW(), key))
 		require.Error(t, err, `jwe.Decrypt should fail`)
 		require.Contains(t, err.Error(), `invalid 'p2c' value`)
+		require.Contains(t, err.Error(), `not a positive integer`,
+			`error should name the constraint`)
 	})
 
 	t.Run("UseNumber decoder rejects fractional count", func(t *testing.T) {
@@ -1315,6 +1317,56 @@ func TestPBES2RejectsNonIntegerCount(t *testing.T) {
 		_, err := jwe.Decrypt(tampered, jwe.WithKey(jwa.PBES2_HS256_A128KW(), key))
 		require.Error(t, err, `jwe.Decrypt should fail`)
 		require.Contains(t, err.Error(), `invalid 'p2c' value`)
+		require.Contains(t, err.Error(), `not a valid integer`,
+			`UseNumber path should name the integer-parse failure`)
+	})
+}
+
+// TestPBES2P2cBoundsErrors locks the error-wording contract that the
+// max/min bound errors name the option, the offending value, and the
+// configured bound — replacing the previous opaque
+// "invalid 'p2c' value" string. Same site also moved from float-space
+// to int64-space comparison so the cap is enforced exactly.
+func TestPBES2P2cBoundsErrors(t *testing.T) {
+	password := []byte(`supersecret`)
+	key, err := jwk.Import[jwk.Key](password)
+	require.NoError(t, err)
+
+	encrypted, err := jwe.Encrypt(
+		[]byte(`hello world`),
+		jwe.WithKey(jwa.PBES2_HS256_A128KW(), key),
+		jwe.WithPBES2Count(2000),
+	)
+	require.NoError(t, err)
+
+	t.Run("error names the bound that was violated (max)", func(t *testing.T) {
+		tampered := rewriteCompactPBES2Count(t, encrypted, 99999999)
+		_, err := jwe.Decrypt(tampered,
+			jwe.WithKey(jwa.PBES2_HS256_A128KW(), key),
+			jwe.WithMaxPBES2Count(10000),
+		)
+		require.Error(t, err)
+		require.Contains(t, err.Error(), `WithMaxPBES2Count`,
+			`max-bound error must name the option`)
+		require.Contains(t, err.Error(), `99999999`,
+			`max-bound error must include the offending value`)
+		require.Contains(t, err.Error(), `10000`,
+			`max-bound error must include the cap`)
+	})
+
+	t.Run("error names the bound that was violated (min)", func(t *testing.T) {
+		tampered := rewriteCompactPBES2Count(t, encrypted, 100)
+		_, err := jwe.Decrypt(tampered,
+			jwe.WithKey(jwa.PBES2_HS256_A128KW(), key),
+			jwe.WithMinPBES2Count(1000),
+		)
+		require.Error(t, err)
+		require.Contains(t, err.Error(), `WithMinPBES2Count`,
+			`min-bound error must name the option`)
+		require.Contains(t, err.Error(), `100`,
+			`min-bound error must include the offending value`)
+		require.Contains(t, err.Error(), `1000`,
+			`min-bound error must include the floor`)
 	})
 }
 

jwe/options.yaml

@@ -190,7 +190,21 @@ options:
     comment: |
       WithMinPBES2Count specifies the minimum number of PBES2 iterations
       to accept when decrypting a message. If not specified, the default
-      value of 1,000 is used. Set to 0 to disable the minimum check.
+      value of 1,000 is used (RFC 7518 §4.8.1.2 floor). Set to 0 to
+      disable the minimum check (NOT RECOMMENDED for untrusted issuers
+      — without a floor, recipients accept arbitrarily-weak password-
+      derived keys and silently spend the producer-chosen amount of
+      crypto work to verify them).
+
+      Threat model: a malicious or careless issuer signs a PBES2-wrapped
+      JWE with a very low p2c (e.g. 100) so they spend almost no CPU on
+      their side, while the recipient happily derives the wrap key with
+      the same low iteration count. The result is an authenticated
+      message whose key-derivation strength is well below industry
+      practice (OWASP 2023 recommends ≥600,000 PBKDF2-SHA256 iterations
+      for password-derived keys; the RFC floor of 1,000 is far below
+      that). For receiver-side hardening against under-iteration, raise
+      WithMinPBES2Count above 1,000.
 
       This option can be used for `jwe.Settings()`, which changes the behavior
       globally, or for `jwe.Decrypt()`, which changes the behavior for that