From 0dac69919725930e3c9ab7695d7a64b138c5422a Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Tue, 23 Jun 2020 13:24:24 +0530 Subject: [PATCH] src: remove multiple uses of atoi atoi() and related functions lack a mechanism for reporting errors for invalid values. Replace them with calls to the appropriate ByteExtractString* functions. Closes redmine ticket 3053. --- src/detect-byte-extract.c | 36 +++++++++++-------- src/detect-cipservice.c | 10 +++--- src/detect-engine-port.c | 41 +++++++++++----------- src/detect-fast-pattern.c | 22 +++++++----- src/detect-ipproto.c | 3 +- src/detect-iprep.c | 5 ++- src/detect-krb5-errcode.c | 7 ++-- src/detect-krb5-msgtype.c | 7 ++-- src/detect-modbus.c | 74 +++++++++++++++++++++++++++++++-------- 9 files changed, 134 insertions(+), 71 deletions(-) diff --git a/src/detect-byte-extract.c b/src/detect-byte-extract.c index 8305c226b5..cdcf7f24fa 100644 --- a/src/detect-byte-extract.c +++ b/src/detect-byte-extract.c @@ -240,7 +240,12 @@ static inline DetectByteExtractData *DetectByteExtractParse(DetectEngineCtx *de_ "for arg 1 for byte_extract"); goto error; } - bed->nbytes = atoi(nbytes_str); + if (StringParseUint8(&bed->nbytes, 10, 0, + (const char *)nbytes_str) < 0) { + SCLogError(SC_ERR_INVALID_SIGNATURE, "Invalid value for number of bytes" + " to be extracted: \"%s\".", nbytes_str); + goto error; + } /* offset */ char offset_str[64] = ""; @@ -251,10 +256,9 @@ static inline DetectByteExtractData *DetectByteExtractParse(DetectEngineCtx *de_ "for arg 2 for byte_extract"); goto error; } - int offset = atoi(offset_str); - if (offset < -65535 || offset > 65535) { - SCLogError(SC_ERR_INVALID_SIGNATURE, "byte_extract offset invalid - %d. " - "The right offset range is -65535 to 65535", offset); + int32_t offset; + if (StringParseI32RangeCheck(&offset, 10, 0, (const char *)offset_str, -65535, 65535) < 0) { + SCLogError(SC_ERR_INVALID_SIGNATURE, "Invalid value for offset: \"%s\".", offset_str); goto error; } bed->offset = offset; @@ -307,14 +311,13 @@ static inline DetectByteExtractData *DetectByteExtractParse(DetectEngineCtx *de_ "for arg %d for byte_extract", i); goto error; } - int multiplier = atoi(multiplier_str); - if (multiplier < DETECT_BYTE_EXTRACT_MULTIPLIER_MIN_LIMIT || - multiplier > DETECT_BYTE_EXTRACT_MULTIPLIER_MAX_LIMIT) { - SCLogError(SC_ERR_INVALID_SIGNATURE, "multipiler_value invalid " - "- %d. The range is %d-%d", - multiplier, - DETECT_BYTE_EXTRACT_MULTIPLIER_MIN_LIMIT, - DETECT_BYTE_EXTRACT_MULTIPLIER_MAX_LIMIT); + int32_t multiplier; + if (StringParseI32RangeCheck(&multiplier, 10, 0, + (const char *)multiplier_str, + DETECT_BYTE_EXTRACT_MULTIPLIER_MIN_LIMIT, + DETECT_BYTE_EXTRACT_MULTIPLIER_MAX_LIMIT) < 0) { + SCLogError(SC_ERR_INVALID_SIGNATURE, "Invalid value for" + "multiplier: \"%s\".", multiplier_str); goto error; } bed->multiplier_value = multiplier; @@ -411,7 +414,12 @@ static inline DetectByteExtractData *DetectByteExtractParse(DetectEngineCtx *de_ "for arg %d in byte_extract", i); goto error; } - bed->align_value = atoi(align_str); + if (StringParseUint8(&bed->align_value, 10, 0, + (const char *)align_str) < 0) { + SCLogError(SC_ERR_INVALID_SIGNATURE, "Invalid align_value: " + "\"%s\".", align_str); + goto error; + } if (!(bed->align_value == 2 || bed->align_value == 4)) { SCLogError(SC_ERR_INVALID_SIGNATURE, "Invalid align_value for " "byte_extract - \"%d\"", bed->align_value); diff --git a/src/detect-cipservice.c b/src/detect-cipservice.c index 712a7d8ae3..765a8a76d2 100644 --- a/src/detect-cipservice.c +++ b/src/detect-cipservice.c @@ -346,14 +346,14 @@ static DetectEnipCommandData *DetectEnipCommandParse(const char *rulestr) goto error; } - unsigned long cmd = atol(rulestr); - if (cmd > MAX_ENIP_CMD) //if command greater than 16 bit - { - SCLogError(SC_ERR_INVALID_SIGNATURE, "invalid ENIP command %lu", cmd); + uint16_t cmd; + if (StringParseUint16(&cmd, 10, 0, rulestr) < 0) { + SCLogError(SC_ERR_INVALID_SIGNATURE, "invalid ENIP command" + ": \"%s\"", rulestr); goto error; } - enipcmdd->enipcommand = (uint16_t) atoi(rulestr); + enipcmdd->enipcommand = cmd; return enipcmdd; diff --git a/src/detect-engine-port.c b/src/detect-engine-port.c index f0e768b78b..88fb5b23cd 100644 --- a/src/detect-engine-port.c +++ b/src/detect-engine-port.c @@ -51,12 +51,13 @@ #include "host.h" #include "util-profiling.h" #include "util-var.h" +#include "util-byte.h" static int DetectPortCutNot(DetectPort *, DetectPort **); static int DetectPortCut(DetectEngineCtx *, DetectPort *, DetectPort *, DetectPort **); DetectPort *PortParse(const char *str); -int DetectPortIsValidRange(char *); +static bool DetectPortIsValidRange(char *, uint16_t *); /** * \brief Alloc a DetectPort structure and update counters @@ -1295,20 +1296,20 @@ DetectPort *PortParse(const char *str) } /* see if the address is an ipv4 or ipv6 address */ - if ((port2 = strchr(port, ':')) != NULL) { + if ((port2 = strchr(port, ':')) != NULL) { /* 80:81 range format */ port2[0] = '\0'; port2++; - if (DetectPortIsValidRange(port)) - dp->port = atoi(port); - else - goto error; + if (strcmp(port, "") != 0) { + if (!DetectPortIsValidRange(port, &dp->port)) + goto error; + } else { + dp->port = 0; + } if (strcmp(port2, "") != 0) { - if (DetectPortIsValidRange(port2)) - dp->port2 = atoi(port2); - else + if (!DetectPortIsValidRange(port2, &dp->port2)) goto error; } else { dp->port2 = 65535; @@ -1321,10 +1322,10 @@ DetectPort *PortParse(const char *str) if (strcasecmp(port,"any") == 0) { dp->port = 0; dp->port2 = 65535; - } else if(DetectPortIsValidRange(port)){ - dp->port = dp->port2 = atoi(port); } else { - goto error; + if (!DetectPortIsValidRange(port, &dp->port)) + goto error; + dp->port2 = dp->port; } } @@ -1342,18 +1343,16 @@ error: * * \param str Pointer to the port string * - * \retval 1 if port is in the valid range - * \retval 0 if invalid + * + * \retval true if port is in the valid range + * \retval false if invalid */ -int DetectPortIsValidRange(char *port) +static bool DetectPortIsValidRange(char *port, uint16_t *port_val) { - char *end; - long r = strtol(port, &end, 10); + if (StringParseUint16(port_val, 10, 0, (const char *)port) < 0) + return false; - if(*end == 0 && r >= 0 && r <= 65535) - return 1; - else - return 0; + return true; } /********************** End parsing routines ********************/ diff --git a/src/detect-fast-pattern.c b/src/detect-fast-pattern.c index 96286c0fec..b7c60078bc 100644 --- a/src/detect-fast-pattern.c +++ b/src/detect-fast-pattern.c @@ -33,6 +33,7 @@ #include "detect-fast-pattern.h" #include "util-error.h" +#include "util-byte.h" #include "util-debug.h" #include "util-unittest.h" #include "util-unittest-helper.h" @@ -283,10 +284,11 @@ static int DetectFastPatternSetup(DetectEngineCtx *de_ctx, Signature *s, const c "for fast_pattern offset"); goto error; } - int offset = atoi(arg_substr); - if (offset > 65535) { - SCLogError(SC_ERR_INVALID_SIGNATURE, "Fast pattern offset exceeds " - "limit"); + uint16_t offset; + if (StringParseUint16(&offset, 10, 0, + (const char *)arg_substr) < 0) { + SCLogError(SC_ERR_INVALID_SIGNATURE, "Invalid fast pattern offset:" + " \"%s\"", arg_substr); goto error; } @@ -297,14 +299,16 @@ static int DetectFastPatternSetup(DetectEngineCtx *de_ctx, Signature *s, const c "for fast_pattern offset"); goto error; } - int length = atoi(arg_substr); - if (length > 65535) { - SCLogError(SC_ERR_INVALID_SIGNATURE, "Fast pattern length exceeds " - "limit"); + uint16_t length; + if (StringParseUint16(&length, 10, 0, + (const char *)arg_substr) < 0) { + SCLogError(SC_ERR_INVALID_SIGNATURE, "Invalid value for fast " + "pattern: \"%s\"", arg_substr); goto error; } - if (offset + length > 65535) { + // Avoiding integer overflow + if (offset > (65535 - length)) { SCLogError(SC_ERR_INVALID_SIGNATURE, "Fast pattern (length + offset) " "exceeds limit pattern length limit"); goto error; diff --git a/src/detect-ipproto.c b/src/detect-ipproto.c index ea0373e67f..023aa02c84 100644 --- a/src/detect-ipproto.c +++ b/src/detect-ipproto.c @@ -478,7 +478,8 @@ static int DetectIPProtoTestParse02(void) static int DetectIPProtoTestSetup01(void) { const char *value_str = "14"; - int value = atoi(value_str); + int value; + FAIL_IF(StringParseInt32(&value, 10, 0, (const char *)value_str) < 0); int i; Signature *sig = SigAlloc(); diff --git a/src/detect-iprep.c b/src/detect-iprep.c index 27616c5be0..e418fb2fd6 100644 --- a/src/detect-iprep.c +++ b/src/detect-iprep.c @@ -41,6 +41,7 @@ #include "detect-engine-state.h" #include "util-debug.h" +#include "util-byte.h" #include "util-unittest.h" #include "util-unittest-helper.h" #include "util-fmemopen.h" @@ -321,10 +322,8 @@ int DetectIPRepSetup (DetectEngineCtx *de_ctx, Signature *s, const char *rawstr) } if (value != NULL && strlen(value) > 0) { - int ival = atoi(value); - if (ival < 0 || ival > 127) + if (StringParseU8RangeCheck(&val, 10, 0, (const char *)value, 0, 127) < 0) goto error; - val = (uint8_t)ival; } cd = SCMalloc(sizeof(DetectIPRepData)); diff --git a/src/detect-krb5-errcode.c b/src/detect-krb5-errcode.c index 16bd257152..1b7de6f214 100644 --- a/src/detect-krb5-errcode.c +++ b/src/detect-krb5-errcode.c @@ -23,6 +23,7 @@ #include "suricata-common.h" #include "util-unittest.h" +#include "util-byte.h" #include "detect-parse.h" #include "detect-engine.h" @@ -156,8 +157,10 @@ static DetectKrb5ErrCodeData *DetectKrb5ErrCodeParse (const char *krb5str) krb5d = SCMalloc(sizeof (DetectKrb5ErrCodeData)); if (unlikely(krb5d == NULL)) goto error; - krb5d->err_code = (int32_t)atoi(arg1); - + if (StringParseInt32(&krb5d->err_code, 10, 0, + (const char *)arg1) < 0) { + goto error; + } return krb5d; error: diff --git a/src/detect-krb5-msgtype.c b/src/detect-krb5-msgtype.c index e10ad3abca..a7b71a829a 100644 --- a/src/detect-krb5-msgtype.c +++ b/src/detect-krb5-msgtype.c @@ -23,6 +23,7 @@ #include "suricata-common.h" #include "util-unittest.h" +#include "util-byte.h" #include "detect-parse.h" #include "detect-engine.h" @@ -153,8 +154,10 @@ static DetectKrb5MsgTypeData *DetectKrb5MsgTypeParse (const char *krb5str) krb5d = SCMalloc(sizeof (DetectKrb5MsgTypeData)); if (unlikely(krb5d == NULL)) goto error; - krb5d->msg_type = (uint8_t)atoi(arg1); - + if (StringParseUint8(&krb5d->msg_type, 10, 0, + (const char *)arg1) < 0) { + goto error; + } return krb5d; error: diff --git a/src/detect-modbus.c b/src/detect-modbus.c index f9beb4126a..24075cd8f3 100644 --- a/src/detect-modbus.c +++ b/src/detect-modbus.c @@ -51,6 +51,7 @@ #include "detect-engine-modbus.h" #include "util-debug.h" +#include "util-byte.h" #include "app-layer-modbus.h" @@ -190,14 +191,21 @@ static DetectModbus *DetectModbusAccessParse(DetectEngineCtx *de_ctx, const char if (unlikely(modbus->address == NULL)) goto error; + uint8_t idx; if (arg[0] == '>') { - modbus->address->min = atoi((const char*) (arg+1)); + idx = 1; modbus->address->mode = DETECT_MODBUS_GT; } else if (arg[0] == '<') { - modbus->address->min = atoi((const char*) (arg+1)); + idx = 1; modbus->address->mode = DETECT_MODBUS_LT; } else { - modbus->address->min = atoi((const char*) arg); + idx = 0; + } + if (StringParseUint16(&modbus->address->min, 10, 0, + (const char *) (arg + idx)) < 0) { + SCLogError(SC_ERR_INVALID_VALUE, "Invalid value for min " + "address: %s", (const char *)(arg + idx)); + goto error; } SCLogDebug("and min/equal address %d", modbus->address->min); @@ -209,7 +217,12 @@ static DetectModbus *DetectModbusAccessParse(DetectEngineCtx *de_ctx, const char } if (*arg != '\0') { - modbus->address->max = atoi((const char*) (arg+2)); + if (StringParseUint16(&modbus->address->max, 10, 0, + (const char*) (arg + 2)) < 0) { + SCLogError(SC_ERR_INVALID_VALUE, "Invalid value for max " + "address: %s", (const char*)(arg + 2)); + goto error; + } modbus->address->mode = DETECT_MODBUS_RA; SCLogDebug("and max address %d", modbus->address->max); } @@ -235,14 +248,22 @@ static DetectModbus *DetectModbusAccessParse(DetectEngineCtx *de_ctx, const char if (unlikely(modbus->data == NULL)) goto error; + + uint8_t idx_mode; if (arg[0] == '>') { - modbus->data->min = atoi((const char*) (arg+1)); + idx_mode = 1; modbus->data->mode = DETECT_MODBUS_GT; } else if (arg[0] == '<') { - modbus->data->min = atoi((const char*) (arg+1)); + idx_mode = 1; modbus->data->mode = DETECT_MODBUS_LT; } else { - modbus->data->min = atoi((const char*) arg); + idx_mode = 0; + } + if (StringParseUint16(&modbus->data->min, 10, 0, + (const char*) (arg + idx_mode)) < 0) { + SCLogError(SC_ERR_INVALID_VALUE, "Invalid value for " + "min data: %s", (const char*)(arg + idx_mode)); + goto error; } SCLogDebug("and min/equal value %d", modbus->data->min); @@ -254,7 +275,13 @@ static DetectModbus *DetectModbusAccessParse(DetectEngineCtx *de_ctx, const char } if (*arg != '\0') { - modbus->data->max = atoi((const char*) (arg+2)); + if (StringParseUint16(&modbus->data->max, + 10, 0, (const char*) (arg + 2)) < 0) { + SCLogError(SC_ERR_INVALID_VALUE, + "Invalid value for max data: %s", + (const char*)(arg + 2)); + goto error; + } modbus->data->mode = DETECT_MODBUS_RA; SCLogDebug("and max value %d", modbus->data->max); } @@ -306,7 +333,11 @@ static DetectModbus *DetectModbusFunctionParse(DetectEngineCtx *de_ctx, const ch goto error; if (isdigit((unsigned char)ptr[0])) { - modbus->function = atoi((const char*) ptr); + if (StringParseUint8(&modbus->function, 10, 0, (const char *)ptr) < 0) { + SCLogError(SC_ERR_INVALID_VALUE, "Invalid value for " + "modbus function: %s", (const char *)ptr); + goto error; + } /* Function code 0 is managed by decoder_event INVALID_FUNCTION_CODE */ if (modbus->function == MODBUS_FUNC_NONE) { SCLogError(SC_ERR_INVALID_SIGNATURE, @@ -329,7 +360,12 @@ static DetectModbus *DetectModbusFunctionParse(DetectEngineCtx *de_ctx, const ch if (modbus->subfunction == NULL) goto error; - *(modbus->subfunction) = atoi((const char*) arg); + if (StringParseUint16(&(*modbus->subfunction), 10, 0, (const char *)arg) < 0) { + SCLogError(SC_ERR_INVALID_VALUE, "Invalid value for " + "modbus subfunction: %s", (const char*)arg); + goto error; + } + SCLogDebug("and subfunction %d", *(modbus->subfunction)); } } else { @@ -421,14 +457,20 @@ static DetectModbus *DetectModbusUnitIdParse(DetectEngineCtx *de_ctx, const char if (unlikely(modbus->unit_id == NULL)) goto error; + uint8_t idx; if (arg[0] == '>') { - modbus->unit_id->min = atoi((const char*) (arg+1)); + idx = 1; modbus->unit_id->mode = DETECT_MODBUS_GT; } else if (arg[0] == '<') { - modbus->unit_id->min = atoi((const char*) (arg+1)); + idx = 1; modbus->unit_id->mode = DETECT_MODBUS_LT; } else { - modbus->unit_id->min = atoi((const char*) arg); + idx = 0; + } + if (StringParseUint16(&modbus->unit_id->min, 10, 0, (const char *) (arg + idx)) < 0) { + SCLogError(SC_ERR_INVALID_VALUE, "Invalid value for " + "modbus min unit id: %s", (const char*)(arg + idx)); + goto error; } SCLogDebug("and min/equal unit id %d", modbus->unit_id->min); @@ -440,7 +482,11 @@ static DetectModbus *DetectModbusUnitIdParse(DetectEngineCtx *de_ctx, const char } if (*arg != '\0') { - modbus->unit_id->max = atoi((const char*) (arg+2)); + if (StringParseUint16(&modbus->unit_id->max, 10, 0, (const char *) (arg + 2)) < 0) { + SCLogError(SC_ERR_INVALID_VALUE, "Invalid value for " + "modbus max unit id: %s", (const char*)(arg + 2)); + goto error; + } modbus->unit_id->mode = DETECT_MODBUS_RA; SCLogDebug("and max unit id %d", modbus->unit_id->max); } -- 2.47.2