Skip to content

Comments

http: complete multipart data on open#8899

Closed
catenacyber wants to merge 1 commit intoOISF:masterfrom
catenacyber:multipart-mime-3487-v15.2
Closed

http: complete multipart data on open#8899
catenacyber wants to merge 1 commit intoOISF:masterfrom
catenacyber:multipart-mime-3487-v15.2

Conversation

@catenacyber
Copy link
Contributor

Link to redmine ticket:
preliminary work for https://redmine.openinfosecfoundation.org/issues/3487

Describe changes:

  • Fix HTTP multipart file open then truncate to consume as many bytes as possible

See #8886 QA run that produced this

By the way, I do not know when the file gets closed (to get a chance to add the bytes it missed)

Modifies #8896 by fixing unit tests

Take as much as we can when opening, by making sure that the
boundary is not present
@codecov
Copy link

codecov bot commented May 18, 2023

Codecov Report

Merging #8899 (efe124a) into master (3247e39) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8899      +/-   ##
==========================================
- Coverage   82.30%   82.26%   -0.04%     
==========================================
  Files         969      969              
  Lines      273335   273341       +6     
==========================================
- Hits       224960   224875      -85     
- Misses      48375    48466      +91     
Flag Coverage Δ
fuzzcorpus 64.52% <100.00%> (-0.07%) ⬇️
suricata-verify 60.45% <100.00%> (+0.02%) ⬆️
unittests 62.95% <83.33%> (+<0.01%) ⬆️

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

@suricata-qa
Copy link

ERROR:

ERROR: QA failed on SURI_TLPW1_files_sha256.

Pipeline 13832

@victorjulien
Copy link
Member

@ct0br0 can you provide a pcap for this to Philippe?

@catenacyber
Copy link
Contributor Author

can you provide a pcap for this to Philippe?

This is the wrong way ;-)
I made this PR out of the pcap already supplied by Corey for the rust multipart stuff #8886

Question for you @victorjulien : Do we really want to fix this C before merging rust ?
Either way, we must accept this QA hash change
( Should SURI_TLPW1_files_sha256 consider truncated files anyways ? )

@victorjulien
Copy link
Member

can you provide a pcap for this to Philippe?

This is the wrong way ;-) I made this PR out of the pcap already supplied by Corey for the rust multipart stuff #8886

Question for you @victorjulien : Do we really want to fix this C before merging rust ? Either way, we must accept this QA hash change ( Should SURI_TLPW1_files_sha256 consider truncated files anyways ? )

I think so, esp if it is a bug. Then we'll have to backport a fix anyway.

@catenacyber
Copy link
Contributor Author

I think so, esp if it is a bug.

I am not sure if it can be called a bug...

But I think it best to be zealous and consume as many bytes as the limit tells us before setting the file to truncated...
So, I vote for this PR

@victorjulien victorjulien mentioned this pull request Jun 5, 2023
@victorjulien
Copy link
Member

Merged in #8965, thanks!

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.

3 participants