diff mbox

sunrpc: xdr_bytes/xdr_string need to free buffer on error [BZ #21461]

Message ID 91e971f6-a9d6-f6d3-5202-5b6392e23c94@redhat.com
State New, archived
Headers show

Commit Message

Florian Weimer May 8, 2017, 8:15 a.m. UTC
The attached patch adds the CVE name and uses a more obvious condition 
(I think) for the allocation check in xdr_bytes/xdr_string.

Any comments?

Thanks,
Florian

Comments

Andreas Schwab May 8, 2017, 8:39 a.m. UTC | #1
On Mai 08 2017, Florian Weimer <fweimer@redhat.com> wrote:

> +* The xdr_bytes and xdr_string routines free the internally allocated
> +  buffer if deserialization of the buffer contents fails for any reason.

Isn't it the caller's responsibility to call the XDR functions with
XDR_FREE in any case?

Andreas.
Florian Weimer May 8, 2017, 8:58 a.m. UTC | #2
On 05/08/2017 10:39 AM, Andreas Schwab wrote:
> On Mai 08 2017, Florian Weimer <fweimer@redhat.com> wrote:
> 
>> +* The xdr_bytes and xdr_string routines free the internally allocated
>> +  buffer if deserialization of the buffer contents fails for any reason.
> 
> Isn't it the caller's responsibility to call the XDR functions with
> XDR_FREE in any case?

Hmm.  Interesting point.  I don't have Sun RPC documentation which is 
that precise.

But it's certainly true for complex types—the code rpcgen produces seems 
to assume that the caller calls XDR_FREE on deserialization failure.

But things get a bit weird for xdr_string here.  It calls strlen on the 
allocated buffer, which is not initialized after a deserialization 
failure.  This is done so that a value can be passed to mem_free, but I 
don't think we have a mem_free implementation which uses this value.  It 
would also be wrong if the successfully deserialized string contains a 
NUL byte.

Thanks,
Florian
Andreas Schwab May 15, 2017, 12:48 p.m. UTC | #3
On Mai 08 2017, Florian Weimer <fweimer@redhat.com> wrote:

> But things get a bit weird for xdr_string here.  It calls strlen on the
> allocated buffer, which is not initialized after a deserialization
> failure.

It is always zero-terminated, at least.

Andreas.
Florian Weimer June 6, 2018, 8:17 a.m. UTC | #4
On 05/08/2017 10:39 AM, Andreas Schwab wrote:
> On Mai 08 2017, Florian Weimer <fweimer@redhat.com> wrote:
> 
>> +* The xdr_bytes and xdr_string routines free the internally allocated
>> +  buffer if deserialization of the buffer contents fails for any reason.
> 
> Isn't it the caller's responsibility to call the XDR functions with
> XDR_FREE in any case?

I've decided to follow this interpretation and requested that MITRE 
rejects CVE-2017-8804.

Thanks,
Florian
diff mbox

Patch

CVE-2017-8804: xdr_bytes/xdr_string need to free buffer on error [BZ #21461]

2017-05-08  Florian Weimer  <fweimer@redhat.com>

	[BZ #21461]
	CVE-2017-8804
	* sunrpc/xdr.c (xdr_bytes): Deallocate allocated buffer on error.
	(xdr_string): Likewise.
	* sunrpc/Makefile (tests): Add tst-xdrmem3.
	(tests-special): Add mtrace-tst-xdrmem3.out.
	(generated): Add mtrace-tst-xdrmem3.out, tst-xdrmem3.mtrace.
	(tst-xdrmem3-ENV): Set MALLOC_TRACE.
	(mtrace-tst-xdrmem3.out): Run mtrace.
	(tst-xdrmem3): Link against full libc.
	* sunrpc/tst-xdrmem3.c: New file.

diff --git a/NEWS b/NEWS
index 5558ca3..9960aea 100644
--- a/NEWS
+++ b/NEWS
@@ -62,6 +62,10 @@  Security related changes:
 * The DNS stub resolver limits the advertised UDP buffer size to 1200 bytes,
   to avoid fragmentation-based spoofing attacks.
 
+* The xdr_bytes and xdr_string routines free the internally allocated
+  buffer if deserialization of the buffer contents fails for any reason.
+  (CVE-2017-8804)
+
 The following bugs are resolved with this release:
 
   [The release manager will add the list generated by
diff --git a/sunrpc/Makefile b/sunrpc/Makefile
index a5177ff..7a9117e 100644
--- a/sunrpc/Makefile
+++ b/sunrpc/Makefile
@@ -95,9 +95,16 @@  others += rpcgen
 endif
 
 tests = tst-xdrmem tst-xdrmem2 test-rpcent tst-udp-error tst-udp-timeout \
-  tst-udp-nonblocking
+  tst-udp-nonblocking tst-xdrmem3
 xtests := tst-getmyaddr
 
+tests-special += $(objpfx)mtrace-tst-xdrmem3.out
+generated += mtrace-tst-xdrmem3.out tst-xdrmem3.mtrace
+tst-xdrmem3-ENV = MALLOC_TRACE=$(objpfx)tst-xdrmem3.mtrace
+$(objpfx)mtrace-tst-xdrmem3.out: $(objpfx)tst-xdrmem3.out
+	$(common-objpfx)malloc/mtrace $(objpfx)tst-xdrmem3.mtrace > $@; \
+	$(evaluate-test)
+
 ifeq ($(have-thread-library),yes)
 xtests += thrsvc
 tests += tst-svc_register tst-udp-garbage
@@ -162,6 +169,7 @@  BUILD_CPPFLAGS += $(sunrpc-CPPFLAGS)
 $(objpfx)tst-getmyaddr: $(common-objpfx)linkobj/libc.so
 $(objpfx)tst-xdrmem: $(common-objpfx)linkobj/libc.so
 $(objpfx)tst-xdrmem2: $(common-objpfx)linkobj/libc.so
+$(objpfx)tst-xdrmem3: $(common-objpfx)linkobj/libc.so
 $(objpfx)tst-udp-error: $(common-objpfx)linkobj/libc.so
 $(objpfx)tst-svc_register: \
   $(common-objpfx)linkobj/libc.so $(shared-thread-library)
diff --git a/sunrpc/tst-xdrmem3.c b/sunrpc/tst-xdrmem3.c
new file mode 100644
index 0000000..b3c72ae
--- /dev/null
+++ b/sunrpc/tst-xdrmem3.c
@@ -0,0 +1,83 @@ 
+/* Test xdr_bytes, xdr_string behavior on deserialization failure.
+   Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <mcheck.h>
+#include <rpc/rpc.h>
+#include <support/check.h>
+#include <support/support.h>
+
+static int
+do_test (void)
+{
+  mtrace ();
+
+  /* If do_own_buffer, allocate the buffer and pass it to the
+     deserialization routine.  Otherwise the routine is requested to
+     allocate the buffer.  */
+  for (int do_own_buffer = 0; do_own_buffer < 2; ++do_own_buffer)
+    {
+      /* Length 16 MiB, but only 2 bytes of data in the packet.  */
+      unsigned char buf[] = "\x01\x00\x00\x00\xff";
+      XDR xdrs;
+      char *result;
+      unsigned int result_len;
+
+      /* Test xdr_bytes.  */
+      xdrmem_create (&xdrs, (char *) buf, sizeof (buf), XDR_DECODE);
+      result_len = 0;
+      if (do_own_buffer)
+        {
+          char *own_buffer = xmalloc (10);
+          result = own_buffer;
+          TEST_VERIFY (!xdr_bytes (&xdrs, &result, &result_len, 10));
+          TEST_VERIFY (result == own_buffer);
+          free (own_buffer);
+        }
+      else
+        {
+          result = NULL;
+          TEST_VERIFY (!xdr_bytes (&xdrs, &result, &result_len, -1));
+          TEST_VERIFY (result == NULL);
+        }
+      TEST_VERIFY (result_len == 16 * 1024 * 1024);
+      xdr_destroy (&xdrs);
+
+      /* Test xdr_string.  */
+      xdrmem_create (&xdrs, (char *) buf, sizeof (buf), XDR_DECODE);
+      if (do_own_buffer)
+        {
+          char *own_buffer = xmalloc (10);
+          result = own_buffer;
+          TEST_VERIFY (!xdr_string (&xdrs, &result, 10));
+          TEST_VERIFY (result == own_buffer);
+          free (own_buffer);
+        }
+      else
+        {
+          result = NULL;
+          TEST_VERIFY (!xdr_string (&xdrs, &result, -1));
+          TEST_VERIFY (result == NULL);
+        }
+      xdr_destroy (&xdrs);
+    }
+
+  return 0;
+}
+
+#include <support/test-driver.c>
+
diff --git a/sunrpc/xdr.c b/sunrpc/xdr.c
index bfabf33..3fa7218 100644
--- a/sunrpc/xdr.c
+++ b/sunrpc/xdr.c
@@ -620,14 +620,23 @@  xdr_bytes (XDR *xdrs, char **cpp, u_int *sizep, u_int maxsize)
 	}
       if (sp == NULL)
 	{
-	  *cpp = sp = (char *) mem_alloc (nodesize);
+	  sp = (char *) mem_alloc (nodesize);
+	  if (sp == NULL)
+	    {
+	      (void) __fxprintf (NULL, "%s: %s", __func__,
+				 _("out of memory\n"));
+	      return FALSE;
+	    }
 	}
-      if (sp == NULL)
+      if (!xdr_opaque (xdrs, sp, nodesize))
 	{
-	  (void) __fxprintf (NULL, "%s: %s", __func__, _("out of memory\n"));
+	  if (*cpp == NULL)
+	    /* Deallocate the buffer allocated by this function.  */
+	    free (sp);
 	  return FALSE;
 	}
-      /* fall into ... */
+      *cpp = sp;
+      return TRUE;
 
     case XDR_ENCODE:
       return xdr_opaque (xdrs, sp, nodesize);
@@ -781,14 +790,26 @@  xdr_string (XDR *xdrs, char **cpp, u_int maxsize)
     {
     case XDR_DECODE:
       if (sp == NULL)
-	*cpp = sp = (char *) mem_alloc (nodesize);
-      if (sp == NULL)
 	{
-	  (void) __fxprintf (NULL, "%s: %s", __func__, _("out of memory\n"));
-	  return FALSE;
+	  sp = (char *) mem_alloc (nodesize);
+	  if (sp == NULL)
+	    {
+	      (void) __fxprintf (NULL, "%s: %s", __func__,
+				 _("out of memory\n"));
+	      return FALSE;
+	    }
 	}
       sp[size] = 0;
-      /* fall into ... */
+
+      if (!xdr_opaque (xdrs, sp, size))
+	{
+	  if (*cpp == NULL)
+	    /* Deallocate the buffer allocated by this function.  */
+	    free (sp);
+	  return FALSE;
+	}
+      *cpp = sp;
+      return TRUE;
 
     case XDR_ENCODE:
       return xdr_opaque (xdrs, sp, size);