June 2017 - tor-commits - lists.torproject.org

[tor/release-0.3.1] Fix an errant memset() into the middle of a struct in cell_pack().
by nickm＠torproject.org 27 Jun '17

27 Jun '17

commit 8d2978b13c7462776d8a6ba6a6df5b8d9883a633 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Jun 27 10:45:29 2017 -0400 Fix an errant memset() into the middle of a struct in cell_pack(). This mistake causes two possible bugs. I believe they are both harmless IRL. BUG 1: memory stomping When we call the memset, we are overwriting two 0 bytes past the end of packed_cell_t.body. But I think that's harmless in practice, because the definition of packed_cell_t is: // ... typedef struct packed_cell_t { TOR_SIMPLEQ_ENTRY(packed_cell_t) next; char body[CELL_MAX_NETWORK_SIZE]; uint32_t inserted_time; } packed_cell_t; So we will overwrite either two bytes of inserted_time, or two bytes of padding, depending on how the platform handles alignment. If we're overwriting padding, that's safe. If we are overwriting the inserted_time field, that's also safe: In every case where we call cell_pack() from connection_or.c, we ignore the inserted_time field. When we call cell_pack() from relay.c, we don't set or use inserted_time until right after we have called cell_pack(). SO I believe we're safe in that case too. BUG 2: memory exposure The original reason for this memset was to avoid the possibility of accidentally leaking uninitialized ram to the network. Now remember, if wide_circ_ids is false on a connection, we shouldn't actually be sending more than 512 bytes of packed_cell_t.body, so these two bytes can only leak to the network if there is another bug somewhere else in the code that sends more data than is correct. Fortunately, in relay.c, where we allocate packed_cell_t in packed_cell_new() , we allocate it with tor_malloc_zero(), which clears the RAM, right before we call cell_pack. So those packed_cell_t.body bytes can't leak any information. That leaves the two calls to cell_pack() in connection_or.c, which use stack-alocated packed_cell_t instances. In or_handshake_state_record_cell(), we pass the cell's contents to crypto_digest_add_bytes(). When we do so, we get the number of bytes to pass using the same setting of wide_circ_ids as we passed to cell_pack(). So I believe that's safe. In connection_or_write_cell_to_buf(), we also use the same setting of wide_circ_ids in both calls. So I believe that's safe too. I introduced this bug with 1c0e87f6d8c7a0abdadf1b5cd9082c10abc7f4e2 back in 0.2.4.11-alpha; it is bug 22737 and CID 1401591 --- changes/bug22737 | 12 ++++++++++++ src/or/connection_or.c | 4 +++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/changes/bug22737 b/changes/bug22737 new file mode 100644 index 0000000..f0de8e6 --- /dev/null +++ b/changes/bug22737 @@ -0,0 +1,12 @@ + o Minor bugfixes (defensive programming, undefined behavior): + + - Fix a memset() off the end of an array when packing cells. This + bug should be harmless in practice, since the corrupted bytes + are still in the same structure, and are always padding bytes, + ignored, or immediately overwritten, depending on compiler + behavior. Nevertheless, because the memset()'s purpose is to + make sure that any other cell-handling bugs can't expose bytes + to the network, we need to fix it. Fixes bug 22737; bugfix on + 0.2.4.11-alpha. Fixes CID 1401591. + + diff --git a/src/or/connection_or.c b/src/or/connection_or.c index 8e7cd9e..99ea1ef 100644 --- a/src/or/connection_or.c +++ b/src/or/connection_or.c @@ -358,9 +358,11 @@ cell_pack(packed_cell_t *dst, const cell_t *src, int wide_circ_ids) set_uint32(dest, htonl(src->circ_id)); dest += 4; } else { + /* Clear the last two bytes of dest, in case we can accidentally + * send them to the network somehow. */ + memset(dest+CELL_MAX_NETWORK_SIZE-2, 0, 2); set_uint16(dest, htons(src->circ_id)); dest += 2; - memset(dest+CELL_MAX_NETWORK_SIZE-2, 0, 2); /*make sure it's clear */ } set_uint8(dest, src->command); memcpy(dest+1, src->payload, CELL_PAYLOAD_SIZE);

