Hpke

[hhund]

No description provided.

[github-advanced-security]

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

• The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
• Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
• You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

[schwzr]

Claude says:

Architecture overview

An RFC 9180 implementation (modes 0 and 1) with a custom chunk-based wire format for streaming large payloads:

[MAGIC "HPKEF"][version 0x01][ProtocolV1 header][encapsulation][chunk0]...[chunkN]

Layer	Files	Responsibility
Wire format	ProtocolFactory, ProtocolV1	versioned header with magic
KEM	KemId, DhKemWrapper, RsaKemWrapper, AbstractKemWrapper	encap/decap
Key schedule	KeySchedule	RFC 9180 §5.1
AEAD + chunking	AeadId, ChunkedInputStreamEnumeration, ChunkLength	per-chunk crypto
Streaming	SequenceInputStreamForRuntimeIOException, RuntimeIOException	exception bridging
Key lookup	KeyProvider, PreSharedKeyProvider, ReceiverPrivateKeyProvider	ID → key

Strengths

1. RFC 9180 §5.1 key schedule is faithful (KeySchedule.java:54-88): correct suite_id construction, correct labels (HPKE-v1, psk_id_hash, info_hash, secret, key, base_nonce), and key_schedule_context = mode ‖ psk_id_hash ‖ info_hash. The RFC test-vector test (commit ef1acb1) is very valuable.
2. Defense-in-depth length validation in AbstractKemWrapper.java:32-36,56-57 — encapsulation and shared-secret lengths are checked against KemId after every KEM call.
3. Truncation protection via AAD = [sequence][finished flag]. The finished flag cryptographically binds the fact that the final chunk really is the last one; if the tail is dropped, the tag check fails rather than silently truncating. HpkeTest.decryptionTruncatedStreamTest exercises this systematically.
4. Empty-plaintext handling (ChunkedInputStreamEnumeration.java:86-90,110-111): emits a single empty final chunk so that even empty messages produce an AEAD tag — prevents silent acceptance of a zero-byte stream.
5. Chunk length bound to the key schedule (commit 7dd5a0d, ProtocolV1.getKdfInfo()): different chunk sizes derive different keys, so an attacker cannot reinterpret a stream encrypted with 1 KiB chunks as 32 MiB chunks.
6. Sequence overflow raises RuntimeIOException("Message limit reached") (ChunkedInputStreamEnumeration.java:212) — critical, because nonce reuse would be catastrophic for GCM.
7. Versioned wire format via ProtocolFactory.ProtocolSerializer with a mandatory "no duplicate version" check.
8. PSK-id and receiver-key-id fixed at exactly 32 bytes and stored in the header — clear, unambiguous selection.

Findings

🟡 MEDIUM — Hpke.decrypt reads the encapsulation with read(byte[]) instead of readNBytes

Hpke.java:197-198:

byte[] encapsulation = new byte[protocol.getKemId().getEncapsulationLength()];
ByteEncoding.expectRead(protocol.getKemId().getEncapsulationLength(), encrypted.read(encapsulation));

InputStream.read(byte[]) is allowed to return fewer bytes than the buffer length (e.g. on slow network streams) without indicating EOF. That triggers a false-positive "Truncated stream". ProtocolV1.from already does this correctly with readNBytes. Fix: use the same pattern here:

byte[] encapsulation = encrypted.readNBytes(protocol.getKemId().getEncapsulationLength());
ByteEncoding.expectRead(protocol.getKemId().getEncapsulationLength(), encapsulation.length);

🟡 MEDIUM — RSA-KEM uses private-use IDs and should be documented explicitly

KemId uses IDs 0xFF10–0xFF13 for RSA-KEM. RFC 9180 §7.1 reserves 0xFF00–0xFFFF as private use and does not define an RSA-KEM — another HPKE implementation will not interoperate. In addition, the RSA decapsulation via RSA/ECB/NoPadding (raw RSA) plus KDF2 (RsaKemWrapper.java:72-83) is a deliberate custom construction that only becomes IND-CCA2 through the downstream AEAD tag. Both points should be called out explicitly in the Hpke / KemId Javadoc — otherwise a future consumer may assume RFC conformance where there is none.

🟡 MEDIUM — RuntimeIOException.getCause() violates the Throwable contract

RuntimeIOException.java:20-26:

@Override
public synchronized IOException getCause() {
    if (super.getCause() != null)
        return (IOException) super.getCause();
    else
        return new IOException(getMessage());
}

Throwable.getCause() is allowed to return null per its contract; manufacturing a fresh IOException on every call surprises tooling (logger stack traces, test assertions on identical causes). Recommendation: introduce a dedicated asIOException() method and leave getCause() with its default behavior.

🟡 MEDIUM — KemId.isKeySupported checks RSA bitLength() for exact equality

KemId.java:46:

key instanceof RSAKey rsaKey && rsaKey.getModulus().bitLength() == length

A nominally 2048-bit key can have bitLength() == 2047 whenever the top bit of the modulus happens to be zero (roughly 50% probability when the second-highest bit is not forced during generation). Keys produced locally via KeyPairGeneratorFactory are fine; externally imported keys (PEM, HSM) may be rejected incorrectly. A more lenient check would be >= length-1 && <= length, or compare ((length+7)/8) * 8 to ((bitLength+7)/8) * 8.

🟢 LOW — Test cosmetic: duplicated unit in display name

HpkeTest.java:58: protocol.getChunkLength() + " KiB" produces e.g. "MiB_32 KiB" — the enum constant already carries the unit in its name.

🟢 LOW — Mode.from(BASE_VALUE, somePsk) silently ignores the supplied pskId

If a caller accidentally constructs BASE mode with a pskId there is no signal. Not reachable through the current reader path, but the API is permissive. Not urgent.

🟢 LOW — ChunkedInputStreamEnumeration.loadNextChunk always allocates a full byte[chunkLength]

Even when the final chunk is small, the full buffer is allocated and copied to lastChunk (:167-168). Noticeable for 32 MiB chunks and small payloads. Would not optimize without a concrete need.

🟢 LOW — SequenceInputStreamForRuntimeIOException does not cover all methods

available(), skip(long), close() are not overridden. In practice these don't raise RuntimeIOException because they only inspect the current InputStream's state. Just noting it.

🟢 LOW — KeySchedule.executeKeySchedule instantiates KDF twice

KeySchedule.java:60,71: kdfId.toKdf() is called twice. Presumably because of JCE KDF internal-state semantics. Not a bug — micro-overhead.

Security argument (cryptographic binding of the header)

Header tampering is detected as follows:

Tampered field	Consequence
MAGIC HPKEF	IOException("Protocol not supported") in ProtocolFactory.read
Version	IOException("Protocol not supported")
Mode	key-schedule context byte changes → different keys → AEAD tag fails
KEM/KDF/AEAD id	feeds into suite_id → different keys → tag fails
chunkLengthExponent	now in KDF info (commit 7dd5a0d) → different keys → tag fails
receiverKeyId	key lookup fails or returns a different key → decapsulation/tag fails
PSK id (mode 1)	feeds into psk_id_hash → tag fails

testModifiedMessages exercises these paths systematically — very good.

Summary

The feature is solidly built: RFC conformance, defense-in-depth length checks, truncation protection, clean separation of wire format and crypto, good test coverage. The custom extensions (chunked format, RSA-KEM suite) are deliberate. Highest-priority concrete action: the read(byte[]) vs readNBytes bug in Hpke.java:197-198, which breaks streamed input. After that: a Javadoc note about RSA-KEM being private-use, and the getCause() contract violation in RuntimeIOException.