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

Jump to bottom

prevent overflows in config_parseduration and config_parsebytesize #4536

Merged
merged 4 commits into from
Jul 4, 2023
Merged

prevent overflows in config_parseduration and config_parsebytesize #4536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
06071ae
util: add buf_cstring_or_empty()
elliefm Jun 30, 2023
ddbfb07
libconfig: prevent overflow in config_parseduration
elliefm Jun 23, 2023
a4423be
libconfig: tighten up negative duration parsing
elliefm Jun 26, 2023
d45651f
libconfig: prevent overflow in config_parsebytesize
elliefm Jun 26, 2023
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
52 changes: 51 additions & 1 deletion cunit/libconfig.testc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,10 @@
* OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*/

#include <unistd.h>
#include <limits.h>
#include <stdint.h>
#include <sysexits.h>
#include <unistd.h>

#include "cunit/cyrunit.h"

Expand Down Expand Up @@ -438,14 +440,44 @@ static const struct duration_parse_data duration_parse_tests[] = {

{ "-1", 0, -1 },
{ "-0", 0, 0 },
{ "-", -1, 0xdeadbeef },
{ "-s", -1, 0xdeadbeef },
{ "--1", -1, 0xdeadbeef },
{ "-1s", 0, -1 },
{ "-1m", 0, -60 },
{ "1-2m", -1, 0xdeadbeef },
{ "1m-2s", -1, 0xdeadbeef },
{ "1m0-2s", -1, 0xdeadbeef },

{ "1h1h", 0, 2 * 60 * 60 }, /* silly, but let it work */
{ "1hh", -1, 0xdeadbeef }, /* bogus, reject it */
{ "1hhh", -1, 0xdeadbeef }, /* bogus, reject it */

/* XXX config_parseduration uses int type and INT_MAX, but these limit
* XXX tests use hardcoded strings, which means they'll fail on a platform
* XXX where int is something other than 32 bits and has different limits.
* XXX I figure we can worry about that later, if/when it ever happens.
*/
/* exercise all the multipliers against overflow */
{ "2147483647s", 0, INT_MAX },
{ "2147483648s", -1, 0xdeadbeef },
{ "-2147483647s", 0, -INT_MAX },
{ "-2147483648s", -1, 0xdeadbeef },
{ "35791394m", 0, 60 * (INT_MAX / 60) },
{ "35791395m", -1, 0xdeadbeef },
{ "-35791394m", 0, -60 * (INT_MAX / 60) },
{ "-35791395m", -1, 0xdeadbeef },
{ "596523h", 0, 3600 * (INT_MAX / 3600) },
{ "596524h", -1, 0xdeadbeef },
{ "-596523h", 0, -3600 * (INT_MAX / 3600) },
{ "-596524h", -1, 0xdeadbeef },
{ "24855d", 0, 86400 * (INT_MAX / 86400) },
{ "24856d", -1, 0xdeadbeef },
{ "-24855d", 0, -86400 * (INT_MAX / 86400) },
{ "-24856d", -1, 0xdeadbeef },

{ "24855d3h14m7s", 0, INT_MAX },
{ "24855d3h14m8s", -1, 0xdeadbeef },
};

static void test_duration_parse(void)
Expand Down Expand Up @@ -734,6 +766,24 @@ static const struct bytesize_parse_data bytesize_parse_tests[] = {
{ "12 K", 0, 12LL * 1024 },
{ "12 KB", 0, 12LL * 1024 },
{ "12 KiB", 0, 12LL * 1024 },

/* overflow tests */
{ "9223372036854775807 B", 0, INT64_MAX },
{ "-9223372036854775808 B", 0, INT64_MIN },
{ "9223372036854775808 B", -1, 0xdeadbeefLL },
{ "-9223372036854775809 B", -1, 0xdeadbeefLL },
{ "9007199254740991 K", 0, 1024LL * (INT64_MAX / 1024LL) },
{ "-9007199254740992 K", 0, 1024LL * (INT64_MIN / 1024LL) },
{ "9007199254740992 K", -1, 0xdeadbeefLL },
{ "-9007199254740993 K", -1, 0xdeadbeefLL },
{ "8796093022207 M", 0, 1048576LL * (INT64_MAX / 1048576LL) },
{ "-8796093022208 M", 0, 1048576LL * (INT64_MIN / 1048576LL) },
{ "8796093022208 M", -1, 0xdeadbeefLL },
{ "-8796093022209 M", -1, 0xdeadbeefLL },
{ "8589934591 G", 0, 1073741824LL * (INT64_MAX / 1073741824LL) },
{ "-8589934592 G", 0, 1073741824LL * (INT64_MIN / 1073741824LL) },
{ "8589934592 G", -1, 0xdeadbeefLL },
{ "-8589934593 G", -1, 0xdeadbeefLL },
};

static void test_bytesize_parse(void)
Expand Down
106 changes: 80 additions & 26 deletions lib/libconfig.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,28 @@ EXPORTED unsigned long config_getbitfield(enum imapopt opt)
return imapopts[opt].val.x;
}

static inline int accumulate(int *val, int mult, int nextchar,
struct buf *parse_err)
{
int newdigit = 0;

assert(val != NULL);

if (cyrus_isdigit(nextchar)) newdigit = nextchar - '0';

if (*val > INT_MAX / mult
|| (*val == INT_MAX / mult
&& newdigit > INT_MAX % mult))
{
if (parse_err)
buf_printf(parse_err, "would overflow at '%c'", nextchar);
return -1;
}

*val = *val * mult + newdigit;
return 0;
}

/* Parse a duration value, converted to seconds.
*
* defunit is one of 'd', 'h', 'm', 's' and determines how
Expand All @@ -198,6 +220,7 @@ EXPORTED int config_parseduration(const char *str, int defunit, int *out_duratio
const char *p;
int accum = 0, duration = 0, neg = 0, sawdigit = 0, r = 0;
char *copy = NULL;
struct buf parse_err = BUF_INITIALIZER;

/* the default default unit is seconds */
if (!defunit) defunit = 's';
Expand All @@ -210,40 +233,51 @@ EXPORTED int config_parseduration(const char *str, int defunit, int *out_duratio

p = copy;
if (*p == '-') {
if (!cyrus_isdigit(p[1])) {
buf_setcstr(&parse_err, "no digit after '-'");
r = -1;
goto done;
}
neg = 1;
p++;
}
for (; *p; p++) {
if (cyrus_isdigit(*p)) {
accum *= 10;
accum += (*p - '0');
r = accumulate(&accum, 10, *p, &parse_err);
if (r) goto done;
sawdigit = 1;
}
else {
if (!sawdigit) {
syslog(LOG_DEBUG, "%s: no digit before '%c' in '%s'",
__func__, *p, str);
buf_printf(&parse_err, "no digit before '%c'", *p);
r = -1;
goto done;
}
sawdigit = 0;
switch (*p) {
case 'd':
accum *= 24;
r = accumulate(&accum, 24, *p, &parse_err);
if (r) goto done;
/* fall through */
case 'h':
accum *= 60;
r = accumulate(&accum, 60, *p, &parse_err);
if (r) goto done;
/* fall through */
case 'm':
accum *= 60;
r = accumulate(&accum, 60, *p, &parse_err);
if (r) goto done;
/* fall through */
case 's':
if (duration > INT_MAX - accum) {
buf_printf(&parse_err, "would overflow at '%c'", *p);
r = -1;
goto done;
}
duration += accum;
accum = 0;
break;
default:
syslog(LOG_DEBUG, "%s: bad unit '%c' in %s",
__func__, *p, str);
buf_printf(&parse_err, "bad unit '%c'", *p);
r = -1;
goto done;
}
Expand All @@ -257,7 +291,14 @@ EXPORTED int config_parseduration(const char *str, int defunit, int *out_duratio
if (out_duration) *out_duration = duration;

done:
if (copy) free(copy);
if (r) {
xsyslog(LOG_ERR, "unable to parse duration from string",
"value=<%s> parse_err=<%s>",
str, buf_cstring_or_empty(&parse_err));
}

buf_free(&parse_err);
free(copy);
return r;
}

Expand Down Expand Up @@ -304,6 +345,7 @@ EXPORTED int config_parsebytesize(const char *str,
int64_t bytesize;
int i_allowed = 0, r = 0;
char *copy = NULL, *p;
struct buf parse_err = BUF_INITIALIZER;

assert(strchr("BKMG", defunit) != NULL); /* n.b. also permits \0 */

Expand All @@ -320,26 +362,16 @@ EXPORTED int config_parsebytesize(const char *str,
errno = 0;
bytesize = strtoll(copy, &p, 10);
if (errno) {
xsyslog(LOG_ERR, "unable to parse byte size from string",
"value=<%s>", str);
buf_setcstr(&parse_err, strerror(errno));
errno = 0;
r = -1;
goto done;
}

/* better be some digits */
if (p == copy) {
struct buf msg = BUF_INITIALIZER;

buf_appendcstr(&msg, "no digit ");
if (*p) {
buf_printf(&msg, "before '%c' ", *p);
}
buf_printf(&msg, "in '%s'", str);

syslog(LOG_DEBUG, "%s: %s", __func__, buf_cstring(&msg));
buf_free(&msg);

buf_setcstr(&parse_err, "no digit");
if (*p) buf_printf(&parse_err, " before '%c'", *p);
r = -1;
goto done;
}
Expand All @@ -351,14 +383,29 @@ EXPORTED int config_parsebytesize(const char *str,
switch (*p) {
case 'g':
case 'G':
if (bytesize > INT64_MAX / 1024 || bytesize < INT64_MIN / 1024) {
buf_printf(&parse_err, "would overflow at '%c'", *p);
r = -1;
goto done;
}
bytesize *= 1024;
/* fall through */
case 'm':
case 'M':
if (bytesize > INT64_MAX / 1024 || bytesize < INT64_MIN / 1024) {
buf_printf(&parse_err, "would overflow at '%c'", *p);
r = -1;
goto done;
}
bytesize *= 1024;
/* fall through */
case 'k':
case 'K':
if (bytesize > INT64_MAX / 1024 || bytesize < INT64_MIN / 1024) {
buf_printf(&parse_err, "would overflow at '%c'", *p);
r = -1;
goto done;
}
bytesize *= 1024;
i_allowed = 1;
p++;
Expand All @@ -373,15 +420,22 @@ EXPORTED int config_parsebytesize(const char *str,

/* we'd better be at end of string! */
if (*p) {
syslog(LOG_DEBUG, "%s: bad unit '%c' in %s",
__func__, *p, str);
buf_printf(&parse_err, "bad unit '%c'", *p);
r = -1;
goto done;
}

done:
if (!r && out_bytesize) *out_bytesize = bytesize;
if (r) {
xsyslog(LOG_ERR, "unable to parse bytesize from string",
"value=<%s> parse_err=<%s>",
str, buf_cstring_or_empty(&parse_err));
}
else if (out_bytesize) {
*out_bytesize = bytesize;
}

buf_free(&parse_err);
free(copy);
return r;
}
Expand Down
30 changes: 18 additions & 12 deletions lib/util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1120,6 +1120,24 @@ EXPORTED const char *buf_cstring(const struct buf *buf)
return buf->s;
}

EXPORTED const char *buf_cstringnull(const struct buf *buf)
{
if (!buf->s) return NULL;
return buf_cstring(buf);
}

EXPORTED const char *buf_cstringnull_ifempty(const struct buf *buf)
{
if (!buf->len) return NULL;
return buf_cstring(buf);
}

EXPORTED const char *buf_cstring_or_empty(const struct buf *buf)
{
if (!buf->s) return "";
return buf_cstring(buf);
}

EXPORTED char *buf_newcstring(struct buf *buf)
{
char *ret = xstrdup(buf_cstring(buf));
Expand All @@ -1136,18 +1154,6 @@ EXPORTED char *buf_release(struct buf *buf)
return ret;
}

EXPORTED const char *buf_cstringnull(const struct buf *buf)
{
if (!buf->s) return NULL;
return buf_cstring(buf);
}

EXPORTED const char *buf_cstringnull_ifempty(const struct buf *buf)
{
if (!buf->len) return NULL;
return buf_cstring(buf);
}

EXPORTED char *buf_releasenull(struct buf *buf)
{
char *ret = (char *)buf_cstringnull(buf);
Expand Down
3 changes: 2 additions & 1 deletion lib/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,8 +295,9 @@ void _buf_ensure(struct buf *buf, size_t len);
const char *buf_cstring(const struct buf *buf);
const char *buf_cstringnull(const struct buf *buf);
const char *buf_cstringnull_ifempty(const struct buf *buf);
char *buf_release(struct buf *buf);
const char *buf_cstring_or_empty(const struct buf *buf);
char *buf_newcstring(struct buf *buf);
char *buf_release(struct buf *buf);
char *buf_releasenull(struct buf *buf);
void buf_getmap(struct buf *buf, const char **base, size_t *len);
int buf_getline(struct buf *buf, FILE *fp);
Expand Down