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

Base64 data issue/v8 #9160

Closed
wants to merge 7 commits into from
Closed

Conversation

inashivb
Copy link
Member

@inashivb inashivb commented Jul 6, 2023

Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/6135

SV_BRANCH=OISF/suricata-verify#1292

Previous PR: #9159

Changes since v7:

  • Fix clang formatting

RFC 2045 states that any invalid character should be skipped over, this
is the RFC used by mime handler in Suricata code to deal with base64
encoded data.
So far, only spaces were skipped as a part of implementation of this
RFC, extend it to also skip over any other invalid character. Add
corresponding test.
Padding bytes for the last remainder data should be as follows:

Case   |    Remainder bytes     |    Padding
----------------------------------------------
  I    |              1         |      3
  II   |              2         |      2
  III  |              3         |      1

However, we calculate the decoded_bytes with the formula:
decoded_bytes = ASCII_BLOCK - padding

this means for Case I when padding is 3 bytes, the decoded_bytes would
be 0. This is incorrect for any trailing data. In any of the above
cases, if the parsing was successful, there should at least be 1 decoded
byte.
Just like the check for destination buffer size done previously for
complete data, it should also be done for the trailing data to avoid
goind out of bounds.
The destination buffer should be able to hold at least 3 Bytes during
the processing of the last block of data. If it cannot hold at least 3
Bytes, then that may lead to dynamic buffer overflow while decoding.
@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPR1_stats_chk
.flow.memuse 592373632 519668040 87.73%

Pipeline 15021

@victorjulien
Copy link
Member

  CC       util-base64.o
util-base64.c:177:9: warning: Value stored to 'sp' is never read [deadcode.DeadStores]
        sp = 0;
        ^    ~
1 warning generated.

scan-build warning ^

@victorjulien
Copy link
Member

SV PR needs rebase?

@victorjulien victorjulien marked this pull request as draft July 7, 2023 04:32
This was referenced Jul 7, 2023
@inashivb inashivb closed this Jul 7, 2023
@inashivb inashivb deleted the base64-data-issue/v8 branch July 7, 2023 11:24
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