From 115782bdbe42e4b3d5cb386d2939a883bc381d12 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 13 Jan 2011 14:36:41 -0500 Subject: [PATCH] Fix a heap overflow found by debuger, and make it harder to make that mistake again Our public key functions assumed that they were always writing into a large enough buffer. In one case, they weren't. (Incorporates fixes from sebastian) --- changes/tolen_asserts | 9 +++++++++ src/common/crypto.c | 53 +++++++++++++++++++++++++++++++++++++++----------- src/common/crypto.h | 12 +++++++----- src/or/config.c | 3 ++- src/or/networkstatus.c | 1 + src/or/onion.c | 2 ++ src/or/rendclient.c | 1 + src/or/rendcommon.c | 4 +++- src/or/rendservice.c | 6 ++++-- src/or/routerlist.c | 3 ++- src/or/routerparse.c | 16 +++++++++------ src/or/test.c | 37 +++++++++++++++++++++-------------- 12 files changed, 105 insertions(+), 42 deletions(-) create mode 100644 changes/tolen_asserts diff --git a/changes/tolen_asserts b/changes/tolen_asserts new file mode 100644 index 0000000000..90cdb2d75e --- /dev/null +++ b/changes/tolen_asserts @@ -0,0 +1,9 @@ + o Major bugfixes (security) + - Fix a heap overflow bug where an adversary could cause heap + corruption. Since the contents of the corruption would need to be + the output of an RSA decryption, we do not think this is easy to + turn in to a remote code execution attack, but everybody should + upgrade anyway. Found by debuger. Bugfix on 0.1.2.10-rc. + o Defensive programming + - Introduce output size checks on all of our decryption functions. + diff --git a/src/common/crypto.c b/src/common/crypto.c index 3343980331..7cb849a64b 100644 --- a/src/common/crypto.c +++ b/src/common/crypto.c @@ -717,9 +717,12 @@ crypto_pk_copy_full(crypto_pk_env_t *env) * in env, using the padding method padding. On success, * write the result to to, and return the number of bytes * written. On failure, return -1. + * + * tolen is the number of writable bytes in to, and must be + * at least the length of the modulus of env. */ int -crypto_pk_public_encrypt(crypto_pk_env_t *env, char *to, +crypto_pk_public_encrypt(crypto_pk_env_t *env, char *to, size_t tolen, const char *from, size_t fromlen, int padding) { int r; @@ -727,6 +730,7 @@ crypto_pk_public_encrypt(crypto_pk_env_t *env, char *to, tor_assert(from); tor_assert(to); tor_assert(fromlen= crypto_pk_keysize(env)); r = RSA_public_encrypt((int)fromlen, (unsigned char*)from, (unsigned char*)to, @@ -742,9 +746,13 @@ crypto_pk_public_encrypt(crypto_pk_env_t *env, char *to, * in env, using the padding method padding. On success, * write the result to to, and return the number of bytes * written. On failure, return -1. + * + * tolen is the number of writable bytes in to, and must be + * at least the length of the modulus of env. */ int crypto_pk_private_decrypt(crypto_pk_env_t *env, char *to, + size_t tolen, const char *from, size_t fromlen, int padding, int warnOnFailure) { @@ -754,6 +762,7 @@ crypto_pk_private_decrypt(crypto_pk_env_t *env, char *to, tor_assert(to); tor_assert(env->key); tor_assert(fromlen= crypto_pk_keysize(env)); if (!env->key->p) /* Not a private key */ return -1; @@ -774,9 +783,13 @@ crypto_pk_private_decrypt(crypto_pk_env_t *env, char *to, * public key in env, using PKCS1 padding. On success, write the * signed data to to, and return the number of bytes written. * On failure, return -1. + * + * tolen is the number of writable bytes in to, and must be + * at least the length of the modulus of env. */ int crypto_pk_public_checksig(crypto_pk_env_t *env, char *to, + size_t tolen, const char *from, size_t fromlen) { int r; @@ -784,6 +797,7 @@ crypto_pk_public_checksig(crypto_pk_env_t *env, char *to, tor_assert(from); tor_assert(to); tor_assert(fromlen < INT_MAX); + tor_assert(tolen >= crypto_pk_keysize(env)); r = RSA_public_decrypt((int)fromlen, (unsigned char*)from, (unsigned char*)to, env->key, RSA_PKCS1_PADDING); @@ -806,6 +820,7 @@ crypto_pk_public_checksig_digest(crypto_pk_env_t *env, const char *data, { char digest[DIGEST_LEN]; char *buf; + size_t buflen; int r; tor_assert(env); @@ -818,8 +833,9 @@ crypto_pk_public_checksig_digest(crypto_pk_env_t *env, const char *data, log_warn(LD_BUG, "couldn't compute digest"); return -1; } - buf = tor_malloc(crypto_pk_keysize(env)+1); - r = crypto_pk_public_checksig(env,buf,sig,siglen); + buflen = crypto_pk_keysize(env)+1; + buf = tor_malloc(buflen); + r = crypto_pk_public_checksig(env,buf,buflen,sig,siglen); if (r != DIGEST_LEN) { log_warn(LD_CRYPTO, "Invalid signature"); tor_free(buf); @@ -839,9 +855,12 @@ crypto_pk_public_checksig_digest(crypto_pk_env_t *env, const char *data, * env, using PKCS1 padding. On success, write the signature to * to, and return the number of bytes written. On failure, return * -1. + * + * tolen is the number of writable bytes in to, and must be + * at least the length of the modulus of env. */ int -crypto_pk_private_sign(crypto_pk_env_t *env, char *to, +crypto_pk_private_sign(crypto_pk_env_t *env, char *to, size_t tolen, const char *from, size_t fromlen) { int r; @@ -849,6 +868,7 @@ crypto_pk_private_sign(crypto_pk_env_t *env, char *to, tor_assert(from); tor_assert(to); tor_assert(fromlen < INT_MAX); + tor_assert(tolen >= crypto_pk_keysize(env)); if (!env->key->p) /* Not a private key */ return -1; @@ -867,16 +887,19 @@ crypto_pk_private_sign(crypto_pk_env_t *env, char *to, * from; sign the data with the private key in env, and * store it in to. Return the number of bytes written on * success, and -1 on failure. + * + * tolen is the number of writable bytes in to, and must be + * at least the length of the modulus of env. */ int -crypto_pk_private_sign_digest(crypto_pk_env_t *env, char *to, +crypto_pk_private_sign_digest(crypto_pk_env_t *env, char *to, size_t tolen, const char *from, size_t fromlen) { int r; char digest[DIGEST_LEN]; if (crypto_digest(digest,from,fromlen)<0) return -1; - r = crypto_pk_private_sign(env,to,digest,DIGEST_LEN); + r = crypto_pk_private_sign(env,to,tolen,digest,DIGEST_LEN); memset(digest, 0, sizeof(digest)); return r; } @@ -900,7 +923,7 @@ crypto_pk_private_sign_digest(crypto_pk_env_t *env, char *to, */ int crypto_pk_public_hybrid_encrypt(crypto_pk_env_t *env, - char *to, + char *to, size_t tolen, const char *from, size_t fromlen, int padding, int force) @@ -923,8 +946,13 @@ crypto_pk_public_hybrid_encrypt(crypto_pk_env_t *env, if (!force && fromlen+overhead <= pkeylen) { /* It all fits in a single encrypt. */ - return crypto_pk_public_encrypt(env,to,from,fromlen,padding); + return crypto_pk_public_encrypt(env,to, + tolen, + from,fromlen,padding); } + tor_assert(tolen >= fromlen + overhead + CIPHER_KEY_LEN); + tor_assert(tolen >= pkeylen); + cipher = crypto_new_cipher_env(); if (!cipher) return -1; if (crypto_cipher_generate_key(cipher)<0) @@ -946,7 +974,7 @@ crypto_pk_public_hybrid_encrypt(crypto_pk_env_t *env, /* Length of symmetrically encrypted data. */ symlen = fromlen-(pkeylen-overhead-CIPHER_KEY_LEN); - outlen = crypto_pk_public_encrypt(env,to,buf,pkeylen-overhead,padding); + outlen = crypto_pk_public_encrypt(env,to,tolen,buf,pkeylen-overhead,padding); if (outlen!=(int)pkeylen) { goto err; } @@ -972,6 +1000,7 @@ crypto_pk_public_hybrid_encrypt(crypto_pk_env_t *env, int crypto_pk_private_hybrid_decrypt(crypto_pk_env_t *env, char *to, + size_t tolen, const char *from, size_t fromlen, int padding, int warnOnFailure) @@ -985,11 +1014,12 @@ crypto_pk_private_hybrid_decrypt(crypto_pk_env_t *env, pkeylen = crypto_pk_keysize(env); if (fromlen <= pkeylen) { - return crypto_pk_private_decrypt(env,to,from,fromlen,padding, + return crypto_pk_private_decrypt(env,to,tolen,from,fromlen,padding, warnOnFailure); } + buf = tor_malloc(pkeylen+1); - outlen = crypto_pk_private_decrypt(env,buf,from,pkeylen,padding, + outlen = crypto_pk_private_decrypt(env,buf,pkeylen+1,from,pkeylen,padding, warnOnFailure); if (outlen<0) { log_fn(warnOnFailure?LOG_WARN:LOG_DEBUG, LD_CRYPTO, @@ -1007,6 +1037,7 @@ crypto_pk_private_hybrid_decrypt(crypto_pk_env_t *env, } memcpy(to,buf+CIPHER_KEY_LEN,outlen-CIPHER_KEY_LEN); outlen -= CIPHER_KEY_LEN; + tor_assert(tolen - outlen >= fromlen - pkeylen); r = crypto_cipher_decrypt(cipher, to+outlen, from+pkeylen, fromlen-pkeylen); if (r<0) goto err; diff --git a/src/common/crypto.h b/src/common/crypto.h index 4fb06be41d..9cfb41444b 100644 --- a/src/common/crypto.h +++ b/src/common/crypto.h @@ -93,23 +93,25 @@ crypto_pk_env_t *crypto_pk_dup_key(crypto_pk_env_t *orig); crypto_pk_env_t *crypto_pk_copy_full(crypto_pk_env_t *orig); int crypto_pk_key_is_private(const crypto_pk_env_t *key); -int crypto_pk_public_encrypt(crypto_pk_env_t *env, char *to, +int crypto_pk_public_encrypt(crypto_pk_env_t *env, char *to, size_t tolen, const char *from, size_t fromlen, int padding); -int crypto_pk_private_decrypt(crypto_pk_env_t *env, char *to, +int crypto_pk_private_decrypt(crypto_pk_env_t *env, char *to, size_t tolen, const char *from, size_t fromlen, int padding, int warnOnFailure); -int crypto_pk_public_checksig(crypto_pk_env_t *env, char *to, +int crypto_pk_public_checksig(crypto_pk_env_t *env, char *to, size_t tolen, const char *from, size_t fromlen); int crypto_pk_public_checksig_digest(crypto_pk_env_t *env, const char *data, size_t datalen, const char *sig, size_t siglen); -int crypto_pk_private_sign(crypto_pk_env_t *env, char *to, +int crypto_pk_private_sign(crypto_pk_env_t *env, char *to, size_t tolen, const char *from, size_t fromlen); -int crypto_pk_private_sign_digest(crypto_pk_env_t *env, char *to, +int crypto_pk_private_sign_digest(crypto_pk_env_t *env, char *to, size_t tolen, const char *from, size_t fromlen); int crypto_pk_public_hybrid_encrypt(crypto_pk_env_t *env, char *to, + size_t tolen, const char *from, size_t fromlen, int padding, int force); int crypto_pk_private_hybrid_decrypt(crypto_pk_env_t *env, char *to, + size_t tolen, const char *from, size_t fromlen, int padding, int warnOnFailure); diff --git a/src/or/config.c b/src/or/config.c index 45f6114579..f8cfd29f84 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -5321,7 +5321,8 @@ or_state_save(time_t now) tor_free(state); fname = get_datadir_fname("state"); if (write_str_to_file(fname, contents, 0)<0) { - log_warn(LD_FS, "Unable to write state to file \"%s\"", fname); + log_warn(LD_FS, "Unable to write state to file \"%s\"; " + "will try again later", fname); tor_free(fname); tor_free(contents); return -1; diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 53e8a63ea3..7106294d54 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -362,6 +362,7 @@ networkstatus_check_voter_signature(networkstatus_t *consensus, signed_digest = tor_malloc(signed_digest_len); if (crypto_pk_public_checksig(cert->signing_key, signed_digest, + signed_digest_len, voter->signature, voter->signature_len) != DIGEST_LEN || memcmp(signed_digest, consensus->networkstatus_digest, DIGEST_LEN)) { diff --git a/src/or/onion.c b/src/or/onion.c index 45c75b0717..bf72b4cab1 100644 --- a/src/or/onion.c +++ b/src/or/onion.c @@ -188,6 +188,7 @@ onion_skin_create(crypto_pk_env_t *dest_router_key, /* set meeting point, meeting cookie, etc here. Leave zero for now. */ if (crypto_pk_public_hybrid_encrypt(dest_router_key, onion_skin_out, + ONIONSKIN_CHALLENGE_LEN, challenge, DH_KEY_LEN, PK_PKCS1_OAEP_PADDING, 1)<0) goto err; @@ -230,6 +231,7 @@ onion_skin_server_handshake(const char *onion_skin, /*ONIONSKIN_CHALLENGE_LEN*/ break; note_crypto_pk_op(DEC_ONIONSKIN); len = crypto_pk_private_hybrid_decrypt(k, challenge, + ONIONSKIN_CHALLENGE_LEN, onion_skin, ONIONSKIN_CHALLENGE_LEN, PK_PKCS1_OAEP_PADDING,0); if (len>0) diff --git a/src/or/rendclient.c b/src/or/rendclient.c index edd24d8a3b..ab18d35298 100644 --- a/src/or/rendclient.c +++ b/src/or/rendclient.c @@ -193,6 +193,7 @@ rend_client_send_introduction(origin_circuit_t *introcirc, /*XXX maybe give crypto_pk_public_hybrid_encrypt a max_len arg, * to avoid buffer overflows? */ r = crypto_pk_public_hybrid_encrypt(intro_key, payload+DIGEST_LEN, + sizeof(payload)-DIGEST_LEN, tmp, (int)(dh_offset+DH_KEY_LEN), PK_PKCS1_OAEP_PADDING, 0); diff --git a/src/or/rendcommon.c b/src/or/rendcommon.c index 1d96f3daf5..d6f5443815 100644 --- a/src/or/rendcommon.c +++ b/src/or/rendcommon.c @@ -700,7 +700,9 @@ rend_encode_service_descriptor(rend_service_descriptor_t *desc, cp += ipoint_len+1; } note_crypto_pk_op(REND_SERVER); - r = crypto_pk_private_sign_digest(key, cp, *str_out, cp-*str_out); + r = crypto_pk_private_sign_digest(key, + cp, buflen - (cp - *str_out), + *str_out, cp-*str_out); if (r<0) { tor_free(*str_out); return -1; diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 9035ea4730..07f01aead9 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -979,7 +979,8 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request, /* Next N bytes is encrypted with service key */ note_crypto_pk_op(REND_SERVER); r = crypto_pk_private_hybrid_decrypt( - intro_key,buf,(char*)(request+DIGEST_LEN),request_len-DIGEST_LEN, + intro_key,buf,sizeof(buf), + (char*)(request+DIGEST_LEN),request_len-DIGEST_LEN, PK_PKCS1_OAEP_PADDING,1); if (r<0) { log_warn(LD_PROTOCOL, "Couldn't decrypt INTRODUCE2 cell."); @@ -1424,7 +1425,8 @@ rend_service_intro_has_opened(origin_circuit_t *circuit) goto err; len += 20; note_crypto_pk_op(REND_SERVER); - r = crypto_pk_private_sign_digest(intro_key, buf+len, buf, len); + r = crypto_pk_private_sign_digest(intro_key, buf+len, sizeof(buf)-len, + buf, len); if (r<0) { log_warn(LD_BUG, "Internal error: couldn't sign introduction request."); reason = END_CIRC_REASON_INTERNAL; diff --git a/src/or/routerlist.c b/src/or/routerlist.c index c6c84a877d..7c8e36e402 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -4676,7 +4676,8 @@ routerinfo_incompatible_with_extrainfo(routerinfo_t *ri, extrainfo_t *ei, if (ei->pending_sig) { char signed_digest[128]; - if (crypto_pk_public_checksig(ri->identity_pkey, signed_digest, + if (crypto_pk_public_checksig(ri->identity_pkey, + signed_digest, sizeof(signed_digest), ei->pending_sig, ei->pending_sig_len) != DIGEST_LEN || memcmp(signed_digest, ei->cache_info.signed_descriptor_digest, DIGEST_LEN)) { diff --git a/src/or/routerparse.c b/src/or/routerparse.c index fc30c625bf..9ad84ed8db 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -571,10 +571,12 @@ router_append_dirobj_signature(char *buf, size_t buf_len, const char *digest, crypto_pk_env_t *private_key) { char *signature; - size_t i; + size_t i, keysize; - signature = tor_malloc(crypto_pk_keysize(private_key)); - if (crypto_pk_private_sign(private_key, signature, digest, DIGEST_LEN) < 0) { + keysize = crypto_pk_keysize(private_key); + signature = tor_malloc(keysize); + if (crypto_pk_private_sign(private_key, signature, keysize, + digest, DIGEST_LEN) < 0) { log_warn(LD_BUG,"Couldn't sign digest."); goto err; @@ -924,6 +926,7 @@ check_signature_token(const char *digest, const char *doctype) { char *signed_digest; + size_t keysize; const int check_authority = (flags & CST_CHECK_AUTHORITY); const int check_objtype = ! (flags & CST_NO_CHECK_OBJTYPE); @@ -945,9 +948,10 @@ check_signature_token(const char *digest, } } - signed_digest = tor_malloc(tok->object_size); - if (crypto_pk_public_checksig(pkey, signed_digest, tok->object_body, - tok->object_size) + keysize = crypto_pk_keysize(pkey); + signed_digest = tor_malloc(keysize); + if (crypto_pk_public_checksig(pkey, signed_digest, keysize, + tok->object_body, tok->object_size) != DIGEST_LEN) { log_warn(LD_DIR, "Error reading %s: invalid signature.", doctype); tor_free(signed_digest); diff --git a/src/or/test.c b/src/or/test.c index 66fa5604be..103866b45a 100644 --- a/src/or/test.c +++ b/src/or/test.c @@ -701,25 +701,27 @@ test_crypto_pk(void) test_eq(128, crypto_pk_keysize(pk1)); test_eq(128, crypto_pk_keysize(pk2)); - test_eq(128, crypto_pk_public_encrypt(pk2, data1, "Hello whirled.", 15, + test_eq(128, crypto_pk_public_encrypt(pk2, data1, sizeof(data1), + "Hello whirled.", 15, PK_PKCS1_OAEP_PADDING)); - test_eq(128, crypto_pk_public_encrypt(pk1, data2, "Hello whirled.", 15, + test_eq(128, crypto_pk_public_encrypt(pk1, data2, sizeof(data2), + "Hello whirled.", 15, PK_PKCS1_OAEP_PADDING)); /* oaep padding should make encryption not match */ test_memneq(data1, data2, 128); - test_eq(15, crypto_pk_private_decrypt(pk1, data3, data1, 128, + test_eq(15, crypto_pk_private_decrypt(pk1, data3, sizeof(data3), data1, 128, PK_PKCS1_OAEP_PADDING,1)); test_streq(data3, "Hello whirled."); memset(data3, 0, 1024); - test_eq(15, crypto_pk_private_decrypt(pk1, data3, data2, 128, + test_eq(15, crypto_pk_private_decrypt(pk1, data3, sizeof(data3), data2, 128, PK_PKCS1_OAEP_PADDING,1)); test_streq(data3, "Hello whirled."); /* Can't decrypt with public key. */ - test_eq(-1, crypto_pk_private_decrypt(pk2, data3, data2, 128, + test_eq(-1, crypto_pk_private_decrypt(pk2, data3, sizeof(data3), data2, 128, PK_PKCS1_OAEP_PADDING,1)); /* Try again with bad padding */ memcpy(data2+1, "XYZZY", 5); /* This has fails ~ once-in-2^40 */ - test_eq(-1, crypto_pk_private_decrypt(pk1, data3, data2, 128, + test_eq(-1, crypto_pk_private_decrypt(pk1, data3, sizeof(data3), data2, 128, PK_PKCS1_OAEP_PADDING,1)); /* File operations: save and load private key */ @@ -734,19 +736,22 @@ test_crypto_pk(void) get_fname("xyzzy")) < 0); test_assert(! crypto_pk_read_private_key_from_filename(pk2, get_fname("pkey1"))); - test_eq(15, crypto_pk_private_decrypt(pk2, data3, data1, 128, + test_eq(15, crypto_pk_private_decrypt(pk2, data3, sizeof(data3), data1, 128, PK_PKCS1_OAEP_PADDING,1)); /* Now try signing. */ strlcpy(data1, "Ossifrage", 1024); - test_eq(128, crypto_pk_private_sign(pk1, data2, data1, 10)); - test_eq(10, crypto_pk_public_checksig(pk1, data3, data2, 128)); + test_eq(128, crypto_pk_private_sign(pk1, data2, sizeof(data2), data1, 10)); + test_eq(10, crypto_pk_public_checksig(pk1, data3, sizeof(data3), data2, 128)); test_streq(data3, "Ossifrage"); /* Try signing digests. */ - test_eq(128, crypto_pk_private_sign_digest(pk1, data2, data1, 10)); - test_eq(20, crypto_pk_public_checksig(pk1, data3, data2, 128)); - test_eq(0, crypto_pk_public_checksig_digest(pk1, data1, 10, data2, 128)); - test_eq(-1, crypto_pk_public_checksig_digest(pk1, data1, 11, data2, 128)); + test_eq(128, crypto_pk_private_sign_digest(pk1, data2, sizeof(data2), + data1, 10)); + test_eq(20, crypto_pk_public_checksig(pk1, data3, sizeof(data1), data2, 128)); + test_eq(0, crypto_pk_public_checksig_digest(pk1, data1, + 10, data2, 128)); + test_eq(-1, crypto_pk_public_checksig_digest(pk1, data1, + 11, data2, 128)); /*XXXX test failed signing*/ /* Try encoding */ @@ -767,9 +772,11 @@ test_crypto_pk(void) continue; p = (i==0)?PK_NO_PADDING: (i==1)?PK_PKCS1_PADDING:PK_PKCS1_OAEP_PADDING; - len = crypto_pk_public_hybrid_encrypt(pk1,data2,data1,j,p,0); + len = crypto_pk_public_hybrid_encrypt(pk1,data2,sizeof(data2), + data1,j,p,0); test_assert(len>=0); - len = crypto_pk_private_hybrid_decrypt(pk1,data3,data2,len,p,1); + len = crypto_pk_private_hybrid_decrypt(pk1,data3,sizeof(data3), + data2,len,p,1); test_eq(len,j); test_memeq(data1,data3,j); } -- 2.11.4.GIT