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

Socks/v1 #12303

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Socks/v1 #12303

wants to merge 5 commits into from

Conversation

victorjulien
Copy link
Member

Initial draft of socks parser work.

Lots of todos:

  • GSSAPI support
  • IPv6 logging
  • frames
  • detection
  • protocol upgrade has anomaly warnings if protocol is not TLS
  • docs
  • etc

SV_BRANCH=OISF/suricata-verify#2195

https://redmine.openinfosecfoundation.org/issues/4965

} else if t == 4 {
parse_connect_command_request_ipv6(i)?
} else {
return Err(Err::Error(make_error(i, ErrorKind::Verify)));
Copy link
Member Author

Choose a reason for hiding this comment

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

should be unreachable but the compiler is not smart enough to understand

Copy link
Member

Choose a reason for hiding this comment

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

You could throw an #[unreachable] there, however it will panic if its every reached.

Copy link
Member Author

Choose a reason for hiding this comment

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

think I prefer a debug validate bug on then, with a error handler. Would like to avoid any risk of reachable assertions.

let (i, _ver) = verify(be_u8, |&v| v == 5)(i)?;
let (i, results) = be_u8(i)?;
let (i, _res) = verify(be_u8, |&v| v == 0)(i)?;
let (i, at) = verify(be_u8, |&v| v == 1)(i)?; // domain
Copy link
Member Author

Choose a reason for hiding this comment

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

needs to be checked

tx_id: u64,
tx_data: AppLayerTxData,
complete: bool,
pub connect: Option<SocksTransactionConnect>,
Copy link
Member Author

Choose a reason for hiding this comment

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

want to consolidate these 3 into an enum

}
}
}
SocksConnectionState::SocksStateAuthMethodSent => {}
Copy link
Member Author

Choose a reason for hiding this comment

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

still need to handle these and other "invalid" states for this direction.

unsafe extern "C" fn rs_socks_probing_parser(
_flow: *const Flow, _direction: u8, input: *const u8, input_len: u32, _rdir: *mut u8,
) -> AppProto {
// Need at least 2 bytes.
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO haven't looked much as this yet

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 23989

Copy link

codecov bot commented Dec 18, 2024

Codecov Report

Attention: Patch coverage is 75.09434% with 132 lines in your changes missing coverage. Please review.

Project coverage is 83.22%. Comparing base (0e4faba) to head (eb8853b).
Report is 17 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12303      +/-   ##
==========================================
- Coverage   83.22%   83.22%   -0.01%     
==========================================
  Files         912      915       +3     
  Lines      257311   258155     +844     
==========================================
+ Hits       214154   214844     +690     
- Misses      43157    43311     +154     
Flag Coverage Δ
fuzzcorpus 60.94% <20.00%> (-0.13%) ⬇️
livemode 19.36% <9.24%> (-0.17%) ⬇️
pcap 44.48% <70.18%> (+0.12%) ⬆️
suricata-verify 62.90% <74.52%> (+0.06%) ⬆️
unittests 59.08% <9.24%> (-0.11%) ⬇️

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

rust/src/socks/socks.rs Outdated Show resolved Hide resolved
@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 24014

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 24017

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 24018

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 24027

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 24032

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.

3 participants