Bug #5868
closedfilestore: not saving files when filestore enabled by rule matching on file_data (instead saves 0 bytes)
Description
So I noticed that Suricata was occasionally creating zero sized files in the file store, despite the related JSON saying it should be larger.
All these settings should be fine as from the looks of https://suricata.readthedocs.io/en/suricata-6.0.10/file-extraction/file-extraction.html it should not be placing a upper limit on size.
I've found I can repeatedly reproduce this issue via grabbing putty. That said dropped a copy at http://vvelox.net/test.exe to make testing easier via wgetting as I could not think of where else off hand there is a executable to test with that is easily available via http and not https as well as not redirecting to https.
Example JSON in which it created a zero sized file...
{ "timestamp": "2023-02-16T20:41:58.380676+0000", "flow_id": 1064488670622519, "in_iface": "eth1", "event_type": "fileinfo", "src_ip": "98.102.84.2", "src_port": 80, "dest_ip": "10.112.39.45", "dest_port": 58856, "proto": "TCP", "http": { "hostname": "vvelox.net", "url": "/test.exe", "http_user_agent": "Wget/1.18 (linux-gnu)", "http_content_type": "application/x-msdownload", "http_method": "GET", "protocol": "HTTP/1.1", "status": 200, "length": 1647912 }, "app_proto": "http", "fileinfo": { "filename": "/test.exe", "sid": [ 2018959 ], "magic": "PE32+ executable (GUI) x86-64, for MS Windows", "gaps": false, "state": "CLOSED", "md5": "f838fdafd0881cf1e6040a07d78e840d", "sha1": "2a35456b2f67bd12905378beb6eaf373f6a0d0d1", "sha256": "fc6f9dbdf4b9f8dd1f5f3a74cb6e55119d3fe2c9db52436e10ba07842e6c3d7c", "stored": true, "file_id": 243, "size": 1647912, "tx_id": 0 } }
Config bits for .stream
...
memcap: 64mb checksum-validation: no # reject wrong csums inline: auto reassembly: memcap: 256mb depth: 0 toserver-chunk-size: 2560 toclient-chunk-size: 2560 randomize-chunk-size: yes
Config bits from .app-layer.protocols.http
...
enabled: yes libhtp: default-config: personality: IDS request-body-limit: 0 response-body-limit: 0 request-body-minimal-inspect-size: 32kb request-body-inspect-window: 4kb response-body-minimal-inspect-size: 40kb response-body-inspect-window: 16kb response-body-decompress-layer-limit: 2 http-body-inline: auto swf-decompression: enabled: yes type: both compress-depth: 0 decompress-depth: 0 double-decode-path: no double-decode-query: no server-config:
Output section...
- file-store: version: 2 enabled: yes dir: /var/log/suricata/files write-fileinfo: yes stream-depth: 5242880 force-hash: [sha1, md5] xff: enabled: no mode: extra-data deployment: reverse header: X-Forwarded-For
Output from suricata --build-info
...
This is Suricata version 6.0.10 RELEASE Features: PCAP_SET_BUFF AF_PACKET HAVE_PACKET_FANOUT LIBCAP_NG LIBNET1.1 HAVE_HTP_URI_NORMALIZE_HOOK PCRE_JIT HAVE_NSS HAVE_LIBJANSSON TLS TLS_C11 MAGIC RUST SIMD support: SSE_4_2 SSE_4_1 SSE_3 Atomic intrinsics: 1 2 4 8 16 byte(s) 64-bits, Little-endian architecture GCC version 6.3.0 20170516, C version 201112 compiled with -fstack-protector compiled with _FORTIFY_SOURCE=2 L1 cache line size (CLS)=64 thread local storage method: _Thread_local compiled with LibHTP v0.5.42, linked against LibHTP v0.5.42 Suricata Configuration: AF_PACKET support: yes eBPF support: no XDP support: no PF_RING support: no NFQueue support: no NFLOG support: no IPFW support: no Netmap support: no using new api: no DAG enabled: no Napatech enabled: no WinDivert enabled: no Unix socket enabled: yes Detection enabled: yes Libmagic support: yes libnss support: yes libnspr support: yes libjansson support: yes hiredis support: yes hiredis async with libevent: yes Prelude support: no PCRE jit: yes LUA support: no libluajit: no GeoIP2 support: no Non-bundled htp: no Hyperscan support: yes Libnet support: yes liblz4 support: yes HTTP2 decompression: no Rust support: yes Rust strict mode: no Rust compiler path: /usr/local/bin/rustc Rust compiler version: rustc 1.56.1 (59eed8a2a 2021-11-01) Cargo path: /usr/local/bin/cargo Cargo version: cargo 1.56.0 (4ed5d137b 2021-10-04) Cargo vendor: yes Python support: yes Python path: /usr/bin/python3 Install suricatactl: yes Install suricatasc: yes Install suricata-update: yes Profiling enabled: no Profiling locks enabled: no Plugin support (experimental): yes Development settings: Coccinelle / spatch: no Unit tests enabled: no Debug output enabled: no Debug validation enabled: no Generic build parameters: Installation prefix: /usr Configuration directory: /etc/suricata/ Log directory: /var/log/suricata/ --prefix /usr --sysconfdir /etc --localstatedir /var --datarootdir /usr/share Host: x86_64-pc-linux-gnu Compiler: gcc (exec name) / g++ (real) GCC Protect enabled: yes GCC march native enabled: yes GCC Profile enabled: yes Position Independent Executable enabled: no CFLAGS -g -O2 -std=c11 -pg -march=native -I${srcdir}/../rust/gen -I${srcdir}/../rust/dist PCAP_CFLAGS -I/usr/include SECCFLAGS -fstack-protector -D_FORTIFY_SOURCE=2 -Wformat -Wformat-security
Files
Updated by Zane B-H over 1 year ago
Just realized I forgot to include the rule in question...
alert http any any -> any any (msg:"ET POLICY PE EXE or DLL Windows file download HTTP"; flow:established,to_client; flowbits:isnotset,ET.http.binary; flowbits:isnotset,ET.INFO.WindowsUpdate; file_data; content:"MZ"; within:2; byte_jump:4,58,relative,little; content:"PE|00 00|"; distance:-64; within:4; flowbits:set,ET.http.binary; filestore; flowbits:noalert; classtype:policy-violation; sid:2018959; rev:13;)
Also did some more testing and very indifferent in regards to file size. Tested with one file that triggers this that is only 10k in size and it fails. That said I know it works as I've seen it grab ones that are up to 500k or so in size.
Updated by Jason Ish over 1 year ago
- Status changed from New to Assigned
- Assignee changed from OISF Dev to Jason Ish
Assigning to me for reproduction.
Updated by Victor Julien over 1 year ago
- Related to Bug #3809: Thresholding file-store rule with flowbits saves empty file to disk added
Updated by Jason Ish over 1 year ago
- File test.exe.pcap test.exe.pcap added
I have successfully reproduced on the 6.0.x branch. It is not reproducible on the master branch. The traffic does not need to be live and can be reproduced using a pcap as well.
Updated by Jason Ish over 1 year ago
- Subject changed from filestore recording some files as zero sized, despite the json saying it should be larger to filestore: not saving files when filestore enabled from rules (instead saves 0 bytes)
Further notes:
- Works when filestore is forced
- Fails when a rule matches on file_data to save the file
- Works on match without file_data
Doesn't appear to be just this test exe file. Fails for me as well with the Suricata user manual which is a PDF.
Updated by Jason Ish over 1 year ago
- Subject changed from filestore: not saving files when filestore enabled from rules (instead saves 0 bytes) to filestore: not saving files when filestore enabled by rule matching on file_data (instead saves 0 bytes)
Updated by Andreas Herz over 1 year ago
I can confirm that bug as well and found another diff based on your notes.
If you use `force_filestore` the file is 100k but with the rule match it reports 1012k in filesize which is actually the correct one, since fileinfo reports in both cases the same size (just in bytes instead of kb).
So the `force-filestore` might be wrong somehow as well.
Updated by Jason Ish over 1 year ago
Andreas Herz wrote in #note-8:
I can confirm that bug as well and found another diff based on your notes.
If you use `force_filestore` the file is 100k but with the rule match it reports 1012k in filesize which is actually the correct one, since fileinfo reports in both cases the same size (just in bytes instead of kb).
So the `force-filestore` might be wrong somehow as well.
If HTTP, 100k is inline with the default libhtp request-body-limit
. But I'd expect that to apply when enabling the file-store via a rule as well.
Updated by Jason Ish over 1 year ago
- Status changed from Assigned to In Review
- Target version changed from TBD to 6.0.13
Updated by Andreas Herz over 1 year ago
- Related to Bug #4016: filesize with filestore store empty files added
Updated by Victor Julien over 1 year ago
- Target version changed from 6.0.13 to 6.0.14
Updated by Jason Ish over 1 year ago
- Related to Bug #6120: streaming-buffer: exceeds limit when downloading large file with file-store enabled added
Updated by Jason Ish over 1 year ago
- Related to Bug #6170: streaming-buffer: exceeds limit when downloading large file with file-store enabled and inspecing file_data content added
Updated by Jason Ish over 1 year ago
- Target version changed from 6.0.14 to 7.0.0
- Affected Versions 7.0.0-rc2 added
- Label Needs backport to 6.0 added
Adding 7.0 as an affected version now, and adding backport label as git master and master-6.0.x are now in sync on this issue.
Updated by OISF Ticketbot over 1 year ago
- Label deleted (
Needs backport to 6.0)
Updated by Jason Ish over 1 year ago
PR for review: https://github.com/OISF/suricata/pull/9063
Updated by Victor Julien over 1 year ago
- Related to Optimization #4141: file.data: inspect File objects for HTTP added
Updated by Jason Ish over 1 year ago
- Status changed from In Review to New
- Assignee changed from Jason Ish to OISF Dev
Re-assigning back to OISF Dev as I likely won't complete it before vacation.
Looking at it from a 6.0.0 perspective as #4141 is the ideal fix for master branch we see:
- In detect-file-data.c, file_data
inspection is deferred until the complete response body is seen (IDS mode)
- FilePruneFile
meanwhile slides the file buffer forward without storing.
- By the time file-data
does detection, the file has been completely pruned
This is using the pcap in https://github.com/OISF/suricata-verify/pull/1225, which is a small file of 17869 bytes.
#6170 is more or less the same bug, but also results in the file stream buffer growing to its maximum size. This is because we see the above behaviour until htp_state->cfg->response.inspect_min_size
is met, assuming the content that is matching is within this region, file_data
will match and FILE_STORE
is now set, however left_edge
is consistently 0 so the stream buffer never slides forward, exhausting the buffer.
Its important to note that is only happens in IDS mode, it does not happen inline mode is that check for a minimum amount of response body is skipped, resulting in file_data
detection running sooner, resulting in FILE_STORE
being set sooner and proper file pruning happening sooner. A simplistic fix might be to remove this check so IPS mode and IDS mode have the same logic here. In fact, doing so appears to fix the reproduction cases for this bug and #6170 with no noticeable side affects. I see a repeatable 5% or so performance increase on large files. I'm not really clear why we treat IDS and IPS mode differently here.
Another fix appears to be delaying file pruning until at least htp_state->cfg->response.inspect_min_size
(must be larger than file->inspect_min_size) have been seen. This will let file_data
run and set FILE_STORE
sooner fixing this bug and #6170. Perhaps when creating the file container, the http1 code could set a size before pruning would take effect?
TODO: Observe what happens with the file_data
content occurs well after inspect_min_size
, for example a few hundred kb into the response body.
Updated by Victor Julien over 1 year ago
- Target version changed from 7.0.0 to 7.0.1
Updated by Victor Julien over 1 year ago
- Status changed from New to Resolved
- Assignee changed from OISF Dev to Jeff Lucovsky
- Target version changed from 7.0.1 to 7.0.0
Updated by Victor Julien over 1 year ago
- Related to Task #6217: research: increased tcp.overlap after file data changes added
Updated by Victor Julien over 1 year ago
- Related to Bug #6171: filestore: not saving files when filestore enabled by rule matching on file_data (instead saves 0 bytes) (6.0.x backport) added
Updated by Victor Julien over 1 year ago
- Status changed from Resolved to Closed