netbios: parse every address entry in a name query response - #5090
Conversation
NBNS_ADD_ENTRY never ended itself, so the first entry took the rest of RDATA as its payload and PacketListField stopped after one pass. A name with several owners came back as one entry followed by a Raw, and nbns_resolve() handed the caller only the first address. NBNSNodeStatusResponseService already yields Padding for the same reason. AI-Assisted: no
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5090 +/- ##
=======================================
Coverage 80.58% 80.58%
=======================================
Files 390 390
Lines 96800 96802 +2
=======================================
+ Hits 78006 78008 +2
Misses 18794 18794
🚀 New features to boost your workflow:
|
|
Hi. Thanks for your interest in Scapy ! The PR looks good, but the details you provided look awfully like they were AI-generated. If that's the case, please note that it's against the contribution guidelines to not disclose it. Consider this a warning. If you did not use AI, please excuse my reaction. I would advise to make less verbose reports in the future :) Have a good week |
Hey. Im really sorry that I gave you such a hard time. I am relatively young and quite new, lets say junior level, in Cybersecurity, coding, etc... I study everyday outside of school - just in my free-time. Next to my job. I don't wanna yap about my boring life, but I hope you understand some of my qualities have a shortage in quality since its just my passion and not profession yet. I gladly accept your feedback though. Now that you mention it its actually horrible to read lol. I didn't use AI at all besides to help me phrase it. Is it necessary to flag that as whole AI? I feel like it discredits my work if I flag this as "AI" since its just for the phrase and not the quality itself - please lmk! Have a nice one! |
The bug
NBNSQueryResponse.ADDR_ENTRYis aPacketListFieldbounded byRDLENGTH, butNBNS_ADD_ENTRYdefines neitherextract_paddingnordefault_payload_class.PacketListField.getfieldmoves on to the next element only when the one it justdissected left a
Paddingbehind (scapy/fields.py:1838-1848); otherwise it setsremain = b""and the loop ends. The first entry therefore takes the rest of RDATAas a
Rawpayload and the list holds exactly one element, whateverRDLENGTHsays.On master (
b36473a), a response carrying two addresses:datais the POTATO name query response already intest/scapy/layers/netbios.uts, with a second address entry appended andRDLENGTHadjusted. I do not have a capture of a real multi-address answer.RFC 1002 sect 4.2.13 describes RDATA as "a sequence of zero or more ADDR_ENTRY
records. Each ADDR_ENTRY record represents an owner of a name. For group names there
may be multiple entries. However, the list may be incomplete due to packet size
limitations." Querying a group name, a 0x1C domain controller name for instance, is
the case that answers with several.
The consequence reaches the API:
nbns_resolve()builds its return value with[x.NB_ADDRESS for x in res.ADDR_ENTRY], so a caller gets one address out of howevermany were answered. Nothing raises.
The fix
Give
NBNS_ADD_ENTRYthedefault_payload_classthatNBNSNodeStatusResponseServicealready carries further down the same file. Thatclass is the structural counterpart, its
NODE_NAMElist works, and the existingnode status test already asserts five entries come back from it. The omission looks
like an oversight rather than a decision.
Bytes are untouched.
raw()reproduces the input before and after the change,including when trailing data follows the record, so this changes how the record is
presented and not what is written.
Verification
Full Windows suite, same machine, master versus this branch:
The 96 are pre-existing here and environmental (no tshark, no tcpdump, no loopback
adapter, no
scapy-rpcextension). I diffed the failing set by name: identical onboth sides, nothing new and nothing fixed by accident. The one extra pass is the new
test.
Reverting only the source change and keeping the test fails exactly the new case, so
it binds to the defect and not to the implementation:
flake8 scapy/,mypy_check.py linux,mypy_check.py win32andcodespellallexit 0.
Beyond the test I checked
RDLENGTHvalues that lie in both directions, anRDLENGTHthat is not a multiple of 6,RDLENGTH0, and the OSS-Fuzz sample alreadyin the test file. None of them raises and
raw()reproduces the input in every case.Where RDATA does not divide into whole entries the remainder comes back as a
Rawinthe list, and
RDLENGTH0 gives an empty list.One thing worth flagging
conf.max_list_count(100) now applies toADDR_ENTRY. The collapse made itunreachable before, so a response claiming more than 100 entries used to yield one
entry and now drops back to
Raw.NBNSNodeStatusResponse.NODE_NAMEbehaves thesame way at 101 entries and
DNSraisesMaximumItemsCountin the same situation,so this puts
ADDR_ENTRYin the regime the other list fields are already in.raw()round-trips in that case too.
Scope
I left the other layers alone.
ICMPv6MLReport2.recordsandHCI_Cmd_LE_Set_Extended_Advertise_Enable.setscollapse the same way, three and twoelements coming back as one, and there are further candidates I have not verified.
Those are separate changes. Happy to follow up if you want them.