seladb
SMTP Protocol Support
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
f49f009) 82.48% compared to head (b3dc78a) 82.64%. Report is 5 commits behind head on dev.
Additional details and impacted files
@@ Coverage Diff @@
## dev #1201 +/- ##
==========================================
+ Coverage 82.48% 82.64% +0.15%
==========================================
Files 161 163 +2
Lines 20770 20962 +192
Branches 7847 7962 +115
==========================================
+ Hits 17133 17324 +191
- Misses 2961 2964 +3
+ Partials 676 674 -2
| Flag | Coverage Δ | |
|---|---|---|
| alpine317 | 72.40% <95.71%> (+0.19%) |
:arrow_up: |
| centos7 | 74.14% <96.99%> (+0.24%) |
:arrow_up: |
| fedora37 | 72.41% <95.71%> (+0.19%) |
:arrow_up: |
| macos-11 | 61.34% <81.81%> (+0.18%) |
:arrow_up: |
| macos-12 | 61.40% <82.46%> (+0.19%) |
:arrow_up: |
| macos-ventura | 61.46% <84.10%> (+0.21%) |
:arrow_up: |
| mingw32 | 70.25% <96.40%> (+0.18%) |
:arrow_up: |
| mingw64 | 70.27% <96.40%> (+0.18%) |
:arrow_up: |
| npcap | 83.35% <100.00%> (+0.11%) |
:arrow_up: |
| ubuntu1804 | 74.80% <96.18%> (+0.20%) |
:arrow_up: |
| ubuntu2004 | 73.22% <95.45%> (+0.23%) |
:arrow_up: |
| ubuntu2204 | 72.24% <95.55%> (+0.19%) |
:arrow_up: |
| ubuntu2204-icpx | 59.02% <45.73%> (-0.08%) |
:arrow_down: |
| unittest | 82.64% <100.00%> (+0.15%) |
:arrow_up: |
| windows-2019 | 83.39% <100.00%> (+0.14%) |
:arrow_up: |
| windows-2022 | 83.40% <100.00%> (+0.14%) |
:arrow_up: |
| winpcap | 83.38% <100.00%> (+0.16%) |
:arrow_up: |
| xdp | 59.02% <95.55%> (+0.33%) |
:arrow_up: |
| zstd | 73.78% <96.10%> (+0.17%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@seladb I'll double check parsing with different SMTP packets but it's ready for review.
@seladb I'll double check parsing with different SMTP packets but it's ready for review.
Thank you @egecetin ! It might take me a few days to review but I'll get to it ASAP
I noticed a bug after double check this https://github.com/seladb/PcapPlusPlus/pull/1201#discussion_r1413051164. I completely misuse functions find_last_of and find_first_of instead of rfind/find to search multicharacter sequences. Since all test cases (and of course normal SMTP and FTP packets) data ends with both \r\n none of the unit test failed and I am so lucky that despite this error, I was able to write the edit and craft functions to work correctly. I fixed this error with https://github.com/seladb/PcapPlusPlus/pull/1201/commits/5b9d76d99099950840ea75fb8ddc35cb20846d71. Now it shows mostly data packets as payload layer instead of SMTP and parses correctly. A rare possibility occurs in SMTP example pcap which data packet bounces with ICMP and SMTP layer in one of these ICMP packets exactly end with \r\n so both port check and SingleCommandTextProtocol::isDataValid succeeds.
@seladb But since this is a change not related with SMTP and not a really small fix, I can open a separate PR for the fix if you want from this commit. And converted this PR to draft prevent merging, I think I need to check FTP and SMTP again
A rare possibility occurs in SMTP example pcap which data packet bounces with ICMP and SMTP layer in one of these ICMP packets exactly end with \r\n so both port check and SingleCommandTextProtocol::isDataValid succeeds.
@egecetin I'm not sure I understand this comment... 🤔
@seladb Normally since the data packets are binary they are not end with CRLF sequence but since they are binary they might 😀. Since the data does not fit into single packet it can be send over multiple packets so there is a still chance for the smtp data packet might end with CRLF sequence. In addition to this ICMP protocol truncates and adds the failed packet in its payload so also there is a chance for even the original SMTP packet does not end with CRLF sequence, because of the truncation the SMTP, layer after ICMP might end with CRLF and can pass the checks during parsing. Wireshark's SMTP example pcap have an example for this situation. Even though SMTP packet ends with random bytes, one of the ICMP packet exactly cuts the packet from right place so binary data packet is assumed as SMTP command/reply. I'll send the screenshot of the packet later.
es not fit into single p
@egecetin I think I understand now. But if the TCP port is SMTP (25 or 587) and the packet ends with \r\n - isn't it a good enough indication that this is an SMTP packet? Are there additional checks we can do? 🤔
Probably nothing we can do instead of checking of the session (at least i dont know)
@seladb I saw your comments but still don't have time to fix them. I try to fix your comments and update the PR as soon as possible for protocol type changes. Sorry for the delay
@seladb I saw your comments but still don't have time to fix them. I try to fix your comments and update the PR as soon as possible for protocol type changes. Sorry for the delay
Sure @egecetin no worries. We can keep the PR open until you're available to address the comments
@seladb It's ready for review. You can check the PR when you have time
@egecetin you can update the branch, the FreeBSD issue should be fixed. I'll try to review the PR in the coming days