apple / apple/swift-crypto

X-Wing integrityCheckedRepresentation initializer should validate length like ML-DSA does

Closed Beginner friendly
#443 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
Assembly
Stars
1.7k
Forks
223
Avg merge
1d 2h
Merged PRs (30d)
7

Description

### Summary

The `integrityCheckedRepresentation` initializer for X-Wing keys does not validate the length of the input representation before processing it, unlike the equivalent ML-DSA initializer which includes an explicit length guard. Both types follow the same pattern - reconstruct a key from a representation that includes an integrity-check suffix - but only ML-DSA validates the input length upfront.

### ML-DSA (has the guard - `MLDSA.swift`):

```swift
init(integrityCheckedRepresentation: some DataProtocol) throws {
guard integrityCheckedRepresentation.count == seedSize + 32 else {
throw CryptoKitError.incorrectParameterSize
}
// ... proceeds with dropLast(32) / dropFirst(32)
}
```

### X-Wing (missing the guard - `XWing.swift:121-127`):

```swift
init(integrityCheckedRepresentation: some DataProtocol) throws {
// No length check here
let seedRepresentation = integrityCheckedRepresentation.dropLast(32)
let hashSuffix = integrityCheckedRepresentation.suffix(32)
// ... proceeds to hash and compare
}
```

The X-Wing path is incidentally safe because `SHA3_256Digest(bytes:)` tolerates arbitrary-length input and the downstream seed initializer at line 178 validates `privateKeyBytes.count == XWING_PRIVATE_KEY_BYTES`. However, the missing guard is an inconsistency: if the downstream validation ever changes, the missing length check becomes a latent bug. More immediately, passing a wrong-length representation to the X-Wing initializer silently computes a hash comparison on incorrect data before eventually failing at the seed init, rather than failing fast with `incorrectParameterSize`.

### Proposed fix

Add the same length guard to match ML-DSA's pattern:

```swift
init(integrityCheckedRepresentation: some DataProtocol) throws {
guard integrityCheckedRepresentation.count == /* expected seed size */ + 32 else {
throw CryptoKitError.incorrectParameterSize
}
let seedRepresentation = integrityCheckedRepresentation.dropLast(32)
let hashSuffix = integrityCheckedRepresentation.suffix(32)
// ... rest unchanged
}
```

Replace `/* expected seed size */` with the appropriate constant (likely `XWING_PRIVATE_KEY_BYTES` or the corresponding seed byte count - verify against the X-Wing spec).

### Impact

Consistency / defense-in-depth. Not a vulnerability - the downstream seed init catches wrong-length input. Adding the upfront guard makes the X-Wing path fail fast with a clear error and matches the ML-DSA pattern, reducing the risk of a future regression.

### References

- The ML-DSA equivalent guard for comparison
- X-Wing spec (draft-connolly-cfrg-xwing-kem)

Contributor guide

Open the contributing guide

Research direction

Start in XWing.swift:121-127 and compare the initializer with the guard in MLDSA.swift. Verify the expected representation length against XWing.swift:178 and the X-Wing specification, then add the upfront validation so incorrect-length inputs fail with incorrectParameterSize before hashing.

Written by the indexing model from the issue text.

Assessment

Tech stack
swift
Domain
cryptography
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
74/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.