Bug#885069: stretch-pu: package open-iscsi/2.0.874-3~deb9u1

[Adam D. Barratt]

On Fri, 2019-04-26 at 21:42 +0200, Salvatore Bonaccorso wrote:
> Hi Christian,
> 
> On Fri, Nov 09, 2018 at 06:53:07AM +0100, Salvatore Bonaccorso wrote:
> > Hi Christian,
> > 
> > On Sat, Feb 10, 2018 at 10:15:48AM +0100, Julien Cristau wrote:
> > > Control: tag -1 moreinfo
> > > 
> > > On Sat, Dec 23, 2017 at 13:40:43 +0100, Christian Seiler wrote:
[...]
> > > The above makes little sense to me.  We find out the peer uid,
> > > then
> > > instead of just comparing that against 0 we turn it into a struct
> > > passwd
> > > and compare pw_name against "root".  Why?
> > 
> > Did you had any chance to look at Julien's concerns/questions back
> > on
> > this proposed update for stretch?
> 
> Friendly ping :)
> 

If there's no follow-up by the time the 9.10 point release happens
(~2.5 weeks time) then I will close this request.

Regards,

Adam

Bug#885069: stretch-pu: package open-iscsi/2.0.874-3~deb9u1

[Salvatore Bonaccorso]

Hi Christian,

