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.

Sign up for GitHub

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

file/swf: Use lzma-rs decompression instead of libhtp. v2 #8132

Closed
wants to merge 2 commits into from
Closed

file/swf: Use lzma-rs decompression instead of libhtp. v2 #8132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
98a0eb5
file/swf: Use lzma-rs decompression instead of libhtp.
cccs-rtmorti Jul 8, 2022
db1114e
swf-decompression: Disable by default.
cccs-rtmorti Oct 31, 2022
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
1 change: 1 addition & 0 deletions rust/Cargo.toml.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ bitflags = "~1.2.1"
byteorder = "~1.4.2"
uuid = "~0.8.2"
crc = "~1.8.1"
lzma-rs = { version = "~0.2.0", features = ["stream"] }
memchr = "~2.4.1"
num = "~0.2.1"
num-derive = "~0.2.5"
Expand Down
1 change: 1 addition & 0 deletions rust/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,5 +120,6 @@ pub mod http2;
pub mod quic;
pub mod bittorrent_dht;
pub mod plugin;
pub mod lzma;
pub mod util;
pub mod ffi;
63 changes: 63 additions & 0 deletions rust/src/lzma.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
use lzma_rs::decompress::{Options, Stream};
use lzma_rs::error::Error;
use std::io::{Cursor, Write};

/// Propagate lzma crate errors
#[repr(C)]
pub enum LzmaStatus {
LzmaOk,
LzmaIoError,
LzmaHeaderTooShortError,
LzmaError,
LzmaXzError,
}

impl From<Error> for LzmaStatus {
fn from(e: Error) -> LzmaStatus {
match e {
Error::IoError(_) => LzmaStatus::LzmaIoError,
Error::HeaderTooShort(_) => LzmaStatus::LzmaHeaderTooShortError,
Error::LzmaError(_) => LzmaStatus::LzmaError,
Error::XzError(_) => LzmaStatus::LzmaXzError,
}
}
}

impl From<std::io::Error> for LzmaStatus {
fn from(_e: std::io::Error) -> LzmaStatus {
LzmaStatus::LzmaIoError
}
}

