File 0002-Fix-integer-overflow-in-the-BER-decoder.patch of Package libksba.2545

Overview Repositories Revisions Requests Users Attributes Meta

File 0002-Fix-integer-overflow-in-the-BER-decoder.patch of Package libksba.2545 (Revision f34b700ea2992faacd5d705633a72349)

Currently displaying revision f34b700ea2992faacd5d705633a72349 , Show latest

From aea7b6032865740478ca4b706850a5217f1c3887 Mon Sep 17 00:00:00 2001
From: Werner Koch <wk@gnupg.org>
Date: Thu, 9 Apr 2015 11:17:28 +0200
Subject: [PATCH 2/3] Fix integer overflow in the BER decoder.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/ber-decoder.c (ber_decoder_s): Change val.length from int to
size_t.
(sum_a1_a2_gt_b, sum_a1_a2_ge_b): New.
(decoder_next): Check for integer overflow.  Use new sum function for
size check.
(_ksba_ber_decoder_dump): Use size_t for n to match change of
val.length.  Adjust printf fomrat.  Check for integer overflow and use
gpg_error_from_syserror instead of GPG_ERR_ENOMEM.
(_ksba_ber_decoder_decode): Use new sum function for size check.
Check for integer overflow.  Use size_t for n to match change of
val.length.
--

The actual bug described below is due to assigning an int
(val.length) to a size_t (ti.length).  The int was too large and thus
negative so that the condition to check for too large objects didn't
worked.  Changing the type would have been enough but other conditions
are possible.  Thus the introduction of sum_a1_a2_ge_b for overflow
checking and checks when adding 100 extra bytes to malloc calls are
added.

Use "gpgsm --verify FILE" to exhibit the problem.  FILE is
-----BEGIN PGP ARMORED FILE-----

MDAGCSqGSIb3DQEHAqCAMIACAQExDzANBgkwMDAwMDAwMDAwADAwBhcwMDAwMDAw
MDAwMDAwMDAwMDAwMDAwMAAwMTAGCTAwMDAwMDAwMDAwBgkwMDAwMDAwMDAwMAYJ
MDAwMDAwMDAwMDAXLDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAw
MDAwMDAwMDAwCoYwMP////UwMDAwMDAwMDAwMDAwMDAwMA==
=tvju
-----END PGP ARMORED FILE-----

Without the patch this error occured:

gpgsm: unknown hash algorithm '1.8.48.48.48.48.48.48.48.48'
  gpgsm: detached signature w/o data - assuming certs-only
  =================================================================
  ==14322==ERROR: AddressSanitizer: heap-buffer-overflow on address
   0x60b00000aded at pc 0x462ca8 bp 0x7fffd5928d90 sp 0x7fffd5928d80
  WRITE of size 1 at 0x60b00000aded thread T0
      #0 0x462ca7 in base64_reader_cb [...]-2.1.2/sm/base64.c:363
      #1 0x7f35e70b6365 (/usr/lib64/libksba.so.8+0x7365)
      #2 0x7f35e70bee11 (/usr/lib64/libksba.so.8+0xfe11)
      #3 0x7f35e70c75ed (/usr/lib64/libksba.so.8+0x185ed)
      #4 0x7f35e70c7a9d (/usr/lib64/libksba.so.8+0x18a9d)
      #5 0x7f35e70c356f (/usr/lib64/libksba.so.8+0x1456f)
      #6 0x7f35e70c58bf (/usr/lib64/libksba.so.8+0x168bf)
      #7 0x48cbee in gpgsm_verify [...]/gnupg-2.1.2/sm/verify.c:171
      #8 0x412901 in main /data/gnupg/gnupg-2.1.2/sm/gpgsm.c:1795
      #9 0x7f35e68d5f9f in __libc_start_main ([...]
      #10 0x415a91 (/data/gnupg/gnupg-2.1.2/sm/gpgsm+0x415a91)

0x60b00000aded is located 0 bytes to the right of 109-byte region
   [0x60b00000ad80,0x60b00000aded)
  allocated by thread T0 here:
      #0 0x7f35e782e6f7 in malloc [...]
      #1 0x7f35e75040b0 (/usr/lib64/libgcrypt.so.20+0xc0b0)

SUMMARY: AddressSanitizer: heap-buffer-overflow [...]
  Shadow bytes around the buggy address:
    0x0c167fff9560: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    0x0c167fff9570: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    0x0c167fff9580: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    0x0c167fff9590: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    0x0c167fff95a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  =>0x0c167fff95b0: 00 00 00 00 00 00 00 00 00 00 00 00 00[05]fa fa
    0x0c167fff95c0: fa fa fa fa fa fa 00 00 00 00 00 00 00 00 00 00
    0x0c167fff95d0: 00 00 00 fa fa fa fa fa fa fa fa fa 00 00 00 00

Reported-by: Hanno Böck
Signed-off-by: Werner Koch <wk@gnupg.org>
---
 src/ber-decoder.c | 71 ++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 54 insertions(+), 17 deletions(-)

diff --git a/src/ber-decoder.c b/src/ber-decoder.c
index 873f810..b4689fa 100644
--- a/src/ber-decoder.c
+++ b/src/ber-decoder.c
@@ -100,7 +100,7 @@ struct ber_decoder_s
   struct
   {
     int primitive;  /* current value is a primitive one */
-    int length;     /* length of the primitive one */
+    size_t length;  /* length of the primitive one */
     int nhdr;       /* length of the header */
     int tag;
     int is_endtag;
@@ -109,6 +109,23 @@ struct ber_decoder_s
 };
 
 
