Improve SIP packet detection using heuristic parsing #2024
Conversation
Packet++/header/SipLayer.h
Outdated
| /// @param[in] data Pointer to the raw data buffer | ||
| /// @param[in] dataLen Length of the data buffer in bytes | ||
| /// @return True if the first line matches SIP request/response syntax, false otherwise | ||
| static bool dissectSipHeuristic(const uint8_t* data, size_t dataLen) |
There was a problem hiding this comment.
We already have SipRequestFirstLine and SipResponseFirstLine that parse the first line, maybe we could use this instead of adding more logic to parse the first line?
There was a problem hiding this comment.
I agree that we already have SipRequestFirstLine and SipResponseFirstLine for parsing the first line, but I think keeping the heuristic logic separate is still necessary because the goals are different.
The Sip*FirstLine classes assume we already decided that the payload is SIP, operate on a Sip*Layer instance, update internal state (m_IsComplete, offsets, logging, etc.), and are meant for full parsing.
In contrast, dissectSipHeuristic() is a stateless, side-effect-free check that runs directly on raw data to answer a simpler question: “does this buffer look like a SIP message at all?”. This also matches Wireshark’s design, where heuristic detection is separate from the actual SIP dissector that parses the first line and fields.
This separation is particularly important for TCP: when we inspect data per segment, the first line may be incomplete. In that case the heuristic must be able to say “need more data / undecided” without constructing SIP layers or marking anything as invalid, which is a different lifecycle than the existing first-line parsers.
In this pull request I’m not yet handling TCP segmentation or IP fragmentation — the heuristic currently assumes it sees at least one complete first line. I plan to address proper TCP stream reassembly / IP fragmentation handling in a separate follow-up PR.
There was a problem hiding this comment.
I agree that we already have
SipRequestFirstLineandSipResponseFirstLinefor parsing the first line, but I think keeping the heuristic logic separate is still necessary because the goals are different.The
Sip*FirstLineclasses assume we already decided that the payload is SIP, operate on aSip*Layerinstance, update internal state (m_IsComplete, offsets, logging, etc.), and are meant for full parsing.In contrast,
dissectSipHeuristic()is a stateless, side-effect-free check that runs directly on raw data to answer a simpler question: “does this buffer look like a SIP message at all?”. This also matches Wireshark’s design, where heuristic detection is separate from the actual SIP dissector that parses the first line and fields.
I just noticed Sip*FirstLine classes do accept a request/response pointer in their constructor, so they can't be used directly. However, they do contain static methods such as parseStatusCode(), parseVersion(), parseMethod() that can definitely be used. If we see we still have a lot of common code between these classes and the parsing logic you need we can think what's the best way to refactor them so they can be used in both scenarios.
In this pull request I’m not yet handling TCP segmentation or IP fragmentation — the heuristic currently assumes it sees at least one complete first line. I plan to address proper TCP stream reassembly / IP fragmentation handling in a separate follow-up PR.
Handling TCP segmentation or IP fragmentation is more tricky - PcapPlusPlus parses packets one by one, there is currently no built-in way to use TcpReassembly or IPReassembly and use the outcome to parse the message again as a packet
There was a problem hiding this comment.
Thanks for the suggestion!
I refactored SipLayer::dissectSipHeuristic() to use the static parsing helpers from SipResponseFirstLine and SipRequestFirstLine instead of manually tokenizing the first line.
For responses I'm now using parseVersion() and parseStatusCode(), and for requests I'm using parseMethod(), parseVersion() and parseUri(). This removes the duplicated parsing logic and keeps the heuristic in sync with the actual SIP first-line parsers.
I didn't use the Sip*FirstLine constructors themselves, as they still require a request/response pointer as you mentioned.
There was a problem hiding this comment.
You’re absolutely right that handling TCP segmentation and IP fragmentation is more complex. Since PcapPlusPlus currently processes packets one by one, in this PR I focused only on heuristic detection on the first line of a single packet. My plan is to add built-in IP fragmentation support to PcapPlusPlus itself in a separate PR, and I’m really excited to work on that.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #2024 +/- ##
=======================================
Coverage 83.87% 83.88%
=======================================
Files 307 307
Lines 53952 54046 +94
Branches 11352 11335 -17
=======================================
+ Hits 45254 45334 +80
- Misses 7483 7494 +11
- Partials 1215 1218 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@sorooshm78 I just saw we already have most of the logic that you implemented: https://github.com/seladb/PcapPlusPlus/blob/master/Packet%2B%2B/src/UdpLayer.cpp#L113-L120 The 2 things you implemented on top of what we have is parsing the request version and the URI, but are they actually required? 🤔 |
|
@seladb Upon reviewing the code in Wireshark, I concluded that further investigation is needed. The logic in Wireshark is essentially designed to search the first line and split it into three tokens. Since the format for request and response is defined in the RFC, this determination was made based on that. Specifically, the RFC defines SIP requests and responses in the following way:
However, based on your comment, I noticed that part of the logic in your parsing already includes this check, so I decided to build on that. For SIP responses, everything was straightforward and clear because of the fixed length of the version and the three-digit status code, which made detection easy. I could use the However, for Therefore, I added two static methods to the
This was necessary to ensure that the parsing of SIP requests was handled correctly without violating the existing design logic. Please let me know if you think there's a better way to approach this or if there are any concerns with my changes. |
|
@sorooshm78 I think I understand it better now: you'd like to be able to parse SIP messages that are not on port 5060. Since the parsing logic is part of the fast path and can potentially run on every captured packet, it should be as efficient as possible, which is why we need to be careful when parsing strings. That's the reason we check the port first and then try to parse the first line. To be honest, I don't see a good way to run this parsing logic on every packet in an efficient manner that would not hurt the performance. Do you know if there are standard SIP ports other than 5060? If yes, maybe we can add them to the list and be able to parse a larger variaty of SIP packets... 🤔 |
@seladb A solution I have considered plausible is a 2 pass system. Basically the ports work only as a hint to which protocol type to try to parse as first. This would allow the library to match packets from "known" protocol ports fast, while also allowing parsing capability of packets coming from "unknown" ports. Below is a flow chart on how it can work. flowchart TD
start[Inbound Packet]-->portCheck{Check Ports}
portCheck -->|Port Matches| discectHintCheck{Run dissector for hinted port}
discectHintCheck --> |No match| runAllDissectors
portCheck -->|No match| runAllDissectors{Run all dissectors}
discectHintCheck -->|Data matches| packetParsed[Parsed Packet]
runAllDissectors -->|Data matches| packetParsed
runAllDissectors--> |No match| unknownPacket[Unknown Packet]
Basically if the inbound packet is HTTP and comes on port 80, it will be fast tracked to the HTTP parser by the port 80 -> HTTP hint. If it came on port 41518, it would fail all port hints and go to the second stage where all the possible protocols are tested for signature match, eventually getting to the HTTP parser and matching. Such architecture should keep performance on the fast path, while allowing protocols on unknown ports to be parsed. The only trade-off is that completely unparsable packets need to go through more checks until they are discarded. |
|
@seladb
|
sorooshm78
changed the title
@sorooshm78 I think this aligns with @Dimi1010 's suggestion above ☝️ The main issue I see with this approach is that all of the non-classified protocols (meaning protocols PcapPlusPlus doesn't yet parse) will fall into this bucket and will be checked for SIP matching (and in the future, maybe more protocols). Maybe we can combine this with option (2) - we can have add 2 static // Parse with checking the port
static SipLayer* parseSipLayer(uint8_t* data, size_t dataLen, Layer* prevLayer, Packet* packet, uint16_t srcPort, uint16_t dstPort);
// Parse without checking the port
static SipLayer* parseSipLayer(uint8_t* data, size_t dataLen, Layer* prevLayer, Packet* packet);The first method will do roughly what the current parsing logic does: check the ports first, then check if it's a request or a response. It will be called instead of the current SIP parsing. The second method will do what option (2) suggests: check the first 3 characters: if it looks like a SIP response - continue checking the version, status code, etc. If it looks like a request - continue checking the SIP method. This method will be called last - after all the port checks. Please let me know what you think. |
|
The approach you suggested works very well for SIP, and I agree it’s a good fit there. However, what I’m proposing (and I believe this is also what @Dimi1010 intends) is a more general change to how PcapPlusPlus detects protocols. Right now detection relies heavily on ports, but changing default ports is not rare at all – it’s actually quite common in real deployments. This is not specific to SIP; any supported protocol might run on a non-default port. In such cases, PcapPlusPlus will simply fail to identify the packet if we only look at ports. I understand that PcapPlusPlus doesn’t support every protocol, but even for the protocols it does support, port-only detection means we don’t get full coverage. As soon as someone changes the default port, those packets are treated as “unknown” by the library. So the trade-off is:
What I’m suggesting is:
Any new protocol added in the future can follow the same pattern. Instead of removing heuristic functions, we can focus on keeping them as lightweight and fast as possible. For example, in my initial SIP heuristic I was taking the first line, splitting it into three tokens, and then parsing each one, which is more expensive. A faster approach is to just look at the first 3 characters of the payload:
This keeps the heuristic very cheap, but still doesn’t rely on the port. |
|
I was indeed talking in the general case. Not just for SIP, but as a general solution to the current limitation of port based detection, which is brittle. Yes, it will mean that the packets that don't fit the fast-path "known ports" heuristic will take slower to parse as they have to run through all dissectors, but I deem that acceptable for the improved functionality. Otherwise, they won't be parsed at all. As for the heuristic dissector functions. I think it's fine if they have some more expensive checks as long as they are later in the order. IMO, the first checks in a detector should be cheap ones that can reject as many true negatives as possible. I'm not super familiar with SIP, but the proposed solution seems fine? If the first characters are not "SIP," that can reject the majority of the true negatives without further inspection? |
|
Another idea that came to my mind is to make the use of heuristics configurable. That way, users who care more about processing speed can disable them and keep the current behavior, while users who prefer higher accuracy and fully parsed results can enable them. |
|
@sorooshm78 @Dimi1010 my suggestion is very similar to what both of you suggest: // Parse with checking the port
static SipLayer* parseSipLayer(uint8_t* data, size_t dataLen, Layer* prevLayer, Packet* packet, uint16_t srcPort, uint16_t dstPort);
// Parse without checking the port
static SipLayer* parseSipLayer(uint8_t* data, size_t dataLen, Layer* prevLayer, Packet* packet);The first overload is the "parse SIP by port" which will be called in the same place it's called today. The second overload is the one who does "heuristic parsing" and will be called at the end, after the port search is exhausted. I agree we can think of a more generic approach which will have heuristics to parse more protocols, but for now maybe we can start with SIP only and see how it goes.
@sorooshm78 I agree that could be a good heuristic that can minimize the performance impact. |
|
@seladb |
@seladb Just to clarify, do you mean that the first one does only the port check or port check + heuristic for parse test? Because I think only the heuristic parse check overload is needed (without the port params). The dispatch by port hint should be handled inside the Note: IMO, since the SipLayer constructors accept pure data buffers during parse, the actual heuristic function should be implemented as a standalone function, instead of inside This is a quick draft on how I can imagine the 2 stage parser to work. enum class SipPacketDissectResult
{
Rejected = 0,
Request = 1,
Response = 2,
};
SipPacketDissectResult dissectSipHeuristic(const uint8_t* data, size_t dataLen)
{
// Do heuristic checks here.
// Return SipPacketDissectResult::Rejected on reject.
// Return SipPacketDissectResult::Request or SipPacketDissectResult::Response if it matches a type.
}
UdpLayer::parseNextLayer()
{
// Guards against insufficient data length here.
uint16_t portSrc, portDst;
// Unpack ports
bool skipPortCheck = false;
for (int i = 0; i < 2; i++)
{
/* Other if (skipPortCheck || TLayer::isTPort(portSrc) || TLayer::isTPort(portDst)) */
// SIP check
if (skipPortCheck || SipLayer::isSipPort(portSrc) || SipLayer::isSipPort(portDst))
{
auto dissectResult = SipLayer::dissectSipHeuristic(udpData, udpDataLen));
// Possibly add initial "if(dissectResult == SipPacketDissectResult::Rejected) {} else"
// to short-circuit the other ifs but probably won't be necessary. Benchmark first.
if (dissectResult == SipPacketDissectResult::Request)
{
constructNextLayer<SipRequestLayer>(udpData, udpDataLen, m_Packet);
break; // We constructed the next layer. Exit the for-loop.
}
else if (dissectResult == SipPacketDissectResult::Response)
{
constructNextLayer<SipResponseLayer>(udpData, udpDataLen, m_Packet);
break; // We constructed the next layer. Exit the for-loop.
}
}
/* Other if (skipPortCheck || TLayer::isTPort(portSrc) || TLayer::isTPort(portDst)) */
// ----- After all if parse checks ------
// After the first iteration is done. Set skipPortCheck to true to ignore the ports on the second run.
skipPortCheck = true;
}
// Final check if no layer could be identified.
if(!hasNextLayer())
{
// Construct a generic payload layer.
constructNextLayer<PayloadLayer>(udpData, udpDataLen, m_Packet);
}
}
bool SipLayer::isSipPort(uint16_t port) { /*... */}
// Maybe not needed?
static SipLayer* SipLayer::parseSipLayer(uint8_t* data, size_t dataLen, Layer* prevLayer, Packet* packet)
{
// Does heuristic checks here.
// Return `nullptr` on reject.
// Return SipLayer object on success.
}PS: This architecture would also allow multiple protocol types to share a "port hint" and still work, although the more overlap there is the less benefits there are to actually hinting. |
|
@Dimi1010 I think that parsing a SIP layer if a port is known is quite different vs. any packet: if you know it's a SIP port you can expect a request or response and parse them accordingly. For example: if the dst port is 5060 it can't be a SIP request and vise versa. However, for any packet, you should first decide if it's a request or a response (probably by examining the first 3 characters), and then continue parsing. That's why I suggested 2 parsing methods vs. just one.
Instead of
I think we should find a better approach than the 2 iteration for-loop, because packets that don't match any layer will be parsed twice as slow, as they will throw the whole check again... |
@seladb Yes, but you don't know its a SIP packet when you are comparing the ports. If one of the ports is 5060 it is highly likely it might be SIP, but not guaranteed. You still have to examine the packet, possibly by the first 3 chars. I could very well pass an HTTP packet on port 5060 otherwise. I could also very well do a unholy abomination that runs the SIP request / response layer in reverse. This is why I said that the main dissector should be port agnostic and always run and ports are just a hint on which type of dissector to run first. At the end of the day conceptually you only get one incoming stream of "any packets".
What I am worried about is that that solution essentially locks the layers to always be constructed on heap, which somewhat interferes with my experiments for the arena based allocation packet I have mentioned before, as the heap allocations are the main bottleneck I discovered in the parser library. I am fine with the parse method existing, but I would prefer for the actual dissector logic to be a separate function that can be utilized as standalone too, essentially leading to
In the first pass, the majority of the handlers are skipped by the short circuit by the ports, which is the same as we have currently. In the second pass they do run everything. Unfortunately this is unavoidable, as you need to run those because you are out of hints when the ports don't work and you have no guarantee that the packets are truly unknown yet. This is why I said that the dissectors should reject non-matching packets as fast as possible. Otherwise you have faster pipeline for unknown packets, but you also throw packets that could have been known into it. |
You're right, that's why we check the port + the first line. However, if the dstport is 5060 and it's a SIP packet - it has to be a SIP reuqest, and same if the srcport is 5060 - it cannot be a SIP request.
Why would this block the option of arena/pool based allocation? It doesn't matter who creates the layer instance, once we have a different way to allocate packets - we can change the code to use it 🤔
I'd like the second pass to be shorter and only include protocols that have a heuristic-based dissector. The assumption is that most protocol don't have such a dissector |
Why is that assumed? As far as I know, the SIP protocol is port agnostic. There is nothing in the spec that requires that port 5060 sends only responses and not requests. The server and client are not bound to specific ports. The "well known" port is, at the end of the day, a suggestion. Hypothetically you could have a connection where the server runs on port 6000 and port 5060 is used by the client to send requests and it would be a valid connection, even if unusual.
Sure, we can change that when we get to it. I just wanted to give an example on why I think it would be beneficial to have the pure dissector checks in a separate function that is not the factory function.
Sure, we can do that. The for-loop was drafted only for protocols that support dissectors to be inside it. The old style protocols that don't have dissectors can go before the for loop, since we don't need a second pass on them. Personally I think we should make it a milestone to have a dissector for every protocol to allow port agnostic parsing. Essentially we would end up with something like this: {
// Init and port unpack.
/* Old style protocols w/o dissector support (port only heuristic) go here. */
// Return if old style protocol was matched.
if(hasNextLayer()) { return; }
bool skipPortCheck = false;
for (int i = 0; i < 2; i++)
{
/* Protocol w/ dissector support checks go here */
/* if (skipPortCheck || TLayer::isTPort(portSrc) || TLayer::isTPort(portDst)) { ... } */
// ----- After all if parse checks ------
// After the first iteration is done. Set skipPortCheck to true to ignore the ports on the second run.
skipPortCheck = true;
}
// Final check if no layer could be identified.
if(!hasNextLayer())
{
// Construct a generic payload layer.
constructNextLayer<PayloadLayer>(udpData, udpDataLen, m_Packet);
}
} |
3 participants
This PR improves SIP packet detection in PcapPlusPlus by introducing heuristic parsing based on Wireshark’s SIP dissector. SIP messages are now detected from the UDP payload itself, not only when using port 5060.
static bool dissectSipHeuristic(const uint8_t* data, size_t dataLen)to detect SIP requests/responses from payload content (Wireshark-style logic)UdpLayerso SIP packets on non-standard ports are correctly classifiedRelated issue: #2022
Note: I’m not yet fully familiar with PcapPlusPlus’ internal structure, so if there are better places, names, or patterns for this logic, I’m happy to adjust the PR based on your feedback