1 0

[tor/release-0.3.1] Merge branch 'maint-0.2.8' into maint-0.2.9
by nickm＠torproject.org 27 Jun '17

27 Jun '17

commit 711160a46f5fbba7106c23c29f99bed75928a5a7 Merge: 1c64133 3483f7c Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Jun 27 11:04:44 2017 -0400 Merge branch 'maint-0.2.8' into maint-0.2.9 changes/bug22737 | 12 ++++++++++++ src/or/connection_or.c | 4 +++- 2 files changed, 15 insertions(+), 1 deletion(-)

1 0

[tor/release-0.2.9] Merge branch 'maint-0.2.7-redux' into maint-0.2.8
by nickm＠torproject.org 27 Jun '17

27 Jun '17

commit 3483f7c00349be6cba7b703f2e5ee94563d0c6eb Merge: 307be8d 9a0fd2d Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Jun 27 11:04:44 2017 -0400 Merge branch 'maint-0.2.7-redux' into maint-0.2.8 changes/bug22737 | 12 ++++++++++++ src/or/connection_or.c | 4 +++- 2 files changed, 15 insertions(+), 1 deletion(-)

1 0

[tor/release-0.2.9] Merge branch 'maint-0.2.8' into maint-0.2.9
by nickm＠torproject.org 27 Jun '17

27 Jun '17

commit 711160a46f5fbba7106c23c29f99bed75928a5a7 Merge: 1c64133 3483f7c Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Jun 27 11:04:44 2017 -0400 Merge branch 'maint-0.2.8' into maint-0.2.9 changes/bug22737 | 12 ++++++++++++ src/or/connection_or.c | 4 +++- 2 files changed, 15 insertions(+), 1 deletion(-)

1 0

[tor/release-0.2.9] Merge branch 'maint-0.2.9' into release-0.2.9
by nickm＠torproject.org 27 Jun '17

27 Jun '17

commit cd47dfdb47eb138587cac7343a63e8c98f54b907 Merge: 78988f1 711160a Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Jun 27 11:04:44 2017 -0400 Merge branch 'maint-0.2.9' into release-0.2.9 changes/bug22737 | 12 ++++++++++++ src/or/connection_or.c | 4 +++- 2 files changed, 15 insertions(+), 1 deletion(-)

1 0

[tor/maint-0.2.5] Fix an errant memset() into the middle of a struct in cell_pack().
by nickm＠torproject.org 27 Jun '17

27 Jun '17

