Project

General

Profile

Actions

Bug #5868

closed

filestore: not saving files when filestore enabled by rule matching on file_data (instead saves 0 bytes)

Added by Zane B-H almost 2 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
Normal
Assignee:
Target version:
Affected Versions:
Effort:
Difficulty:
Label:

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

test.exe.pcap (1.72 MB) test.exe.pcap Jason Ish, 05/25/2023 10:25 PM

Related issues 7 (3 open4 closed)

Related to Suricata - Bug #3809: Thresholding file-store rule with flowbits saves empty file to diskNewOISF DevActions
Related to Suricata - Bug #4016: filesize with filestore store empty filesNewCommunity TicketActions
Related to Suricata - Bug #6120: streaming-buffer: exceeds limit when downloading large file with file-store enabledClosedVictor JulienActions
Related to Suricata - Bug #6170: streaming-buffer: exceeds limit when downloading large file with file-store enabled and inspecing file_data contentClosedOISF DevActions
Related to Suricata - Optimization #4141: file.data: inspect File objects for HTTPClosedJeff LucovskyActions
Related to Suricata - Task #6217: research: increased tcp.overlap after file data changesNewVictor JulienActions
Related to Suricata - Bug #6171: filestore: not saving files when filestore enabled by rule matching on file_data (instead saves 0 bytes) (6.0.x backport)RejectedActions
Actions #1

Updated by Zane B-H almost 2 years 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.

Actions #2

Updated by Zane B-H over 1 year ago

  • Affected Versions 6.0.11, 6.0.12 added
Actions #3

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.

Actions #4

Updated by Victor Julien over 1 year ago

  • Related to Bug #3809: Thresholding file-store rule with flowbits saves empty file to disk added
Actions #5

Updated by Jason Ish over 1 year ago

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.

Actions #6

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.

Actions #7

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)
Actions #8

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.

Actions #9

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.

Actions #10

Updated by Jason Ish over 1 year ago

  • Status changed from Assigned to In Review
  • Target version changed from TBD to 6.0.13
Actions #11

Updated by Andreas Herz over 1 year ago

  • Related to Bug #4016: filesize with filestore store empty files added
Actions #12

Updated by Victor Julien over 1 year ago

  • Target version changed from 6.0.13 to 6.0.14
Actions #13

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
Actions #14

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
Actions #15

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.

Actions #16

Updated by Jason Ish over 1 year ago

  • Affected Versions 6.0.13 added
Actions #17

Updated by OISF Ticketbot over 1 year ago

  • Subtask #6171 added
Actions #18

Updated by OISF Ticketbot over 1 year ago

  • Label deleted (Needs backport to 6.0)
Actions #20

Updated by Victor Julien over 1 year ago

Actions #21

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.

Actions #22

Updated by Victor Julien over 1 year ago

  • Target version changed from 7.0.0 to 7.0.1
Actions #23

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
Actions #24

Updated by Victor Julien over 1 year ago

  • Related to Task #6217: research: increased tcp.overlap after file data changes added
Actions #25

Updated by Victor Julien over 1 year ago

  • Subtask deleted (#6171)
Actions #26

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
Actions #27

Updated by Victor Julien over 1 year ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF