Integer overflow 4527 v2 #6676

catenacyber · 2021-12-06T13:09:51Z

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

Describe changes:

Fix integer warnings in all files beginning with a (like app)

libhtp-pr: 339

There remains one warning about the use of StreamTcpUpdateAppLayerProgress
cf discussion in #6649

Changes #6674 to get gcc happy about u16 + 2 < u32 where u16 + 2 should not become a signed integer

Explicitly truncate a file name if it is longer than UINT16_MAX

Explicitly truncate file names to UINT16_MAX Before, they got implicitly truncated, meaning a UINT16_MAX + 1 file name, went to 0 file name (because of modulo 65536)

and explicitly truncating filename's length

especially increasing padding_len size

codecov · 2021-12-06T13:57:36Z

Codecov Report

Merging #6676 (5e3f3c0) into master (c9d222a) will decrease coverage by 2.82%.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##           master    #6676      +/-   ##
==========================================
- Coverage   77.19%   74.37%   -2.83%     
==========================================
  Files         613      613              
  Lines      185646   185652       +6     
==========================================
- Hits       143310   138073    -5237     
- Misses      42336    47579    +5243

Flag	Coverage Δ
fuzzcorpus	`47.43% <57.14%> (-5.77%)`	⬇️
suricata-verify	`52.14% <71.15%> (-0.03%)`	⬇️
unittests	`63.09% <66.03%> (-0.01%)`	⬇️

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

suricata-qa · 2021-12-07T06:50:03Z

Information: QA ran without warnings.

Pipeline 5146

victorjulien · 2021-12-08T07:26:11Z

src/app-layer-ftp.c

@@ -642,14 +642,18 @@ static AppLayerResult FTPParseRequest(Flow *f, void *ftp_state,
                     * Min size has been checked in FTPParseRequestCommand
                     * PATH_MAX includes the null
                     */
-                    int file_name_len = MIN(PATH_MAX - 1, state->current_line_len - 5);
+                    uint32_t file_name_len = MIN(PATH_MAX - 1, state->current_line_len - 5);
+                    if (file_name_len > UINT16_MAX) {


this is a runtime check for something that shouldn't be possible in practice, as iirc PATH_MAX is 4k. Should we use a static assertion here?
https://riptutorial.com/c/example/1842/static-assertion

to clarify: if we have a static assert that PATH_MAX < UINT16_MAX we can get rid of a runtime check like this

Indeed, will do

What about

#if PATH_MAX > UINT16_MAX #error PATH_MAX is greater than UINT16_MAX #endif

I did not know about static assertions

why not use the thing that is designed to do this, meaning static assertions?

why not use the thing that is designed to do this, meaning static assertions?

For what I knew #error is designed to do this, I did not know about static assertions

They seem to be equivalent here :
https://wiki.sei.cmu.edu/confluence/display/c/DCL03-C.+Use+a+static+assertion+to+test+the+value+of+a+constant+expression

static_assert style looks more compact.
So, it is in branch v4.
Are there other comments or should I make a PR with v4 ?

victorjulien · 2021-12-08T07:31:07Z

src/app-layer-htp.c

@@ -524,7 +524,7 @@ static uint32_t AppLayerHtpComputeChunkLength(uint64_t content_len_so_far, uint3
 /* below error messages updated up to libhtp 0.5.7 (git 379632278b38b9a792183694a4febb9e0dbd1e7a) */
 struct {
    const char *msg;
-    int  de;
+    uint8_t de;


should this not be unsigned int as the values are defined in an enum?

An enum value such as HTTP_DECODER_EVENT_UNKNOWN_ERROR should be defined as an int(signed)
But we can safely cast it to uint8_t as we do not have more than 256 values. I even think that this is some kind of preprocessing that acts as #define
Furthermore, we later call HTPSetEvent which uses uint8_t (because it calls AppLayerDecoderEventsSetEventRaw which uses uint8_t)

And it does not look like there is warning flag for Wimplicit-enum-int-conversion
cf https://releases.llvm.org/11.0.0/tools/clang/docs/DiagnosticsReference.html#wenum-conversion

So, I think this patch is right

hmm I always thought an enum would be an unsigned int, and be 32 bits. But now I'm reading that it may actually be up to the compiler to pick a type as long as it fits? Wonder what that means for using enums in Rust - C - FFI.

Wonder what that means for using enums in Rust - C - FFI.

I guess it means the enums should be defined in Rust, and exported to be used in C with cbindgen :-p

Wonder what that means for using enums in Rust - C - FFI.

I guess it means the enums should be defined in Rust, and exported to be used in C with cbindgen :-p

cbindgen outputs the following, I guess in a way to fix the type:

enum HTTP2TransactionState { HTTP2StateIdle = 0, HTTP2StateOpen = 1, HTTP2StateReserved = 2, HTTP2StateDataClient = 3, HTTP2StateHalfClosedClient = 4, HTTP2StateDataServer = 5, HTTP2StateHalfClosedServer = 6, HTTP2StateClosed = 7, HTTP2StateGlobal = 8, }; typedef uint8_t HTTP2TransactionState;

So that is one option.

But it looks like the representation of enums has been an issue where the compilers use a different algorithm for layout, like rustc and msvc:

rust-lang/rust#81996

Luckily it looks like rustc, clang, gcc all agree here.

So, let's rustify all the things

So, let's rustify all the things

I think it puts us on a path of one big super header file for C. Not sure if thats really that bad of a thing tho.

Could not cbindgen generate one header file per rust module/directory ?

Could not cbindgen generate one header file per rust module/directory ?

No, thats not really how its designed. You could call it for each file manually and put those into headers. Then you'd have to worry about ordering of dependent data structures and so on.

victorjulien · 2021-12-08T07:33:24Z

src/app-layer-htp.c

@@ -1630,7 +1630,11 @@ static int HtpRequestBodyHandlePOSTorPUT(HtpState *hstate, HtpTxUserData *htud,
        }

        if (filename != NULL) {
-            result = HTPFileOpen(hstate, htud, filename, (uint32_t)filename_len, data, data_len,
+            if (filename_len > UINT16_MAX) {


should this lead to an event? Also 64k seems very high still. If we consider PATH_MAX to be 4k for example.

we also have to consider that a 64k name leads to JSON records > 64k

Event seems relevant.
PATH_MAX seems a better limit as well.
Will do

I think I would like us to define our own PATH_MAX(-like) limit, as I think this behaviour in suri should not depend on whatever an OS might define. Since on Linux the default seems 4k I think that might be a good value to pick.

Going with SC_FILENAME_MAX

victorjulien · 2021-12-08T07:34:58Z

src/app-layer-smtp.c

@@ -457,9 +457,12 @@ int SMTPProcessDataChunk(const uint8_t *chunk, uint32_t len,
            SCLogDebug("StreamTcpReassemblySetMinInspectDepth STREAM_TOSERVER %"PRIu32, depth);
            StreamTcpReassemblySetMinInspectDepth(flow->protoctx, STREAM_TOSERVER, depth);

+            uint16_t flen = (uint16_t)entity->filename_len;
+            if (entity->filename_len > UINT16_MAX) {


same comment as for http: this seems far to big

victorjulien · 2021-12-08T07:37:11Z

src/app-layer-smtp.c

@@ -1154,7 +1157,11 @@ static int SMTPParseCommandWithParam(SMTPState *state, uint8_t prefix_len, uint8
        return -1;
    memcpy(*target, state->current_line + i, spc_i - i);
    (*target)[spc_i - i] = '\0';
-    *target_len = spc_i - i;
+    if (spc_i - i > UINT16_MAX) {


does this mean we're limiting lines to a max length now where previously we didn't?

does this mean we're limiting lines to a max length now where previously we didn't?

Eheh no it does not mean that

Previously (that is currently), we are already limiting lines to a max length

But as there is an implicit cast from 'int' to 'uint16_t', if the line length is 65536, it will turn to zero (instead of saturating it to UINT16_MAX)

app-layer-smtp.c:1157:25: warning: implicit conversion loses integer precision: 'int' to 'uint16_t' (aka 'unsigned short') [-Wimplicit-int-conversion] *target_len = spc_i - i;

So, should I rather increase the sizes of uint16_t helo_len; and such ?

I see. No lets not allow for ridiculously long lines. But I think it should lead to another event if we reach the limit.

catenacyber · 2021-12-08T20:21:38Z

Replaced by #6690
Thanks for the good review

catenacyber added 5 commits December 6, 2021 14:03

ftp: fix int warnings

6b90d20

Explicitly truncate a file name if it is longer than UINT16_MAX

http: : fix int warnings

9604c72

Explicitly truncate file names to UINT16_MAX Before, they got implicitly truncated, meaning a UINT16_MAX + 1 file name, went to 0 file name (because of modulo 65536)

app: fix int warnings in generic app files

05335e4

smtp: fix int warnings

3961d54

and explicitly truncating filename's length

ssl: fix int warnings

5e3f3c0

especially increasing padding_len size

catenacyber requested a review from a team as a code owner December 6, 2021 13:09

catenacyber mentioned this pull request Dec 6, 2021

Integer overflow 4527 v1 #6674

Closed

victorjulien reviewed Dec 8, 2021

View reviewed changes

catenacyber mentioned this pull request Dec 8, 2021

Integer overflow 4527 v3 #6690

Closed

inashivb closed this Dec 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integer overflow 4527 v2 #6676

Integer overflow 4527 v2 #6676

catenacyber commented Dec 6, 2021

codecov bot commented Dec 6, 2021 •

edited

Loading

suricata-qa commented Dec 7, 2021

victorjulien Dec 8, 2021

victorjulien Dec 8, 2021

catenacyber Dec 8, 2021

catenacyber Dec 8, 2021

victorjulien Dec 9, 2021

catenacyber Dec 9, 2021

victorjulien Dec 8, 2021

catenacyber Dec 8, 2021

victorjulien Dec 8, 2021

catenacyber Dec 8, 2021

jasonish Dec 8, 2021

catenacyber Dec 8, 2021

jasonish Dec 8, 2021

catenacyber Dec 8, 2021

jasonish Dec 9, 2021

victorjulien Dec 8, 2021

victorjulien Dec 8, 2021

catenacyber Dec 8, 2021

victorjulien Dec 8, 2021

catenacyber Dec 8, 2021

victorjulien Dec 8, 2021

catenacyber Dec 8, 2021

victorjulien Dec 8, 2021

catenacyber Dec 8, 2021

victorjulien Dec 8, 2021

catenacyber Dec 8, 2021

catenacyber commented Dec 8, 2021

Integer overflow 4527 v2 #6676

Integer overflow 4527 v2 #6676

Conversation

catenacyber commented Dec 6, 2021

codecov bot commented Dec 6, 2021 • edited Loading

Codecov Report

suricata-qa commented Dec 7, 2021

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

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 commented Dec 8, 2021

codecov bot commented Dec 6, 2021 •

edited

Loading