Opened 4 years ago

Closed 4 years ago

Last modified 18 months ago

#17796 closed defect (implemented)

Make crypto_digest_t allocated using minimal memory

Reported by: nickm Owned by:
Priority: Low Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: alex_y_xu@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We will soon have:

struct crypto_digest_t {
  union {
    SHA_CTX sha1; 
    SHA256_CTX sha2; 
    SHA512_CTX sha512; // added in 0.2.8
    keccak_state sha3; // added by #17783
  } d; 
  digest_algorithm_bitfield_t algorithm : 8;
};

On my 64-bit host:

Size of SHA_CTX == 96
Size of SHA256_CTX == 112
Size of SHA512_CTX == 216
Size of keccak_state == 432

This means that when we allocate a sha1 digest object in order to compute the running SHA1 of an input stream (for the current tor relay protocol), we have been wasting 16 bytes, we are now wasting 120 bytes, and we will soon be wasting 336 bytes.

We should probably adjust the way we lay out and allocate the crypto_digest_t structure so that it looks more like this:

struct crypto_digest_t {
  digest_algorithm_t algorithm; // no point in a bitfield
  union {
    SHA_CTX sha1; 
    SHA256_CTX sha2; 
    SHA512_CTX sha512; // added in 0.2.8
    keccak_state sha3; // added by #17783
  } d; 

and we should have crypto_digest*_new allocate no more bytes than the algorithm will actually use.

Child Tickets

Change History (10)

comment:1 Changed 4 years ago by nickm

Priority: MediumLow
Status: newneeds_review

Please review 'feature17796' in my public repository.

comment:2 Changed 4 years ago by cypherpunks

Status: needs_reviewneeds_revision

There are some instances of sizeof(crypto_digest_t) left in src/common/crypto.c which could lead to buffer over-reads.

comment:3 Changed 4 years ago by nickm

Status: needs_revisionneeds_review

good catch; should be fixed now.

comment:4 Changed 4 years ago by teor

In crypto_digest_get_digest, this block is now unreachable:

    default:
      log_warn(LD_BUG, "Called with unknown algorithm %d", digest->algorithm);
      /* If fragile_assert is not enabled, then we should at least not
       * leak anything. */
      memwipe(r, 0xff, sizeof(r));
      memwipe(&tmpenv, 0, sizeof(crypto_digest_t));
      tor_fragile_assert();
      break;

as this line will assert before the default case is reached:

  const size_t alloc_bytes = crypto_digest_alloc_bytes(digest->algorithm);

If it wasn't unreachable, this one remaining instance of sizeof(crypto_digest_t) could write into unallocated memory:

      memwipe(&tmpenv, 0, sizeof(crypto_digest_t));

comment:5 Changed 4 years ago by teor

Status: needs_reviewneeds_revision

comment:6 Changed 4 years ago by nickm

Status: needs_revisionneeds_review

I've removed the log code in the switch statement and turned it into a hard assert.

I don't see how that memwipe could write into unallocated memory, though: tmpenv is stack-allocated, at the largest size required.

comment:7 in reply to:  6 Changed 4 years ago by teor

Looks good to me, let's get it merged.

Replying to nickm:

I've removed the log code in the switch statement and turned it into a hard assert.

Looks much clearer.

I don't see how that memwipe could write into unallocated memory, though: tmpenv is stack-allocated, at the largest size required.

Oops, I was confused by the call to crypto_digest_alloc_bytes, and missed how the structure was actually allocated.

comment:8 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

squashed and merged!

comment:9 Changed 18 months ago by Hello71

I think that this is technically not defined by ISO C. it would be "better" to use a char flexible array member here. that would actually make the code simpler, since we use the digest as a "black box" anyways and just pass it to openssl. unfortunately, that requires C11 alignas. fortunately, MSVC supports that already.

but... in practice, I highly doubt any compiler will compile this in an unexpected manner, especially since we only pass the buffer as a pointer to openssl anyways.

comment:10 Changed 18 months ago by Hello71

Cc: alex_y_xu@… added
Note: See TracTickets for help on using tickets.