Security #6770
closedlog: arbitrary-length value can be logged
Description
Found by oss-fuzz:
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64345&q=label%3AProj-suricata&can=2
Fuzz target triggers the following rulealert tcp any any -> any any (msg:"SURICATA STREAM ESTABLISHED packet out of window"; stream-event:est_packet_out_of_window; classtype:protocol-command-decode; sid:2210020; rev:2;)
on many packets leading to most time spent in jsonbuild set_string_from_bytes (doing escaping on binary buffer) for dummy overlong ssh software version
Files
Subtasks
Updated by Philippe Antoine over 1 year ago
- Status changed from New to In Review
- Label Needs backport to 6.0, Needs backport to 7.0 added
Gitlab
Updated by Philippe Antoine over 1 year ago
- Label deleted (
Needs backport to 6.0, Needs backport to 7.0)
Updated by Victor Julien over 1 year ago
For per packet anomaly events (decoder, stream) should we not log app-layer.
We need to consider thresholding these events.
Updated by Victor Julien over 1 year ago
Could also log app-layer and other expensive records only once per {decoder,stream} events.
Updated by Victor Julien over 1 year ago
Per protocol review of limits of sizes of fields to log.
Updated by Victor Julien over 1 year ago
- Severity changed from MODERATE to HIGH
Updated by Philippe Antoine over 1 year ago
- Status changed from In Review to In Progress
Victor Julien wrote in #note-7:
For per packet anomaly events (decoder, stream) should we not log app-layer.
We need to consider thresholding these events.
Fuzzer found them because it uses alerts that use these events... So, this is not about anomaly logging, right ?
Updated by Philippe Antoine over 1 year ago
- Related to Bug #6846: eve/alerts: wrongly using tx id 0 when there is no tx added
Updated by Victor Julien over 1 year ago
- Copied to Security #6866: eve: excessive ssh long banner logging added
Updated by Philippe Antoine over 1 year ago
What is the next step ?
Looking for other protocols besides SSH ?
Updated by Philippe Antoine over 1 year ago
- Related to Security #6900: http2: timeout logging headers added
Updated by Philippe Antoine over 1 year ago
#6900 is a variant where it is not an especially long field, but an array of 30k elements...
Updated by Philippe Antoine about 1 year ago
- Related to Bug #6984: mqtt: do not log non-string messages? added
Updated by Philippe Antoine about 1 year ago
I guess the question is now, with a field like MQTT msg cf #6984 could an attacker trigger the logging of this arbitrary long value an arbitrary number of times ?
Updated by Philippe Antoine about 1 year ago
Philippe Antoine wrote in #note-19:
I guess the question is now, with a field like MQTT msg cf #6984 could an attacker trigger the logging of this arbitrary long value an arbitrary number of times ?
The only way I see to do that requires that there is a PACKET_ALERT_FLAG_STREAM_MATCH
, that means a signature using content
on stream without any app-layer transaction keyword
1. The attacker creates a first transaction with a big MQTT msg, but does not end it, (no response from the other side except TCP ACK)
2. The attacker creates then a very big MQTT PDU, for which Suricata will skip the body, but sends many packet matching the signatures.
So, We have alert ip any any -> any any (content: "evilpattern"; sid: 1;)
We have one MQTT flow with a first MQTT PDU being a CONNECT with a big and binary will message (limited by app-layer.protocols.mqtt.max-msg-length
1Mbyte by default)
The server acks it, but does not send a mqtt CONNACK
The client sends a MQTT header with a payload length greater than app-layer.protocols.mqtt.max-msg-length
like 256 Mbyte
The client then sends many packets with "evilpattern" up to 256 Mbytes
For each packet, we get an alert, and we log the closest tx app-layer metadata
TL;DR
Keeping track of modified txs and having AppLayerParserGetTransactionInspectId
return PACKET_ALERT_NOTX
when there was no modified tx would solve this (and other problems)
Updated by Philippe Antoine about 1 year ago
- Related to Security #6987: modbus: txs without responses are never freed added
Updated by Philippe Antoine about 1 year ago
Keeping track of modified txs and having AppLayerParserGetTransactionInspectId return PACKET_ALERT_NOTX when there was no modified tx would solve this (and other problems)
Not enough if I match on the stream data of the transaction (like a big file over HTTP/2), after having had too many/big headers to log...
Investigating if we can avoid to duplicate logging the same app-layer metadata for multiple alerts over one stream-only signature
Updated by Philippe Antoine about 1 year ago
- Status changed from In Progress to In Review
Poc Gitlab MR
Updated by Philippe Antoine about 1 year ago
- Related to Security #7085: eve: transactions can be logged an arbitrary number of times added
Updated by Philippe Antoine 8 months ago
There were already some fixes merged and going on like #7280
I wonder what we want to do next here...
Updated by Philippe Antoine 6 months ago
- Status changed from In Review to In Progress
- Assignee changed from Philippe Antoine to OISF Dev
Updated by Philippe Antoine 5 months ago
- Status changed from In Progress to Closed
I think all the work is done