suricata icon indicating copy to clipboard operation
suricata copied to clipboard

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

Open cccs-rtmorti opened this issue 1 year ago • 9 comments

Use the lzma-rs crate 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.

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

  • [ X] I have read the contributing guide lines at https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Contributing
  • [ X] I have signed the Open Information Security Foundation contribution agreement at https://suricata.io/about/contribution-agreement/
  • [ X] 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.

cccs-rtmorti avatar Jul 13 '22 12:07 cccs-rtmorti

Codecov Report

Merging #7625 (55d13aa) into master (a2f857e) will increase coverage by 0.06%. The diff coverage is 20.00%.

@@            Coverage Diff             @@
##           master    #7625      +/-   ##
==========================================
+ Coverage   75.73%   75.80%   +0.06%     
==========================================
  Files         659      659              
  Lines      185740   185727      -13     
==========================================
+ Hits       140669   140783     +114     
+ Misses      45071    44944     -127     
Flag Coverage Δ
fuzzcorpus 60.26% <0.00%> (+0.41%) :arrow_up:
suricata-verify 52.49% <0.00%> (+0.05%) :arrow_up:
unittests 60.71% <20.00%> (-0.01%) :arrow_down:

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

codecov[bot] avatar Jul 25 '22 16:07 codecov[bot]

Thanks Todd. How does this deal with the issue of lzma preallocating some attacker controlled u32 value? The reason we vendored the lzma code was to address this.

victorjulien avatar Jul 26 '22 08:07 victorjulien

@victorjulien if I remember correctly, you mentioned that this whole code (SWF decompression) should be deprecated/removed at some point.

Should it be removed now ?

catenacyber avatar Jul 26 '22 09:07 catenacyber

Thanks Todd. How does this deal with the issue of lzma preallocating some attacker controlled u32 value? The reason we vendored the lzma code was to address this.

We actually sent the lzma-rs project some patches to add limits to prevent this kind of attack. It was part of our assessment for whether or not we could use this crate in libhtp-rs for lzma decompression.

https://github.com/gendx/lzma-rs/commit/fcbb11fab6f8389067a266f40a74d15f02bf30ea

cccs-rtmorti avatar Jul 26 '22 12:07 cccs-rtmorti

Looks like we could drop the check for zlib in configure.ac?

jasonish avatar Aug 03 '22 18:08 jasonish

Looks like we could drop the check for zlib in configure.ac?

Why ? zlib will still be used in libhtp

catenacyber avatar Aug 03 '22 18:08 catenacyber

Why ? zlib will still be used in libhtp

libhtp has its own check for zlib which in the end results in suricata dynamically linking with it anyways.

jasonish avatar Aug 03 '22 18:08 jasonish

Looks like we could drop the check for zlib in configure.ac?

util-file-swf-decompression still uses zlib to decompress CWS compressed swf archives. This PR only changes the lzma decompression path. zlib is still directly used for the CWS path, and this file includes zlib.h.

cccs-rtmorti avatar Aug 03 '22 18:08 cccs-rtmorti

Reposting as it is lost in GitHub flow :

@victorjulien if I remember correctly, you mentioned that this whole code (SWF decompression) should be deprecated/removed at some point.

Should it be removed now ?

catenacyber avatar Aug 24 '22 12:08 catenacyber

Reposting as it is lost in GitHub flow :

@victorjulien if I remember correctly, you mentioned that this whole code (SWF decompression) should be deprecated/removed at some point.

Should it be removed now ?

I don't think so, but it should probably not be enabled (at runtime) by default as flash is supposed to be a thing of the past.

victorjulien avatar Oct 28 '22 10:10 victorjulien

if I remember correctly, you mentioned that this whole code (SWF decompression) should be deprecated/removed at some point. Should it be removed now ?

I don't think so, but it should probably not be enabled (at runtime) by default as flash is supposed to be a thing of the past.

@cccs-rtmorti could you add that to your PR ? That is change the default suricata.yaml config option for swf-decompression fields

catenacyber avatar Oct 28 '22 19:10 catenacyber

@cccs-rtmorti could you add that to your PR ? That is change the default suricata.yaml config option for swf-decompression fields

Yeah, no problem.

cccs-rtmorti avatar Oct 28 '22 20:10 cccs-rtmorti

Updated in #8132

cccs-rtmorti avatar Oct 31 '22 18:10 cccs-rtmorti