From dbc77dbd7a247957602aa8d23bdf92dff5a4f35a Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 16 Aug 2022 18:22:29 +0100 Subject: [PATCH] Change the rules for how we free a linked cipher and MAC. 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. --- crypto/chacha20-poly1305.c | 25 ++++++++++++++++++++----- ssh/bpp2.c | 9 --------- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/crypto/chacha20-poly1305.c b/crypto/chacha20-poly1305.c index dd25b997..f06f7dfe 100644 --- a/crypto/chacha20-poly1305.c +++ b/crypto/chacha20-poly1305.c @@ -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) diff --git a/ssh/bpp2.c b/ssh/bpp2.c index dc98e27c..240101e3 100644 --- a/ssh/bpp2.c +++ b/ssh/bpp2.c @@ -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)