May 2016 - tor-commits - lists.torproject.org

[tor/release-0.2.8] Merge branch 'maint-0.2.8' into release-0.2.8
by nickm＠torproject.org 25 May '16

25 May '16

commit 9d199893feabefc6fcc6849f939ec5cd4d8891b8 Merge: b5e2b38 6d375f1 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed May 25 10:21:04 2016 -0400 Merge branch 'maint-0.2.8' into release-0.2.8 changes/bug19161 | 3 +++ configure.ac | 1 + src/common/crypto_s2k.c | 2 +- src/test/test_crypto_slow.c | 15 ++++++++------- 4 files changed, 13 insertions(+), 8 deletions(-)

1 0

[tor/maint-0.2.8] Fix a pointer arithmetic bug in memarea_alloc()
by nickm＠torproject.org 25 May '16

25 May '16

commit be2d37ad3cbb5a36fee410f2e36e53b1ee019f48 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu May 19 10:52:27 2016 -0400 Fix a pointer arithmetic bug in memarea_alloc() Fortunately, the arithmetic cannot actually overflow, so long as we *always* check for the size of potentially hostile input before copying it. I think we do, though. We do check each line against MAX_LINE_LENGTH, and each object name or object against MAX_UNPARSED_OBJECT_SIZE, both of which are 128k. So to get this overflow, we need to have our memarea allocated way way too high up in RAM, which most allocators won't actually do. Bugfix on 0.2.1.1-alpha, where memarea was introduced. Found by Guido Vranken. --- changes/memarea_overflow | 7 +++++++ src/common/memarea.c | 8 +++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/changes/memarea_overflow b/changes/memarea_overflow new file mode 100644 index 0000000..8fdc38c --- /dev/null +++ b/changes/memarea_overflow @@ -0,0 +1,7 @@ + o Minor bugfixes (pointer arithmetic): + - Fix a bug in memarea_alloc() that could have resulted in remote heap + write access, if Tor had ever passed an unchecked size to + memarea_alloc(). Fortunately, all the sizes we pass to memarea_alloc() + are pre-checked to be less than 128 kilobytes. Fixes bug 19150; bugfix + on 0.2.1.1-alpha. Bug found by Guido Vranken. + diff --git a/src/common/memarea.c b/src/common/memarea.c index 6841ba5..d6cad11 100644 --- a/src/common/memarea.c +++ b/src/common/memarea.c @@ -80,8 +80,7 @@ typedef struct memarea_chunk_t { struct memarea_chunk_t *next_chunk; size_t mem_size; /**< How much RAM is available in mem, total? */ char *next_mem; /**< Next position in mem to allocate data at. If it's - * greater than or equal to mem+mem_size, this chunk is - * full. */ + * equal to mem+mem_size, this chunk is full. */ #ifdef USE_ALIGNED_ATTRIBUTE char mem[FLEXIBLE_ARRAY_MEMBER] __attribute__((aligned(MEMAREA_ALIGN))); #else @@ -237,7 +236,10 @@ memarea_alloc(memarea_t *area, size_t sz) tor_assert(sz < SIZE_T_CEILING); if (sz == 0) sz = 1; - if (chunk->next_mem+sz > chunk->U_MEM+chunk->mem_size) { + tor_assert(chunk->next_mem <= chunk->U_MEM + chunk->mem_size); + const size_t space_remaining = + (chunk->U_MEM + chunk->mem_size) - chunk->next_mem; + if (sz > space_remaining) { if (sz+CHUNK_HEADER_SIZE >= CHUNK_SIZE) { /* This allocation is too big. Stick it in a special chunk, and put * that chunk second in the list. */

1 0

[tor/master] Merge branch 'bug19152_024_v2' into maint-0.2.8
by nickm＠torproject.org 25 May '16

25 May '16

commit fdfc528f85c11a1d29b6a67e32180278d1ca7cbb Merge: 6abceca c4c4380 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed May 25 09:26:45 2016 -0400 Merge branch 'bug19152_024_v2' into maint-0.2.8 changes/rsa_init_bug | 7 +++++++ src/common/crypto.c | 4 +++- 2 files changed, 10 insertions(+), 1 deletion(-)

1 0

