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

detect/filestore: fix options handling and impact #12312

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 1 addition & 22 deletions doc/userguide/file-extraction/file-extraction.rst
Original file line number Diff line number Diff line change
Expand Up @@ -133,28 +133,7 @@ Or rather all actual pdf files?

::

alert http any any -> any any (msg:"FILE pdf detected"; filemagic:"PDF document"; filestore; sid:3; rev:1;)


Or rather only store files from black list checksum md5 ?

::

alert http any any -> any any (msg:"Black list checksum match and extract MD5"; filemd5:fileextraction-chksum.list; filestore; sid:4; rev:1;)


Or only store files from black list checksum sha1 ?

::

alert http any any -> any any (msg:"Black list checksum match and extract SHA1"; filesha1:fileextraction-chksum.list; filestore; sid:5; rev:1;)


Or finally store files from black list checksum sha256 ?

::

alert http any any -> any any (msg:"Black list checksum match and extract SHA256"; filesha256:fileextraction-chksum.list; filestore; sid:6; rev:1;)
alert http any any -> any any (msg:"FILE pdf detected"; file.magic; content:"PDF document"; filestore; sid:3; rev:1;)

Bundled with the Suricata download, is a file with more example rules. In the archive, go to the `rules` directory and check the ``files.rules`` file.

Expand Down
18 changes: 9 additions & 9 deletions doc/userguide/rules/file-keywords.rst
Original file line number Diff line number Diff line change
Expand Up @@ -162,12 +162,12 @@ Syntax::

The filename is expanded to include the rule dir. In the default case
it will become /etc/suricata/rules/filename. Use the exclamation mark
to get a negated match. This allows for white listing.
to get a negated match. This allows for safe listing.

Examples::

filemd5:md5-blacklist;
filemd5:!md5-whitelist;
filemd5:md5-badlist;
filemd5:!md5-goodlist;

*File format*

Expand Down Expand Up @@ -206,12 +206,12 @@ Syntax::

The filename is expanded to include the rule dir. In the default case
it will become /etc/suricata/rules/filename. Use the exclamation mark
to get a negated match. This allows for white listing.
to get a negated match. This allows for safe listing.

Examples::

filesha1:sha1-blacklist;
filesha1:!sha1-whitelist;
filesha1:sha1-badlist;
filesha1:!sha1-goodlist;

*File format*

Expand All @@ -228,12 +228,12 @@ Syntax::

The filename is expanded to include the rule dir. In the default case
it will become /etc/suricata/rules/filename. Use the exclamation mark
to get a negated match. This allows for white listing.
to get a negated match. This allows for safe listing.

Examples::

filesha256:sha256-blacklist;
filesha256:!sha256-whitelist;
filesha256:sha256-badlist;
filesha256:!sha256-goodlist;

*File format*

Expand Down
29 changes: 12 additions & 17 deletions rules/files.rules
Original file line number Diff line number Diff line change
Expand Up @@ -12,45 +12,40 @@
#alert http any any -> any any (msg:"FILESTORE pdf"; flow:established,to_server; fileext:"pdf"; filestore; sid:8; rev:1;)

# Store all PDF files, regardless of their name.
#alert http any any -> any any (msg:"FILEMAGIC pdf"; flow:established,to_server; filemagic:"PDF document"; filestore; sid:9; rev:1;)
#alert http any any -> any any (msg:"FILEMAGIC pdf"; flow:established,to_server; file.magic; content:"PDF document"; filestore; sid:9; rev:1;)

# Same for JPEG's.
#alert http any any -> any any (msg:"FILEMAGIC jpg(1)"; flow:established,to_server; filemagic:"JPEG image data"; filestore; sid:10; rev:1;)
#alert http any any -> any any (msg:"FILEMAGIC jpg(2)"; flow:established,to_server; filemagic:"JFIF"; filestore; sid:11; rev:1;)
#alert http any any -> any any (msg:"FILEMAGIC jpg(1)"; flow:established,to_server; file.magic; content:"JPEG image data"; filestore; sid:10; rev:1;)
#alert http any any -> any any (msg:"FILEMAGIC jpg(2)"; flow:established,to_server; file.magic; content:"JFIF"; filestore; sid:11; rev:1;)

# Unusually short file
#alert http any any -> any any (msg:"FILEMAGIC short"; flow:established,to_server; filemagic:"very short file (no magic)"; filestore; sid:12; rev:1;)
#alert http any any -> any any (msg:"FILEMAGIC short"; flow:established,to_server; file.magic; content:"very short file (no magic)"; filestore; sid:12; rev:1;)

