Mailing List Archive

Zero Diffie-Hellman shared secret before free
I noticed from clang warnings that the Diffie-Hellman shared secret buffer, dh_shared_secret in vpnc.c, is not correctly zeroed before being freed.

In vpnc.c:1901:

memset(dh_shared_secret, 0, sizeof(dh_shared_secret))

The pointer size of dh_shared_secret is not the same as the buffer length. This only zeros the first byte of the shared secret buffer.



@@ -1851,10 +1851,12 @@ static void do_phase1_am_packet2(struct sa_block *s, const char *shared_key)
int i;
static const unsigned char c012[3] = { 0, 1, 2 };
unsigned char *skeyid_e;
+ int dh_shared_secret_len;
unsigned char *dh_shared_secret;

/* Determine the shared secret. */
- dh_shared_secret = xallocc(dh_getlen(s->ike.dh_grp));
+ dh_shared_secret_len = dh_getlen(s->ike.dh_grp);
+ dh_shared_secret = xallocc(dh_shared_secret_len);
dh_create_shared(s->ike.dh_grp, dh_shared_secret, ke->u.ke.data);
hex_dump("dh_shared_secret", dh_shared_secret, dh_getlen(s->ike.dh_grp), NULL);

@@ -1898,7 +1900,7 @@ static void do_phase1_am_packet2(struct sa_block *s, const char *shared_key)
gcry_md_close(hm);
hex_dump("skeyid_e", skeyid_e, s->ike.md_len, NULL);

- memset(dh_shared_secret, 0, sizeof(dh_shared_secret));
+ memset(dh_shared_secret, 0, dh_shared_secret_len);
free(dh_shared_secret);

/* Determine the IKE encryption key. */
Re: Zero Diffie-Hellman shared secret before free [ In reply to ]
On 11/13/2014 07:39 AM, Brian Reiter wrote:
> + memset(dh_shared_secret, 0, dh_shared_secret_len);
> free(dh_shared_secret);

I think GCC will remove the memset call because the written values are
never read.

--
Florian Weimer / Red Hat Product Security
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/
Re: Zero Diffie-Hellman shared secret before free [ In reply to ]
> On Nov 13, 2014, at 2:01 PM, Florian Weimer <fweimer@redhat.com> wrote:
>
> On 11/13/2014 07:39 AM, Brian Reiter wrote:
>> + memset(dh_shared_secret, 0, dh_shared_secret_len);
>> free(dh_shared_secret);
>
> I think GCC will remove the memset call because the written values are never read.

Interesting. That seems like an optimization that a compiler might do but it also seems like the intent here is to zero the buffer in memory before freeing it to keep it from being accessed by another process.