| 1 | From a4e1b7755ebbdee2689d40683ba69f09e38a8d7f Mon Sep 17 00:00:00 2001 |
| 2 | From: Qualys Security Advisory <qsa@qualys.com> |
| 3 | Date: Sun, 21 Feb 2021 22:30:03 -0800 |
| 4 | Subject: [PATCH 21/29] Security: Avoid modification of constant data in dkim |
| 5 | handling |
| 6 | |
| 7 | Based on Heiko Schlittermann's commits f880c7f3 and c118c7f4. This |
| 8 | fixes: |
| 9 | |
| 10 | 6/ In src/pdkim/pdkim.c, pdkim_update_ctx_bodyhash() is sometimes called |
| 11 | with a global orig_data and hence canon_data, and the following line can |
| 12 | therefore modify data that should be constant: |
| 13 | |
| 14 | 773 canon_data->len = b->bodylength - b->signed_body_bytes; |
| 15 | |
| 16 | For example, the following proof of concept sets lineending.len to 0 |
| 17 | (this should not be possible): |
| 18 | |
| 19 | (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 |
| 20 | |
| 21 | (gdb) print lineending |
| 22 | $1 = {data = 0x55e18035b2ad "\r\n", len = 2} |
| 23 | (gdb) print &lineending.len |
| 24 | $3 = (size_t *) 0x55e180385948 <lineending+8> |
| 25 | (gdb) watch *(size_t *) 0x55e180385948 |
| 26 | |
| 27 | Hardware watchpoint 1: *(size_t *) 0x55e180385948 |
| 28 | Old value = 2 |
| 29 | New value = 0 |
| 30 | (gdb) print lineending |
| 31 | $5 = {data = 0x55e18035b2ad "\r\n", len = 0} |
| 32 | --- |
| 33 | src/pdkim/pdkim.c | 21 ++++++++++++--------- |
| 34 | 1 file changed, 12 insertions(+), 9 deletions(-) |
| 35 | |
| 36 | diff --git a/src/pdkim/pdkim.c b/src/pdkim/pdkim.c |
| 37 | index e3233e9f0..512a3e352 100644 |
| 38 | --- a/src/pdkim/pdkim.c |
| 39 | +++ b/src/pdkim/pdkim.c |
| 40 | @@ -107,7 +107,7 @@ pdkim_combined_canon_entry pdkim_combined_canons[] = { |
| 41 | }; |
| 42 | |
| 43 | |
| 44 | -static blob lineending = {.data = US"\r\n", .len = 2}; |
| 45 | +static const blob lineending = {.data = US"\r\n", .len = 2}; |
| 46 | |
| 47 | /* -------------------------------------------------------------------------- */ |
| 48 | uschar * |
| 49 | @@ -719,9 +719,11 @@ return NULL; |
| 50 | If we have to relax the data for this sig, return our copy of it. */ |
| 51 | |
| 52 | static blob * |
| 53 | -pdkim_update_ctx_bodyhash(pdkim_bodyhash * b, blob * orig_data, blob * relaxed_data) |
| 54 | +pdkim_update_ctx_bodyhash(pdkim_bodyhash * b, const blob * orig_data, blob * relaxed_data) |
| 55 | { |
| 56 | -blob * canon_data = orig_data; |
| 57 | +const blob * canon_data = orig_data; |
| 58 | +size_t left; |
| 59 | + |
| 60 | /* Defaults to simple canon (no further treatment necessary) */ |
| 61 | |
| 62 | if (b->canon_method == PDKIM_CANON_RELAXED) |
| 63 | @@ -767,16 +769,17 @@ if (b->canon_method == PDKIM_CANON_RELAXED) |
| 64 | } |
| 65 | |
| 66 | /* Make sure we don't exceed the to-be-signed body length */ |
| 67 | +left = canon_data->len; |
| 68 | if ( b->bodylength >= 0 |
| 69 | - && b->signed_body_bytes + (unsigned long)canon_data->len > b->bodylength |
| 70 | + && left > (unsigned long)b->bodylength - b->signed_body_bytes |
| 71 | ) |
| 72 | - canon_data->len = b->bodylength - b->signed_body_bytes; |
| 73 | + left = (unsigned long)b->bodylength - b->signed_body_bytes; |
| 74 | |
| 75 | -if (canon_data->len > 0) |
| 76 | +if (left > 0) |
| 77 | { |
| 78 | - exim_sha_update(&b->body_hash_ctx, CUS canon_data->data, canon_data->len); |
| 79 | - b->signed_body_bytes += canon_data->len; |
| 80 | - DEBUG(D_acl) pdkim_quoteprint(canon_data->data, canon_data->len); |
| 81 | + exim_sha_update(&b->body_hash_ctx, CUS canon_data->data, left); |
| 82 | + b->signed_body_bytes += left; |
| 83 | + DEBUG(D_acl) pdkim_quoteprint(canon_data->data, left); |
| 84 | } |
| 85 | |
| 86 | return relaxed_data; |
| 87 | -- |
| 88 | 2.30.2 |
| 89 | |