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

Change the rules for how we free a linked cipher and MAC.

Browse Source 
In the situation where a MAC and cipher implementation are tied
together by being facets of the same underlying object (used by the
inseparable ChaCha20 + Poly1305 pair), previously we freed them by
having the cipher_free function actually do the freeing, having the
mac_free function do nothing, and taking great care to call those in
the right order. (Otherwise, the mac_free function dereferences a
no-longer-valid vtable pointer and doesn't get as far as _finding out_
that it doesn't have to do anything.)

That's a time bomb in general, and especially awkward in situations
like testcrypt where we don't get precise control over freeing order
in any case. So I've replaced that system with one in which there are
two flags in the ChaCha20-Poly1305 structure, saying whether each of
the cipher and MAC facets is currently considered to be allocated.
When the last of those flags is cleared, the object is actually freed.
So now they can be freed in either order.
This commit is contained in:
Simon Tatham 2022-08-16 18:22:29 +01:00
parent e52087719c
commit dbc77dbd7a
2 changed files with 20 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

25
crypto/chacha20-poly1305.c
View File

@ -869,6 +869,7 @@ struct ccp_context {
BinarySink_IMPLEMENTATION;
ssh_cipher ciph;
ssh2_mac mac_if;
bool ciph_allocated, mac_allocated;
};
static ssh2_mac *poly_ssh2_new(
@ -876,13 +877,27 @@ static ssh2_mac *poly_ssh2_new(
{
struct ccp_context *ctx = container_of(cipher, struct ccp_context, ciph);
ctx->mac_if.vt = alg;
ctx->mac_allocated = true;
BinarySink_DELEGATE_INIT(&ctx->mac_if, ctx);
return &ctx->mac_if;
}
static void ccp_common_free(struct ccp_context *ctx)
{
if (ctx->ciph_allocated || ctx->mac_allocated)
return;
smemclr(&ctx->a_cipher, sizeof(ctx->a_cipher));
smemclr(&ctx->b_cipher, sizeof(ctx->b_cipher));
smemclr(&ctx->mac, sizeof(ctx->mac));
sfree(ctx);
}
static void poly_ssh2_free(ssh2_mac *mac)
{
/* Not allocated, just forwarded, no need to free */
struct ccp_context *ctx = container_of(mac, struct ccp_context, mac_if);
ctx->mac_allocated = false;
ccp_common_free(ctx);
}
static void poly_setkey(ssh2_mac *mac, ptrlen key)
@ -963,16 +978,16 @@ static ssh_cipher *ccp_new(const ssh_cipheralg *alg)
BinarySink_INIT(ctx, poly_BinarySink_write);
poly1305_init(&ctx->mac);
ctx->ciph.vt = alg;
ctx->ciph_allocated = true;
ctx->mac_allocated = false;
return &ctx->ciph;
}
static void ccp_free(ssh_cipher *cipher)
{
struct ccp_context *ctx = container_of(cipher, struct ccp_context, ciph);
smemclr(&ctx->a_cipher, sizeof(ctx->a_cipher));
smemclr(&ctx->b_cipher, sizeof(ctx->b_cipher));
smemclr(&ctx->mac, sizeof(ctx->mac));
sfree(ctx);
ctx->ciph_allocated = false;
ccp_common_free(ctx);
}
static void ccp_iv(ssh_cipher *cipher, const void *iv)

9
ssh/bpp2.c
View File

@ -73,15 +73,6 @@ BinaryPacketProtocol *ssh2_bpp_new(
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 ssh_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)