On Fri, Nov 09, 2018 at 06:53:07AM +0100, Salvatore Bonaccorso wrote:
> Hi Christian,
> 
> On Sat, Feb 10, 2018 at 10:15:48AM +0100, Julien Cristau wrote:
> > Control: tag -1 moreinfo
> > 
> > On Sat, Dec 23, 2017 at 13:40:43 +0100, Christian Seiler wrote:
> > 
> > > diff -Nru 
> > > open-iscsi-2.0.874/debian/patches/security/Check-for-root-peer-user-for-iscsiuio-IPC.patch
> > >  
> > > open-iscsi-2.0.874/debian/patches/security/Check-for-root-peer-user-for-iscsiuio-IPC.patch
> > > --- 
> > > open-iscsi-2.0.874/debian/patches/security/Check-for-root-peer-user-for-iscsiuio-IPC.patch
> > > 1970-01-01 01:00:00.0 +0100
> > > +++ 
> > > open-iscsi-2.0.874/debian/patches/security/Check-for-root-peer-user-for-iscsiuio-IPC.patch
> > > 2017-12-23 13:09:13.0 +0100
> > > @@ -0,0 +1,122 @@
> > > +From e313bd648a4c8a9526421e270eb597a5de1e0c7f Mon Sep 17 00:00:00 2001
> > > +From: Lee Duncan 
> > > +Date: Fri, 15 Dec 2017 10:36:11 -0800
> > > +Subject: [PATCH 1/8] Check for root peer user for iscsiuio IPC
> > > +
> > > +This fixes a possible vulnerability where a non-root
> > > +process could connect with iscsiuio. Fouund by Qualsys.
> > > +---
> > > + iscsiuio/src/unix/Makefile.am  |  3 ++-
> > > + iscsiuio/src/unix/iscsid_ipc.c | 47 
> > > ++
> > > + 2 files changed, 49 insertions(+), 1 deletion(-)
> > > +
> > [...]
> > > +@@ -1029,6 +1035,40 @@ static void iscsid_loop_close(void *arg)
> > > + LOG_INFO(PFX "iSCSI daemon socket closed");
> > > + }
> > > + 
> > > ++/*
> > > ++ * check that the peer user is privilidged
> > > ++ *
> > 
> > This function doesn't actually do that.
> > 
> > > ++ * return 1 if peer is ok else 0
> > > ++ *
> > > ++ * XXX: this function is copied from iscsid_ipc.c and should be
> > > ++ * moved into a common library
> > > ++ */
> > > ++static int
> > > ++mgmt_peeruser(int sock, char *user)
> > > ++{
> > > ++struct ucred peercred;
> > > ++socklen_t so_len = sizeof(peercred);
> > > ++struct passwd *pass;
> > > ++
> > > ++errno = 0;
> > > ++if (getsockopt(sock, SOL_SOCKET, SO_PEERCRED, &peercred,
> > > ++&so_len) != 0 || so_len != sizeof(peercred)) {
> > > ++/* We didn't get a valid credentials struct. */
> > > ++LOG_ERR(PFX "peeruser_unux: error receiving 
> > > credentials: %m");
> > > ++return 0;
> > > ++}
> > > ++
> > > ++pass = getpwuid(peercred.uid);
> > > ++if (pass == NULL) {
> > > ++LOG_ERR(PFX "peeruser_unix: unknown local user with uid 
> > > %d",
> > > ++(int) peercred.uid);
> > > ++return 0;
> > > ++}
> > > ++
> > > ++strlcpy(user, pass->pw_name, PEERUSER_MAX);
> > > ++return 1;
> > > ++}
> > > ++
> > > + /**
> > > +  *  iscsid_loop() - This is the function which will process the 
> > > broadcast
> > > +  *  messages from iscsid
> > > +@@ -1038,6 +1078,7 @@ static void *iscsid_loop(void *arg)
> > > + {
> > > + int rc;
> > > + sigset_t set;
> > > ++char user[PEERUSER_MAX];
> > > + 
> > > + pthread_cleanup_push(iscsid_loop_close, arg);
> > > + 
> > > +@@ -1077,6 +1118,12 @@ static void *iscsid_loop(void *arg)
> > > + continue;
> > > + }
> > > + 
> > > ++if (!mgmt_peeruser(iscsid_opts.fd, user) || 
> > > strncmp(user, "root", PEERUSER_MAX)) {
> > > ++close(s2);
> > > ++LOG_ERR(PFX "Access error: non-administrative 
> > > connection rejected");
> > > ++break;
> > > ++}
> > > ++
> > > + process_iscsid_broadcast(s2);
> > > + close(s2);
> > > + }
> > 
> > The above makes little sense to me.  We find out the peer uid, then
> > instead of just comparing that against 0 we turn it into a struct passwd
> > and compare pw_name against "root".  Why?
> 
> Did you had any chance to look at Julien's concerns/questions back on
> this proposed update for stretch?

Friendly ping :)

Regards,
Salvatore

Bug#885069: stretch-pu: package open-iscsi/2.0.874-3~deb9u1

[Salvatore Bonaccorso]

Hi Christian,

On Sat, Feb 10, 2018 at 10:15:48AM +0100, Julien Cristau wrote:
> Control: tag -1 moreinfo
> 
> On Sat, Dec 23, 2017 at 13:40:43 +0100, Christian Seiler wrote:
> 
> > diff -Nru 
> > open-iscsi-2.0.874/debian/patches/security/Check-for-root-peer-user-for-iscsiuio-IPC.patch
> >  
> > open-iscsi-2.0.874/debian/patches/security/Check-for-root-peer-user-for-iscsiuio-IPC.patch
> > --- 
> > open-iscsi-2.0.874/debian/patches/security/Check-for-root-peer-user-for-iscsiuio-IPC.patch
> >   1970-01-01 01:00:00.0 +0100
> > +++ 
> > open-iscsi-2.0.874/debian/patches/security/Check-for-root-peer-user-for-iscsiuio-IPC.patch
> >   2017-12-23 13:09:13.0 +0100
> > @@ -0,0 +1,122 @@
> > +From e313bd648a4c8a9526421e270eb597a5de1e0c7f Mon Sep 17 00:00:00 2001
> > +From: Lee Duncan 
> > +Date: Fri, 15 Dec 2017 10:36:11 -0800
> > +Subject: [PATCH 1/8] Check for root peer user for iscsiuio IPC
> > +
> > +This fixes a possible vulnerability where a non-root
> > +process could connect with iscsiuio. Fouund by Qualsys.
> > +---
> > + iscsiuio/src/unix/Makefile.am  |  3 ++-
> > + iscsiuio/src/unix/iscsid_ipc.c | 47 
> > ++
> > + 2 files changed, 49 insertions(+), 1 deletion(-)
> > +
> [...]
> > +@@ -1029,6 +1035,40 @@ static void iscsid_loop_close(void *arg)
> > +   LOG_INFO(PFX "iSCSI daemon socket closed");
> > + }
> > + 
> > ++/*
> > ++ * check that the peer user is privilidged
> > ++ *
> 
> This function doesn't actually do that.
> 
> > ++ * return 1 if peer is ok else 0
> > ++ *
> > ++ * XXX: this function is copied from iscsid_ipc.c and should be
> > ++ * moved into a common library
> > ++ */
> > ++static int
> > ++mgmt_peeruser(int sock, char *user)
> > ++{
> > ++  struct ucred peercred;
> > ++  socklen_t so_len = sizeof(peercred);
> > ++  struct passwd *pass;
> > ++
> > ++  errno = 0;
> > ++  if (getsockopt(sock, SOL_SOCKET, SO_PEERCRED, &peercred,
> > ++  &so_len) != 0 || so_len != sizeof(peercred)) {
> > ++  /* We didn't get a valid credentials struct. */
> > ++  LOG_ERR(PFX "peeruser_unux: error receiving credentials: %m");
> > ++  return 0;
> > ++  }
> > ++
> > ++  pass = getpwuid(peercred.uid);
> > ++  if (pass == NULL) {
> > ++  LOG_ERR(PFX "peeruser_unix: unknown local user with uid %d",
> > ++  (int) peercred.uid);
> > ++  return 0;
> > ++  }
> > ++
> > ++  strlcpy(user, pass->pw_name, PEERUSER_MAX);
> > ++  return 1;
> > ++}
> > ++
> > + /**
> > +  *  iscsid_loop() - This is the function which will process the broadcast
> > +  *  messages from iscsid
> > +@@ -1038,6 +1078,7 @@ static void *iscsid_loop(void *arg)
> > + {
> > +   int rc;
> > +   sigset_t set;
> > ++  char user[PEERUSER_MAX];
> > + 
> > +   pthread_cleanup_push(iscsid_loop_close, arg);
> > + 
> > +@@ -1077,6 +1118,12 @@ static void *iscsid_loop(void *arg)
> > +   continue;
> > +   }
> > + 
> > ++  if (!mgmt_peeruser(iscsid_opts.fd, user) || strncmp(user, 
> > "root", PEERUSER_MAX)) {
> > ++  close(s2);
> > ++  LOG_ERR(PFX "Access error: non-administrative 
> > connection rejected");
> > ++  break;
> > ++  }
> > ++
> > +   process_iscsid_broadcast(s2);
> > +   close(s2);
> > +   }
> 
> The above makes little sense to me.  We find out the peer uid, then
> instead of just comparing that against 0 we turn it into a struct passwd
> and compare pw_name against "root".  Why?

Did you had any chance to look at Julien's concerns/questions back on
this proposed update for stretch?

Regards,
Salvatore

Bug#885069: stretch-pu: package open-iscsi/2.0.874-3~deb9u1

[Julien Cristau]

Control: tag -1 moreinfo

On Sat, Dec 23, 2017 at 13:40:43 +0100, Christian Seiler wrote:

> diff -Nru 
> open-iscsi-2.0.874/debian/patches/security/Check-for-root-peer-user-for-iscsiuio-IPC.patch
>  
> open-iscsi-2.0.874/debian/patches/security/Check-for-root-peer-user-for-iscsiuio-IPC.patch
> --- 
> open-iscsi-2.0.874/debian/patches/security/Check-for-root-peer-user-for-iscsiuio-IPC.patch
> 1970-01-01 01:00:00.0 +0100
> +++ 
> open-iscsi-2.0.874/debian/patches/security/Check-for-root-peer-user-for-iscsiuio-IPC.patch
> 2017-12-23 13:09:13.0 +0100
> @@ -0,0 +1,122 @@
> +From e313bd648a4c8a9526421e270eb597a5de1e0c7f Mon Sep 17 00:00:00 2001
> +From: Lee Duncan 
> +Date: Fri, 15 Dec 2017 10:36:11 -0800
> +Subject: [PATCH 1/8] Check for root peer user for iscsiuio IPC
> +
> +This fixes a possible vulnerability where a non-root
> +process could connect with iscsiuio. Fouund by Qualsys.
> +---
> + iscsiuio/src/unix/Makefile.am  |  3 ++-
> + iscsiuio/src/unix/iscsid_ipc.c | 47 
> ++
> + 2 files changed, 49 insertions(+), 1 deletion(-)
> +
[...]
> +@@ -1029,6 +1035,40 @@ static void iscsid_loop_close(void *arg)
> + LOG_INFO(PFX "iSCSI daemon socket closed");
> + }
> + 
> ++/*
> ++ * check that the peer user is privilidged
> ++ *

This function doesn't actually do that.

> ++ * return 1 if peer is ok else 0
> ++ *
> ++ * XXX: this function is copied from iscsid_ipc.c and should be
> ++ * moved into a common library
> ++ */
> ++static int
> ++mgmt_peeruser(int sock, char *user)
> ++{
> ++struct ucred peercred;
> ++socklen_t so_len = sizeof(peercred);
> ++struct passwd *pass;
> ++
> ++errno = 0;
> ++if (getsockopt(sock, SOL_SOCKET, SO_PEERCRED, &peercred,
> ++&so_len) != 0 || so_len != sizeof(peercred)) {
> ++/* We didn't get a valid credentials struct. */
> ++LOG_ERR(PFX "peeruser_unux: error receiving credentials: %m");
> ++return 0;
> ++}
> ++
> ++pass = getpwuid(peercred.uid);
> ++if (pass == NULL) {
> ++LOG_ERR(PFX "peeruser_unix: unknown local user with uid %d",
> ++(int) peercred.uid);
> ++return 0;
> ++}
> ++
> ++strlcpy(user, pass->pw_name, PEERUSER_MAX);
> ++return 1;
> ++}
> ++
> + /**
> +  *  iscsid_loop() - This is the function which will process the broadcast
> +  *  messages from iscsid
> +@@ -1038,6 +1078,7 @@ static void *iscsid_loop(void *arg)
> + {
> + int rc;
> + sigset_t set;
> ++char user[PEERUSER_MAX];
> + 
> + pthread_cleanup_push(iscsid_loop_close, arg);
> + 
> +@@ -1077,6 +1118,12 @@ static void *iscsid_loop(void *arg)
> + continue;
> + }
> + 
> ++if (!mgmt_peeruser(iscsid_opts.fd, user) || strncmp(user, 
> "root", PEERUSER_MAX)) {
> ++close(s2);
> ++LOG_ERR(PFX "Access error: non-administrative 
> connection rejected");
> ++break;
> ++}
> ++
> + process_iscsid_broadcast(s2);
> + close(s2);
> + }

The above makes little sense to me.  We find out the peer uid, then
instead of just comparing that against 0 we turn it into a struct passwd
and compare pw_name against "root".  Why?

Cheers,
Julien

Bug#885069: stretch-pu: package open-iscsi/2.0.874-3~deb9u1

[Cyril Brulebois]

Hi Christian,

Christian Seiler  (2017-12-23):
> I'd like to ask for a stable update for open-iscsi in Stretch to fix
> #885021, which was marked no-DSA by the security team. I've CC'd
> debian-boot@ and KiBi as the package builds udebs (though the change
> itself does not affect udebs, as 'iscsiuio' is not included in d-i).

Accordingly: no objections, and sorry for the late reply.


Cheers,
-- 
Cyril Brulebois (k...@debian.org)
D-I release manager -- Release team member -- Freelance Consultant


signature.asc
Description: PGP signature

Bug#885069: stretch-pu: package open-iscsi/2.0.874-3~deb9u1

[Christian Seiler]

Package: release.debian.org
Severity: normal
Tags: stretch
User: release.debian@packages.debian.org
Usertags: pu
X-Debbugs-Cc: debian-b...@lists.debian.org, k...@debian.org

Dear release team,

I'd like to ask for a stable update for open-iscsi in Stretch to fix
#885021, which was marked no-DSA by the security team. I've CC'd
debian-boot@ and KiBi as the package builds udebs (though the change
itself does not affect udebs, as 'iscsiuio' is not included in d-i).

I've fixed the bug in unstable in 2.0.874-5.

debdiff against stretch is attached.

I've built and tested the package in Stretch itself. Note, however,
that the affected program, iscsiuio, is there to enable support for
special hardware that neither I nor Ritesh have access to. The patches
themselves come directly from upstream, and a review appears to
indicate they are sane, but - in the interest of full transparency - I
cannot guarantee 100% that they don't introduce a regression. If you'd
like to wait and see if sid/testing users report regressions before
accepting this p-u I'd understand, though I would like to note that
open-iscsi is one of those packages where next to no users actually use
it outside of stable itself, so most reports we get are actually from
users of stable, and not testing/sid.

The 'jessie' package doesn't build iscsiuio though it contains the
source for it, and even if people built iscsiuio from Jessie's source
directly, that was broken in a lot of other ways (which is why it was
disabled again before the release of Jessie and only properly enabled
in Stretch), so I don't think it makes sense to update Jessie.

Regards,
Christian

-- System Information:
Debian Release: 9.3
  APT prefers stable-updates
  APT policy: (500, 'stable-updates'), (500, 'stable-debug'), (500, 'stable')
Architecture: amd64 (x86_64)
Foreign Architectures: i386

Kernel: Linux 4.9.0-4-amd64 (SMP w/4 CPU cores)
Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8), 
LANGUAGE=en_US:en (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
Init: systemd (via /run/systemd/system)
diff -Nru open-iscsi-2.0.874/debian/changelog 
open-iscsi-2.0.874/debian/changelog
--- open-iscsi-2.0.874/debian/changelog 2017-06-18 23:06:16.0 +0200
+++ open-iscsi-2.0.874/debian/changelog 2017-12-23 13:09:13.0 +0100
@@ -1,3 +1,10 @@
+open-iscsi (2.0.874-3~deb9u2) stretch; urgency=high
+
+  * [4b930f8] Fix multiple security issues in iscsiuio. (CVE-2017-17840)
+(Closes: #885021)
+
+ -- Christian Seiler   Sat, 23 Dec 2017 13:09:13 +0100
+
 open-iscsi (2.0.874-3~deb9u1) stretch; urgency=medium
 
   * [8de3092] udeb: don't update initramfs when iSCSI is not used.
diff -Nru 
open-iscsi-2.0.874/debian/patches/security/Check-for-root-peer-user-for-iscsiuio-IPC.patch
 
open-iscsi-2.0.874/debian/patches/security/Check-for-root-peer-user-for-iscsiuio-IPC.patch
--- 
open-iscsi-2.0.874/debian/patches/security/Check-for-root-peer-user-for-iscsiuio-IPC.patch
  1970-01-01 01:00:00.0 +0100
+++ 
open-iscsi-2.0.874/debian/patches/security/Check-for-root-peer-user-for-iscsiuio-IPC.patch
  2017-12-23 13:09:13.0 +0100
@@ -0,0 +1,122 @@
+From e313bd648a4c8a9526421e270eb597a5de1e0c7f Mon Sep 17 00:00:00 2001
+From: Lee Duncan 
+Date: Fri, 15 Dec 2017 10:36:11 -0800
+Subject: [PATCH 1/8] Check for root peer user for iscsiuio IPC
+
+This fixes a possible vulnerability where a non-root
+process could connect with iscsiuio. Fouund by Qualsys.
+---
+ iscsiuio/src/unix/Makefile.am  |  3 ++-
+ iscsiuio/src/unix/iscsid_ipc.c | 47 ++
+ 2 files changed, 49 insertions(+), 1 deletion(-)
+
+--- a/iscsiuio/src/unix/Makefile.am
 b/iscsiuio/src/unix/Makefile.am
+@@ -20,7 +20,8 @@ iscsiuio_SOURCES =   build_date.c\
+   nic_utils.c \
+   packet.c\
+   iscsid_ipc.c\
+-  ping.c
++  ping.c  \
++  ${top_srcdir}/../utils/sysdeps/sysdeps.c
+ 
+ iscsiuio_CFLAGS = $(AM_CFLAGS)\
+   $(LIBNL_CFLAGS) \
+--- a/iscsiuio/src/unix/iscsid_ipc.c
 b/iscsiuio/src/unix/iscsid_ipc.c
+@@ -37,6 +37,8 @@
+  *
+  */
+ 
++#define _GNU_SOURCE
++
+ #include 
+ #include 
+ #include 
+@@ -47,6 +49,8 @@
+ #include 
+ #include 
+ #include 
++#include 
++#include 
+ 
+ #define PFX "iscsi_ipc "
+ 
+@@ -61,6 +65,7 @@
+ #include "iscsid_ipc.h"
+ #include "uip.h"
+ #include "uip_mgmt_ipc.h"
++#include "sysdeps.h"
+ 
+ #include "logger.h"
+ #include "uip.h"
+@@ -102,6 +107,7 @@ struct iface_rec_decode {
+   uint16_tmtu;
+ };
+ 
++#define PEERUSER_MAX  64
+ 
+ 
/**
+  *  iscsid_ipc Constants
+@@ -1029,6 +1035,40 @@ static void iscsid_loop_close(void *arg)
+   LOG_INFO(PFX "iSCSI daemon socket closed");
+ }
+ 
++/*
++ * check that the peer