scapy icon indicating copy to clipboard operation
scapy copied to clipboard

fixed a runtime bug when the cert attribute doesn't exists

Open Frank-Buss opened this issue 1 year ago • 7 comments

When running this test: https://gist.github.com/Frank-Buss/c27c663f79ccd1eeb6d74f7795f8bdde it generates this error:

  File "/home/frank/tmp/scapy/scapy/packet.py", line 1883, in getfield_and_val
    raise AttributeError(attr)
AttributeError: cert

This PR fixes it. Not sure about the underlying reason, the TLS packets might be broken. Maybe better to fix it elsewhere or show a warning message as well, but this works at least.

Frank-Buss avatar Aug 06 '24 06:08 Frank-Buss

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 79.14%. Comparing base (d23dda3) to head (aaeb1ce). Report is 15 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4494      +/-   ##
==========================================
- Coverage   81.67%   79.14%   -2.54%     
==========================================
  Files         355      330      -25     
  Lines       84830    79663    -5167     
==========================================
- Hits        69289    63050    -6239     
- Misses      15541    16613    +1072     
Files with missing lines Coverage Δ
scapy/layers/tls/handshake.py 87.05% <100.00%> (-1.28%) :arrow_down:

... and 281 files with indirect coverage changes

codecov[bot] avatar Aug 06 '24 06:08 codecov[bot]

PS: would be also nice if parse_tls_records could be written better, is it possible to get individual TLS packets without low-level splitting the TLS packets myself? I tried it with the full stream, but then I get one TLS packet, with nested TLS packets as payload, and I couldn't extract the underlying layers of one packet.

Frank-Buss avatar Aug 06 '24 06:08 Frank-Buss

Thanks for the PR. Could you share a small snippet that reproduces the issue ?Sent from my iPhoneOn 6 Aug 2024, at 08:41, Frank Buss @.***> wrote:When running this test: https://gist.github.com/Frank-Buss/c27c663f79ccd1eeb6d74f7795f8bdde it generates this error: File "/home/frank/tmp/scapy/scapy/packet.py", line 1883, in getfield_and_val raise AttributeError(attr) AttributeError: cert

This PR fixes it. Not sure about the underlying reason, the TLS packets might be broken. Maybe better to fix it elsewhere or show a warning message as well, but this works at least.

You can view, comment on, or merge this pull request online at:   https://github.com/secdev/scapy/pull/4494

Commit Summary

aaeb1ce fixed a runtime bug when the cert attribute doesn't exists

File Changes (1 file)

M
scapy/layers/tls/handshake.py
(4)

Patch Links:

https://github.com/secdev/scapy/pull/4494.patch https://github.com/secdev/scapy/pull/4494.diff

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: @.***>

guedou avatar Aug 06 '24 06:08 guedou

The gist I linked reproduces the error. Couldn't make it shorter, because I couldn't find a way to display the decrypted data to create just one TLS packet which shows the bug. If you show me how to show the decrypted data of a TLS packet, I can try to make the snippet shorter. But not sure if this would work anyway because of the session object.

Frank-Buss avatar Aug 06 '24 07:08 Frank-Buss

PS: would be also nice if parse_tls_records could be written better, is it possible to get individual TLS packets without low-level splitting the TLS packets myself? I tried it with the full stream, but then I get one TLS packet, with nested TLS packets as payload, and I couldn't extract the underlying layers of one packet.

You can try to use pkt[TLS].iterpayloads().

You'll still see the payloads in each one, but you can simply ignore them.

One note though: in your example the data field seems to include TLS data from both the client and server. I'm not sure how you got there but Scapy won't be able to handle that nicely. (You did well splitting it into the client part and the server part, the passing the tls_session. This is more likely to work.)

Ideally you'd need to keep track of whether the incoming data is from the client or server in the parent parser of your application, and call mirror() accordingly.

gpotter2 avatar Aug 07 '24 08:08 gpotter2

Hi again, Could you ellaborate a tiny bit about where that TLS data is coming from? There certificate appears to be malformed

gpotter2 avatar Aug 08 '24 20:08 gpotter2

Yes, it is probably malformed, it is created by a program on the fly. Can't tell much more about it, as it might be security relevant. But I think scapy shouldn't throw an exception in this case, given the fact that it is used to debug such traffic. But an additional warning message would be probably a good idea.

Frank-Buss avatar Aug 08 '24 20:08 Frank-Buss