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

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

Closed
wants to merge 1 commit into from

Conversation

cccs-rtmorti
Copy link
Contributor

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.

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.

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
Copy link

codecov bot commented Jul 25, 2022

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%) ⬆️
suricata-verify 52.49% <0.00%> (+0.05%) ⬆️
unittests 60.71% <20.00%> (-0.01%) ⬇️

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

@victorjulien
Copy link
Member

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.

@catenacyber
Copy link
Contributor

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

@cccs-rtmorti
Copy link
Contributor Author

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.

gendx/lzma-rs@fcbb11f

@jasonish
Copy link
Member

jasonish commented Aug 3, 2022

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

@catenacyber
Copy link
Contributor

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

Why ? zlib will still be used in libhtp

@jasonish
Copy link
Member

jasonish commented Aug 3, 2022

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.

@cccs-rtmorti
Copy link
Contributor Author

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.


#include <htp/lzma/LzmaDec.h>
#include <zlib.h>
Copy link
Member

Choose a reason for hiding this comment

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

Can the zlib.h include be removed?

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 don't think so. It is still used in the CWS decompression path.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, make sense. Even if removed from this file, its pulled in via one of the other includes, so we shouldn't remove it.

@catenacyber
Copy link
Contributor

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 ?

@victorjulien
Copy link
Member

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 victorjulien marked this pull request as draft October 28, 2022 10:16
@catenacyber
Copy link
Contributor

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

@cccs-rtmorti
Copy link
Contributor Author

@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
Copy link
Contributor Author

Updated in #8132

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants