MySQL protocol parser v1 #11800

Kotodian · 2024-09-19T08:17:39Z

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

I have read the contributing guide lines at
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
I have signed the Open Information Security Foundation contribution agreement at
https://suricata.io/about/contribution-agreement/ (note: this is only required once)
I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)

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

Describe changes:
This is a WIP patchset that implements an application layer for MySQL protocol, many features are not supported.

TBD:
Support Protocol:Handshake V9
Support Mysql Protocol 32
Support Protocol:AuthSwitchRequest
Support Protocol:AuthSwitchResponse
Support Protocol:AuthMoreData
Support Protocol:AuthNextFactor
Support Compression
Add documentation
Add SV tests
Fix CI

victorjulien · 2024-09-19T09:30:20Z

@glongo I think you mentioned you had some WIP code as well. Care to have a look at this?

github-actions · 2024-09-19T10:56:47Z

NOTE: This PR may contain new authors.

codecov · 2024-09-19T11:10:32Z

Codecov Report

Attention: Patch coverage is 3.36927% with 717 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@1420c83). Learn more about missing BASE report.
Report is 23 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #11800   +/-   ##
=========================================
  Coverage          ?   78.80%           
=========================================
  Files             ?      923           
  Lines             ?   251982           
  Branches          ?        0           
=========================================
  Hits              ?   198566           
  Misses            ?    53416           
  Partials          ?        0

Flag	Coverage Δ
fuzzcorpus	`59.78% <0.53%> (?)`
livemode	`18.51% <3.36%> (?)`
pcap	`43.55% <3.09%> (?)`
unittests	`58.90% <0.53%> (?)`

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

jufajardini

For the TBD list ;)

update etc/schema.json
update commit messages to contain Ticket number in the commit body (Task #3446 )
update your git author to follow the format FirstName LastName

:P

nit: in the commit messages, you can remove the rust/ portion

glongo · 2024-09-19T13:37:43Z

@glongo I think you mentioned you had some WIP code as well. Care to have a look at this?

Yes sure.

Task OISF#3446

Kotodian · 2024-09-20T01:18:36Z

For the TBD list ;)
* update etc/schema.json

* update commit messages to contain Ticket number in the commit body (`Task #3446` )

* update your git author to follow the format `FirstName LastName`
:P

nit: in the commit messages, you can remove the rust/ portion

I have fixed, please check again, thx.

Task OISF#3446

catenacyber · 2024-09-23T13:50:40Z

src/output-json-mysql.c