commit 8d2978b13c7462776d8a6ba6a6df5b8d9883a633 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Jun 27 10:45:29 2017 -0400 Fix an errant memset() into the middle of a struct in cell_pack(). This mistake causes two possible bugs. I believe they are both harmless IRL. BUG 1: memory stomping When we call the memset, we are overwriting two 0 bytes past the end of packed_cell_t.body. But I think that's harmless in practice, because the definition of packed_cell_t is: // ... typedef struct packed_cell_t { TOR_SIMPLEQ_ENTRY(packed_cell_t) next; char body[CELL_MAX_NETWORK_SIZE]; uint32_t inserted_time; } packed_cell_t; So we will overwrite either two bytes of inserted_time, or two bytes of padding, depending on how the platform handles alignment. If we're overwriting padding, that's safe. If we are overwriting the inserted_time field, that's also safe: In every case where we call cell_pack() from connection_or.c, we ignore the inserted_time field. When we call cell_pack() from relay.c, we don't set or use inserted_time until right after we have called cell_pack(). SO I believe we're safe in that case too. BUG 2: memory exposure The original reason for this memset was to avoid the possibility of accidentally leaking uninitialized ram to the network. Now remember, if wide_circ_ids is false on a connection, we shouldn't actually be sending more than 512 bytes of packed_cell_t.body, so these two bytes can only leak to the network if there is another bug somewhere else in the code that sends more data than is correct. Fortunately, in relay.c, where we allocate packed_cell_t in packed_cell_new() , we allocate it with tor_malloc_zero(), which clears the RAM, right before we call cell_pack. So those packed_cell_t.body bytes can't leak any information. That leaves the two calls to cell_pack() in connection_or.c, which use stack-alocated packed_cell_t instances. In or_handshake_state_record_cell(), we pass the cell's contents to crypto_digest_add_bytes(). When we do so, we get the number of bytes to pass using the same setting of wide_circ_ids as we passed to cell_pack(). So I believe that's safe. In connection_or_write_cell_to_buf(), we also use the same setting of wide_circ_ids in both calls. So I believe that's safe too. I introduced this bug with 1c0e87f6d8c7a0abdadf1b5cd9082c10abc7f4e2 back in 0.2.4.11-alpha; it is bug 22737 and CID 1401591 --- changes/bug22737 | 12 ++++++++++++ src/or/connection_or.c | 4 +++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/changes/bug22737 b/changes/bug22737 new file mode 100644 index 0000000..f0de8e6 --- /dev/null +++ b/changes/bug22737 @@ -0,0 +1,12 @@ + o Minor bugfixes (defensive programming, undefined behavior): + + - Fix a memset() off the end of an array when packing cells. This + bug should be harmless in practice, since the corrupted bytes + are still in the same structure, and are always padding bytes, + ignored, or immediately overwritten, depending on compiler + behavior. Nevertheless, because the memset()'s purpose is to + make sure that any other cell-handling bugs can't expose bytes + to the network, we need to fix it. Fixes bug 22737; bugfix on + 0.2.4.11-alpha. Fixes CID 1401591. + + diff --git a/src/or/connection_or.c b/src/or/connection_or.c index 8e7cd9e..99ea1ef 100644 --- a/src/or/connection_or.c +++ b/src/or/connection_or.c @@ -358,9 +358,11 @@ cell_pack(packed_cell_t *dst, const cell_t *src, int wide_circ_ids) set_uint32(dest, htonl(src->circ_id)); dest += 4; } else { + /* Clear the last two bytes of dest, in case we can accidentally + * send them to the network somehow. */ + memset(dest+CELL_MAX_NETWORK_SIZE-2, 0, 2); set_uint16(dest, htons(src->circ_id)); dest += 2; - memset(dest+CELL_MAX_NETWORK_SIZE-2, 0, 2); /*make sure it's clear */ } set_uint8(dest, src->command); memcpy(dest+1, src->payload, CELL_PAYLOAD_SIZE);

1 0

[tor/maint-0.2.5] Merge branch 'maint-0.2.4' into maint-0.2.5
by nickm＠torproject.org 27 Jun '17

27 Jun '17

commit ccae991662adb3b64bdb1dcd4a942e4d1e163414 Merge: 325c507 d56f699 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Jun 27 11:04:44 2017 -0400 Merge branch 'maint-0.2.4' into maint-0.2.5 changes/bug22737 | 12 ++++++++++++ src/or/connection_or.c | 4 +++- 2 files changed, 15 insertions(+), 1 deletion(-)

1 0

[tor/maint-0.2.4] Merge branch 'bug22737_024' into maint-0.2.4
by nickm＠torproject.org 27 Jun '17

27 Jun '17

commit d56f6993990ab75b6ab8a80027ee60e610489a2c Merge: 104e8fa 8d2978b Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Jun 27 11:04:41 2017 -0400 Merge branch 'bug22737_024' into maint-0.2.4 changes/bug22737 | 12 ++++++++++++ src/or/connection_or.c | 4 +++- 2 files changed, 15 insertions(+), 1 deletion(-)

1 0

[tor/maint-0.2.6] Fix an errant memset() into the middle of a struct in cell_pack().
by nickm＠torproject.org 27 Jun '17

27 Jun '17

commit 8d2978b13c7462776d8a6ba6a6df5b8d9883a633 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Jun 27 10:45:29 2017 -0400 Fix an errant memset() into the middle of a struct in cell_pack(). This mistake causes two possible bugs. I believe they are both harmless IRL. BUG 1: memory stomping When we call the memset, we are overwriting two 0 bytes past the end of packed_cell_t.body. But I think that's harmless in practice, because the definition of packed_cell_t is: // ... typedef struct packed_cell_t { TOR_SIMPLEQ_ENTRY(packed_cell_t) next; char body[CELL_MAX_NETWORK_SIZE]; uint32_t inserted_time; } packed_cell_t; So we will overwrite either two bytes of inserted_time, or two bytes of padding, depending on how the platform handles alignment. If we're overwriting padding, that's safe. If we are overwriting the inserted_time field, that's also safe: In every case where we call cell_pack() from connection_or.c, we ignore the inserted_time field. When we call cell_pack() from relay.c, we don't set or use inserted_time until right after we have called cell_pack(). SO I believe we're safe in that case too. BUG 2: memory exposure The original reason for this memset was to avoid the possibility of accidentally leaking uninitialized ram to the network. Now remember, if wide_circ_ids is false on a connection, we shouldn't actually be sending more than 512 bytes of packed_cell_t.body, so these two bytes can only leak to the network if there is another bug somewhere else in the code that sends more data than is correct. Fortunately, in relay.c, where we allocate packed_cell_t in packed_cell_new() , we allocate it with tor_malloc_zero(), which clears the RAM, right before we call cell_pack. So those packed_cell_t.body bytes can't leak any information. That leaves the two calls to cell_pack() in connection_or.c, which use stack-alocated packed_cell_t instances. In or_handshake_state_record_cell(), we pass the cell's contents to crypto_digest_add_bytes(). When we do so, we get the number of bytes to pass using the same setting of wide_circ_ids as we passed to cell_pack(). So I believe that's safe. In connection_or_write_cell_to_buf(), we also use the same setting of wide_circ_ids in both calls. So I believe that's safe too. I introduced this bug with 1c0e87f6d8c7a0abdadf1b5cd9082c10abc7f4e2 back in 0.2.4.11-alpha; it is bug 22737 and CID 1401591 --- changes/bug22737 | 12 ++++++++++++ src/or/connection_or.c | 4 +++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/changes/bug22737 b/changes/bug22737 new file mode 100644 index 0000000..f0de8e6 --- /dev/null +++ b/changes/bug22737 @@ -0,0 +1,12 @@ + o Minor bugfixes (defensive programming, undefined behavior): + + - Fix a memset() off the end of an array when packing cells. This + bug should be harmless in practice, since the corrupted bytes + are still in the same structure, and are always padding bytes, + ignored, or immediately overwritten, depending on compiler + behavior. Nevertheless, because the memset()'s purpose is to + make sure that any other cell-handling bugs can't expose bytes + to the network, we need to fix it. Fixes bug 22737; bugfix on + 0.2.4.11-alpha. Fixes CID 1401591. + + diff --git a/src/or/connection_or.c b/src/or/connection_or.c index 8e7cd9e..99ea1ef 100644 --- a/src/or/connection_or.c +++ b/src/or/connection_or.c @@ -358,9 +358,11 @@ cell_pack(packed_cell_t *dst, const cell_t *src, int wide_circ_ids) set_uint32(dest, htonl(src->circ_id)); dest += 4; } else { + /* Clear the last two bytes of dest, in case we can accidentally + * send them to the network somehow. */ + memset(dest+CELL_MAX_NETWORK_SIZE-2, 0, 2); set_uint16(dest, htons(src->circ_id)); dest += 2; - memset(dest+CELL_MAX_NETWORK_SIZE-2, 0, 2); /*make sure it's clear */ } set_uint8(dest, src->command); memcpy(dest+1, src->payload, CELL_PAYLOAD_SIZE);

1 0

[tor/maint-0.2.5] Merge branch 'bug22737_024' into maint-0.2.4
by nickm＠torproject.org 27 Jun '17

27 Jun '17

commit d56f6993990ab75b6ab8a80027ee60e610489a2c Merge: 104e8fa 8d2978b Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Jun 27 11:04:41 2017 -0400 Merge branch 'bug22737_024' into maint-0.2.4 changes/bug22737 | 12 ++++++++++++ src/or/connection_or.c | 4 +++- 2 files changed, 15 insertions(+), 1 deletion(-)

1 0