/// Use the lzma algorithm to decompress a chunk of data.
#[no_mangle]
pub unsafe extern "C" fn lzma_decompress(
input: *const u8, input_len: &mut usize, output: *mut u8, output_len: &mut usize,
allow_incomplete: bool, memlimit: usize,
Copy link
Contributor

Choose a reason for hiding this comment

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

If allow_incomplete is always true, maybe it should not be part of this function's arguments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair. I think this was trying to maintain the kind of API that was used before (with LZMA_FINISH_ANY), but you are correct that if it is always true then it's not needed. I will remove it.

) -> LzmaStatus {
let input = std::slice::from_raw_parts(input, *input_len);
let output = std::slice::from_raw_parts_mut(output, *output_len);
let output = Cursor::new(output);

let options = Options {
memlimit: Some(memlimit),
allow_incomplete,
..Default::default()
};

let mut stream = Stream::new_with_options(&options, output);

if let Err(e) = stream.write_all(input) {
if !allow_incomplete {
return e.into();
}
}

match stream.finish() {
Ok(output) => {
*output_len = output.position() as usize;
LzmaStatus::LzmaOk
}
Err(e) => e.into(),
}
}
8 changes: 3 additions & 5 deletions src/detect-engine.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,12 +117,10 @@ SCEnumCharMap det_ctx_event_table[] = {
{ "Z_STREAM_ERROR", FILE_DECODER_EVENT_Z_STREAM_ERROR },
{ "Z_BUF_ERROR", FILE_DECODER_EVENT_Z_BUF_ERROR },
{ "Z_UNKNOWN_ERROR", FILE_DECODER_EVENT_Z_UNKNOWN_ERROR },
{ "LZMA_IO_ERROR", FILE_DECODER_EVENT_LZMA_IO_ERROR },
Copy link
Contributor

Choose a reason for hiding this comment

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

These needs new events in rules/http-events.rules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see that all of the rules in rules/http-events.rules are triggering off of the events defined in app-layer-htp.c::http_decoder_event_table. These events here for lzma decompression are defined in detect-engine.c and none of them have corresponding signatures in http-events.rules, and I don't see a corresponding .rules file that is matching on these file events.

Do you want me to define a new HTTP_DECODER_EVENT, and then set it in util-file-swf-decompression.c whenever we have FILE_DECODER events (and do we want to match all of them, or just some of them?), or do you want me to create new rules in http-events.rules that trigger on FILE_DECODER events? Or something else? I am just not clear what the preferred way to go about this is.

Copy link
Member

Choose a reason for hiding this comment

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

FYI, there is a known issue with these events, see https://redmine.openinfosecfoundation.org/issues/4482 (@jlucovsky seems I lost track of these)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack. So then fixing the file decoder events logging, and making rules to trigger on them, is a separate ticket / PR?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

{ "LZMA_HEADER_TOO_SHORT_ERROR", FILE_DECODER_EVENT_LZMA_HEADER_TOO_SHORT_ERROR },
{ "LZMA_DECODER_ERROR", FILE_DECODER_EVENT_LZMA_DECODER_ERROR },
{ "LZMA_MEMLIMIT_ERROR", FILE_DECODER_EVENT_LZMA_MEMLIMIT_ERROR },
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we lose this mem limit error ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When lzma-rs exceeds the memory cap for decoding, it returns a LzmaError, which is mapped to FILE_DECODER_EVENT_LZMA_DECODER_ERROR here. The functionality it still there, but the event name has changed. Other kinds of errors are also returned as LzmaError, so there is no specific error type for "You exceeded your memory quota or I could not allocate memory for you" (which is what SZ_ERROR_MEM semantically mapped to).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, would it be possible/simple for lzma-rs to do this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be a major bump for them to change the public API for their Error type to add a specific variant.

It would be trivial though to check the error message. When lzma-rs produces this kind of error the error message is obvious. They have a test that does this here and it would be trivial to check the error message in our lzma.rs and map it to a different local error type.

{ "LZMA_OPTIONS_ERROR", FILE_DECODER_EVENT_LZMA_OPTIONS_ERROR },
{ "LZMA_FORMAT_ERROR", FILE_DECODER_EVENT_LZMA_FORMAT_ERROR },
{ "LZMA_DATA_ERROR", FILE_DECODER_EVENT_LZMA_DATA_ERROR },
{ "LZMA_BUF_ERROR", FILE_DECODER_EVENT_LZMA_BUF_ERROR },
{ "LZMA_XZ_ERROR", FILE_DECODER_EVENT_LZMA_XZ_ERROR },
Copy link
Contributor

Choose a reason for hiding this comment

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

We should only allow pure lzma, no xz wrapper, right ?

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, the only reason this is here is because XZ errors are part of the public Error type in lzma-rs, and we match exhaustively. The decompression path here only uses Lzma.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, maybe a debug validation to check that this is never reached ?

{ "LZMA_UNKNOWN_ERROR", FILE_DECODER_EVENT_LZMA_UNKNOWN_ERROR },
{
"TOO_MANY_BUFFERS",
Expand Down
8 changes: 3 additions & 5 deletions src/detect.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1250,12 +1250,10 @@ enum {
FILE_DECODER_EVENT_Z_STREAM_ERROR,
FILE_DECODER_EVENT_Z_BUF_ERROR,
FILE_DECODER_EVENT_Z_UNKNOWN_ERROR,
FILE_DECODER_EVENT_LZMA_IO_ERROR,
FILE_DECODER_EVENT_LZMA_HEADER_TOO_SHORT_ERROR,
FILE_DECODER_EVENT_LZMA_DECODER_ERROR,
FILE_DECODER_EVENT_LZMA_MEMLIMIT_ERROR,
FILE_DECODER_EVENT_LZMA_OPTIONS_ERROR,
FILE_DECODER_EVENT_LZMA_FORMAT_ERROR,
FILE_DECODER_EVENT_LZMA_DATA_ERROR,
FILE_DECODER_EVENT_LZMA_BUF_ERROR,
FILE_DECODER_EVENT_LZMA_XZ_ERROR,
FILE_DECODER_EVENT_LZMA_UNKNOWN_ERROR,

DETECT_EVENT_TOO_MANY_BUFFERS,
Expand Down
47 changes: 13 additions & 34 deletions src/util-file-swf-decompression.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@
#include "util-misc.h"
#include "util-print.h"

#include <zlib.h>
#include "rust.h"

#include <htp/lzma/LzmaDec.h>
#include <zlib.h>

#define MAX_SWF_DECOMPRESSED_LEN 50000000
/*
Expand Down Expand Up @@ -129,10 +129,6 @@ int FileSwfZlibDecompression(DetectEngineThreadCtx *det_ctx,
return ret;
}

static void *SzAlloc(ISzAllocPtr p, size_t size) { return malloc(size); }
static void SzFree(ISzAllocPtr p, void *address) { free(address); }
static const ISzAlloc suri_lzma_Alloc = { SzAlloc, SzFree };

/* ZWS format */
/*
* | 4 bytes | 4 bytes | 4 bytes | 5 bytes | n bytes | 6 bytes |
Expand All @@ -144,46 +140,30 @@ int FileSwfLzmaDecompression(DetectEngineThreadCtx *det_ctx,
{
int ret = 0;

CLzmaDec strm;
LzmaDec_Construct(&strm);
ELzmaStatus status;

if (compressed_data_len < LZMA_PROPS_SIZE + 8) {
DetectEngineSetEvent(det_ctx, FILE_DECODER_EVENT_LZMA_FORMAT_ERROR);
return 0;
}
ret = LzmaDec_Allocate(&strm, compressed_data, LZMA_PROPS_SIZE, &suri_lzma_Alloc);
if (ret != SZ_OK) {
DetectEngineSetEvent(det_ctx, FILE_DECODER_EVENT_LZMA_DECODER_ERROR);
return 0;
}
LzmaDec_Init(&strm);
compressed_data += LZMA_PROPS_SIZE + 8;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did these got removed ? Did you test this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is advancing the input buffer past the header / properties, which were parsed out in LzmaDec_Allocate(). lzma-rs manages this internally, and will first read through all of the properties, and then start decompressing the data.

You can see the relevant part of lzma-rs here. It starts by parsing the properties in the header, and then reads the rest, all within the write() function where you stuff data in.

As for testing, there are several tests in lzma-rs against the short header case and full decompression, suricata has several LZMA tests in tests/detect-http-server-body.c, and we have a few lzma decompression tests in libhtp-rs that use the same API. Those tests all pass, so I don't think manually advancing the compression stream past the lzma header is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok thanks :-)

compressed_data_len -= LZMA_PROPS_SIZE + 8;
size_t inprocessed = compressed_data_len;
size_t outprocessed = decompressed_data_len;

ret = LzmaDec_DecodeToBuf(&strm, decompressed_data, &outprocessed,
compressed_data, &inprocessed, LZMA_FINISH_ANY, &status, MAX_SWF_DECOMPRESSED_LEN);
ret = lzma_decompress(compressed_data, &inprocessed, decompressed_data, &outprocessed, true,
MAX_SWF_DECOMPRESSED_LEN);

switch(ret) {
case SZ_OK:
case LzmaOk:
ret = 1;
break;
case SZ_ERROR_MEM:
DetectEngineSetEvent(det_ctx, FILE_DECODER_EVENT_LZMA_MEMLIMIT_ERROR);
case LzmaIoError:
DetectEngineSetEvent(det_ctx, FILE_DECODER_EVENT_LZMA_IO_ERROR);
ret = 0;
break;
case SZ_ERROR_UNSUPPORTED:
DetectEngineSetEvent(det_ctx, FILE_DECODER_EVENT_LZMA_OPTIONS_ERROR);
case LzmaHeaderTooShortError:
DetectEngineSetEvent(det_ctx, FILE_DECODER_EVENT_LZMA_HEADER_TOO_SHORT_ERROR);
ret = 0;
break;
case SZ_ERROR_DATA:
DetectEngineSetEvent(det_ctx, FILE_DECODER_EVENT_LZMA_DATA_ERROR);
case LzmaError:
DetectEngineSetEvent(det_ctx, FILE_DECODER_EVENT_LZMA_DECODER_ERROR);
ret = 0;
break;
case SZ_ERROR_INPUT_EOF:
DetectEngineSetEvent(det_ctx, FILE_DECODER_EVENT_LZMA_BUF_ERROR);
case LzmaXzError:
DetectEngineSetEvent(det_ctx, FILE_DECODER_EVENT_LZMA_XZ_ERROR);
ret = 0;
break;
default:
Expand All @@ -192,6 +172,5 @@ int FileSwfLzmaDecompression(DetectEngineThreadCtx *det_ctx,
break;
}

LzmaDec_Free(&strm, &suri_lzma_Alloc);
return ret;
}
4 changes: 2 additions & 2 deletions suricata.yaml.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -970,7 +970,7 @@ app-layer:
# auto will use http-body-inline mode in IPS mode, yes or no set it statically
http-body-inline: auto

# Decompress SWF files.
# Decompress SWF files. Disabled by default.
# Two types: 'deflate', 'lzma', 'both' will decompress deflate and lzma
# compress-depth:
# Specifies the maximum amount of data to decompress,
Expand All @@ -979,7 +979,7 @@ app-layer:
# Specifies the maximum amount of decompressed data to obtain,
# set 0 for unlimited.
swf-decompression:
enabled: yes
enabled: no
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs some entry in doc//userguide/upgrade.rst

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need a redmine ticket entry @jasonish ?

Copy link
Member

Choose a reason for hiding this comment

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

Might be a good idea. I think the reasoning for moving from enabled by default to disabled by default needs to be documented. So probably a ticket, and a small note in upgrade.rst.

Copy link
Member

Choose a reason for hiding this comment

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

link to the ticket in the upgrade.rst? :)

Copy link
Contributor Author

@cccs-rtmorti cccs-rtmorti Nov 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

No problem. I will create a ticket for this and link it in the upgrade.rst.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up submitting a separate PR for this part. #8153

type: both
compress-depth: 100kb
decompress-depth: 100kb
Expand Down