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.

Sign up for GitHub

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

Integer overflow 4527 v2 #6676

Closed
wants to merge 5 commits into from
Closed

Integer overflow 4527 v2 #6676

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6b90d20
ftp: fix int warnings
catenacyber Dec 6, 2021
9604c72
http: : fix int warnings
catenacyber Dec 6, 2021
05335e4
app: fix int warnings in generic app files
catenacyber Dec 6, 2021
3961d54
smtp: fix int warnings
catenacyber Dec 6, 2021
5e3f3c0
ssl: fix int warnings
catenacyber Dec 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions src/app-layer-ftp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, will do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about

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

I did not know about static assertions

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

// truncate the file name if too long for util-file.h
file_name_len = UINT16_MAX;
}
data->file_name = FTPCalloc(file_name_len + 1, sizeof(char));
if (data->file_name == NULL) {
FtpTransferCmdFree(data);
SCReturnStruct(APP_LAYER_ERROR);
}
data->file_name[file_name_len] = 0;
data->file_len = file_name_len;
data->file_len = (uint16_t)file_name_len;
memcpy(data->file_name, state->current_line + 5, file_name_len);
data->cmd = state->command;
data->flow_id = FlowGetId(f);
Expand Down Expand Up @@ -1029,9 +1033,8 @@ static StreamingBufferConfig sbcfg = STREAMING_BUFFER_CONFIG_INITIALIZER;
* \retval 1 when the command is parsed, 0 otherwise
*/
static AppLayerResult FTPDataParse(Flow *f, FtpDataState *ftpdata_state,
AppLayerParserState *pstate,
const uint8_t *input, uint32_t input_len,
void *local_data, int direction)
AppLayerParserState *pstate, const uint8_t *input, uint32_t input_len, void *local_data,
uint8_t direction)
{
uint16_t flags = FileFlowToFlags(f, direction);
int ret = 0;
Expand Down Expand Up @@ -1361,7 +1364,7 @@ uint16_t JsonGetNextLineFromBuffer(const char *buffer, const uint16_t len)
}

char *c = strchr(buffer, '\n');
return c == NULL ? len : c - buffer + 1;
return c == NULL ? len : (uint16_t)(c - buffer + 1);
}

void EveFTPDataAddMetadata(const Flow *f, JsonBuilder *jb)
Expand Down
2 changes: 1 addition & 1 deletion src/app-layer-ftp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ typedef struct FtpLineState_ {

typedef struct FTPString_ {
uint8_t *str;
uint16_t len;
uint32_t len;
TAILQ_ENTRY(FTPString_) next;
} FTPString;

Expand Down
3 changes: 1 addition & 2 deletions src/app-layer-htp-file.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,8 +227,7 @@ int HTPFileOpenWithRange(HtpState *s, HtpTxUserData *txud, const uint8_t *filena
HTTPContentRange crparsed;
if (HTPParseAndCheckContentRange(rawvalue, &crparsed, s, htud) != 0) {
// range is invalid, fall back to classic open
return HTPFileOpen(
s, txud, filename, (uint32_t)filename_len, data, data_len, txid, STREAM_TOCLIENT);
return HTPFileOpen(s, txud, filename, filename_len, data, data_len, txid, STREAM_TOCLIENT);
}
flags = FileFlowToFlags(s->f, STREAM_TOCLIENT);
if ((s->flags & HTP_FLAG_STORE_FILES_TS) ||
Expand Down
3 changes: 2 additions & 1 deletion src/app-layer-htp-range.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,8 @@ void HttpRangeContainersInit(void)
}
}
if (ConfGetValue("app-layer.protocols.http.byterange.timeout", &str) == 1) {
if (StringParseUint32(&timeout, 10, strlen(str), str) <= 0) {
size_t slen = strlen(str);
if (slen > UINT16_MAX || StringParseUint32(&timeout, 10, (uint16_t)slen, str) <= 0) {
SCLogWarning(SC_ERR_INVALID_VALUE,
"timeout value cannot be deduced: %s,"
" resetting to default",
Expand Down
41 changes: 25 additions & 16 deletions src/app-layer-htp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, let's rustify all the things

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

} htp_errors[] = {
{ "GZip decompressor: inflateInit2 failed", HTTP_DECODER_EVENT_GZIP_DECOMPRESSION_FAILED},
{ "Request field invalid: colon missing", HTTP_DECODER_EVENT_REQUEST_FIELD_MISSING_COLON},
Expand All @@ -547,7 +547,7 @@ struct {

struct {
const char *msg;
int de;
uint8_t de;
} htp_warnings[] = {
{ "GZip decompressor:", HTTP_DECODER_EVENT_GZIP_DECOMPRESSION_FAILED},
{ "Request field invalid", HTTP_DECODER_EVENT_REQUEST_HEADER_INVALID},
Expand Down Expand Up @@ -594,7 +594,7 @@ struct {
*
* \retval id the id or 0 in case of not found
*/
static int HTPHandleWarningGetId(const char *msg)
static uint8_t HTPHandleWarningGetId(const char *msg)
{
SCLogDebug("received warning \"%s\"", msg);
size_t idx;
Expand All @@ -618,7 +618,7 @@ static int HTPHandleWarningGetId(const char *msg)
*
* \retval id the id or 0 in case of not found
*/
static int HTPHandleErrorGetId(const char *msg)
static uint8_t HTPHandleErrorGetId(const char *msg)
{
SCLogDebug("received error \"%s\"", msg);

Expand Down Expand Up @@ -675,7 +675,7 @@ static void HTPHandleError(HtpState *s, const uint8_t dir)

SCLogDebug("message %s", log->msg);

int id = HTPHandleErrorGetId(log->msg);
uint8_t id = HTPHandleErrorGetId(log->msg);
if (id == 0) {
id = HTPHandleWarningGetId(log->msg);
if (id == 0)
Expand Down Expand Up @@ -1255,9 +1255,9 @@ static void HtpRequestBodyMultipartParseHeader(HtpState *hstate,
ft_len = USHRT_MAX;

*filename = fn;
*filename_len = fn_len;
*filename_len = (uint16_t)fn_len;
*filetype = ft;
*filetype_len = ft_len;
*filetype_len = (uint16_t)ft_len;
}

/**
Expand Down Expand Up @@ -1304,8 +1304,8 @@ static int HtpRequestBodyHandleMultipart(HtpState *hstate, HtpTxUserData *htud,
{
int result = 0;
uint8_t boundary[htud->boundary_len + 4]; /**< size limited to HTP_BOUNDARY_MAX + 4 */
uint32_t expected_boundary_len = htud->boundary_len + 2;
uint32_t expected_boundary_end_len = htud->boundary_len + 4;
uint16_t expected_boundary_len = htud->boundary_len + 2;
uint16_t expected_boundary_end_len = htud->boundary_len + 4;
int tx_progress = 0;

#ifdef PRINT
Expand Down Expand Up @@ -1434,7 +1434,7 @@ static int HtpRequestBodyHandleMultipart(HtpState *hstate, HtpTxUserData *htud,
/* skip empty records */
if (expected_boundary_len == header_len) {
goto next;
} else if ((expected_boundary_len + 2) <= header_len) {
} else if ((uint32_t)(expected_boundary_len + 2) <= header_len) {
header_len -= (expected_boundary_len + 2);
header = (uint8_t *)header_start + (expected_boundary_len + 2); // + for 0d 0a
}
Expand Down Expand Up @@ -1536,7 +1536,7 @@ static int HtpRequestBodyHandleMultipart(HtpState *hstate, HtpTxUserData *htud,
SCLogDebug("offset %u", offset);
htud->request_body.body_parsed += offset;

if (filedata_len >= (expected_boundary_len + 2)) {
if (filedata_len >= (uint32_t)(expected_boundary_len + 2)) {
filedata_len -= (expected_boundary_len + 2 - 1);
SCLogDebug("opening file with partial data");
} else {
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going with SC_FILENAME_MAX

// explicitly truncate the file name if too long
filename_len = UINT16_MAX;
}
result = HTPFileOpen(hstate, htud, filename, (uint16_t)filename_len, data, data_len,
HtpGetActiveRequestTxID(hstate), STREAM_TOSERVER);
if (result == -1) {
goto end;
Expand Down Expand Up @@ -1703,11 +1707,15 @@ static int HtpResponseBodyHandle(HtpState *hstate, HtpTxUserData *htud,
if (filename != NULL) {
// set range if present
htp_header_t *h_content_range = htp_table_get_c(tx->response_headers, "content-range");
if (filename_len > UINT16_MAX) {
// explicitly truncate the file name if too long
filename_len = UINT16_MAX;
}
if (h_content_range != NULL) {
result = HTPFileOpenWithRange(hstate, htud, filename, (uint32_t)filename_len, data,
result = HTPFileOpenWithRange(hstate, htud, filename, (uint16_t)filename_len, data,
data_len, HtpGetActiveResponseTxID(hstate), h_content_range->value, htud);
} else {
result = HTPFileOpen(hstate, htud, filename, (uint32_t)filename_len, data, data_len,
result = HTPFileOpen(hstate, htud, filename, (uint16_t)filename_len, data, data_len,
HtpGetActiveResponseTxID(hstate), STREAM_TOCLIENT);
}
SCLogDebug("result %d", result);
Expand Down Expand Up @@ -3025,7 +3033,7 @@ static int HTPRegisterPatternsForProtocolDetection(void)
* but the pattern matching should only be one char
*/
register_result = AppLayerProtoDetectPMRegisterPatternCI(IPPROTO_TCP, ALPROTO_HTTP1,
method_buffer, strlen(method_buffer) - 3, 0, STREAM_TOSERVER);
method_buffer, (uint16_t)strlen(method_buffer) - 3, 0, STREAM_TOSERVER);
if (register_result < 0) {
return -1;
}
Expand All @@ -3035,7 +3043,8 @@ static int HTPRegisterPatternsForProtocolDetection(void)
/* Loop through all the http verions patterns that are TO_CLIENT */
for (versions_pos = 0; versions[versions_pos]; versions_pos++) {
register_result = AppLayerProtoDetectPMRegisterPatternCI(IPPROTO_TCP, ALPROTO_HTTP1,
versions[versions_pos], strlen(versions[versions_pos]), 0, STREAM_TOCLIENT);
versions[versions_pos], (uint16_t)strlen(versions[versions_pos]), 0,
STREAM_TOCLIENT);
if (register_result < 0) {
return -1;
}
Expand Down
4 changes: 2 additions & 2 deletions src/app-layer-parser.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ AppLayerParserThreadCtx *AppLayerParserThreadCtxAlloc(void)
SCEnter();

AppProto alproto = 0;
int flow_proto = 0;
uint8_t flow_proto = 0;
AppLayerParserThreadCtx *tctx;

tctx = SCMalloc(sizeof(*tctx));
Expand All @@ -275,7 +275,7 @@ void AppLayerParserThreadCtxFree(AppLayerParserThreadCtx *tctx)
SCEnter();

AppProto alproto = 0;
int flow_proto = 0;
uint8_t flow_proto = 0;

for (flow_proto = 0; flow_proto < FLOW_PROTO_DEFAULT; flow_proto++) {
for (alproto = 0; alproto < ALPROTO_MAX; alproto++) {
Expand Down
2 changes: 1 addition & 1 deletion src/app-layer-register.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
typedef struct AppLayerParser {
const char *name;
const char *default_port;
int ip_proto;
uint8_t ip_proto;

ProbingParserFPtr ProbeTS;
ProbingParserFPtr ProbeTC;
Expand Down
31 changes: 19 additions & 12 deletions src/app-layer-smtp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

same comment as for http: this seems far to big

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

flen = UINT16_MAX;
}
if (FileOpenFileWithId(files, &smtp_config.sbcfg, smtp_state->file_track_id++,
(uint8_t *) entity->filename, entity->filename_len,
(uint8_t *) chunk, len, flags) != 0) {
(uint8_t *)entity->filename, flen, (uint8_t *)chunk, len, flags) != 0) {
ret = MIME_DEC_ERR_DATA;
SCLogDebug("FileOpenFile() failed");
}
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

*target_len = UINT16_MAX;
} else {
*target_len = (uint16_t)(spc_i - i);
}

return 0;
}
Expand Down Expand Up @@ -1215,6 +1222,9 @@ static int NoNewTx(SMTPState *state)
return 0;
}

/* XXX have a better name */
#define rawmsgname "rawmsg"

static int SMTPProcessRequest(SMTPState *state, Flow *f,
AppLayerParserState *pstate)
{
Expand Down Expand Up @@ -1255,7 +1265,6 @@ static int SMTPProcessRequest(SMTPState *state, Flow *f,
SCMemcmpLowercase("data", state->current_line, 4) == 0) {
state->current_command = SMTP_COMMAND_DATA;
if (smtp_config.raw_extraction) {
const char *msgname = "rawmsg"; /* XXX have a better name */
if (state->files_ts == NULL)
state->files_ts = FileContainerAlloc();
if (state->files_ts == NULL) {
Expand All @@ -1272,10 +1281,9 @@ static int SMTPProcessRequest(SMTPState *state, Flow *f,
TAILQ_INSERT_TAIL(&state->tx_list, tx, next);
tx->tx_id = state->tx_cnt++;
}
if (FileOpenFileWithId(state->files_ts, &smtp_config.sbcfg,
state->file_track_id++,
(uint8_t*) msgname, strlen(msgname), NULL, 0,
FILE_NOMD5|FILE_NOMAGIC|FILE_USE_DETECT) == 0) {
if (FileOpenFileWithId(state->files_ts, &smtp_config.sbcfg, state->file_track_id++,
(uint8_t *)rawmsgname, strlen(rawmsgname), NULL, 0,
FILE_NOMD5 | FILE_NOMAGIC | FILE_USE_DETECT) == 0) {
SMTPNewFile(state->curr_tx, state->files_ts->tail);
}
} else if (smtp_config.decode_mime) {
Expand Down Expand Up @@ -1378,10 +1386,9 @@ static int SMTPProcessRequest(SMTPState *state, Flow *f,
}
}

static AppLayerResult SMTPParse(int direction, Flow *f, SMTPState *state,
AppLayerParserState *pstate, const uint8_t *input,
uint32_t input_len,
SMTPThreadCtx *thread_data)
static AppLayerResult SMTPParse(uint8_t direction, Flow *f, SMTPState *state,
AppLayerParserState *pstate, const uint8_t *input, uint32_t input_len,
SMTPThreadCtx *thread_data)
{
SCEnter();

Expand Down
Loading