+
+/* Evaluate with overflow check:  A1 + A2 > B  */
+static inline int
+sum_a1_a2_gt_b (size_t a1, size_t a2, size_t b)
+{
+  size_t sum = a1 + a2;
+  return (sum < a1 || sum > b);
+}
+
+/* Evaluate with overflow check:  A1 + A2 >= B  */
+static inline int
+sum_a1_a2_ge_b (size_t a1, size_t a2, size_t b)
+{
+  size_t sum = a1 + a2;
+  return (sum < a1 || sum >= b);
+}
+
 
 
 static DECODER_STATE
@@ -839,14 +856,16 @@ decoder_next (BerDecoder d)
         {
           /* We need some extra bytes to store the stuff we read ahead
              at the end of the module which is later pushed back. */
-          d->image.length = ti.length + 100;
           d->image.used = 0;
+          d->image.length = ti.length + 100;
+          if (d->image.length < ti.length)
+            return gpg_error (GPG_ERR_BAD_BER);
           d->image.buf = xtrymalloc (d->image.length);
           if (!d->image.buf)
             return gpg_error (GPG_ERR_ENOMEM);
         }
 
-      if (ti.nhdr + d->image.used >= d->image.length)
+      if (sum_a1_a2_ge_b (ti.nhdr, d->image.used, d->image.length))
         return set_error (d, NULL, "image buffer too short to store the tag");
 
       memcpy (d->image.buf + d->image.used, ti.buf, ti.nhdr);
@@ -1041,7 +1060,7 @@ _ksba_ber_decoder_dump (BerDecoder d, FILE *fp)
   int depth = 0;
   AsnNode node;
   unsigned char *buf = NULL;
-  size_t buflen = 0;;
+  size_t buflen = 0;
 
   if (!d)
     return gpg_error (GPG_ERR_INV_VALUE);
@@ -1063,9 +1082,9 @@ _ksba_ber_decoder_dump (BerDecoder d, FILE *fp)
       if (node)
         depth = distance (d->root, node);
 
-      fprintf (fp, "%4lu %4u:%*s",
+      fprintf (fp, "%4lu %4lu:%*s",
                ksba_reader_tell (d->reader) - d->val.nhdr,
-               d->val.length,
+               (unsigned long)d->val.length,
                depth*2, "");
       if (node)
         _ksba_asn_node_dump (node, fp);
@@ -1074,16 +1093,22 @@ _ksba_ber_decoder_dump (BerDecoder d, FILE *fp)
 
       if (node && d->val.primitive)
         {
-          int i, n, c;
+          size_t n;
+          int i, c;
           char *p;
 
           if (!buf || buflen < d->val.length)
             {
               xfree (buf);
               buflen = d->val.length + 100;
-              buf = xtrymalloc (buflen);
-              if (!buf)
-                err = gpg_error (GPG_ERR_ENOMEM);
+              if (buflen < d->val.length)
+                err = gpg_error (GPG_ERR_BAD_BER); /* Overflow */
+              else
+                {
+                  buf = xtrymalloc (buflen);
+                  if (!buf)
+                    err = gpg_error_from_syserror ();
+                }
             }
 
           for (n=0; !err && n < d->val.length; n++)
@@ -1171,8 +1196,6 @@ _ksba_ber_decoder_decode (BerDecoder d, const char *start_name,
 
   while (!(err = decoder_next (d)))
     {
-      int n, c;
-
       node = d->val.node;
       /* Fixme: USE_IMAGE is only not used with the ber-dump utility
          and thus of no big use.  We should remove the other code
@@ -1188,7 +1211,7 @@ _ksba_ber_decoder_decode (BerDecoder d, const char *start_name,
               if (node->type == TYPE_ANY)
                 node->actual_type = d->val.tag;
             }
-          if (d->image.used + d->val.length > d->image.length)
+          if (sum_a1_a2_gt_b (d->image.used, d->val.length, d->image.length))
             err = set_error(d, NULL, "TLV length too large");
           else if (d->val.primitive)
             {
@@ -1196,18 +1219,32 @@ _ksba_ber_decoder_decode (BerDecoder d, const char *start_name,
                                d->image.buf + d->image.used, d->val.length))
                 err = eof_or_error (d, 1);
               else
-                d->image.used += d->val.length;
+                {
+                  size_t sum = d->image.used + d->val.length;
+                  if (sum < d->image.used)
+                    err = gpg_error (GPG_ERR_BAD_BER);
+                  else
+                    d->image.used = sum;
+                }
             }
         }
       else if (node && d->val.primitive)
         {
+          size_t n;
+          int c;
+
           if (!buf || buflen < d->val.length)
             {
               xfree (buf);
               buflen = d->val.length + 100;
-              buf = xtrymalloc (buflen);
-              if (!buf)
-                err = gpg_error (GPG_ERR_ENOMEM);
+              if (buflen < d->val.length)
+                err = gpg_error (GPG_ERR_BAD_BER);
+              else
+                {
+                  buf = xtrymalloc (buflen);
+                  if (!buf)
+                    err = gpg_error_from_syserror ();
+                }
             }
 
           for (n=0; !err && n < d->val.length; n++)
-- 
2.6.2

Places

File 0002-Fix-integer-overflow-in-the-BER-decoder.patch of Package libksba.2545 (Revision f34b700ea2992faacd5d705633a72349)

Places