Skip to content

netbios: parse every address entry in a name query response - #5090

Merged
gpotter2 merged 1 commit into
secdev:masterfrom
mmadersbacher:netbios-addr-entry-list
Aug 16, 2026
Merged

netbios: parse every address entry in a name query response#5090
gpotter2 merged 1 commit into
secdev:masterfrom
mmadersbacher:netbios-addr-entry-list

Conversation

@mmadersbacher

Copy link
Copy Markdown
Contributor

The bug

NBNSQueryResponse.ADDR_ENTRY is a PacketListField bounded by RDLENGTH, but
NBNS_ADD_ENTRY defines neither extract_padding nor default_payload_class.
PacketListField.getfield moves on to the next element only when the one it just
dissected left a Padding behind (scapy/fields.py:1838-1848); otherwise it sets
remain = b"" and the loop ends. The first entry therefore takes the rest of RDATA
as a Raw payload and the list holds exactly one element, whatever RDLENGTH says.

On master (b36473a), a response carrying two addresses:

>>> z = NBNSHeader(data)
>>> z.RDLENGTH
12
>>> len(z.ADDR_ENTRY)
1
>>> [x.NB_ADDRESS for x in z.ADDR_ENTRY]
['192.168.1.65']
>>> z.ADDR_ENTRY[0].payload
<Raw  load=b'\x00\x00\xc0\xa8\x01B' |>

data is the POTATO name query response already in
test/scapy/layers/netbios.uts, with a second address entry appended and
RDLENGTH adjusted. 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 however
many were answered. Nothing raises.

The fix

Give NBNS_ADD_ENTRY the default_payload_class that
NBNSNodeStatusResponseService already carries further down the same file. That
class is the structural counterpart, its NODE_NAME list works, and the existing
node 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:

master:  6161 passed / 96 failed
branch:  6162 passed / 96 failed

The 96 are pre-existing here and environmental (no tshark, no tcpdump, no loopback
adapter, no scapy-rpc extension). I diffed the failing set by name: identical on
both 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:

###(002)=[failed] NBNSQueryResponse - name registered on several addresses
>>> assert len(z.ADDR_ENTRY) == 2
AssertionError

flake8 scapy/, mypy_check.py linux, mypy_check.py win32 and codespell all
exit 0.

Beyond the test I checked RDLENGTH values that lie in both directions, an
RDLENGTH that is not a multiple of 6, RDLENGTH 0, and the OSS-Fuzz sample already
in 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 Raw in
the list, and RDLENGTH 0 gives an empty list.

One thing worth flagging

conf.max_list_count (100) now applies to ADDR_ENTRY. The collapse made it
unreachable before, so a response claiming more than 100 entries used to yield one
entry and now drops back to Raw. NBNSNodeStatusResponse.NODE_NAME behaves the
same way at 101 entries and DNS raises MaximumItemsCount in the same situation,
so this puts ADDR_ENTRY in the regime the other list fields are already in. raw()
round-trips in that case too.

Scope

I left the other layers alone. ICMPv6MLReport2.records and
HCI_Cmd_LE_Set_Extended_Advertise_Enable.sets collapse the same way, three and two
elements 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.

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

codecov Bot commented Aug 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.58%. Comparing base (b36473a) to head (5e3b99d).

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           
Files with missing lines Coverage Δ
scapy/layers/netbios.py 76.40% <100.00%> (+0.26%) ⬆️

... and 3 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 Aug 16, 2026

Copy link
Copy Markdown
Member

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

@gpotter2
gpotter2 merged commit b1cda72 into secdev:master Aug 16, 2026
23 checks passed
@mmadersbacher

Copy link
Copy Markdown
Contributor Author

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!

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