nhyatt/putty-source
1
0
Fork 0
mirror of https://git.tartarus.org/simon/putty.git synced 2025-01-09 17:38:00 +00:00
Code Issues Projects Releases Wiki Activity

Fix use-after-free when using ChaCha20 + Poly1305.

Browse Source 
When we're running with that cipher/MAC combination, what ssh2bpp.c
sees as separate cipher and MAC objects are actually just different
interfaces of the same object: the Poly1305 constructor function just
returns an alternate view of the cipher object it's passed, and the
free function does nothing.

But Address Sanitiser points out that if we first call
ssh2_cipher_free and then ssh2_mac_free, then although poly1305_free
would do nothing once we reached it, we can't actually reach it
without looking up the vtable in the object we've just thrown away.

Easily fixed by reversing the order of mac_free and cipher_free in the
BPP. Also I've centralised that freeing so that it doesn't have to be
repeated with the same careful ordering in the BPP cleanup function
and the functions that replace a direction of crypto.
This commit is contained in:
Simon Tatham 2019-01-03 13:57:54 +00:00
parent f2174536a8
commit 4397016a51
1 changed files with 34 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

58
ssh2bpp.c
View File

@ -67,22 +67,42 @@ BinaryPacketProtocol *ssh2_bpp_new(
return &s->bpp;
}
static void ssh2_bpp_free_outgoing_crypto(struct ssh2_bpp_state *s)
{
/*
* We must free the MAC before the cipher, because sometimes the
* MAC is not actually separately allocated but just a different
* facet of the same object as the cipher, in which case
* ssh2_mac_free does nothing and ssh2_cipher_free does the actual
* freeing. So if we freed the cipher first and then tried to
* dereference the MAC's vtable pointer to find out how to free
* that too, we'd be accessing freed memory.
*/
if (s->out.mac)
ssh2_mac_free(s->out.mac);
if (s->out.cipher)
ssh2_cipher_free(s->out.cipher);
if (s->out_comp)
ssh_compressor_free(s->out_comp);
}
static void ssh2_bpp_free_incoming_crypto(struct ssh2_bpp_state *s)
{
/* As above, take care to free in.mac before in.cipher */
if (s->in.mac)
ssh2_mac_free(s->in.mac);
if (s->in.cipher)
ssh2_cipher_free(s->in.cipher);
if (s->in_decomp)
ssh_decompressor_free(s->in_decomp);
}
static void ssh2_bpp_free(BinaryPacketProtocol *bpp)
{
struct ssh2_bpp_state *s = container_of(bpp, struct ssh2_bpp_state, bpp);
sfree(s->buf);
if (s->out.cipher)
ssh2_cipher_free(s->out.cipher);
if (s->out.mac)
ssh2_mac_free(s->out.mac);
if (s->out_comp)
ssh_compressor_free(s->out_comp);
if (s->in.cipher)
ssh2_cipher_free(s->in.cipher);
if (s->in.mac)
ssh2_mac_free(s->in.mac);
if (s->in_decomp)
ssh_decompressor_free(s->in_decomp);
ssh2_bpp_free_outgoing_crypto(s);
ssh2_bpp_free_incoming_crypto(s);
sfree(s->pktin);
sfree(s);
}
@ -97,12 +117,7 @@ void ssh2_bpp_new_outgoing_crypto(
assert(bpp->vt == &ssh2_bpp_vtable);
s = container_of(bpp, struct ssh2_bpp_state, bpp);
if (s->out.cipher)
ssh2_cipher_free(s->out.cipher);
if (s->out.mac)
ssh2_mac_free(s->out.mac);
if (s->out_comp)
ssh_compressor_free(s->out_comp);
ssh2_bpp_free_outgoing_crypto(s);
if (cipher) {
s->out.cipher = ssh2_cipher_new(cipher);
@ -164,12 +179,7 @@ void ssh2_bpp_new_incoming_crypto(
assert(bpp->vt == &ssh2_bpp_vtable);
s = container_of(bpp, struct ssh2_bpp_state, bpp);
if (s->in.cipher)
ssh2_cipher_free(s->in.cipher);
if (s->in.mac)
ssh2_mac_free(s->in.mac);
if (s->in_decomp)
ssh_decompressor_free(s->in_decomp);
ssh2_bpp_free_incoming_crypto(s);
if (cipher) {
s->in.cipher = ssh2_cipher_new(cipher);