Skip to content

Conversation

@vk-coder
Copy link
Contributor

Reducing length of 'length' field along with 'ietype' and 'CR_flag+instance'

Checklist:

  • If you are new to Scapy: I have checked CONTRIBUTING.md (esp. section submitting-pull-requests)
  • I squashed commits belonging together
  • I added unit tests or explained why they are not relevant
  • I executed the regression tests (using cd test && ./run_tests or tox)
  • If the PR is still not finished, please create a Draft Pull Request

fixes #xxx

@codecov
Copy link

codecov bot commented Dec 16, 2022

Codecov Report

Merging #3822 (251ac10) into master (5fa6428) will increase coverage by 32.32%.
Report is 5 commits behind head on master.
The diff coverage is 96.55%.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #3822       +/-   ##
===========================================
+ Coverage   49.60%   81.93%   +32.32%     
===========================================
  Files         341      349        +8     
  Lines       76081    81786     +5705     
===========================================
+ Hits        37742    67009    +29267     
+ Misses      38339    14777    -23562     
Files Coverage Δ
scapy/contrib/cdp.py 95.42% <100.00%> (+23.42%) ⬆️
scapy/contrib/rtps/rtps.py 98.82% <ø> (+12.94%) ⬆️
scapy/layers/inet6.py 88.67% <100.00%> (+47.75%) ⬆️
scapy/layers/ipsec.py 90.60% <100.00%> (+61.02%) ⬆️
scapy/contrib/gtp.py 91.54% <0.00%> (+25.36%) ⬆️

... and 260 files with indirect coverage changes

@guedou
Copy link
Member

guedou commented Dec 16, 2022

Thanks for your interest in Scapy. Can you provide a packet example that exhibits the issue that your are fixing? It will help us implement a unit test.

@muelleme
Copy link
Contributor

Hi, not my PR, but I came upon the same issue and can confirm that this change fixes the issue.
The issue with the mismatch in length can be shown using scapy's show2():

Without this change:

>>> (IP(dst="10.5.0.2")/ \
...:     UDP(dport=2123)/ \
...:     GTPHeader(seq=12345, version=2, gtp_type=32)/ \
...:     GTPV2CreateSessionRequest(IE_list=[
...:         IE_IMSI(IMSI=b'001030000000356'),
...:         IE_APN(APN=b'super')
...:         ])).show2()
###[ IP ]###
  version   = 4
  ihl       = 5
  tos       = 0x0
  len       = 62
  id        = 1
  flags     =
  frag      = 0
  ttl       = 64
  proto     = udp
  chksum    = 0xfdd8
  src       = 192.168.178.38
  dst       = 10.5.0.2
  \options   \
###[ UDP ]###
     sport     = gtp_control
     dport     = gtp_control
     len       = 42
     chksum    = 0x4f76
###[ GTP v2 Header ]###
        version   = 2
        P         = 1
        T         = 1
        MP        = 0
        SPARE1    = 0
        SPARE2    = 0
        gtp_type  = create_session_req
        length    = 26
        teid      = 0x0
        seq       = 12345
        SPARE3    = 0
###[ GTPv2 Create Session Request ]###
           \IE_list   \
            |###[ IE IMSI ]###
            |  ietype    = IMSI
            |  length    = 10
            |  CR_flag   = 0
            |  instance  = 0
            |  IMSI      = '0010300000003567400'
            |###[ Raw ]###
            |  load      = '\x08\x00\x05super'

Notice that the APN IE is not parsed properly. Also wireshark is not able to dissect the resulting package.

With this change:

>>> (IP(dst="10.5.0.2")/ \
...:     UDP(dport=2123)/ \
...:     GTPHeader(seq=12345, version=2, gtp_type=32)/ \
...:     GTPV2CreateSessionRequest(IE_list=[
...:         IE_IMSI(IMSI=b'001030000000356'),
...:         IE_APN(APN=b'super')
...:         ])).show2()
###[ IP ]###
  version   = 4
  ihl       = 5
  tos       = 0x0
  len       = 62
  id        = 1
  flags     =
  frag      = 0
  ttl       = 64
  proto     = udp
  chksum    = 0xfdd8
  src       = 192.168.178.38
  dst       = 10.5.0.2
  \options   \
###[ UDP ]###
     sport     = gtp_control
     dport     = gtp_control
     len       = 42
     chksum    = 0x5376
###[ GTP v2 Header ]###
        version   = 2
        P         = 1
        T         = 1
        MP        = 0
        SPARE1    = 0
        SPARE2    = 0
        gtp_type  = create_session_req
        length    = 26
        teid      = 0x0
        seq       = 12345
        SPARE3    = 0
###[ GTPv2 Create Session Request ]###
           \IE_list   \
            |###[ IE IMSI ]###
            |  ietype    = IMSI
            |  length    = 8
            |  CR_flag   = 0
            |  instance  = 0
            |  IMSI      = '001030000000356'
            |###[ IE APN ]###
            |  ietype    = APN
            |  length    = 6
            |  CR_flag   = 0
            |  instance  = 0
            |  APN       = 'super'

@vk-coder
Copy link
Contributor Author

vk-coder commented Jan 2, 2023

Hi @guedou , extremely sorry for not replying to your message. As @muelleme said, the length field should only container the payload length in the IE header. So adjusted length by reducing 2 bytes for the length attribute.

@gpotter2
Copy link
Member

gpotter2 commented Mar 5, 2023

Sorry for the delay, but it would be nice to have a test in https://github.com/secdev/scapy/blob/master/test/contrib/gtp.uts to check this

@vk-coder
Copy link
Contributor Author

vk-coder commented Apr 6, 2023

added a test

@gpotter2 gpotter2 closed this Feb 5, 2024
@gpotter2 gpotter2 reopened this Feb 5, 2024
Reducing length of 'length' field along with 'ietype' and 'CR_flag+instance'
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.

Very sorry for the delay.

@gpotter2 gpotter2 added this to the 2.6.0 milestone Feb 6, 2024
@gpotter2 gpotter2 merged commit 18d4c30 into secdev:master Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants