From 55ab0c211c3e0def1016971d3b3ebd47c006a751 Mon Sep 17 00:00:00 2001 From: "Heiko Schlittermann (HS12-RIPE)" Date: Tue, 30 Mar 2021 22:59:25 +0200 Subject: [PATCH] SECURITY: Avoid modification of constant data in dkim handling Based on Heiko Schlittermann's commits f880c7f3 and c118c7f4. This fixes: 6/ In src/pdkim/pdkim.c, pdkim_update_ctx_bodyhash() is sometimes called with a global orig_data and hence canon_data, and the following line can therefore modify data that should be constant: 773 canon_data->len = b->bodylength - b->signed_body_bytes; For example, the following proof of concept sets lineending.len to 0 (this should not be possible): (sleep 10; echo 'EHLO test'; sleep 3; echo 'MAIL FROM:<>'; sleep 3; echo 'RCPT TO:postmaster'; sleep 3; echo 'DATA'; date >&2; sleep 30; printf 'DKIM-Signature:a=rsa-sha1;c=simple/simple;l=0\r\n\r\n\r\nXXX\r\n.\r\n'; sleep 30) | nc -n -v 192.168.56.102 25 (gdb) print lineending $1 = {data = 0x55e18035b2ad "\r\n", len = 2} (gdb) print &lineending.len $3 = (size_t *) 0x55e180385948 (gdb) watch *(size_t *) 0x55e180385948 Hardware watchpoint 1: *(size_t *) 0x55e180385948 Old value = 2 New value = 0 (gdb) print lineending $5 = {data = 0x55e18035b2ad "\r\n", len = 0} (cherry picked from commit 92359a62a0e31734ad8069c66f64b37f9eaaccbe) (cherry picked from commit c5f2f5cf2a6b45ae7ba0ed15e04fbe014727b210) --- src/src/pdkim/pdkim.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/src/pdkim/pdkim.c b/src/src/pdkim/pdkim.c index 074106b5d..45fa2a104 100644 --- a/src/src/pdkim/pdkim.c +++ b/src/src/pdkim/pdkim.c @@ -107,7 +107,7 @@ pdkim_combined_canon_entry pdkim_combined_canons[] = { }; -static blob lineending = {.data = US"\r\n", .len = 2}; +static const blob lineending = {.data = US"\r\n", .len = 2}; /* -------------------------------------------------------------------------- */ uschar * @@ -720,9 +720,9 @@ return NULL; If we have to relax the data for this sig, return our copy of it. */ static blob * -pdkim_update_ctx_bodyhash(pdkim_bodyhash * b, const blob const * orig_data, blob * relaxed_data) +pdkim_update_ctx_bodyhash(pdkim_bodyhash * b, const blob * orig_data, blob * relaxed_data) { -const blob const * canon_data = orig_data; +const blob * canon_data = orig_data; size_t left; /* Defaults to simple canon (no further treatment necessary) */ @@ -771,9 +771,9 @@ if (b->canon_method == PDKIM_CANON_RELAXED) /* Make sure we don't exceed the to-be-signed body length */ left = canon_data->len; if ( b->bodylength >= 0 - && b->signed_body_bytes + left > b->bodylength + && left > (unsigned long)b->bodylength - b->signed_body_bytes ) - left = b->bodylength - b->signed_body_bytes; + left = (unsigned long)b->bodylength - b->signed_body_bytes; if (left > 0) { -- 2.30.2