# Simply store all files we encounter, no alerts.
#alert http any any -> any any (msg:"FILE store all"; filestore; noalert; sid:15; rev:1;)

# Store all JPG files, don't alert.
#alert http any any -> any any (msg:"FILE magic"; filemagic:"JFIF"; filestore; noalert; sid:16; rev:1;)
#alert http any any -> any any (msg:"FILE magic"; filemagic:"GIF"; filestore; noalert; sid:23; rev:1;)
#alert http any any -> any any (msg:"FILE magic"; filemagic:"PNG"; filestore; noalert; sid:17; rev:1;)
#alert http any any -> any any (msg:"FILE magic"; file.magic; content:"JFIF"; filestore; noalert; sid:16; rev:1;)
#alert http any any -> any any (msg:"FILE magic"; file.magic; content:"GIF"; filestore; noalert; sid:23; rev:1;)
#alert http any any -> any any (msg:"FILE magic"; file.magic; content:"PNG"; filestore; noalert; sid:17; rev:1;)

# Store all Windows executables
#alert http any any -> any any (msg:"FILE magic -- windows"; flow:established,to_client; filemagic:"executable for MS Windows"; filestore; sid:18; rev:1;)
#alert http any any -> any any (msg:"FILE magic -- windows"; flow:established,to_client; file.magic; content:"executable for MS Windows"; filestore; sid:18; rev:1;)

# Alert on PNG with 1x1 pixels (tracking)
#alert http any any -> any any (msg:"FILE tracking PNG (1x1 pixel) (1)"; filemagic:"PNG image data, 1 x 1,"; sid:19; rev:1;)
#alert http any any -> any any (msg:"FILE tracking PNG (1x1 pixel) (2)"; filemagic:"PNG image data, 1 x 1|00|"; sid:20; rev:1;)
#alert http any any -> any any (msg:"FILE tracking PNG (1x1 pixel) (1)"; file.magic; content:"PNG image data, 1 x 1,"; sid:19; rev:1;)
#alert http any any -> any any (msg:"FILE tracking PNG (1x1 pixel) (2)"; file.magic; content:"PNG image data, 1 x 1|00|"; sid:20; rev:1;)

# Alert on GIF with 1x1 pixels (tracking)
# The pattern matches on |00| which is the end of the magic buffer, this way we won't match on 1 x 128.
#alert http any any -> any any (msg:"FILE tracking GIF (1x1 pixel)"; filemagic:"GIF image data, version 89a, 1 x 1|00|"; sid:21; rev:1;)
#alert http any any -> any any (msg:"FILE tracking GIF (1x1 pixel)"; file.magic; content:"GIF image data, version 89a, 1 x 1|00|"; sid:21; rev:1;)

# Alert and store pdf attachment but not pdf file
#alert http any any -> any any (msg:"FILE pdf claimed, but not pdf"; flow:established,to_client; fileext:"pdf"; filemagic:!"PDF document"; filestore; sid:22; rev:1;)
#alert http any any -> any any (msg:"FILE pdf claimed, but not pdf"; flow:established,to_client; fileext:"pdf"; file.magic; content:!"PDF document"; filestore; sid:22; rev:1;)

# Alert and store files over SMTP
#alert smtp any any -> any any (msg:"File Found over SMTP and stored"; filestore; sid:27; rev:1;)

# Alert and store files from black list checksum: md5 or sha1 or sha256
#alert http any any -> any any (msg:"Black list checksum match and extract MD5"; filemd5:fileextraction-chksum.list; filestore; sid:28; rev:1;)
#alert http any any -> any any (msg:"Black list checksum match and extract SHA1"; filesha1:fileextraction-chksum.list; filestore; sid:29; rev:1;)
#alert http any any -> any any (msg:"Black list checksum match and extract SHA256"; filesha256:fileextraction-chksum.list; filestore; sid:30; rev:1;)

# Alert and store files over FTP
#alert ftp-data any any -> any any (msg:"File Found within FTP and stored"; filestore; filename:"password"; ftpdata_command:stor; sid:31; rev:1;)

Expand Down
8 changes: 5 additions & 3 deletions src/app-layer-htp.c
Original file line number Diff line number Diff line change
Expand Up @@ -5802,6 +5802,8 @@ libhtp:\n\

