fix(kerberos): use minimal DER for KDC-REQ nonce and AP-REP seq-number - #759
Benoît Cortier (CBenoit) merged 2 commits into
Conversation
The Kerberos client sent random nonces in the KDC-REQ-BODY without minimal
DER encoding. The bytes were copied as-is, so a nonce starting with 00 followed
by a byte below 0x80, or FF followed by a byte of 0x80 or above, was not valid
DER. The Windows KDC checks the TGS-REQ authenticator checksum against its own
re-encoding of the body. With such a nonce the two differ, and about 1 in 128
TGS-REQs failed with KRB_AP_ERR_MODIFIED (SEC_E_MESSAGE_ALTERED). Nonces are
now generated in the positive Int32 range, as MIT krb5 and Heimdal do, and
encoded as minimal DER. This covers the AS-REQ, TGS-REQ, change-password, PKU2U
and PKINIT authenticator nonces. The value is fixed when the nonce is generated,
not when it is encoded, so PKU2U's check that the AS-REP returns the same nonce
still holds.
The client also required the AP-REP seq-number to be exactly 4 bytes. The
Windows acceptor sends it as minimal DER, so values below 0x800000 arrive in 3
bytes or fewer, and about 1 in 256 AP-REPs failed with SEC_E_INVALID_TOKEN
("invalid ApRep sequence number"). The parser now accepts 1 to 4 bytes,
sign-extending short negative values, as well as 5 bytes with a leading 00.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The Kerberos server’s outbound AS-REQ path still generates unrestricted nonces that can exceed Windows’ signed Int32 range.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Fixes intermittent Kerberos failures caused by non-minimal DER nonce encoding and variable-width AP-REP sequence numbers.
Changes:
- Generates Windows-compatible positive
Int32nonces and encodes them minimally. - Accepts valid 1–5-byte AP-REP sequence numbers.
- Adds nonce and sequence-number regression tests.
| File | Description |
|---|---|
src/pku2u/mod.rs |
Uses shared nonce generation for PKU2U. |
src/pku2u/generators.rs |
Updates AP-REP encoding commentary. |
src/pk_init.rs |
Minimally encodes PKINIT nonces. |
src/kerberos/client/mod.rs |
Uses constrained AS-REQ and PKINIT nonces. |
src/kerberos/client/generators.rs |
Adds nonce generation, minimal encoding, and tests. |
src/kerberos/client/extractors.rs |
Parses variable-width sequence numbers and adds tests. |
src/kerberos/client/change_password.rs |
Uses constrained big-endian nonces. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The previous change encoded nonces as unsigned minimal DER, so a nonce with the high bit set became a 5-byte INTEGER. The Windows KDC rejects such a request outright. The Kerberos acceptor's user-to-user TGT request still used an unrestricted random nonce and would fail about half the time, as would any caller of the public GenerateAsReqOptions passing raw random bytes. Nonces are now encoded as signed minimal DER, matching how Windows re-encodes them: values from generate_nonce are unchanged, and a nonce with the high bit set stays a 4-byte INTEGER as it was before. The user-to-user AS-REQ now uses generate_nonce, and the PKINIT authenticator nonce uses the same encoder. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| /// Encodes a 4-byte big-endian nonce as a minimal DER INTEGER in the `Int32` range. | ||
| /// | ||
| /// The TGS-REQ authenticator checksum covers the DER encoded KDC-REQ-BODY and the Windows KDC | ||
| /// verifies it against its own re-encoding of the body. Random bytes used as-is are not minimal | ||
| /// DER when they start with `00` followed by a byte below `0x80`, or `FF` followed by a byte of | ||
| /// `0x80` or above. The re-encoded body then differs and the KDC fails the request with | ||
| /// `KRB_AP_ERR_MODIFIED`. | ||
| /// | ||
| /// The bytes are encoded as a signed value, the way Windows re-encodes the nonce. A nonce with the | ||
| /// high bit set stays a 4-byte negative INTEGER, an unsigned encoding would add a fifth `00` octet | ||
| /// and the Windows KDC rejects the request outright. Nonces from [generate_nonce] are positive and | ||
| /// encode the same either way. | ||
| pub(crate) fn nonce_to_asn1(nonce: &[u8]) -> IntegerAsn1 { | ||
| IntegerAsn1::from_bytes_be_signed(nonce.to_vec()) | ||
| } |
There was a problem hiding this comment.
nit: Instead of this helper, consider making GenerateAsReqOptions::nonce a u32.
All nonce-generation call sites now produce a u32 and immediately convert it to big-endian bytes, while the request builder converts those bytes back into a DER INTEGER. Keeping the nonce as a u32 through the options would make the positive Int32 contract explicit and avoid the repeated to_be_bytes() conversions:
pub nonce: u32The request builder could then perform the single DER conversion at the encoding boundary.
This is probably better kept separate from this bug fix because changing the options type may affect external callers, but it would also remove the need to have to think about calling nonce_to_asn1 at all.
There was a problem hiding this comment.
It does sounds like a good follow up change but I'll leave it up to your as to whether you want to do this here or not.
Benoît Cortier (CBenoit)
left a comment
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
Thank you Jordan Borean (@jborean93)
I left a few minor comments, but otherwise LGTM.
cc Pavlo Myroniuk (@PavloMyroniuk-apriorit)
Does it look good to you too?
Pavlo Myroniuk (TheBestTvarynka)
left a comment
Pavlo Myroniuk (TheBestTvarynka)
left a comment
There was a problem hiding this comment.
LGTM. Thank you!
Benoît Cortier (CBenoit)
left a comment
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
Thank you, LGTM on my side as well
c724a94
into
Devolutions:master
|
Thanks for the review, appreciate you looking through it!. I'll have a test on #762 as well and comment there in case something else regressed there. |
Summary
Kerberos logons through sspi-rs against a Windows domain fail intermittently.
Two separate DER INTEGER bugs in the Kerberos client cause this. Each one
depends on a random value, so the failures follow a steady rate and have
nothing to do with concurrency or shared state.
SEC_E_MESSAGE_ALTERED(0x8009030F), "message stream modified"InitializeSecurityContext(TGS exchange)KRB_AP_ERR_MODIFIED)SEC_E_INVALID_TOKEN(0x80090308), "invalid ApRep sequence number"InitializeSecurityContext(processing the AP-REP)0x800000)Changes
generate_noncereturns a random value in the positiveInt32range, as MIT krb5 and Heimdal do. It is used for the AS-REQ, TGS-REQ, change-password and PKU2U nonces and the PKINIT authenticator nonce. The value is fixed at generation, not while encoding, so PKU2U's check that the AS-REP returns the same nonce still holds.IntegerAsn1::from_bytes_be_unsigned, which produces minimal DER. The PKINITpaChecksumalso covers the AS-REQ body, so the AS-REQ nonce needs to be correct too.extract_seq_number_from_ap_repnow accepts 1 to 4 bytes (sign-extending short negative values from signedInt32encoders) or 5 bytes with a leading00(unsignedUInt32encoders). Anything else is still rejected.Testing
EncApRepPartwith a 3-byte seq-number).cargo test --features network_client,dns_resolver,scard,__test-data, CI's clippy command andcargo fmt --checkall pass.masterMESSAGE_ALTERED, 1×invalid ApRep sequence number: [50, 188, 157])