Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#2927 closed defect (invalid)

Tor doesn't overwrite rotated keys

Reported by: asn Owned by:
Priority: Medium Milestone:
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Tor doesn't zero out rotated onion keys, or rotated x509s and currently leaves them into memory for anyone to have fun with them.

The fact that Tor uses PFS TLS ciphersuites saves the day, but still sensitive stuff should be overwritten to be on the safe side.

Onion keys should get memsetted somewhere around the crypto_free_pk_env(lastonionkey);
of rotate_onion_key()

and TLS certificates should get memsetted in:
tor_tls_context_decref().

OpenSSL has functions that clean keys correctly, iirc.

Child Tickets

Change History (6)

comment:1 Changed 8 years ago by cypherpunks

/** A public key, or a public/private key-pair. */
struct crypto_pk_env_t
{
  int refs; /* reference counting so we don't have to copy keys */
  RSA *key;
};
crypto_free_pk_env(crypto_pk_env_t *env)
{
  tor_assert(env);

  if (--env->refs > 0)
    return;

  if (env->key)
    RSA_free(env->key);

  tor_free(env);
}
void RSA_free(RSA *r)
	{
...
	if (r->d != NULL) BN_clear_free(r->d);
	if (r->p != NULL) BN_clear_free(r->p);
	if (r->q != NULL) BN_clear_free(r->q);
	if (r->dmp1 != NULL) BN_clear_free(r->dmp1);
	if (r->dmq1 != NULL) BN_clear_free(r->dmq1);
	if (r->iqmp != NULL) BN_clear_free(r->iqmp);
...
void BN_clear_free(BIGNUM *a)
	{
	int i;

	if (a == NULL) return;
	bn_check_top(a);
	if (a->d != NULL)
		{
		OPENSSL_cleanse(a->d,a->dmax*sizeof(a->d[0]));
		if (!(BN_get_flags(a,BN_FLG_STATIC_DATA)))
			OPENSSL_free(a->d);
		}
	i=BN_get_flags(a,BN_FLG_MALLOCED);
	OPENSSL_cleanse(a,sizeof(BIGNUM));
	if (i)
		OPENSSL_free(a);
	}
void OPENSSL_cleanse(void *ptr, size_t len)
	{
	unsigned char *p = ptr;
	size_t loop = len, ctr = cleanse_ctr;
	while(loop--)
		{
		*(p++) = (unsigned char)ctr;
		ctr += (17 + ((size_t)p & 0xF));
		}
	p=memchr(ptr, (unsigned char)ctr, len);
	if(p)
		ctr += (63 + (size_t)p);
	cleanse_ctr = (unsigned char)ctr;
	}

comment:2 in reply to:  description ; Changed 8 years ago by rransom

Replying to asn:

Onion keys should get memsetted somewhere around the crypto_free_pk_env(lastonionkey);
of rotate_onion_key()

‘Onion keys’ are stored on disk, too. Do you plan to securely erase them there? If so, how?

comment:3 in reply to:  2 Changed 8 years ago by asn

Replying to rransom:

Replying to asn:

Onion keys should get memsetted somewhere around the crypto_free_pk_env(lastonionkey);
of rotate_onion_key()

‘Onion keys’ are stored on disk, too. Do you plan to securely erase them there? If so, how?

Yes, you are right.
I guess that even creating something like shred(1), would be a lost cause and probably also give a false sense of security, considering modern journaled, versioning and what not filesystems.

comment:4 in reply to:  1 Changed 8 years ago by asn

Resolution: invalid
Status: newclosed

Replying to cypherpunks:

<snip>

Yap, you are right.
Stupid me, I didn't follow after RSA_free() and I just checked it using gdb where I was probably confused by the way that OPENSSL_cleanse() cleanses.

Thank you.

comment:5 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:6 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.