SECURITY: Avoid modification of constant data

Credits: Qualys

    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 <lineending+8>
    (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 9fce76f56459dde7489eb21ce1ff822e04e10f43)
(cherry picked from commit 667fb25b8f0dc3fbac57bce4051e345555fa776a)

diff --git a/src/src/pdkim/pdkim.c b/src/src/pdkim/pdkim.c
index 82dab3db11775c6e035d18a47d9cac561fe32d49..0ad4d12d8171288b0d085bcea8b386e8add4d1f3 100644 (file)
--- a/src/src/pdkim/pdkim.c
+++ b/src/src/pdkim/pdkim.c
@@ -720,10 +720,10 @@ return NULL;
 If we have to relax the data for this sig, return our copy of it. */
 
 static blob *
 If we have to relax the data for this sig, return our copy of it. */
 
 static blob *

-pdkim_update_ctx_bodyhash(pdkim_bodyhash * b, blob * orig_data, blob * relaxed_data)
+pdkim_update_ctx_bodyhash(pdkim_bodyhash * b, const blob const * orig_data, blob * relaxed_data)

 {
 const blob const * canon_data = orig_data;
 {
 const blob const * canon_data = orig_data;

-size_t len;
+size_t left;

 
 /* Defaults to simple canon (no further treatment necessary) */
 
 
 /* Defaults to simple canon (no further treatment necessary) */
 


@@ -769,16 +769,17 @@ if (b->canon_method == PDKIM_CANON_RELAXED)
   }
 
 /* Make sure we don't exceed the to-be-signed body length */
   }
 
 /* Make sure we don't exceed the to-be-signed body length */

+left = canon_data->len;

 if (  b->bodylength >= 0
 if (  b->bodylength >= 0

-   && b->signed_body_bytes + (unsigned long)canon_data->len > b->bodylength
+   && b->signed_body_bytes + left > b->bodylength

    )
    )

-  len = b->bodylength - b->signed_body_bytes;
+  left = b->bodylength - b->signed_body_bytes;

 
 

-if (len > 0)
+if (left > 0)

   {
   {

-  exim_sha_update(&b->body_hash_ctx, CUS canon_data->data, len);
-  b->signed_body_bytes += len;
-  DEBUG(D_acl) pdkim_quoteprint(canon_data->data, len);
+  exim_sha_update(&b->body_hash_ctx, CUS canon_data->data, left);
+  b->signed_body_bytes += left;
+  DEBUG(D_acl) pdkim_quoteprint(canon_data->data, left);

   }
 
 return relaxed_data;
   }
 
 return relaxed_data;