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

Add DHCP option 121: Classless Static Routes with regression test #3536

Merged
merged 6 commits into from
May 16, 2022

Conversation

coatesamzn
Copy link
Contributor

Description

This PR adds functionality to provide support for DHCP option 121 Classless Static Routes as defined in rfc3442. I ran into situation that required manipulating this DHCP option and found converting raw binary was tiresome for packets with many routes, so this creates a simpler human usable design.

This is done by using a FieldListField, and since rfc3442 defines routes as dynamic length I added a little work to be done to calculate correct number of octets for each route based on prefix. The formatting of the raw bytes is exact to the RFC definition. Also this has been tested in both python2.7 and python3.

Usage

>>> d = DHCP(options=[('classless_static_routes', ["1.2.3.4/31:10.11.12.13", "169.0.0.0/8:10.0.0.1"])])
>>> d
<DHCP  options=[classless_static_routes=['1.2.3.4/31:10.11.12.13', '169.0.0.0/8:10.0.0.1']] |>
>>> d.show2()
###[ DHCP options ]### 
  options= [classless_static_routes=['1.2.3.0/31:10.11.12.13', '169.0.0.0/8:10.0.0.1']]

>>> raw(d)
'y\x0e\x1f\x01\x02\x03\n\x0b\x0c\r\x08\xa9\n\x00\x00\x01'

Regression Test Results

% ./run_tests -t scapy/layers/dhcp.uts -F                   
━ UTScapy - Scapy 2.4.5rc1.dev199 - 3.7.10                                               
 └ Non-root mode                                                                         
 └ Booting scapy...                                                                      
 └ Discovering tests files...                                                            
━ Loading: scapy/layers/dhcp.uts                                                         
passed E40548DD 000.00s BOOTP - misc                                                     
passed E3D749AC 000.00s DHCPOptionsField                                                 
passed 17B5E536 000.01s DHCP - build                                                     
passed 09E80597 000.00s DHCP - dissection                                                
passed B1A86CEC 000.00s DHCPOptions                                                      
passed 6694DB0D 000.21s Check that the dhcpd alias is properly defined and documented    
Campaign CRC=A21E97B0 in 000.23s SHA=EB7F6F8E95FA48D5197924BA77403ECDE5C04EA6            
PASSED=6 FAILED=0                                                                        
✓ All campaigns executed. Writing output...                                              
                                                                                         
DHCP regression tests for Scapy                                                          
━ Run at 19:49:43 from [scapy/layers/dhcp.uts] by UTscapy in 0.2255096435546875          
 └ Passed=6                                                                              
 └ Failed=0                                                                              
                                                                                         
                                                                                         
UTscapy ended successfully                                                               

@codecov
Copy link

codecov bot commented Feb 21, 2022

Codecov Report

Merging #3536 (68ca011) into master (064e0d2) will increase coverage by 0.16%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##           master    #3536      +/-   ##
==========================================
+ Coverage   86.01%   86.18%   +0.16%     
==========================================
  Files         285      285              
  Lines       64983    65017      +34     
==========================================
+ Hits        55896    56033     +137     
+ Misses       9087     8984     -103     
Impacted Files Coverage Δ
scapy/layers/dhcp.py 86.66% <82.35%> (-0.63%) ⬇️
scapy/packet.py 82.13% <0.00%> (+0.07%) ⬆️
scapy/fields.py 91.13% <0.00%> (+0.11%) ⬆️
scapy/volatile.py 85.94% <0.00%> (+0.11%) ⬆️
scapy/utils.py 76.90% <0.00%> (+0.12%) ⬆️
scapy/supersocket.py 58.78% <0.00%> (+0.30%) ⬆️
scapy/contrib/automotive/ecu.py 94.66% <0.00%> (+0.33%) ⬆️
scapy/arch/windows/__init__.py 67.73% <0.00%> (+0.56%) ⬆️
scapy/layers/l2.py 76.58% <0.00%> (+0.70%) ⬆️
scapy/arch/libpcap.py 68.51% <0.00%> (+1.03%) ⬆️
... and 2 more

@coatesamzn
Copy link
Contributor Author

I have a version that removes math to do simple ceiling calculation: (prefix + (8 - 1)) / 8 however I left it with math.ceil for readability. I'm open to removing that dependency.

@KhazAkar
Copy link
Contributor

Hi, thanks for PR.
Can you expand a little those tests, like creation of desired packet out of raw bytes and pcap if you have one in hand? It's not necessary, but great to have :)
About code - can you add type hints where possible?
And about math as dependency - if it's available in Python standard library for 2.7 (it's there in 3.x) then at least I'm fine with it. :)

Copy link
Member

@gpotter2 gpotter2 left a comment

Choose a reason for hiding this comment

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

Hi ! Thanks for the PR. A small comment


route = self.m2i(pkt, s[:route_len])
ret.append(route)
s = s[route_len:]
Copy link
Member

@gpotter2 gpotter2 Feb 24, 2022

Choose a reason for hiding this comment

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

This will get into an infinite loop if route_len is 0. This is CVE material 😠

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't route_len always be minimum 5 since it is 5 + the calculated octets from prefix?

Copy link
Contributor

Choose a reason for hiding this comment

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

If there's possibility to forge packet with broken route_len (one of purposes of using Scapy-forge even invalid packets), then it will become 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initializing to 5 every iteration should prevent any potential weirdness that could result in a zero length. Now it starts at 5 and will increase up to 9.

@coatesamzn
Copy link
Contributor Author

Added type hints in same style as seen in scapy/fields.py

scapy/layers/dhcp.py Outdated Show resolved Hide resolved
@coatesamzn
Copy link
Contributor Author

Added warning for invalid prefix values:

>>> d = DHCP(options=[('classless_static_routes', ["1.2.3.4/127:10.0.0.1"])])
>>> d.show2()                                                                
WARNING: Invalid prefix value: 0x7f 

@guedou
Copy link
Member

guedou commented Mar 27, 2022

Thanks for this PR. To ease the review, could you start by correcting the linting errors https://github.com/secdev/scapy/runs/5558414953?check_suite_focus=true#step:5:1 ?

gpotter2
gpotter2 previously approved these changes May 9, 2022
Copy link
Member

@gpotter2 gpotter2 left a comment

Choose a reason for hiding this comment

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

Rebased & slightly cleaned up the field.

@gpotter2 gpotter2 merged commit f5b7188 into secdev:master May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants