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

Initial support for SCTP protocol. #70

Merged
merged 1 commit into from
Aug 30, 2016
Merged

Initial support for SCTP protocol. #70

merged 1 commit into from
Aug 30, 2016

Conversation

jeffreymyers
Copy link
Contributor

This commits adds support for processing the headers for SCTP packets.
This does not include support for any interpretation of the packet
payload of SCTP data chunks. This also does no validation of the
checksum value in the header.

This commits adds support for processing the headers for SCTP packets.
This does not include support for any interpretation of the packet
payload of SCTP data chunks. This also does no validation of the
checksum value in the header.
@jeffreymyers
Copy link
Contributor Author

Just noticed I failed to update the copyright header dates. Also I guessed at the version to include in the @SInCE tag.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 68.062% when pulling 6a96551 on jeffreymyers:v1 into ee3a944 on kaitoy:v1.

@kaitoy
Copy link
Owner

kaitoy commented Aug 26, 2016

Thank you for your big work!
I will merge it after completing GTP work.

@jeffreymyers
Copy link
Contributor Author

Great, thanks! In case you're interested, I was able to verify that the packet headers were interpreted properly using nmap port scans with the -sY and -sZ options, which exercise the SCTP protocol: https://nmap.org/book/man-port-scanning-techniques.html

@kaitoy
Copy link
Owner

kaitoy commented Aug 27, 2016

That's great!

@kaitoy kaitoy merged commit 6a96551 into kaitoy:v1 Aug 30, 2016
@kaitoy
Copy link
Owner

kaitoy commented Aug 30, 2016

I will implement the checksum calculation.
And, chunks must be included in SctpHeader because they don't have upper layer port number and SctpPacket need to build its payload using the port number its header has. I will do the change, too.

@kaitoy
Copy link
Owner

kaitoy commented Aug 30, 2016

Other than that, your implementation looks perfect! Thanks.

@jeffreymyers
Copy link
Contributor Author

I encountered that issue with the SctpPacket payload when I started working on the chunk support. Is there any way to have the payload be a SimplePacket until the chunk processing is added in?

On the checksum - I thought that'd be a little tricky since the protocol initially used the Adler-32 algorithm, then was updated to be based on CRC32c. I found there was an implementation of Adler-32 and CRC32 in java.util.zip, but I haven't found CRC32c anywhere in the base library (it is present in Guava).

@kaitoy
Copy link
Owner

kaitoy commented Aug 30, 2016

Eventually we will need to collect payload data from Payload Data chunks, concatenate them, and pass it to the packet factory to build a payload. Until then, I think there is no other way but to leave the payload field null.

On the checksum, we can provide uses an option (a system property and/or a method argument) to choose Adler-32 or CRC32c.
I will implement CRC32c myself. It seems not so difficult.

@jeffreymyers
Copy link
Contributor Author

jeffreymyers commented Aug 30, 2016

I would like to get your thoughts and advice on how to go about handling the payload chunks. I was planning on implementing each chunk type as an extension of an abstract SctpChunkPacket (which handles the chunk header containing the chunk type, flags and length), then including an List within the SctpPacket in a field called chunks. I couldn't find any examples of an existing packet type with a variable number of packets as a payload, and the AbstractPacket.getPayload() method returns a single Packet. To maintain the API contract I was thinking of having SctpPacket.getPayload() return the first element in the chunks list (if present) and have a separate getChunks() method to return all of the chunks in the packet. What do you think?
Alternately, there could be an abstract class CompositePacket that SctpPacket would extend, with an abstract method:
List<? extends Packet> getPayloads()

I also wasn't sure whether it makes sense to have the chunk objects extend Packet, but it seems like they'd work nicely as such.

@kaitoy
Copy link
Owner

kaitoy commented Aug 30, 2016

My idea is to copy SctpHeader and build a pseudo packet which has the header and a single payload for each payload chunk.
E.g. when Pcap4J receives a packet like below (A and B are not fragmented):

[Ethernet
  [IPv4
    [SCTP with payload chunk A and payload chunk B]
  ]
]

Pcap4J returns the following 3 packet objects:

actual packet:
[Ethernet
  [IPv4
    [SCTP with payload chunk A and payload chunk B]
  ]
]
pseudo packet for A:
[Ethernet
  [IPv4
    [SCTP with payload chunk A and payload chunk B
      [L5-packet built from A ]
    ]
  ]
]
pseudo packet for B:
[Ethernet
  [IPv4
    [SCTP with payload chunk A and payload chunk B
      [L5-packet built from B ]
    ]
  ]
]

This way fits Pcap4J API such as Packet.get() well.
And this is a similar way as Wireshark does for TCP when its TCP reassembly is enabled.
I guess Wireshark handle SCTP payloads in such way, too.

@jeffreymyers
Copy link
Contributor Author

Sounds good and makes sense. The only weird thing I could see is the header checksum value present in the pseduo packets won't reflect the value of the payload of the pseduo packet. Maybe it'd be better to populate the checksum in the pseudo packets header with null, and only have it populated in the first packet.

@kaitoy
Copy link
Owner

kaitoy commented Aug 31, 2016

The checksum is calculated form the SCTP header and the chunks only (the payload is not included), so it's still valid in a pseudo packet.

@kaitoy kaitoy mentioned this pull request May 14, 2018
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.

3 participants