[tor/maint-0.2.8] Merge branch 'bug19152_024_v2' into maint-0.2.8
by nickm＠torproject.org 25 May '16

25 May '16

commit fdfc528f85c11a1d29b6a67e32180278d1ca7cbb Merge: 6abceca c4c4380 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed May 25 09:26:45 2016 -0400 Merge branch 'bug19152_024_v2' into maint-0.2.8 changes/rsa_init_bug | 7 +++++++ src/common/crypto.c | 4 +++- 2 files changed, 10 insertions(+), 1 deletion(-)

1 0

[tor/master] Merge branch 'memarea_overflow_027_squashed' into maint-0.2.8
by nickm＠torproject.org 25 May '16

25 May '16

commit 6abceca1826a018fb51e419fc4eb9721dd501acf Merge: 87134db be2d37a Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed May 25 09:22:02 2016 -0400 Merge branch 'memarea_overflow_027_squashed' into maint-0.2.8 changes/memarea_overflow | 7 +++++++ src/common/memarea.c | 8 +++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --cc src/common/memarea.c index 0a3fd00,d6cad11..173ed4e --- a/src/common/memarea.c +++ b/src/common/memarea.c @@@ -83,10 -80,8 +83,9 @@@ typedef struct memarea_chunk_t struct memarea_chunk_t *next_chunk; size_t mem_size; /**< How much RAM is available in mem, total? */ char *next_mem; /**< Next position in mem to allocate data at. If it's - * greater than or equal to mem+mem_size, this chunk is - * full. */ + * equal to mem+mem_size, this chunk is full. */ #ifdef USE_ALIGNED_ATTRIBUTE + /** Actual content of the memory chunk. */ char mem[FLEXIBLE_ARRAY_MEMBER] __attribute__((aligned(MEMAREA_ALIGN))); #else union {

1 0

[tor/maint-0.2.8] Fix a dangling pointer issue in our RSA keygen code
by nickm＠torproject.org 25 May '16

25 May '16