+        return TM_ECODE_FAILED;
+    }
+
+    if (!rs_mysql_logger(txptr, thread->mysqllog_ctx->flags, jb)) {


You should not need this file now
rs_mysql_logger (which should rather be named SCMysqlLogger) cf EveJsonSimpleTxLogFunc in output.c

Fixed, please check again.Why keep simple logger only?

catenacyber · 2024-09-23T13:51:23Z

Are you planning on adding keywords as well ?

catenacyber · 2024-09-23T13:52:00Z

rust/src/mysql/logger.rs

+
+fn log_mysql(tx: &MysqlTransaction, _flags: u32, js: &mut JsonBuilder) -> Result<(), JsonError> {
+    js.open_object("mysql")?;
+    js.set_uint("tx_id", tx.tx_id)?;


Is it meaningful to log the tx id ?

No, all of fields are based on my project, if you have any idea in logger, please comment, thx.

Fixed, please check again.

catenacyber · 2024-09-23T13:52:20Z

rust/src/mysql/logger.rs

+        js.set_bool("tls", false)?;
+    }
+
+    if tx.command.is_some() {


if let Some(c) = tx.command ;-)

Fixed, please check again.

catenacyber · 2024-09-23T13:52:56Z

rust/src/mysql/logger.rs

+    Ok(())
+}
+
+fn log_mysql_alert(


What is different with log_mysql ?

Just for my project's demand.I will remove it.

Fixed, please check again.

rust/src/mysql/mysql.rs

catenacyber · 2024-09-23T13:54:10Z

rust/src/mysql/mysql.rs

+
+    client_flags: u32,
+    version: Option<String>,
+    tls: bool,


Do you handle protocol change ? (like STARTTLS does in SMTP)

Yes, Please check this.
https://github.com/Kotodian/suricata/blob/becd84350a53a7b16de80c135d5263908b7b6db1/rust/src/mysql/mysql.rs#L326-L330

catenacyber · 2024-09-23T13:55:08Z

rust/src/mysql/mysql.rs

+
+/// Probe for a valid mysql message
+pub fn probe(input: &[u8]) -> bool {
+    if parse_packet_header(input).is_ok() {


Can not it say "not enough data to know" ?

Looks like parse_packet_header accepts pretty much everything

/// Probe for a valid mysql message pub fn probe(i: &[u8]) -> IResult<&[u8], ()> { let (i, _) = parse_packet_header(i)?; Ok((i, ())) }

Did you mean this?

No, I mean that the probing parser is likely not strict enough

I have another probe function rs_mysql_probing_tc and rs_mysql_probing_ts for real probing

catenacyber · 2024-09-23T13:55:53Z

rust/src/mysql/mysql.rs

+
+/// Get the mysql query
+#[no_mangle]
+pub unsafe extern "C" fn SCMysqlTxGetCommandName(


This is unused now...

Fixed, please check again.

catenacyber · 2024-09-23T13:56:28Z

rust/src/mysql/mysql.rs

+        get_state_data: rs_mysql_get_state_data,
+        apply_tx_config: None,
+        flags: APP_LAYER_PARSER_OPT_ACCEPT_GAPS,
+        get_frame_id_by_name: None,


Are you planning on adding frames support ?

catenacyber

Thanks for this work already.

I left some comments and questions to get deeper

glongo · 2024-09-23T16:59:49Z

rust/src/mysql/mysql.rs

+        }
+    }
+
+    pub fn parse_request(&mut self, flow: *const Flow, i: &[u8]) -> AppLayerResult {


I think there is a corner case to handle here: if a payload length is set to ff ff ff, it means that it is fragmented and the payload should be reassembled before of being parsed.

Ok, i will fix.

When i implement this, i found some problems in rust lifetime that i can't reassemble, do you have any advice?

I have implemented a low performance version, please check.

glongo · 2024-09-23T17:08:15Z

rust/src/mysql/parser.rs

+) -> IResult<&[u8], MysqlRequest> {
+    let (i, header) = parse_packet_header(i)?;
+    let (i, command_code) = be_u8(i)?;
+    match command_code {


This can be rewritten as:

let (i, req) = match command_code { 0x01 => (i, MysqlRequest { command_code, command: MysqlCommand::Quit, }) ... } Ok((i, req))

Fixed, please check again.

glongo · 2024-09-23T17:12:26Z

rust/src/mysql/parser.rs

+    let (i, _tag) = verify(be_u8, |&x| x == 0xfe)(i)?;
+    let (i, warnings) = le_u16(i)?;
+    let (i, status_flags) = le_u16(i)?;
+


warnings and status_flags can be parsed only if the following condition is true:

if capabilities & CLIENT_PROTOCOL_41 != 0

The flags parsed during the handshake phase should be passed here as a parameter to check this condition.

Reference: https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_basic_eof_packet.html

When i parse handshake response, i have checked this.

glongo · 2024-09-23T17:24:50Z

rust/src/mysql/parser.rs

+    ))
+}
+
+fn parse_response_ok(i: &[u8], length: u32) -> IResult<&[u8], MysqlResponse> {


As mentioned above, some fields depend on flags, and certain conditions should be checked before parsing them.
Reference: https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_basic_ok_packet.html

When i parse ok response, i just take the fields what I need.
As mentioned before, when i parse handshake response, i have checked capabilities should contain CLIENT_PROTOCOL_41.

glongo · 2024-09-23T17:25:58Z

rust/src/mysql/parser.rs

+    let (i, error_code) = le_u16(i)?;
+    let (i, _) = take(6_u32)(i)?;
+    // sql state maker & sql state
+    let (i, _) = take(6_u32)(i)?;


sql state marker and state are available only if capabilities & CLIENT_PROTOCOL_41 is true.

When i parse handshake response, i have checked this.

Kotodian · 2024-09-24T04:25:12Z

Are you planning on adding keywords as well ?

No, all of fields are based on my project, if you have any idea in logger, please comment, thx.

#Task 3446

jufajardini · 2024-09-24T13:13:02Z

Are you planning on adding keywords as well ?

No, all of fields are based on my project, if you have any idea in logger, please comment, thx.

The keywords are for making detection easier and more efficient, not for the logging part.
This can, however, be a separate step of the work.

Thanks for incorporating feedback :)

I'll mark this PR as a draft, as there are still a lot of moving parts. Once you consider you've reached something more stable, do submit another PR version, with SV tests, as those work well not only for our CI checks testing but also for showcasing the EVE output.

Task OISF#3446

Kotodian · 2024-09-25T01:51:11Z

Are you planning on adding keywords as well ?

No, all of fields are based on my project, if you have any idea in logger, please comment, thx.

The keywords are for making detection easier and more efficient, not for the logging part. This can, however, be a separate step of the work.

Thanks for incorporating feedback :)

I'll mark this PR as a draft, as there are still a lot of moving parts. Once you consider you've reached something more stable, do submit another PR version, with SV tests, as those work well not only for our CI checks testing but also for showcasing the EVE output.

Ok, I will move my project's detection keywords here, and do more tests.

catenacyber · 2024-10-01T11:46:08Z

Continued in #11842 if I am not mistaken

Kotodian requested review from jasonish, victorjulien and a team as code owners September 19, 2024 08:17

jufajardini reviewed Sep 19, 2024

View reviewed changes

Kotodian force-pushed the dev-3346-mysql-proto-v1 branch from ea503dd to da79f32 Compare September 20, 2024 00:56

mysql: implement app-layer

38ddc06

Task OISF#3446

Kotodian force-pushed the dev-3346-mysql-proto-v1 branch 2 times, most recently from 5083bc9 to 3fe510b Compare September 20, 2024 01:17

mysql: implement logger

becd843

Task OISF#3446

Kotodian force-pushed the dev-3346-mysql-proto-v1 branch from 3fe510b to becd843 Compare September 20, 2024 01:20

catenacyber reviewed Sep 23, 2024

View reviewed changes

rust/src/mysql/mysql.rs Show resolved Hide resolved

catenacyber reviewed Sep 23, 2024

View reviewed changes

glongo reviewed Sep 23, 2024

View reviewed changes

Kotodian force-pushed the dev-3346-mysql-proto-v1 branch from 84f0700 to 512c197 Compare September 24, 2024 04:21

mysql: add packet reassemble and remove unused code

93124a1

#Task 3446

Kotodian force-pushed the dev-3346-mysql-proto-v1 branch from 512c197 to 93124a1 Compare September 24, 2024 09:57

jufajardini marked this pull request as draft September 24, 2024 19:22

mysql: add handhsake protocol link in state and change probe

a4c9962

Task OISF#3446

catenacyber closed this Oct 1, 2024

MySQL protocol parser v1 #11800

MySQL protocol parser v1 #11800

Conversation

Kotodian commented Sep 19, 2024 • edited Loading

victorjulien commented Sep 19, 2024

github-actions bot commented Sep 19, 2024

codecov bot commented Sep 19, 2024 • edited Loading

Codecov Report

jufajardini left a comment • edited Loading

Choose a reason for hiding this comment

glongo commented Sep 19, 2024

Kotodian commented Sep 20, 2024

Choose a reason for hiding this comment

Kotodian Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

catenacyber commented Sep 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kotodian Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catenacyber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kotodian Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glongo Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glongo Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Kotodian Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kotodian commented Sep 24, 2024

jufajardini commented Sep 24, 2024

Kotodian commented Sep 25, 2024

catenacyber commented Oct 1, 2024

Kotodian commented Sep 19, 2024 •

edited

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading

jufajardini left a comment •

edited

Loading

Kotodian Sep 24, 2024 •

edited

Loading

Kotodian Sep 24, 2024 •

edited

Loading

Kotodian Sep 24, 2024 •

edited

Loading

glongo Sep 23, 2024 •

edited

Loading

glongo Sep 23, 2024 •

edited

Loading

Kotodian Sep 24, 2024 •

edited

Loading