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

cccs-rtmorti · 2022-10-31T18:47:09Z

Make sure these boxes are signed before submitting your Pull Request -- thank you.

I have read the contributing guide lines at https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Contributing
I have signed the Open Information Security Foundation contribution agreement at https://suricata.io/about/contribution-agreement/
I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)

Reference libhtp-rs PR comment suggesting this change.

Describe changes:

Create a lzma.rs file which exports a lzma decompression function interface.
In util-file-swf-decompression.c, use this interface instead of the lzma decompression interface presented by libhtp.
Update error types / constants to reflect lzma-rs error conditions / results.
Disable swf decompression by default.

Update to #7625

Use the lzma-rs create for decompressing swf/lzma files instead of the lzma decompressor in libhtp. This decouples suricata from libhtp except for actual http parsing, and means libhtp no longer has to export a lzma decompression interface.

codecov · 2022-10-31T20:46:54Z

Codecov Report

Merging #8132 (db1114e) into master (160c778) will decrease coverage by 0.15%.
The diff coverage is 54.76%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8132      +/-   ##
==========================================
- Coverage   81.67%   81.51%   -0.16%     
==========================================
  Files         962      963       +1     
  Lines      276781   276796      +15     
==========================================
- Hits       226052   225633     -419     
- Misses      50729    51163     +434

Flag	Coverage Δ
fuzzcorpus	`63.04% <0.00%> (-0.33%)`	⬇️
suricata-verify	`59.35% <0.00%> (-0.02%)`	⬇️
unittests	`63.15% <54.76%> (-0.20%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

catenacyber · 2022-11-01T09:17:47Z

suricata.yaml.in

@@ -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


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

Does this need a redmine ticket entry @jasonish ?

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.

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

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

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

catenacyber · 2022-11-01T09:37:06Z

src/detect-engine.c

@@ -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 },


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

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.

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

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

catenacyber · 2022-11-01T09:37:35Z

src/detect-engine.c

    { "LZMA_DECODER_ERROR", FILE_DECODER_EVENT_LZMA_DECODER_ERROR },
-    { "LZMA_MEMLIMIT_ERROR", FILE_DECODER_EVENT_LZMA_MEMLIMIT_ERROR },


Do we lose this mem limit error ?

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).

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

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.

catenacyber · 2022-11-01T09:38:17Z

src/detect-engine.c

-    { "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 },


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

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.

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

catenacyber · 2022-11-01T09:39:55Z

src/util-file-swf-decompression.c

-        return 0;
-    }
-    LzmaDec_Init(&strm);
-    compressed_data += LZMA_PROPS_SIZE + 8;


Why did these got removed ? Did you test this ?

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.

Ok thanks :-)

catenacyber · 2022-11-01T09:41:10Z

rust/src/lzma.rs

+#[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,


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

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.

catenacyber

Thanks for this nice work.
I have some questions...

CI : ✅
Code : looks good to me
Commits segmentation : Perfect
Commit messages : ok
Git ID set : looks fine for me
CLA : should be fine
Doc update : maybe add an entry in upgrade.rst
Redmine ticket : ok for one, maybe needs another one
Rustfmt : fine for new file
Tests : I would say that it is fair enough not to add a test for this feature when we disable it by default and that it has gone untested for years with bugs in it...
Dependencies added: MIT License is good

cccs-rtmorti · 2022-11-04T18:01:17Z

Updated in #8154

cccs-rtmorti added 2 commits October 31, 2022 18:19

swf-decompression: Disable by default.

db1114e

cccs-rtmorti requested review from jasonish and victorjulien as code owners October 31, 2022 18:47

cccs-rtmorti mentioned this pull request Oct 31, 2022

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

Closed

victorjulien requested review from catenacyber and removed request for victorjulien October 31, 2022 20:26

catenacyber reviewed Nov 1, 2022

View reviewed changes

This was referenced Nov 4, 2022

swf-decompression: Disable by default. #8153

Closed

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

Closed

cccs-rtmorti closed this Nov 4, 2022

This was referenced Nov 14, 2022

swf-decompression: Disable by default. #8170

Closed

swf-decompression: Disable by default. #8183

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cccs-rtmorti commented Oct 31, 2022

codecov bot commented Oct 31, 2022

catenacyber Nov 1, 2022

catenacyber Nov 1, 2022

jasonish Nov 1, 2022

victorjulien Nov 1, 2022

cccs-rtmorti Nov 1, 2022 •

edited

Loading

cccs-rtmorti Nov 4, 2022

catenacyber Nov 1, 2022

cccs-rtmorti Nov 1, 2022

victorjulien Nov 1, 2022

cccs-rtmorti Nov 1, 2022

victorjulien Nov 1, 2022

catenacyber Nov 1, 2022

cccs-rtmorti Nov 1, 2022

catenacyber Nov 1, 2022

cccs-rtmorti Nov 4, 2022

catenacyber Nov 1, 2022

cccs-rtmorti Nov 1, 2022

catenacyber Nov 1, 2022

catenacyber Nov 1, 2022

cccs-rtmorti Nov 1, 2022

catenacyber Nov 1, 2022

catenacyber Nov 1, 2022

cccs-rtmorti Nov 1, 2022

catenacyber left a comment

cccs-rtmorti commented Nov 4, 2022

		{ "LZMA_DECODER_ERROR", FILE_DECODER_EVENT_LZMA_DECODER_ERROR },
		{ "LZMA_MEMLIMIT_ERROR", FILE_DECODER_EVENT_LZMA_MEMLIMIT_ERROR },

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

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

Conversation

cccs-rtmorti commented Oct 31, 2022

codecov bot commented Oct 31, 2022

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cccs-rtmorti Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catenacyber left a comment

Choose a reason for hiding this comment

cccs-rtmorti commented Nov 4, 2022

cccs-rtmorti Nov 1, 2022 •

edited

Loading