de_ctx->flags |= DE_QUIET;

/* Signature alerts on http without specifying a way and filestore is just
post match. */
de_ctx->sig_list = SigInit(de_ctx,"alert http any any -> any any "
"(filestore; sid:1; rev:1;)");
FAIL_IF_NULL(de_ctx->sig_list);
Expand All @@ -5816,12 +5818,12 @@ libhtp:\n\
http_state = f.alstate;
FAIL_IF_NULL(http_state);

/* do detect */
/* do detect, first one so will match */
SigMatchSignatures(&th_v, de_ctx, det_ctx, p1);

FAIL_IF((PacketAlertCheck(p1, 1)));
FAIL_IF(!(PacketAlertCheck(p1, 1)));

/* do detect */
/* do detect, second one so will not match */
SigMatchSignatures(&th_v, de_ctx, det_ctx, p1);

FAIL_IF((PacketAlertCheck(p1, 1)));
Expand Down
22 changes: 21 additions & 1 deletion src/detect-engine-file.c
Original file line number Diff line number Diff line change
Expand Up @@ -186,14 +186,34 @@ uint8_t DetectFileInspectGeneric(DetectEngineCtx *de_ctx, DetectEngineThreadCtx
SCEnter();
DEBUG_VALIDATE_BUG_ON(f->alstate != alstate);

const uint8_t direction = flags & (STREAM_TOSERVER|STREAM_TOCLIENT);
const uint8_t direction = flags & (STREAM_TOSERVER | STREAM_TOCLIENT);
AppLayerGetFileState files = AppLayerParserGetTxFiles(f, tx, direction);
FileContainer *ffc = files.fc;
SCLogDebug("tx %p tx_id %" PRIu64 " ffc %p ffc->head %p sid %u", tx, tx_id, ffc,
ffc ? ffc->head : NULL, s->id);
if (ffc == NULL) {
SCReturnInt(DETECT_ENGINE_INSPECT_SIG_CANT_MATCH_FILES);
} else if (ffc->head == NULL) {
if (s->flags & SIG_FLAG_FILESTORE) {
/* If the signature has filestore, we need to check if we are in
a scope of capture where we need to prepare the capture for
an upcoming file. */
if (s->filestore_ctx && (s->filestore_ctx->scope == FILESTORE_SCOPE_TX)) {
/* In scope TX, we need to prepare file storage for file that could
appear on the transaction so we store the transaction.
We need to only increment filestore_cnt if the tx_id is changed.
*/
if (det_ctx->filestore_cnt == 0 ||
det_ctx->filestore[det_ctx->filestore_cnt - 1].tx_id != tx_id) {
det_ctx->filestore[det_ctx->filestore_cnt].file_id = 0;
det_ctx->filestore[det_ctx->filestore_cnt].tx_id = tx_id;
det_ctx->filestore_cnt++;
}
}
/* Other scopes than TX are going to be handled in post match without
Copy link
Contributor

Choose a reason for hiding this comment

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

But postmatch does not make signature fail to match, does it ?

I see (void)sigmatch_table[smd->type].Match(det_ctx, p, s, smd->ctx); return code ignored in DetectRunPostMatch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, before we were not going to postmatch as the filestore without file was the cases where ffc or ffc->head were null and as a result it was a NO_MATCH.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand a bit more.
Why not handle FILESTORE_SCOPE_TX in postmatch as well ?

Is it not a problem if the signature matched so far but will not fully match in the end ?

any setup needed here so we can just return a match for them. */
SCReturnInt(DETECT_ENGINE_INSPECT_SIG_MATCH);
}
SCReturnInt(DETECT_ENGINE_INSPECT_SIG_NO_MATCH);
Copy link
Contributor

Choose a reason for hiding this comment

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

When do we get ffc != NULL && ffc->head == NULL ?

This still looks fishy to me as a signature with file.data that does not match today because of this SCReturnInt(DETECT_ENGINE_INSPECT_SIG_NO_MATCH), will cause FP with this PR...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Talking about fishy, I think it is a good catch ;) I'm going to add a suricata-verify test so we are sure this case is handled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but do you know, in plain English, when we have the cases :

  • ffc == NULL (protocol or direction not supporting files ?)
  • ffc != NULL && ffc->head == NULL protocol supporting files but tx does not have a file in this direction ? (that is what I expected but this seems a wrong expectation)

}

Expand Down
80 changes: 65 additions & 15 deletions src/detect-filestore.c
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,47 @@ void DetectFilestoreRegister(void)
g_file_match_list_id = DetectBufferTypeRegister("files");
}

static void FilestoreTriggerFlowStorage(
Flow *f, int toserver_dir, int toclient_dir, uint64_t init_tx_id)
{
SCLogDebug("init tx_id is: %ld", init_tx_id);
/* set flags in Flow and AppLayerStateData */
AppLayerStateData *sd = AppLayerParserGetStateData(f->proto, f->alproto, f->alstate);
if (toclient_dir) {
f->file_flags |= FLOWFILE_STORE_TC;
if (sd != NULL) {
sd->file_flags |= FLOWFILE_STORE_TC;
}
}
if (toserver_dir) {
f->file_flags |= FLOWFILE_STORE_TS;
if (sd != NULL) {
sd->file_flags |= FLOWFILE_STORE_TS;
}
}
/* Start storage for all existing files attached to the flow in correct direction */
void *alstate = FlowGetAppState(f);
if (alstate != NULL) {
uint64_t total_txs = AppLayerParserGetTxCnt(f, alstate);
for (uint64_t tx_id = init_tx_id; tx_id < total_txs; tx_id++) {
void *txv = AppLayerParserGetTx(f->proto, f->alproto, alstate, tx_id);
DEBUG_VALIDATE_BUG_ON(txv == NULL);
if (txv != NULL) {
AppLayerTxData *txd = AppLayerParserGetTxData(f->proto, f->alproto, txv);
DEBUG_VALIDATE_BUG_ON(txd == NULL);
if (txd != NULL) {
if (toclient_dir) {
txd->file_flags |= FLOWFILE_STORE_TC;
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think we need to set AppLayerTxData file_flags here as we already set AppLayerStateData one, and in the end we use a logical or of both

}
if (toserver_dir) {
txd->file_flags |= FLOWFILE_STORE_TS;
}
}
}
}
}
}

/**
* \brief apply the post match filestore with options
*/
Expand Down Expand Up @@ -170,20 +211,7 @@ static int FilestorePostMatchWithOptions(Packet *p, Flow *f, const DetectFilesto
}
}
} else if (this_flow) {
/* set in flow and AppLayerStateData */
AppLayerStateData *sd = AppLayerParserGetStateData(f->proto, f->alproto, f->alstate);
if (toclient_dir) {
f->file_flags |= FLOWFILE_STORE_TC;
if (sd != NULL) {
sd->file_flags |= FLOWFILE_STORE_TC;
}
}
if (toserver_dir) {
f->file_flags |= FLOWFILE_STORE_TS;
if (sd != NULL) {
sd->file_flags |= FLOWFILE_STORE_TS;
}
}
FilestoreTriggerFlowStorage(f, toserver_dir, toclient_dir, tx_id);
} else {
FileStoreFileById(fc, file_id);
}
Expand All @@ -207,11 +235,33 @@ static int DetectFilestorePostMatch(DetectEngineThreadCtx *det_ctx,
{
SCEnter();

DEBUG_VALIDATE_BUG_ON(p->flow == NULL);

if (det_ctx->filestore_cnt == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, det_ctx->filestore_cnt looks fishy to me cf OISF/suricata-verify#1999

Its scope is one packet when a signature can take multiple packets to match...

/* here we have no file but the signature is fully matched and
filestore option indicate we need to extract for file for the session
so we trigger flow storage. */
if (s->filestore_ctx && (s->filestore_ctx->scope == FILESTORE_SCOPE_SSN)) {
int toserver_dir = 0;
int toclient_dir = 0;
switch (s->filestore_ctx->direction) {
case FILESTORE_DIR_BOTH:
toserver_dir = 1;
toclient_dir = 1;
break;
case FILESTORE_DIR_TOSERVER:
toserver_dir = 1;
break;
case FILESTORE_DIR_TOCLIENT:
toclient_dir = 1;
break;
}
FilestoreTriggerFlowStorage(p->flow, toserver_dir, toclient_dir, det_ctx->tx_id);
}
SCReturnInt(0);
}

if ((s->filestore_ctx == NULL && !(s->flags & SIG_FLAG_FILESTORE)) || p->flow == NULL) {
if (s->filestore_ctx == NULL && !(s->flags & SIG_FLAG_FILESTORE)) {
#ifndef DEBUG
SCReturnInt(0);
#else
Expand Down
Loading