search

kbandla / dpkt

fast, simple packet creation / parsing, with definitions for the basic TCP/IP protocols
Other
1.08k stars 270 forks source link

Fix unpacking of multiple records in TLS messages #624

Closed konstantinbo closed 2 years ago

konstantinbo commented 2 years ago

A TLS message can contain multiple records. This fixes the unpacking of them in their respective objects. In its current implementation the re-assigning of self.data with the newly calculated pointer is unnecessary and breaks the process.

obormot commented 2 years ago

Advancing self.data inside the while block certainly looks like a bug, but the fix you're suggesting @konstantinbo doesn't look correct either. self.data must advance to contain the remaining (unparsed) data. I think the code has indentation problem, the correct fix would be:

while len(self.data[pointer:]) > 0:
    end = pointer + 5 + struct.unpack("!H", buf[pointer + 3:pointer + 5])[0]
    self.records.append(TLSRecord(buf[pointer:end]))
    pointer = end

self.data = self.data[pointer:]      # remove indentation to move out and past the while loop

That's just an educated guess though, I'd love to see some unit tests for this. @konstantinbo do you have an example pcap we could use for a unit test here?

konstantinbo commented 2 years ago

Thanks for the quick response! I re-added the deleted line with the fixed indentation and added an unit test. I used the content of the 6th packet from the PCAP "dump.pcapng" linked in Wireshark's TLS Wiki, which contains 4 TLS records.

coveralls commented 2 years ago

Coverage Status

Coverage increased (+7.0e-05%) to 99.815% when pulling 113f4b76d94ca640145680e504d4851f7488cba9 on konstantinbo:fix-TLS-unpack into 7a91ae53bb20563607f32e6781ef40d2efe6520d on kbandla:master.

brifordwylie commented 2 years ago

LGTM: @obormot please merge when ready...

mattfysh commented 1 year ago

@konstantinbo should the loop be while len(buf... instead of while len(self.data... ?

I just came across a strange bug where the last 5 bytes of my buffer were 1703034018 and I was expecting a NeedData error, but it did not throw

obormot commented 1 year ago

@mattfysh I agree this code doesn't look quite right and can benefit from refactoring. I'll open a new ticket to get this addressed.

obormot commented 1 year ago

@mattfysh Could you test https://github.com/kbandla/dpkt/pull/640 and see if it works for you as expected?

mattfysh commented 1 year ago

This is great! Thanks @obormot - I was also not aware of tls_multi_factory and had rolled my own which I'm happy to replace :) thanks again!