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

Added SMTP layer #587

Closed
wants to merge 3 commits into from
Closed

Added SMTP layer #587

wants to merge 3 commits into from

Conversation

jpbede
Copy link
Contributor

@jpbede jpbede commented Jan 4, 2019

added (basic) unencrypted SMTP decoding and added smtps and submission port with TLS decoder

@notti
Copy link
Collaborator

notti commented Jan 5, 2019

@gconnell what's your take on tcp-protocols? tcp-based protocols don't really fit into the per-packet layer model gopacket uses. There should be a reassembly step in between that merges the packets into a stream which is then processed by the application layer protocol.
As far as I've seen modbustcp and tls already made it in.
Problems that can arise are duplicate protocol-commands caused by retransmitted packets, missing commands due to being split into multiple packets, only decoding one command although there are multiple in a single packet. This probably also affects #584 (although rfc 959 forbids pipelining so ftp might be a bit simpler in this regard)

@notti notti mentioned this pull request Jan 5, 2019
@x-way
Copy link
Contributor

x-way commented Jan 6, 2019

Correct, doing per-packet decoding of stream-based protocols comes with additional problems/complexity.
I assume that's why the tcp layer per default does not decode the tcp payload and only performs the decoding when explicitely being told via the DecodeStreamAsDatagrams option (cf. tcp.go).
Thus making it the responsibility of the 'caller' to take care of the stream complexity when requesting to decode the tcp payload.

@gconnell
Copy link
Collaborator

gconnell commented Jan 7, 2019

I'm happy to add tcp stream decoding, but I think it should be in the context of reassembly/, not layers/. That said, we don't currently have any interfaces or other things built up (like gopacket.Layer*) in reassembly to support starting this, so it'd be a pretty big undertaking.

I have had some pushback in the past (and relented) for TLS in particular, which you'll note is a stream-based protocol that's got layer-level support, but that's because 99.99% of the time, it's sent in single packet messages which are in fact decodable (unless fragmented by some layer).

In short, I agree that we should not do FTP, SMTP, HTTP, etc. in layers/, but also that we should probably consider how best we can build up a system for supporting that type of contribution in reassembly/.

@notti notti mentioned this pull request Sep 18, 2019
@spitfire55
Copy link

Isn't FTP still fundamentally a Layer that can be decoded? I don't see the dependence of these layers on the reassembly package itself. I understand that in order to decode an FTP payload, you must first reassemble the relevant packets in the TCP stream.

The way I currently do it is implement the Stream interface with a struct that has a Payload field. Whenever Stream.ReassembledSG is called, I write all of the bytes available from ScatterGather.Fetch() to that field. Then, when ReassemblyComplete is finished, I send a bool value on a channel to StreamFactory.New(), which blocks on a separate goroutine to receive from that ReassemblyComplete channel. This lets me know that the Stream has been fully reassembled and my Stream object contains the entire application payload (if any was present). I can then pass this Stream object to various protocol-specific TCP protocol analyzers, depending on the contexts of the Payload field.

This way of decoding TCP streams separates the reassembly process from the layer decoding process. I think that layers like FTP, SMTP, etc should still be placed in the Layers package because the decoding process should be separate from the reassembly process.

@notti
Copy link
Collaborator

notti commented Oct 7, 2019

Isn't FTP still fundamentally a Layer that can be decoded? I don't see the dependence of these layers on the reassembly package itself. I understand that in order to decode an FTP payload, you must first reassemble the relevant packets in the TCP stream.

Well yes and no. The problem here is that the layers functionality doesn't really work, since it was built with packets in mind, which is reflected in the interfaces (e.g., DecodeFromBytes is expected to contain everything needed to decode the layer, which is problematic for a stream - where does it end? how much does a layer need?).

The way I currently do it is implement the Stream interface with a struct that has a Payload field. Whenever Stream.ReassembledSG is called, I write all of the bytes available from ScatterGather.Fetch() to that field. Then, when ReassemblyComplete is finished, I send a bool value on a channel to StreamFactory.New(), which blocks on a separate goroutine to receive from that ReassemblyComplete channel. This lets me know that the Stream has been fully reassembled and my Stream object contains the entire application payload (if any was present). I can then pass this Stream object to various protocol-specific TCP protocol analyzers, depending on the contexts of the Payload field.

This would be one way to solve that. But others probably want to decode streams as soon as enough data is available (e.g., the SCADA protocol IEC 60870–5–104 would even make that necessary since it uses endless TCP connections).

This way of decoding TCP streams separates the reassembly process from the layer decoding process. I think that layers like FTP, SMTP, etc should still be placed in the Layers package because the decoding process should be separate from the reassembly process.

I agree, that this shouldn't be part of reassembly (would be nice to not be tied in into a single source interface - so, e.g., one could read a http stream from a txt file). However, it should be different from the layers interface. Something where the decoder can say: I need more data; I consumed x bytes and I'm finished, ... So the input should be some kind of stream, that could be already finished and have all the data, as in your case, and also something that can wait for additional data, so it could be used with reassembly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants