IKEv2: improve dissection of IKEv2 redirect notifications #3779

mspncp · 2022-11-05T13:04:41Z

Commits in chronological order. ~~The first one is a preparation for the second, main commit.~~(update: squashed into one)

Still a draft, because the pull request is based on the unmerged pull request #3766 and because tests are still missing.

commit 733ba89e4ec0f3fd2e0b19d9788afd27a7fdef46

IKEv2: improve dissection of IKEv2 redirect notifications

This commit adds a dispatch hook for the IKEv2_payload_Notify class
to support adding specialized classes for specific notifications
for which the payload can be dissected further.

As a first use-case, it adds two new notification classes

- IKEv2_payload_Notify_Redirect
- IKEv2_payload_Notify_Redirected_from

See RFC 5685, sections 9.2 and 9.3

Checklist:

I added unit tests
I executed the regression tests (using cd test && ./run_tests or tox)

mspncp · 2022-11-05T13:06:48Z

The isotp_packet test currently fails on my machine, not only on this pull branch, also on the master branch:

######
## ISOTP fragment and defragment checks
######


###(046)=[failed] Fragment exception

>>> ex = False
>>> try:
...     fragments = ISOTP(b"a" * (1 << 32)).fragment()
... except Scapy_Exception:
...     ex = True
... 
Traceback (most recent call last):
  File "<input>", line 3, in <module>
MemoryError

The isakmp, ikev2, and ipsec tests succeed, however.

codecov · 2022-11-05T13:30:28Z

Codecov Report

Merging #3779 (4a0a902) into master (2fd2524) will increase coverage by 34.61%.
The diff coverage is 96.07%.

❗ Current head 4a0a902 differs from pull request most recent head 693de59. Consider uploading reports for the commit 693de59 to get more accurate results

@@             Coverage Diff             @@
##           master    #3779       +/-   ##
===========================================
+ Coverage   49.97%   84.59%   +34.61%     
===========================================
  Files         228      296       +68     
  Lines       53157    62179     +9022     
===========================================
+ Hits        26565    52600    +26035     
+ Misses      26592     9579    -17013

Impacted Files	Coverage Δ
scapy/config.py	`80.47% <ø> (+23.44%)`	⬆️
scapy/layers/ipsec.py	`88.86% <92.30%> (+59.99%)`	⬆️
scapy/layers/ikev2.py	`94.17% <100.00%> (ø)`
scapy/layers/isakmp.py	`89.67% <100.00%> (+36.93%)`	⬆️
scapy/contrib/homeplugsg.py	`100.00% <0.00%> (ø)`
scapy/layers/tls/crypto/kx_algs.py	`100.00% <0.00%> (ø)`
scapy/contrib/automotive/obd/pid/pids_60_7F.py	`100.00% <0.00%> (ø)`
...contrib/scada/iec104/iec104_information_objects.py	`100.00% <0.00%> (ø)`
scapy/contrib/automotive/uds.py	`95.40% <0.00%> (ø)`
scapy/contrib/automotive/uds_logging.py	`71.56% <0.00%> (ø)`
... and 247 more

mspncp · 2022-11-05T14:21:11Z

@gpotter2 I have a general question to you (and/or the other maintainers):

As you can see here

IKEv2_payload_Notify_Redirected_from
IKEv2_payload_Notify_Redirected_from

the packet names in the ikev2 (and the isakmp) module tend to be terribly long, due to the redundand ...payload_... infixes. They serve no useful purpose (it's obvious that they are payloads), they just add a lot of noise to the output (in particular since there are a lot of payloads) and are tedious to type. Such infixes are used nowhere else in Scapy.

I would very much like to shorten the names by removing this useless infix (s/IKEv2_payload_/IKEv2_/g). In #3766 (comment) you wrote that everything is allowed for dissectors in 'contrib', so you would probably don't mind. Unfortunately, the ikev2 module is a decendant of the isakmp which has been an official layer for a long time. So changing the names in ikev2 only would be inconsistent.

What is your preferred choice?

shorten the payload names in both modules ikev2 and isakmp
shorten the payload names in module ikev2 only
leave the names as they are.

(I probably already know your answer, but I had to ask nevertheless...)

current names

shortened names

mspncp · 2022-11-06T20:25:18Z

Last force-push fixes the flake8 E501 errors.

gpotter2 · 2022-11-07T08:29:47Z

Current ikev2 layer is a mess, and really not up to what we do nowadays in Scapy.
Plus it's still in contrib. To me it's fine to replace all the _payloads with nil.

Another thing I would do for instance, is remove IKEv2_class.guess_payload_class and turn it into a list of bind_layers, which would be much more standard, much more readable, and avoid the need for setting _overload_fields manually, this kind of stuff.

So feel free to rework the module, again, if it's in contrib/, my personal stand is that if the improvements really make sense, breaking stuff is fine.

mspncp · 2022-11-07T12:01:22Z

So feel free to rework the module, again, if it's in contrib/,

Strictly speeking, it will be in layers/ after #3766 is merged. But still unreleased as official layer.

To me it's fine to replace all the _payloads with nil.

Does your ok apply to isakmp.py as well? Or should I leave it untouched, even though that would be inconsistent?

mspncp · 2022-11-07T18:25:00Z

Another thing I would do for instance, is remove IKEv2_class.guess_payload_class and turn it into a list of bind_layers, which would be much more standard, much more readable, and avoid the need for setting _overload_fields manually, this kind of stuff.

Now that you mention it: I hadn't noticed yet how broken the overloading of the next_payload field was: it works only for the first payload, i.e. only if the predecessor is the IKEv2 layer itself: In the following example, the next_payload entry of the SA is not set correctly by the KE payload:

>>> IKEv2(raw(IKEv2() / IKEv2_payload_SA() / IKEv2_payload_KE())).show()
###[ IKEv2 ]### 
  init_SPI  = ''
  resp_SPI  = ''
  next_payload= SA
  version   = 0x20
  exch_type = 
  flags     = 
  id        = 0
  length    = 40
###[ IKEv2 SA ]### 
     next_payload= None     <===== !!! ====
     res       = 0
     length    = 4
     prop      = None
###[ Raw ]### 
        load      = '\x00\x00\x00\x08\x00\x00\x00\x00'

I noticed it when I started removing the redundand (so I thought) next_payload assignments. Here's how it should have been:

>>> IKEv2(raw(IKEv2() / IKEv2_payload_SA(next_payload='KE') / IKEv2_payload_KE())).show()
###[ IKEv2 ]### 
  init_SPI  = ''
  resp_SPI  = ''
  next_payload= SA
  version   = 0x20
  exch_type = 
  flags     = 
  id        = 0
  length    = 40
###[ IKEv2 SA ]### 
     next_payload= KE
     res       = 0
     length    = 4
     prop      = None
###[ IKEv2 Key Exchange ]### 
        next_payload= None
        res       = 0
        length    = 8
        group     = 0
        res2      = 0
        load      = ''

mspncp · 2022-11-07T18:30:30Z

However, there is also good nows: apart from this 'known bug', the test suite is now complete and succeeds:

...
>>> 
>>> from __future__ import print_function
>>> 
>>> for i, title, data, packet in frames:
...     print(title)
...     if i >= 0:
...         # the raw frame data coincides with the frame from the packet capture
...         assert data == raw(pcap[i])
...     # the scapy packet correctly describes the frame
...     assert raw(packet) == data
...     # reassembling the dissected frame yields the original frame
...     assert raw(Ether(data)) == data
... 
IKE_SA_INIT request
IKE_SA_INIT response
IKE_AUTH request
IKE_AUTH response
IKE_AUTH request, decrypted
IKE_AUTH response, decrypted



UTscapy ended successfully

mspncp · 2022-11-07T23:32:51Z

Last force-push was a pure rebase to remain on top of #3766.

mspncp · 2022-11-12T15:54:47Z

Rebased onto master. Still in draft mode, because tests are still missing.

scapy/contrib/ikev2.py

The IKE messages tend to have a lot of payloads and those unnecessary eight characters per payload add a lot of redundancy and size to the output of the dissected packets. Removing them improves readability of the output and makes typing less tedious. The removal was automated using the following find+sed script: find * -type f -exec sed -i \ -e 's/IKEv2_payload_type/IKEv2_payloax_type/g' \ -e 's/IKEv2_payload_/IKEv2_/g' \ -e 's/IKEv2_payload/IKEv2_Payload/g' \ -e 's/IKEv2_payloax/IKEv2_payload/g' {} +

* IKEv2AttributeTypes Swap names and numbers in the IKEv2AttributeTypes dictionary and its nested algorithm dictionaries to get rid of the awkward code to invert the entries. With this change, the transform types and algorithms can now be extracted using a simple dictionary comprehension. * IKEv2PayloadTypes and IKEv2ExchangeTypes Instead of arrays, use dictionaries like everywhere else in the module. Also change their names to make them consistent with the names of the other dictionaries.

Replace the names of the 'Encrypted', 'Encrypted_Fragement, and 'VendorID' payload with their official IANA abbreviations 'SK', 'SKF, and 'V', to be more consistent with the other payload names. The only remaining exceptions are the 'Nonce' and 'Notify' payloads ('N' resp. 'Ni'/'Nr'), because they would end up with the same abbreviation (the dissector does not distinguish between the initiator's and responder's nonce).

The previous bindings were incomplete: the payloads were only bound to the main IKEv2 layer, but not to the other payloads. For example, the lower binding between KE and Nonce payload is missing here: >>> IKEv2() / IKEv2_KE() / IKEv2_Nonce() <IKEv2 next_payload=KE |<IKEv2_KE |<IKEv2_Nonce |>>> This commit removes the manually implemented bindings and replaces them with bind_layers() calls: For every entry (np, name) in the IKEv2PayloadTypes dictionary it is now expected that there exists a corresponding 'IKEv2_<name>' class, which is derived from IKEv2_Packet. The layers get bound as follows: bind_layers(IKEv2_Packet, IKEv2_<name>, next_payload=np)

* CERT payload [RFC 7296, section 3.6] Previously, the IKEv2_CERT class used a dispatch hook to define three different classes IKEv2_CERT_CRT, IKEv2_CERT_CRL, and IKEv2_CERT_STR. This commit removes the three subclasses and uses a MultipleTypeField for the `cert_data` instead. * CERTREQ payload [RFC 7296, section 3.7] The field description was apparently copied from the vendor id payload and never updated. This commit adds the correct members, however the size and count of the SPIs are not updated automatically yet.

All the fields contain binary data, so it makes the output much more readable to show their value as hexadecimal string. It is also more consistent with the ipsec module, see for example the spi and data fields of the ESP packet.

This commit adds a dispatch hook for the IKEv2_payload_Notify class to support adding specialized classes for specific notifications for which the payload can be dissected further. As a first use-case, it adds two new notification classes - IKEv2_payload_Notify_Redirect - IKEv2_payload_Notify_Redirected_from See RFC 5685, sections 9.2 and 9.3

mspncp · 2023-01-31T19:35:55Z

Last force-push was a rebase on master without further changes.

mspncp · 2023-03-05T16:18:53Z

Superseded by #3925

mspncp force-pushed the dissect-ikev2-redirect branch from 4a0a902 to e1d8c4a Compare November 5, 2022 13:46

mspncp force-pushed the dissect-ikev2-redirect branch 2 times, most recently from 733ba89 to 12aa0a0 Compare November 6, 2022 20:23

mspncp force-pushed the dissect-ikev2-redirect branch from 12aa0a0 to 98fecd0 Compare November 7, 2022 23:28

mspncp mentioned this pull request Nov 8, 2022

Improve dissection of the IKEv2 protocol #3766

Closed

5 tasks

mspncp force-pushed the dissect-ikev2-redirect branch from 98fecd0 to f4eecaa Compare November 12, 2022 15:54

mspncp commented Nov 12, 2022

View reviewed changes

scapy/contrib/ikev2.py Show resolved Hide resolved

mspncp added 7 commits January 31, 2023 20:34

IKEv2: replace Str*Fields with XStr*Fields

8051742

All the fields contain binary data, so it makes the output much more readable to show their value as hexadecimal string. It is also more consistent with the ipsec module, see for example the spi and data fields of the ESP packet.

mspncp force-pushed the dissect-ikev2-redirect branch from f4eecaa to 693de59 Compare January 31, 2023 19:34

mspncp mentioned this pull request Mar 5, 2023

IKEv2: improve dissection of IKEv2 redirect notifications #3925

Merged

2 tasks

mspncp closed this Mar 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IKEv2: improve dissection of IKEv2 redirect notifications #3779

IKEv2: improve dissection of IKEv2 redirect notifications #3779

mspncp commented Nov 5, 2022 •

edited

mspncp commented Nov 5, 2022

codecov bot commented Nov 5, 2022 •

edited

mspncp commented Nov 5, 2022 •

edited

mspncp commented Nov 6, 2022

gpotter2 commented Nov 7, 2022

mspncp commented Nov 7, 2022 •

edited

mspncp commented Nov 7, 2022 •

edited

mspncp commented Nov 7, 2022 •

edited

mspncp commented Nov 7, 2022

mspncp commented Nov 12, 2022

mspncp commented Jan 31, 2023

mspncp commented Mar 5, 2023

IKEv2: improve dissection of IKEv2 redirect notifications #3779

IKEv2: improve dissection of IKEv2 redirect notifications #3779

Conversation

mspncp commented Nov 5, 2022 • edited

mspncp commented Nov 5, 2022

codecov bot commented Nov 5, 2022 • edited

Codecov Report

mspncp commented Nov 5, 2022 • edited

mspncp commented Nov 6, 2022

gpotter2 commented Nov 7, 2022

mspncp commented Nov 7, 2022 • edited

mspncp commented Nov 7, 2022 • edited

mspncp commented Nov 7, 2022 • edited

mspncp commented Nov 7, 2022

mspncp commented Nov 12, 2022

mspncp commented Jan 31, 2023

mspncp commented Mar 5, 2023

mspncp commented Nov 5, 2022 •

edited

codecov bot commented Nov 5, 2022 •

edited

mspncp commented Nov 5, 2022 •

edited

mspncp commented Nov 7, 2022 •

edited

mspncp commented Nov 7, 2022 •

edited

mspncp commented Nov 7, 2022 •

edited