Skip to content

spnego: don't synthesise a GSSAPI blob from an empty token - #5170

Closed
itzzdev09 wants to merge 1 commit into
secdev:masterfrom
itzzdev09:fix/spnego-empty-token
Closed

spnego: don't synthesise a GSSAPI blob from an empty token#5170
itzzdev09 wants to merge 1 commit into
secdev:masterfrom
itzzdev09:fix/spnego-empty-token

Conversation

@itzzdev09

Copy link
Copy Markdown

The bug

Dissecting an empty OCTET STRING builds a sub-packet from b"". Constructing an ASN1_Packet from empty bytes materialises that class's defaults, so GSSAPI_BLOB(b"") yields the 10-byte SPNEGO OID header:

>>> raw(SPNEGO_Token())
b'\x04\x00'
>>> raw(SPNEGO_Token(raw(SPNEGO_Token())))
b'\x04\n\x60\x08\x06\x06\x2b\x06\x01\x05\x05\x02'

An empty mechToken / mechListMIC is legal, so parsing one off the wire and re-serialising it invented ten bytes that were never there. SPNEGO_MechListMIC had the same behaviour. Anything that parses and re-emits SPNEGO — relaying, rewriting, pcap round-trips — silently changed the bytes.

The fix

Two sites, one cause:

  • _SPNEGO_Token_Field.m2i returns the raw value when it is empty. It returns dat.val rather than the ASN1_STRING because i2m() calls bytes() on the result, and bytes() of an ASN1_STRING re-encodes the BER header instead of yielding b"" — returning the string double-wraps to \x04\x02\x04\x00.
  • ASN1F_STRING_ENCAPS.m2i keeps the empty ASN1_STRING instead of calling self.cls(b""). This is the generic fix; SPNEGO_MechListMIC reaches it, as do the kerberos and x509 users of that field. ASN1F_STRING_PacketField.i2m already accepts either a packet or a raw string, so the empty case round-trips without further change.

Non-empty values are untouched — a token with content still dissects to GSSAPI_BLOB or the registered mech dissector.

Tests

Four cases added to test/scapy/layers/spnego.uts: the two empty round-trips, a non-empty token still yielding GSSAPI_BLOB, and the negTokenInit / negTokenResp containers. Two of the four fail on master; the other two are guards that pass in both arms, confirming existing behaviour is preserved.

Run on 190b4ba3 with the branch applied, and the same suites on an unmodified master worktree for comparison:

suite master this PR
spnego 17 passed, 0 failed 21 passed, 0 failed
kerberos 97 passed, 0 failed 97 passed, 0 failed
ntlm 26 passed, 0 failed 26 passed, 0 failed
smb2 39 passed, 0 failed 39 passed, 0 failed
x509 13 passed, 46 failed 13 passed, 46 failed

x509's 46 failures are pre-existing on master in this checkout — a newer cryptography (50.0.1) than it supports — and are byte-identical in both arms. I'm flagging them rather than hiding them; they are not caused by this change.

flake8 output on both touched modules is identical to master (7 pre-existing F401s in both, unchanged).


Disclosure per CONTRIBUTING: prepared with AI assistance (Claude Code, Claude Opus 5). The bug was found by a build/dissect round-trip sweep over scapy's packet classes, and every number above comes from actually running the suites, in both arms, rather than from inspection.

🤖 Generated with Claude Code

Comment thread scapy/asn1fields.py Outdated
Comment thread scapy/layers/spnego.py Outdated
@gpotter2
gpotter2 force-pushed the fix/spnego-empty-token branch 2 times, most recently from 40cdd99 to 66142e2 Compare September 11, 2026 19:17
Dissecting an empty OCTET STRING built a sub-packet from b"". Constructing
an ASN1_Packet from empty bytes materialises that class's defaults, so
GSSAPI_BLOB(b"") yields the 10-byte SPNEGO OID header:

    >>> raw(SPNEGO_Token())
    b'\x04\x00'
    >>> raw(SPNEGO_Token(raw(SPNEGO_Token())))
    b'\x04\n\x60\x08\x06\x06\x2b\x06\x01\x05\x05\x02'

AI-Assisted: yes (Claude Code, Claude Opus 5)

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.27%. Comparing base (4f35d82) to head (66142e2).

Files with missing lines Patch % Lines
scapy/asn1fields.py 0.00% 2 Missing ⚠️
scapy/layers/spnego.py 0.00% 2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (4f35d82) and HEAD (66142e2). Click for more details.

HEAD has 9 uploads less than BASE
Flag BASE (4f35d82) HEAD (66142e2)
11 2
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #5170       +/-   ##
===========================================
- Coverage   80.83%   47.27%   -33.57%     
===========================================
  Files         391      373       -18     
  Lines       97801    97437      -364     
===========================================
- Hits        79056    46059    -32997     
- Misses      18745    51378    +32633     
Files with missing lines Coverage Δ
scapy/asn1fields.py 56.17% <0.00%> (-31.70%) ⬇️
scapy/layers/spnego.py 22.95% <0.00%> (-60.28%) ⬇️

... and 343 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gpotter2

gpotter2 commented Sep 11, 2026

Copy link
Copy Markdown
Member

Hi. Tests don't seem to pass. Considering this is all AI I will close. Please reopen once tests pass.

@gpotter2 gpotter2 closed this Sep 11, 2026
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.

2 participants