commit c4c4380a5e43e97ff9533aa42c73183dd1054c41 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri May 20 13:58:52 2016 -0400 Fix a dangling pointer issue in our RSA keygen code If OpenSSL fails to generate an RSA key, do not retain a dangling pointer to the previous (uninitialized) key value. The impact here should be limited to a difficult-to-trigger crash, if OpenSSL is running an engine that makes key generation failures possible, or if OpenSSL runs out of memory. Fixes bug 19152; bugfix on 0.2.1.10-alpha. Found by Yuan Jochen Kang, Suman Jana, and Baishakhi Ray. This is potentially scary stuff, so let me walk through my analysis. I think this is a bug, and a backport candidate, but not remotely triggerable in any useful way. Observation 1a: Looking over the OpenSSL code here, the only way we can really fail in the non-engine case is if malloc() fails. But if malloc() is failing, then tor_malloc() calls should be tor_asserting -- the only way that an attacker could do an exploit here would be to figure out some way to make malloc() fail when openssl does it, but work whenever Tor does it. (Also ordinary malloc() doesn't fail on platforms like Linux that overcommit.) Observation 1b: Although engines are _allowed_ to fail in extra ways, I can't find much evidence online that they actually _do_ fail in practice. More evidence would be nice, though. Observation 2: We don't call crypto_pk_generate*() all that often, and we don't do it in response to external inputs. The only way to get it to happen remotely would be by causing a hidden service to build new introduction points. Observation 3a: So, let's assume that both of the above observations are wrong, and the attacker can make us generate a crypto_pk_env_t with a dangling pointer in its 'key' field, and not immediately crash. This dangling pointer will point to what used to be an RSA structure, with the fields all set to NULL. Actually using this RSA structure, before the memory is reused for anything else, will cause a crash. In nearly every function where we call crypto_pk_generate*(), we quickly use the RSA key pointer -- either to sign something, or to encode the key, or to free the key. The only exception is when we generate an intro key in rend_consider_services_intro_points(). In that case, we don't actually use the key until the intro circuit is opened -- at which point we encode it, and use it to sign an introduction request. So in order to exploit this bug to do anything besides crash Tor, the attacker needs to make sure that by the time the introduction circuit completes, either: * the e, d, and n BNs look valid, and at least one of the other BNs is still NULL. OR * all 8 of the BNs must look valid. To look like a valid BN, *they* all need to have their 'top' index plus their 'd' pointer indicate an addressable region in memory. So actually getting useful data of of this, rather than a crash, is going to be pretty damn hard. You'd have to force an introduction point to be created (or wait for one to be created), and force that particular crypto_pk_generate*() to fail, and then arrange for the memory that the RSA points to to in turn point to 3...8 valid BNs, all by the time the introduction circuit completes. Naturally, the signature won't check as valid [*], so the intro point will reject the ESTABLISH_INTRO cell. So you need to _be_ the introduction point, or you don't actually see this information. [*] Okay, so if you could somehow make the 'rsa' pointer point to a different valid RSA key, then you'd get a valid signature of an ESTABLISH_INTRO cell using a key that was supposed to be used for something else ... but nothing else looks like that, so you can't use that signature elsewhere. Observation 3b: Your best bet as an attacker would be to make the dangling RSA pointer actually contain a fake method, with a fake RSA_private_encrypt function that actually pointed to code you wanted to execute. You'd still need to transit 3 or 4 pointers deep though in order to make that work. Conclusion: By 1, you probably can't trigger this without Tor crashing from OOM. By 2, you probably can't trigger this reliably. By 3, even if I'm wrong about 1 and 2, you have to jump through a pretty big array of hoops in order to get any kind of data leak or code execution. So I'm calling it a bug, but not a security hole. Still worth patching. --- changes/rsa_init_bug | 7 +++++++ src/common/crypto.c | 4 +++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/changes/rsa_init_bug b/changes/rsa_init_bug new file mode 100644 index 0000000..6b5fb4f --- /dev/null +++ b/changes/rsa_init_bug @@ -0,0 +1,7 @@ + o Major bugfixes (key management): + - If OpenSSL fails to generate an RSA key, do not retain a dangling pointer + to the previous (uninitialized) key value. The impact here should be + limited to a difficult-to-trigger crash, if OpenSSL is running an + engine that makes key generation failures possible, or if OpenSSL runs + out of memory. Fixes bug 19152; bugfix on 0.2.1.10-alpha. Found by + Yuan Jochen Kang, Suman Jana, and Baishakhi Ray. diff --git a/src/common/crypto.c b/src/common/crypto.c index 925beb3..63b274e 100644 --- a/src/common/crypto.c +++ b/src/common/crypto.c @@ -466,8 +466,10 @@ crypto_pk_generate_key_with_bits(crypto_pk_t *env, int bits) { tor_assert(env); - if (env->key) + if (env->key) { RSA_free(env->key); + env->key = NULL; + } { BIGNUM *e = BN_new();

1 0

[tor/maint-0.2.8] Merge branch 'memarea_overflow_027_squashed' into maint-0.2.8
by nickm＠torproject.org 25 May '16

25 May '16

commit 6abceca1826a018fb51e419fc4eb9721dd501acf Merge: 87134db be2d37a Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed May 25 09:22:02 2016 -0400 Merge branch 'memarea_overflow_027_squashed' into maint-0.2.8 changes/memarea_overflow | 7 +++++++ src/common/memarea.c | 8 +++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --cc src/common/memarea.c index 0a3fd00,d6cad11..173ed4e --- a/src/common/memarea.c +++ b/src/common/memarea.c @@@ -83,10 -80,8 +83,9 @@@ typedef struct memarea_chunk_t struct memarea_chunk_t *next_chunk; size_t mem_size; /**< How much RAM is available in mem, total? */ char *next_mem; /**< Next position in mem to allocate data at. If it's - * greater than or equal to mem+mem_size, this chunk is - * full. */ + * equal to mem+mem_size, this chunk is full. */ #ifdef USE_ALIGNED_ATTRIBUTE + /** Actual content of the memory chunk. */ char mem[FLEXIBLE_ARRAY_MEMBER] __attribute__((aligned(MEMAREA_ALIGN))); #else union {

1 0

[tor/master] Fix a pointer arithmetic bug in memarea_alloc()
by nickm＠torproject.org 25 May '16

25 May '16

commit be2d37ad3cbb5a36fee410f2e36e53b1ee019f48 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu May 19 10:52:27 2016 -0400 Fix a pointer arithmetic bug in memarea_alloc() Fortunately, the arithmetic cannot actually overflow, so long as we *always* check for the size of potentially hostile input before copying it. I think we do, though. We do check each line against MAX_LINE_LENGTH, and each object name or object against MAX_UNPARSED_OBJECT_SIZE, both of which are 128k. So to get this overflow, we need to have our memarea allocated way way too high up in RAM, which most allocators won't actually do. Bugfix on 0.2.1.1-alpha, where memarea was introduced. Found by Guido Vranken. --- changes/memarea_overflow | 7 +++++++ src/common/memarea.c | 8 +++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/changes/memarea_overflow b/changes/memarea_overflow new file mode 100644 index 0000000..8fdc38c --- /dev/null +++ b/changes/memarea_overflow @@ -0,0 +1,7 @@ + o Minor bugfixes (pointer arithmetic): + - Fix a bug in memarea_alloc() that could have resulted in remote heap + write access, if Tor had ever passed an unchecked size to + memarea_alloc(). Fortunately, all the sizes we pass to memarea_alloc() + are pre-checked to be less than 128 kilobytes. Fixes bug 19150; bugfix + on 0.2.1.1-alpha. Bug found by Guido Vranken. + diff --git a/src/common/memarea.c b/src/common/memarea.c index 6841ba5..d6cad11 100644 --- a/src/common/memarea.c +++ b/src/common/memarea.c @@ -80,8 +80,7 @@ typedef struct memarea_chunk_t { struct memarea_chunk_t *next_chunk; size_t mem_size; /**< How much RAM is available in mem, total? */ char *next_mem; /**< Next position in mem to allocate data at. If it's - * greater than or equal to mem+mem_size, this chunk is - * full. */ + * equal to mem+mem_size, this chunk is full. */ #ifdef USE_ALIGNED_ATTRIBUTE char mem[FLEXIBLE_ARRAY_MEMBER] __attribute__((aligned(MEMAREA_ALIGN))); #else @@ -237,7 +236,10 @@ memarea_alloc(memarea_t *area, size_t sz) tor_assert(sz < SIZE_T_CEILING); if (sz == 0) sz = 1; - if (chunk->next_mem+sz > chunk->U_MEM+chunk->mem_size) { + tor_assert(chunk->next_mem <= chunk->U_MEM + chunk->mem_size); + const size_t space_remaining = + (chunk->U_MEM + chunk->mem_size) - chunk->next_mem; + if (sz > space_remaining) { if (sz+CHUNK_HEADER_SIZE >= CHUNK_SIZE) { /* This allocation is too big. Stick it in a special chunk, and put * that chunk second in the list. */

1 0

[tor/master] Fix a dangling pointer issue in our RSA keygen code
by nickm＠torproject.org 25 May '16

25 May '16

commit c4c4380a5e43e97ff9533aa42c73183dd1054c41 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri May 20 13:58:52 2016 -0400 Fix a dangling pointer issue in our RSA keygen code If OpenSSL fails to generate an RSA key, do not retain a dangling pointer to the previous (uninitialized) key value. The impact here should be limited to a difficult-to-trigger crash, if OpenSSL is running an engine that makes key generation failures possible, or if OpenSSL runs out of memory. Fixes bug 19152; bugfix on 0.2.1.10-alpha. Found by Yuan Jochen Kang, Suman Jana, and Baishakhi Ray. This is potentially scary stuff, so let me walk through my analysis. I think this is a bug, and a backport candidate, but not remotely triggerable in any useful way. Observation 1a: Looking over the OpenSSL code here, the only way we can really fail in the non-engine case is if malloc() fails. But if malloc() is failing, then tor_malloc() calls should be tor_asserting -- the only way that an attacker could do an exploit here would be to figure out some way to make malloc() fail when openssl does it, but work whenever Tor does it. (Also ordinary malloc() doesn't fail on platforms like Linux that overcommit.) Observation 1b: Although engines are _allowed_ to fail in extra ways, I can't find much evidence online that they actually _do_ fail in practice. More evidence would be nice, though. Observation 2: We don't call crypto_pk_generate*() all that often, and we don't do it in response to external inputs. The only way to get it to happen remotely would be by causing a hidden service to build new introduction points. Observation 3a: So, let's assume that both of the above observations are wrong, and the attacker can make us generate a crypto_pk_env_t with a dangling pointer in its 'key' field, and not immediately crash. This dangling pointer will point to what used to be an RSA structure, with the fields all set to NULL. Actually using this RSA structure, before the memory is reused for anything else, will cause a crash. In nearly every function where we call crypto_pk_generate*(), we quickly use the RSA key pointer -- either to sign something, or to encode the key, or to free the key. The only exception is when we generate an intro key in rend_consider_services_intro_points(). In that case, we don't actually use the key until the intro circuit is opened -- at which point we encode it, and use it to sign an introduction request. So in order to exploit this bug to do anything besides crash Tor, the attacker needs to make sure that by the time the introduction circuit completes, either: * the e, d, and n BNs look valid, and at least one of the other BNs is still NULL. OR * all 8 of the BNs must look valid. To look like a valid BN, *they* all need to have their 'top' index plus their 'd' pointer indicate an addressable region in memory. So actually getting useful data of of this, rather than a crash, is going to be pretty damn hard. You'd have to force an introduction point to be created (or wait for one to be created), and force that particular crypto_pk_generate*() to fail, and then arrange for the memory that the RSA points to to in turn point to 3...8 valid BNs, all by the time the introduction circuit completes. Naturally, the signature won't check as valid [*], so the intro point will reject the ESTABLISH_INTRO cell. So you need to _be_ the introduction point, or you don't actually see this information. [*] Okay, so if you could somehow make the 'rsa' pointer point to a different valid RSA key, then you'd get a valid signature of an ESTABLISH_INTRO cell using a key that was supposed to be used for something else ... but nothing else looks like that, so you can't use that signature elsewhere. Observation 3b: Your best bet as an attacker would be to make the dangling RSA pointer actually contain a fake method, with a fake RSA_private_encrypt function that actually pointed to code you wanted to execute. You'd still need to transit 3 or 4 pointers deep though in order to make that work. Conclusion: By 1, you probably can't trigger this without Tor crashing from OOM. By 2, you probably can't trigger this reliably. By 3, even if I'm wrong about 1 and 2, you have to jump through a pretty big array of hoops in order to get any kind of data leak or code execution. So I'm calling it a bug, but not a security hole. Still worth patching. --- changes/rsa_init_bug | 7 +++++++ src/common/crypto.c | 4 +++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/changes/rsa_init_bug b/changes/rsa_init_bug new file mode 100644 index 0000000..6b5fb4f --- /dev/null +++ b/changes/rsa_init_bug @@ -0,0 +1,7 @@ + o Major bugfixes (key management): + - If OpenSSL fails to generate an RSA key, do not retain a dangling pointer + to the previous (uninitialized) key value. The impact here should be + limited to a difficult-to-trigger crash, if OpenSSL is running an + engine that makes key generation failures possible, or if OpenSSL runs + out of memory. Fixes bug 19152; bugfix on 0.2.1.10-alpha. Found by + Yuan Jochen Kang, Suman Jana, and Baishakhi Ray. diff --git a/src/common/crypto.c b/src/common/crypto.c index 925beb3..63b274e 100644 --- a/src/common/crypto.c +++ b/src/common/crypto.c @@ -466,8 +466,10 @@ crypto_pk_generate_key_with_bits(crypto_pk_t *env, int bits) { tor_assert(env); - if (env->key) + if (env->key) { RSA_free(env->key); + env->key = NULL; + } { BIGNUM *e = BN_new();

1 0

[tor/master] Merge branch 'maint-0.2.8'
by nickm＠torproject.org 25 May '16

25 May '16

commit a3ec811c2ee801bf6dd3d7f65c4997470e807c9d Merge: 0ef3662 fdfc528 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed May 25 09:27:47 2016 -0400 Merge branch 'maint-0.2.8' changes/memarea_overflow | 7 +++++++ changes/rsa_init_bug | 7 +++++++ src/common/crypto.c | 4 +++- src/common/memarea.c | 8 +++++--- 4 files changed, 22 insertions(+), 4 deletions(-)

1 0