Skip to content

Keep every message of a SASL buffer when decoding sealed GSSAPI streams - #160

Open
marbas207 wants to merge 1 commit into
inejge:masterfrom
marbas207:fix-sasl-decode
Open

marbas207 wants to merge 1 commit into
inejge:masterfrom
marbas207:fix-sasl-decode

Conversation

@marbas207

Copy link
Copy Markdown

Problem

With SASL GSSAPI sealing, LdapCodec::decode can silently discard decrypted LDAP messages, and the operation waiting for them never completes. Against Active Directory it shows up as an intermittent hang: a paged search waits forever on an idle connection.

The current decoder unwraps one SASL buffer and decodes one LDAP message from it. The rest of the plaintext is kept only if buf is empty, meaning nothing more has been read from the socket. Otherwise it's dropped:

let res = decode_inner(&mut decoded);
if res.is_ok() && !decoded.is_empty() && buf.is_empty() {
    buf.extend(decoded);
    self.has_decoded_data = true;
}
res

Data is lost in three cases:

  1. Several LDAP messages in one SASL buffer, with more raw data already read. AD seals a whole search page as one SASL buffer (about 1.1 MB in our case: hundreds of SearchResultEntry messages plus SearchResultDone). If the reply to another operation on the same connection is already buffered behind it, the rest of the page is dropped, including SearchResultDone, and the search never completes.
  2. One LDAP message split across two SASL buffers. The partial plaintext is dropped, or appended to the raw buffer and later misread as a SASL length prefix.
  3. Fewer than 4 bytes buffered. That's a normal partial read, but it returns Err("invalid SASL buffer") instead of Ok(None).

Reproduction

Windows client (SSPI, current user) against an AD domain controller (Server 2016/2019), plain LDAP with sealing:

  • a paged subtree search of the domain (*, 999 per page);
  • on the same connection, at the same time, a second, small search (attributes 1.1).

9 of 15 runs hung. Instrumenting the unmodified decoder showed every hang logging, for example, "dropping 1146456 of 1148022 plain bytes (more messages in the frame) because 118496 raw bytes are buffered". Without sealing (LDAPS) the code path isn't used and nothing hangs.

Fix

  • has_decoded_data: bool is replaced by decoded: BytesMut on the codec, which holds unwrapped plaintext separately from the raw sealed bytes.
  • decode_sasl returns every complete message from decoded before unwrapping the next SASL buffer, and appends each buffer's plaintext so a message can span buffers.
  • It returns Ok(None) on a partial length prefix or a partial buffer.

The decode logic takes the unwrap step as a closure, so it's unit-tested without a GSSAPI context:

  • several messages per buffer, with more raw data already buffered;
  • a message split across buffers, fed in every chunk size from 1 byte to the whole stream;
  • a partial length prefix.

cargo test --lib --no-default-features --features gssapi passes (31 tests), as does cargo test with default features.

With the fix, 48 of 48 runs against AD completed: 40 sealed, within one domain and across a parent/child trust, plus 8 over LDAPS.

With GSSAPI sealing, LdapCodec::decode unwrapped one SASL buffer, decoded
one LDAP message from it, and kept the rest of the plaintext only if no
more raw bytes had been read from the socket yet. Otherwise the rest was
dropped. A message continued in the next buffer was lost too, and fewer
than four buffered bytes was reported as an error.

Active Directory seals a whole search page (a megabyte and more, hundreds
of messages) as one buffer. When the reply to another operation on the
same connection was read along with it, the rest of the page, its
SearchResultDone included, was discarded and the search waited forever.

Keep the unwrapped plaintext in its own buffer on the codec, separate from
the raw sealed bytes. Return every complete message from it before
unwrapping the next SASL buffer, append the next buffer's plaintext so
messages can span buffers, and wait for more data on a partial length
prefix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant