Commit 4783ee27 authored by Pieter Noordhuis's avatar Pieter Noordhuis
Browse files

Integer overflow protection

parent d128380b
...@@ -198,9 +198,24 @@ size_t redis_parser_execute(redis_parser_t *parser, redis_protocol_t **dst, cons ...@@ -198,9 +198,24 @@ size_t redis_parser_execute(redis_parser_t *parser, redis_protocol_t **dst, cons
case s_integer_body: case s_integer_body:
{ {
if (ch >= '0' && ch <= '9') { if (ch >= '0' && ch <= '9') {
if (parser->i64.ui64 > (UINT64_MAX / 10)) /* Overflow */
goto error;
parser->i64.ui64 *= 10; parser->i64.ui64 *= 10;
if (parser->i64.ui64 > (UINT64_MAX - (ch-'0'))) /* Overflow */
goto error;
parser->i64.ui64 += ch - '0'; parser->i64.ui64 += ch - '0';
} else if (ch == '\r') { } else if (ch == '\r') {
/* Check if the uint64_t can be safely casted to int64_t */
if (parser->i64.neg) {
if (parser->i64.ui64 > ((uint64_t)(-(INT64_MIN+1))+1)) /* Overflow */
goto error;
parser->i64.i64 = -parser->i64.ui64;
} else {
if (parser->i64.ui64 > INT64_MAX) /* Overflow */
goto error;
parser->i64.i64 = parser->i64.ui64;
}
state = s_integer_lf; state = s_integer_lf;
} else { } else {
goto error; goto error;
...@@ -210,16 +225,10 @@ size_t redis_parser_execute(redis_parser_t *parser, redis_protocol_t **dst, cons ...@@ -210,16 +225,10 @@ size_t redis_parser_execute(redis_parser_t *parser, redis_protocol_t **dst, cons
case s_integer_lf: case s_integer_lf:
{ {
int64_t i64;
if (ch != '\n') { if (ch != '\n') {
goto error; goto error;
} }
i64 = parser->i64.ui64 & INT64_MAX;
if (parser->i64.neg)
i64 *= -1;
/* Protocol length can be set regardless of type */ /* Protocol length can be set regardless of type */
cur->plen = nread - cur->poff + 1; /* include \n */ cur->plen = nread - cur->poff + 1; /* include \n */
...@@ -236,7 +245,7 @@ size_t redis_parser_execute(redis_parser_t *parser, redis_protocol_t **dst, cons ...@@ -236,7 +245,7 @@ size_t redis_parser_execute(redis_parser_t *parser, redis_protocol_t **dst, cons
l_integer_lf_string_t: l_integer_lf_string_t:
/* Trap the nil bulk */ /* Trap the nil bulk */
if (i64 < 0) { if (parser->i64.i64 < 0) {
CALLBACK(nil); CALLBACK(nil);
goto done; goto done;
} }
...@@ -245,22 +254,22 @@ size_t redis_parser_execute(redis_parser_t *parser, redis_protocol_t **dst, cons ...@@ -245,22 +254,22 @@ size_t redis_parser_execute(redis_parser_t *parser, redis_protocol_t **dst, cons
* known upfront, but not necessarily valid (we may see EOF * known upfront, but not necessarily valid (we may see EOF
* before seeing the last content byte). */ * before seeing the last content byte). */
cur->coff = nread + 1; /* include \n */ cur->coff = nread + 1; /* include \n */
cur->clen = (unsigned)i64; cur->clen = (unsigned)parser->i64.i64;
cur->plen += cur->clen + 2; /* include \r\n */ cur->plen += cur->clen + 2; /* include \r\n */
/* Store remaining bytes for a complete bulk */ /* Store remaining bytes for a complete bulk */
cur->remaining = (unsigned)i64; cur->remaining = (unsigned)parser->i64.i64;
state = s_bulk; state = s_bulk;
break; break;
l_integer_lf_array_t: l_integer_lf_array_t:
/* Trap the nil multi bulk */ /* Trap the nil multi bulk */
if (i64 < 0) { if (parser->i64.i64 < 0) {
CALLBACK(nil); CALLBACK(nil);
goto done; goto done;
} }
cur->remaining = (unsigned)i64; cur->remaining = (unsigned)parser->i64.i64;
CALLBACK(array, cur->remaining); CALLBACK(array, cur->remaining);
goto done; goto done;
...@@ -268,7 +277,7 @@ size_t redis_parser_execute(redis_parser_t *parser, redis_protocol_t **dst, cons ...@@ -268,7 +277,7 @@ size_t redis_parser_execute(redis_parser_t *parser, redis_protocol_t **dst, cons
/* Setup content offset and length */ /* Setup content offset and length */
cur->coff = cur->poff + 1; cur->coff = cur->poff + 1;
cur->clen = nread - cur->coff - 1; /* remove \r */ cur->clen = nread - cur->coff - 1; /* remove \r */
CALLBACK(integer, i64); CALLBACK(integer, parser->i64.i64);
goto done; goto done;
} }
......
...@@ -55,8 +55,9 @@ struct redis_parser_s { ...@@ -55,8 +55,9 @@ struct redis_parser_s {
/* private: temporary integer (integer reply, bulk length) */ /* private: temporary integer (integer reply, bulk length) */
struct { struct {
int neg; int neg; /* sign */
uint64_t ui64; uint64_t ui64; /* accumulator */
int64_t i64; /* result */
} i64; } i64;
}; };
......
...@@ -82,6 +82,11 @@ redis_parser_t *new_parser(void) { ...@@ -82,6 +82,11 @@ redis_parser_t *new_parser(void) {
return parser; return parser;
} }
void reinitialize(redis_parser_t *parser) {
reset_cb_log();
redis_parser_init(parser, &callbacks);
}
void free_parser(redis_parser_t *parser) { void free_parser(redis_parser_t *parser) {
free(parser); free(parser);
} }
...@@ -209,6 +214,72 @@ void test_integer(void) { ...@@ -209,6 +214,72 @@ void test_integer(void) {
/* Chunked check */ /* Chunked check */
test_char_by_char(res, buf, len); test_char_by_char(res, buf, len);
/* Negative sign */
buf = ":-123\r\n";
reinitialize(p);
assert(redis_parser_execute(p, &res, buf, strlen(buf)) == strlen(buf));
assert(res != NULL);
assert(cb_log_idx == 1 && cb_log[0].integer_value == -123);
test_char_by_char(res, buf, strlen(buf));
/* Positive sign */
buf = ":+123\r\n";
reinitialize(p);
assert(redis_parser_execute(p, &res, buf, strlen(buf)) == strlen(buf));
assert(res != NULL);
assert(cb_log_idx == 1 && cb_log[0].integer_value == 123);
test_char_by_char(res, buf, strlen(buf));
/* Start with 0 */
buf = ":0123\r\n";
reinitialize(p);
assert(redis_parser_execute(p, &res, buf, strlen(buf)) == 1);
assert(res == NULL);
/* Start with non-digit */
buf = ":x123\r\n";
reinitialize(p);
assert(redis_parser_execute(p, &res, buf, strlen(buf)) == 1);
assert(res == NULL);
/* Non-digit in the middle */
buf = ":12x3\r\n";
reinitialize(p);
assert(redis_parser_execute(p, &res, buf, strlen(buf)) == 3);
assert(res == NULL);
/* Non-digit at the end */
buf = ":123x\r\n";
reinitialize(p);
assert(redis_parser_execute(p, &res, buf, strlen(buf)) == 4);
assert(res == NULL);
/* Signed 64-bit maximum */
buf = ":9223372036854775807\r\n";
reinitialize(p);
assert(redis_parser_execute(p, &res, buf, strlen(buf)) == strlen(buf));
assert(res != NULL);
assert(cb_log_idx == 1 && cb_log[0].integer_value == 9223372036854775807LL);
/* Signed 64-bit maximum overflow */
buf = ":9223372036854775808\r\n";
reinitialize(p);
assert(redis_parser_execute(p, &res, buf, strlen(buf)) == strlen(buf)-2);
assert(res == NULL);
/* Signed 64-bit minimum */
buf = ":-9223372036854775808\r\n";
reinitialize(p);
assert(redis_parser_execute(p, &res, buf, strlen(buf)) == strlen(buf));
assert(res != NULL);
assert(cb_log_idx == 1 && cb_log[0].integer_value == -9223372036854775808LL);
/* Signed 64-bit minimum overflow (or underflow...) */
buf = ":-9223372036854775809\r\n";
reinitialize(p);
assert(redis_parser_execute(p, &res, buf, strlen(buf)) == strlen(buf)-2);
assert(res == NULL);
free_parser(p); free_parser(p);
} }
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment