Skip to content

Comments

rust/sip: register parser for tcp v12#10037

Closed
glongo wants to merge 8 commits intoOISF:masterfrom
glongo:dev-sip-tcp-v12
Closed

rust/sip: register parser for tcp v12#10037
glongo wants to merge 8 commits intoOISF:masterfrom
glongo:dev-sip-tcp-v12

Conversation

@glongo
Copy link
Contributor

@glongo glongo commented Dec 12, 2023

Make sure these boxes are signed before submitting your Pull Request -- thank you.

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/3351

Describe changes:

  • Rebase

Provide values to any of the below to override the defaults.

To use a pull request use a branch name like pr/N where N is the
pull request number.

Alternatively, SV_BRANCH may also be a link to an
OISF/suricata-verify pull-request.

SV_REPO=
SV_BRANCH=pr/1538
SU_REPO=
SU_BRANCH=
LIBHTP_REPO=
LIBHTP_BRANCH=

Accepts valid characters as defined in RFC3261.
The `is_version_char` function incorrectly allowed characters that are not
part of the valid SIP version "SIP/2.0".

For instance, 'HTTP/1.1' was mistakenly accepted as a valid SIP version,
although it's not.

This commit fixes the issue by updating the condition to strictly
check for the correct version string.
This patch lets the parser to work over tcp protocol, taking care of handling
data before calling the request/response parsers.

Ticket OISF#3351.
This patch permits to set a direction when a new transaction is created in order
to avoid 'signature shadowing' as reported by Eric Leblond in commit
5aaf507
This permits to detect the SIP protocol using pattern matching instead of
probing parser.

Since it is no longer used, the respective probing functions have been removed.
@glongo
Copy link
Contributor Author

glongo commented Dec 12, 2023

I've just set the correct SV_BRANCH, please restart the checks if possible.

@codecov
Copy link

codecov bot commented Dec 12, 2023

Codecov Report

Merging #10037 (db4fb6a) into master (75471dd) will increase coverage by 3.99%.
The diff coverage is 90.45%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10037      +/-   ##
==========================================
+ Coverage   78.42%   82.41%   +3.99%     
==========================================
  Files         970      970              
  Lines      271175   271530     +355     
==========================================
+ Hits       212676   223793   +11117     
+ Misses      58499    47737   -10762     
Flag Coverage Δ
fuzzcorpus 64.42% <69.77%> (+0.02%) ⬆️
suricata-verify 61.30% <90.66%> (?)
unittests 62.86% <32.78%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPW2_autofp_stats_chk
.uptime 181 195 107.73%

Pipeline 17018

@ct0br0
Copy link

ct0br0 commented Dec 12, 2023

Re-ran TLPW2, uptime sometimes matches baseline and other times is a bit up.

&stream_slice,
input,
input.len() as i64,
SIPFrameType::Pdu as u8,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This frame does not look right :
A TCP sent here may have 0 or multiple SIP PDUs

This should be done after sip_parse_request returned Ok

Cf #10048 (MQTT current implementation is wrong)

Copy link
Contributor

@catenacyber catenacyber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frames on tcp should support multiple PDUs in one call to parse_request_tcp (same for response)

@glongo
Copy link
Contributor Author

glongo commented Dec 14, 2023

New PR: #10058

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants