(unknown),

(unknown),

[Leo Famulari]

Subject: [PATCH 0/1] libupnp remote filesystem access CVE-2016-6255

You can use libupnp on a remote server to read and write the filesystem
with the privileges of the libupnp process:

http://seclists.org/oss-sec/2016/q3/102

This patch cherry-picks the upstream commit:

https://github.com/mrjimenez/pupnp/commit/d64d6a44906b5aa5306bdf1708531d698654dda5

Leo Famulari (1):
  gnu: libupnp: Fix CVE-2016-6255.

 gnu/local.mk                                     |  1 +
 gnu/packages/libupnp.scm                         |  2 +
 gnu/packages/patches/libupnp-CVE-2016-6255.patch | 86 ++++++++++++++++++++++++
 3 files changed, 89 insertions(+)
 create mode 100644 gnu/packages/patches/libupnp-CVE-2016-6255.patch

-- 
2.10.1

[PATCH 1/1] gnu: libupnp: Fix CVE-2016-6255.

[Leo Famulari]

* gnu/packages/patches/libupnp-CVE-2016-6255.patch: New file.
* gnu/local.mk (dist_patch_DATA): Add it.
* gnu/packages/libupnp.scm (libupnp): Use it.
---
 gnu/local.mk                                     |  1 +
 gnu/packages/libupnp.scm                         |  2 +
 gnu/packages/patches/libupnp-CVE-2016-6255.patch | 86 ++++++++++++++++++++++++
 3 files changed, 89 insertions(+)
 create mode 100644 gnu/packages/patches/libupnp-CVE-2016-6255.patch

diff --git a/gnu/local.mk b/gnu/local.mk
index 867946d..3352b19 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -656,6 +656,7 @@ dist_patch_DATA =						\
   %D%/packages/patches/libtiff-oob-write-in-nextdecode.patch	\
   %D%/packages/patches/libtool-skip-tests2.patch		\
   %D%/packages/patches/libunwind-CVE-2015-3239.patch		\
+  %D%/packages/patches/libupnp-CVE-2016-6255.patch		\
   %D%/packages/patches/libvpx-CVE-2016-2818.patch		\
   %D%/packages/patches/libwmf-CAN-2004-0941.patch		\
   %D%/packages/patches/libwmf-CVE-2006-3376.patch		\
diff --git a/gnu/packages/libupnp.scm b/gnu/packages/libupnp.scm
index 2f9a574..b9209bf 100644
--- a/gnu/packages/libupnp.scm
+++ b/gnu/packages/libupnp.scm
@@ -17,6 +17,7 @@
 ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
 
 (define-module (gnu packages libupnp)
+  #:use-module (gnu packages)
   #:use-module (guix licenses)
   #:use-module (guix packages)
   #:use-module (guix download)
@@ -31,6 +32,7 @@
       (method url-fetch)
       (uri (string-append "mirror://sourceforge/pupnp/pupnp/libUPnP%20"
                           version "/" name "-" version ".tar.bz2"))
+      (patches (search-patches "libupnp-CVE-2016-6255.patch"))
       (sha256
        (base32
         "0qrsdsb1qm85hc4jy04qph895613d148f0x1mmk6z99y3q43fdgf"))))
diff --git a/gnu/packages/patches/libupnp-CVE-2016-6255.patch b/gnu/packages/patches/libupnp-CVE-2016-6255.patch
new file mode 100644
index 0000000..27377b3
--- /dev/null
+++ b/gnu/packages/patches/libupnp-CVE-2016-6255.patch
@@ -0,0 +1,86 @@
+Fix CVE-2016-6255:
+
+https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2016-6255
+http://www.openwall.com/lists/oss-security/2016/07/18/13
+
+Patch copied from upstream source repository:
+
+https://github.com/mrjimenez/pupnp/commit/d64d6a44906b5aa5306bdf1708531d698654dda5
+
+From d64d6a44906b5aa5306bdf1708531d698654dda5 Mon Sep 17 00:00:00 2001
+From: Matthew Garrett <mjg59@srcf.ucam.org>
+Date: Tue, 23 Feb 2016 13:53:20 -0800
+Subject: [PATCH] Don't allow unhandled POSTs to write to the filesystem by
+ default
+
+If there's no registered handler for a POST request, the default behaviour
+is to write it to the filesystem. Several million deployed devices appear
+to have this behaviour, making it possible to (at least) store arbitrary
+data on them. Add a configure option that enables this behaviour, and change
+the default to just drop POSTs that aren't directly handled.
+
+Signed-off-by: Marcelo Roberto Jimenez <mroberto@users.sourceforge.net>
+(cherry picked from commit c91a8a3903367e1163765b73eb4d43be7d7927fa)
+---
+ configure.ac                         | 9 +++++++++
+ upnp/inc/upnpconfig.h.in             | 9 +++++++++
+ upnp/src/genlib/net/http/webserver.c | 4 ++++
+ 3 files changed, 22 insertions(+)
+
+diff --git a/configure.ac b/configure.ac
+index 9548913..a8731b5 100644
+--- a/configure.ac
++++ b/configure.ac
+@@ -501,6 +501,15 @@ if test "x$enable_blocking_tcp_connections" = xyes ; then
+         AC_DEFINE(UPNP_ENABLE_BLOCKING_TCP_CONNECTIONS, 1, [see upnpconfig.h])
+ fi
+ 
++RT_BOOL_ARG_ENABLE([scriptsupport], [yes], [script support for IXML document tree, see ixml.h])
++if test "x$enable_scriptsupport" = xyes ; then
++        AC_DEFINE(IXML_HAVE_SCRIPTSUPPORT, 1, [see upnpconfig.h])
++fi
++
++RT_BOOL_ARG_ENABLE([postwrite], [no], [write to the filesystem on otherwise unhandled POST requests])
++if test "x$enable_postwrite" = xyes ; then
++        AC_DEFINE(UPNP_ENABLE_POST_WRITE, 1, [see upnpconfig.h])
++fi
+ 
+ RT_BOOL_ARG_ENABLE([samples], [yes], [compilation of upnp/sample/ code])
+ 
+diff --git a/upnp/inc/upnpconfig.h.in b/upnp/inc/upnpconfig.h.in
+index 7f4facc..5df8c5a 100644
+--- a/upnp/inc/upnpconfig.h.in
++++ b/upnp/inc/upnpconfig.h.in
+@@ -131,5 +131,14 @@
+  * header (i.e. configure --enable-unspecified_server) */
+ #undef UPNP_ENABLE_UNSPECIFIED_SERVER
+ 
++/** Defined to 1 if the library has been compiled with OpenSSL support
++ *  (i.e. configure --enable-open_ssl) */
++#undef UPNP_ENABLE_OPEN_SSL
++
++/** Defined to 1 if the library has been compiled to support filesystem writes on POST
++ *  (i.e. configure --enable-postwrite) */
++#undef UPNP_ENABLE_POST_WRITE
++
++
+ #endif /* UPNP_CONFIG_H */
+ 
+diff --git a/upnp/src/genlib/net/http/webserver.c b/upnp/src/genlib/net/http/webserver.c
+index 26bf0f7..7ae8c1e 100644
+--- a/upnp/src/genlib/net/http/webserver.c
++++ b/upnp/src/genlib/net/http/webserver.c
+@@ -1367,9 +1367,13 @@ static int http_RecvPostMessage(
+ 		if (Fp == NULL)
+ 			return HTTP_INTERNAL_SERVER_ERROR;
+ 	} else {
++#ifdef UPNP_ENABLE_POST_WRITE
+ 		Fp = fopen(filename, "wb");
+ 		if (Fp == NULL)
+ 			return HTTP_UNAUTHORIZED;
++#else
++		return HTTP_NOT_FOUND;
++#endif
+ 	}
+ 	parser->position = POS_ENTITY;
+ 	do {
-- 
2.10.1

Re: CVE-2016-6255

[Efraim Flashner]

[-- Attachment #1: Type: text/plain, Size: 1087 bytes --]

On Thu, Oct 06, 2016 at 02:16:26AM -0400, Leo Famulari wrote:
> Subject: [PATCH 0/1] libupnp remote filesystem access CVE-2016-6255
> 
> You can use libupnp on a remote server to read and write the filesystem
> with the privileges of the libupnp process:
> 
> http://seclists.org/oss-sec/2016/q3/102
> 
> This patch cherry-picks the upstream commit:
> 
> https://github.com/mrjimenez/pupnp/commit/d64d6a44906b5aa5306bdf1708531d698654dda5
> 
> Leo Famulari (1):
>   gnu: libupnp: Fix CVE-2016-6255.
> 
>  gnu/local.mk                                     |  1 +
>  gnu/packages/libupnp.scm                         |  2 +
>  gnu/packages/patches/libupnp-CVE-2016-6255.patch | 86 ++++++++++++++++++++++++
>  3 files changed, 89 insertions(+)
>  create mode 100644 gnu/packages/patches/libupnp-CVE-2016-6255.patch
> 
> -- 
> 2.10.1
> 

Looks good to me

-- 
Efraim Flashner   <efraim@flashner.co.il>   אפרים פלשנר
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 1/1] gnu: libupnp: Fix CVE-2016-6255.

[Ludovic Courtès]

Leo Famulari <leo@famulari.name> skribis:

> * gnu/packages/patches/libupnp-CVE-2016-6255.patch: New file.
> * gnu/local.mk (dist_patch_DATA): Add it.
> * gnu/packages/libupnp.scm (libupnp): Use it.

[...]

> +From d64d6a44906b5aa5306bdf1708531d698654dda5 Mon Sep 17 00:00:00 2001
> +From: Matthew Garrett <mjg59@srcf.ucam.org>
> +Date: Tue, 23 Feb 2016 13:53:20 -0800
> +Subject: [PATCH] Don't allow unhandled POSTs to write to the filesystem by
> + default
> +
> +If there's no registered handler for a POST request, the default behaviour
> +is to write it to the filesystem. Several million deployed devices appear
> +to have this behaviour, making it possible to (at least) store arbitrary
> +data on them. Add a configure option that enables this behaviour, and change
> +the default to just drop POSTs that aren't directly handled.

Fun.  :-)

> +diff --git a/configure.ac b/configure.ac
> +index 9548913..a8731b5 100644
> +--- a/configure.ac
> ++++ b/configure.ac

Shouldn’t it require an autoreconf phase?

I would suggest shrinking this patch to just:

> +--- a/upnp/src/genlib/net/http/webserver.c
> ++++ b/upnp/src/genlib/net/http/webserver.c
> +@@ -1367,9 +1367,13 @@ static int http_RecvPostMessage(
> + 		if (Fp == NULL)
> + 			return HTTP_INTERNAL_SERVER_ERROR;
> + 	} else {
> ++#ifdef UPNP_ENABLE_POST_WRITE
> + 		Fp = fopen(filename, "wb");
> + 		if (Fp == NULL)
> + 			return HTTP_UNAUTHORIZED;
> ++#else
> ++		return HTTP_NOT_FOUND;
> ++#endif

… with “#if 0” instead of “#ifdef UPNP_ENABLE_POST_WRITE”.

WDYT?

Feel free to commit adjusted as you see fit!

Thanks,
Ludo’.

Re: [PATCH 1/1] gnu: libupnp: Fix CVE-2016-6255.

[Leo Famulari]

On Thu, Oct 06, 2016 at 09:28:34PM +0200, Ludovic Courtès wrote:
> Leo Famulari <leo@famulari.name> skribis:
> > +From d64d6a44906b5aa5306bdf1708531d698654dda5 Mon Sep 17 00:00:00 2001
> > +From: Matthew Garrett <mjg59@srcf.ucam.org>
> > +Date: Tue, 23 Feb 2016 13:53:20 -0800
> > +Subject: [PATCH] Don't allow unhandled POSTs to write to the filesystem by
> > + default
> > +
> > +If there's no registered handler for a POST request, the default behaviour
> > +is to write it to the filesystem. Several million deployed devices appear
> > +to have this behaviour, making it possible to (at least) store arbitrary
> > +data on them. Add a configure option that enables this behaviour, and change
> > +the default to just drop POSTs that aren't directly handled.
> 
> Fun.  :-)

Tons!

> > +diff --git a/configure.ac b/configure.ac
> > +index 9548913..a8731b5 100644
> > +--- a/configure.ac
> > ++++ b/configure.ac
> 
> Shouldn’t it require an autoreconf phase?

Right, it would...

> I would suggest shrinking this patch to just:
> 
> > +--- a/upnp/src/genlib/net/http/webserver.c
> > ++++ b/upnp/src/genlib/net/http/webserver.c
> > +@@ -1367,9 +1367,13 @@ static int http_RecvPostMessage(
> > + 		if (Fp == NULL)
> > + 			return HTTP_INTERNAL_SERVER_ERROR;
> > + 	} else {
> > ++#ifdef UPNP_ENABLE_POST_WRITE
> > + 		Fp = fopen(filename, "wb");
> > + 		if (Fp == NULL)
> > + 			return HTTP_UNAUTHORIZED;
> > ++#else
> > ++		return HTTP_NOT_FOUND;
> > ++#endif
> 
> … with “#if 0” instead of “#ifdef UPNP_ENABLE_POST_WRITE”.
> 
> WDYT?

I agree. Let's disable it unconditionally for now. When upstream cuts a
new release, the conditional feature handling will make it into our
package.

Thanks for the careful review.

> Feel free to commit adjusted as you see fit!

Done as 9e672bcc0b61a007ea29858517b58896dc1b9449