Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Match Netbios responses for hostnames over length 15 #4446

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

lawndoc
Copy link

@lawndoc lawndoc commented Jun 28, 2024

Scapy handles long netbios hostname queries by trimming the hostname to the maximum of 15 characters before sending the request. When a response is sent back, Scapy fails to match the response to the request because it matches the trimmed hostname against the full-length one originally requested.

This PR will trim the original hostname being queried when matching requests in answers.

This is referenced in issue #4443

Copy link

codecov bot commented Jun 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.46%. Comparing base (31b3588) to head (bae22a9).
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4446   +/-   ##
=======================================
  Coverage   81.45%   81.46%           
=======================================
  Files         353      353           
  Lines       84400    84481   +81     
=======================================
+ Hits        68751    68822   +71     
- Misses      15649    15659   +10     
Files Coverage Δ
scapy/fields.py 92.63% <100.00%> (+0.01%) ⬆️

... and 7 files with indirect coverage changes

@@ -193,9 +193,10 @@ def mysummary(self):
)

def answers(self, other):
trimmed_name = self.RR_NAME[:15]
Copy link
Member

@gpotter2 gpotter2 Jun 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR will trim the original hostname being queried when matching requests in answers.

This isn't really doing what you're saying it does. Where you're trimming the answered host name.. I'm unsure how that hostname could be bigger than 15?

Copy link
Author

@lawndoc lawndoc Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went to see what you had changed in your PR where you implemented the query responses, and I assumed that self.RR_NAME is where the query hostname is being saved and that answers is what matches requests to responses. I'm probably making a wrong assumption here because I don't really understand how sr1 finds matches yet.

I'll open it in the debugger tomorrow and gather some screenshots and fix any mistakes I find. Sorry for the confusion.

Copy link
Author

@lawndoc lawndoc Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I may have mixed up which hostname is in which variable. If other.QUESTION_NAME is the original request, then that's the one that would need to be trimmed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the best, and most Scapish-way of achieving this behavior would be to add an any2i() function to the Netbios field that crops the input to the first 15 characters.
This way when building the packet, you'd already see the name being cropped.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I'll look into this and update the PR. Thanks for the input!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I need another nudge. I have added an any2i method to the NetBIOSNameField with a couple of print statements to make sure it is running as expected:

class NetBIOSNameField(StrFixedLenField):
    def __init__(self, name, default, length=31):
        # type: (str, bytes, int) -> None
        StrFixedLenField.__init__(self, name, default, length)

    def any2i(self, pkt, x):
        print(x)
        if len(x) > 15:
            x = x[:15]
        print(x)
        return x

I can see that it is running the code and trimming the field value correctly:

Loremipsumdolorsitamet
Loremipsumdolor

However, the response packet is still not being captured.

I see how it's called in the Packet.__init__() method, and I'm trying to look at other examples in fields.py but I'm a little lost at the moment.

Copy link
Author

@lawndoc lawndoc Jul 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also tried fiddling with the length=31 parameter to change the length of the underlying StrFixedLenField to 15, but that resulted in a malformed packet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, h2i might be more appropriate

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

h2i was what we needed. Thanks for the help! The most recent commit was working for me when testing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great !

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.

None yet

2 participants