From 729f404efec0795f7ed358e7b2fa08bd62cc1ae8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 10 Jan 2011 12:07:34 -0500 Subject: Add logic in routerparse to not read overlong private keys I am not at all sure that it is possible to trigger a bug here, but better safe than sorry. --- src/or/routerparse.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/or/routerparse.c') diff --git a/src/or/routerparse.c b/src/or/routerparse.c index fc30c625bf..6ca2293375 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -3132,7 +3132,7 @@ get_next_token(memarea_t *area, RET_ERR("Couldn't parse public key."); } else if (!strcmp(tok->object_type, "RSA PRIVATE KEY")) { /* private key */ tok->key = crypto_new_pk_env(); - if (crypto_pk_read_private_key_from_string(tok->key, obstart)) + if (crypto_pk_read_private_key_from_string(tok->key, obstart, eol-obstart)) RET_ERR("Couldn't parse private key."); } else { /* If it's something else, try to base64-decode it */ int r; -- cgit v1.2.3-54-g00ecf From 373a1bc40e88d90e75f4a70b7d2018fe7288035a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 10 Jan 2011 12:12:11 -0500 Subject: Impose maximum sizes on parsed objects An object, you'll recall, is something between -----BEGIN----- and -----END----- tags in a directory document. Some of our code, as doorss has noted in bug 2352, could assert if one of these ever overflowed SIZE_T_CEILING but not INT_MAX. As a solution, I'm setting a maximum size on a single object such that neither of these limits will ever be hit. I'm also fixing the INT_MAX checks, just to be sure. --- changes/bug2352 | 6 ++++++ src/or/routerparse.c | 11 +++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) create mode 100644 changes/bug2352 (limited to 'src/or/routerparse.c') diff --git a/changes/bug2352 b/changes/bug2352 new file mode 100644 index 0000000000..744dbdb20f --- /dev/null +++ b/changes/bug2352 @@ -0,0 +1,6 @@ + o Minor bugfixes + - Fix some potential asserts and partsing issues with grossly + malformed router caches. Fixes bug 2352. Found by doorss. + Bugfix on Tor 0.2.1.27. + + diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 6ca2293375..070c61b1a1 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -2549,7 +2549,7 @@ networkstatus_parse_vote_from_string(const char *s, const char **eos_out, goto err; v->good_signature = 1; } else { - if (tok->object_size >= INT_MAX) + if (tok->object_size >= INT_MAX || tok->object_size >= SIZE_T_CEILING) goto err; /* We already parsed a vote from this voter. Use the first one. */ if (v->signature) { @@ -2700,7 +2700,7 @@ networkstatus_parse_detached_signatures(const char *s, const char *eos) voter = tor_malloc_zero(sizeof(networkstatus_voter_info_t)); memcpy(voter->identity_digest, id_digest, DIGEST_LEN); memcpy(voter->signing_key_digest, sk_digest, DIGEST_LEN); - if (tok->object_size >= INT_MAX) + if (tok->object_size >= INT_MAX || tok->object_size >= SIZE_T_CEILING) goto err; voter->signature = tor_memdup(tok->object_body, tok->object_size); voter->signature_len = (int) tok->object_size; @@ -3017,6 +3017,10 @@ static directory_token_t * get_next_token(memarea_t *area, const char **s, const char *eos, token_rule_t *table) { + /** Reject any object at least this big; it is probably an overflow, an + * attack, a bug, or some other nonsense. */ +#define MAX_UNPARSED_OBJECT_SIZE (128*1024) + const char *next, *eol, *obstart; size_t obname_len; int i; @@ -3126,6 +3130,9 @@ get_next_token(memarea_t *area, ebuf[sizeof(ebuf)-1] = '\0'; RET_ERR(ebuf); } + if (next - *s > MAX_UNPARSED_OBJECT_SIZE) + RET_ERR("Couldn't parse object: missing footer or object much too big."); + if (!strcmp(tok->object_type, "RSA PUBLIC KEY")) { /* If it's a public key */ tok->key = crypto_new_pk_env(); if (crypto_pk_read_public_key_from_string(tok->key, obstart, eol-obstart)) -- cgit v1.2.3-54-g00ecf From 1f3b4420233e83ef160ac41398827994ec7ae152 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 15 Jan 2011 10:42:11 -0500 Subject: catch another overlong malloc possibility. found by cypherpunks --- src/or/routerparse.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'src/or/routerparse.c') diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 070c61b1a1..3aaefec681 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -3105,7 +3105,8 @@ get_next_token(memarea_t *area, obstart = *s; /* Set obstart to start of object spec */ if (*s+16 >= eol || memchr(*s+11,'\0',eol-*s-16) || /* no short lines, */ - strcmp_len(eol-5, "-----", 5)) { /* nuls or invalid endings */ + strcmp_len(eol-5, "-----", 5) || /* nuls or invalid endings */ + (eol-*s) > MAX_UNPARSED_OBJECT_SIZE) { /* name too long */ RET_ERR("Malformed object: bad begin line"); } tok->object_type = STRNDUP(*s+11, eol-*s-16); -- cgit v1.2.3-54-g00ecf