Bug#863897: sudo: Further issue in parsing /proc/[pid]/stat when process name contains newline

[Antoine Beaupre]

On Mon, Jun 05, 2017 at 06:32:11AM +0200, Salvatore Bonaccorso wrote:
> Hi!
> 
> On Sun, Jun 04, 2017 at 08:35:05PM +0200, Salvatore Bonaccorso wrote:
> > Hi Bdale
> > 
> > Since time is pressing a bit for the release of stretch, any problem
> > in if I would prepare a NMU for both stretch (targetted) and sid for
> > this issue?
> 
> Attached attempt/proposed debdiff for stretch.

I forgot to link to this bug report in the LTS update changelog, but the
patch there may be useful for the jessie update here as well. The goods
are in 1.8.5p2-1+nmu3+deb7u4 which I just uploaded to wheezy and I
attached the backported patch here.

The code path modified by the upstream patch somewhat changed a lot
between wheezy and stretch, so much so that there's an extra loop around
the patched code. I painstakingly backported this and hopefully that
will be useful for the sudo point release update.

Thank you for your attention,

A.



# HG changeset patch
# User Todd C. Miller 
# Date 1496243671 21600
# Node ID 15a46f4007dde8e819dd2c70e670a529bbb9d312
# Parent  6f3d9816541ba84055ae5aec6ff9d9523c2a96f3
A command name may also contain newline characters so read
/proc/self/stat until EOF.  It is not legal for /proc/self/stat to
contain embedded NUL bytes so treat the file as corrupt if we see
any.  With help from Qualys.

This is not exploitable due to the /dev traversal changes in sudo
1.8.20p1 (thanks Solar!).

--- a/src/ttyname.c
+++ b/src/ttyname.c
@@ -419,29 +419,39 @@ get_process_ttyname(void)
 char *
 get_process_ttyname(void)
 {
-char *line = NULL, *tty = NULL;
-size_t linesize = 0;
-ssize_t len;
+char *tty = NULL;
+char *cp, buf[1024];
+ssize_t nread;
 int i;
 debug_decl(get_process_ttyname, SUDO_DEBUG_UTIL)
 
 /* Try to determine the tty from pr_ttydev in /proc/pid/psinfo. */
 for (i = 0; tty == NULL && i < 2; i++) {
-	FILE *fp;
+int fd;
 	char path[PATH_MAX];
 	(void)snprintf(path, sizeof(path), "/proc/%u/stat",
 	i ? (unsigned int)getppid() : (unsigned int)getpid());
-	if ((fp = fopen(path, "r")) == NULL)
+	if ((fd = open(path, O_RDONLY | O_NOFOLLOW)) == -1)
 	continue;
-	len = getline(, , fp);
-	fclose(fp);
-	if (len != -1) {
+	cp = buf;
+	while ((nread = read(fd, cp, buf + sizeof(buf) - cp)) != 0) {
+	if (nread == -1) {
+		if (errno == EAGAIN || errno == EINTR)
+		continue;
+		break;
+	}
+	cp += nread;
+	if (cp >= buf + sizeof(buf))
+		break;
+	}
+	if (nread == 0 && memchr(buf, '\0', cp - buf) == NULL) {
 	/*
 	 * Field 7 is the tty dev (0 if no tty).
-	 * Since the process name at field 2 "(comm)" may include spaces,
-	 * start at the last ')' found.
+	 * Since the process name at field 2 "(comm)" may include
+	 * whitespace (including newlines), start at the last ')' found.
 	 */
-	char *cp = strrchr(line, ')');
+*cp = '\0';
+cp = strrchr(buf, ')');
 	if (cp != NULL) {
 		char *ep = cp;
 		int field = 1;
@@ -460,8 +470,9 @@ get_process_ttyname(void)
 		}
 	}
 	}
+if (fd != -1)
+  close(fd);
 }
-efree(line);
 
 /* If all else fails, fall back on ttyname(). */
 if (tty == NULL) {


signature.asc
Description: PGP signature

Bug#863897: sudo: Further issue in parsing /proc/[pid]/stat when process name contains newline

[Salvatore Bonaccorso]

Hi!

On Sun, Jun 04, 2017 at 08:35:05PM +0200, Salvatore Bonaccorso wrote:
> Hi Bdale
> 
> Since time is pressing a bit for the release of stretch, any problem
> in if I would prepare a NMU for both stretch (targetted) and sid for
> this issue?

Attached attempt/proposed debdiff for stretch.

Regards,
Salvatore
diff -Nru sudo-1.8.19p1/debian/changelog sudo-1.8.19p1/debian/changelog
--- sudo-1.8.19p1/debian/changelog  2017-05-31 06:35:01.0 +0200
+++ sudo-1.8.19p1/debian/changelog  2017-06-05 06:19:37.0 +0200
@@ -1,3 +1,10 @@
+sudo (1.8.19p1-2.1) stretch; urgency=high
+
+  * Non-maintainer upload.
+  * CVE-2017-1000368: Arbitrary terminal access (Closes: #863897)
+
+ -- Salvatore Bonaccorso   Mon, 05 Jun 2017 06:19:37 +0200
+
 sudo (1.8.19p1-2) stretch; urgency=high
 
   * patch from upstream to fix CVE-2017-1000367, closes: #863731
diff -Nru sudo-1.8.19p1/debian/patches/CVE-2017-1000368.patch 
sudo-1.8.19p1/debian/patches/CVE-2017-1000368.patch
--- sudo-1.8.19p1/debian/patches/CVE-2017-1000368.patch 1970-01-01 
01:00:00.0 +0100
+++ sudo-1.8.19p1/debian/patches/CVE-2017-1000368.patch 2017-06-05 
06:19:37.0 +0200
@@ -0,0 +1,78 @@
+
+# HG changeset patch
+# User Todd C. Miller 
+# Date 1496243671 21600
+# Node ID 15a46f4007dde8e819dd2c70e670a529bbb9d312
+# Parent  6f3d9816541ba84055ae5aec6ff9d9523c2a96f3
+A command name may also contain newline characters so read
+/proc/self/stat until EOF.  It is not legal for /proc/self/stat to
+contain embedded NUL bytes so treat the file as corrupt if we see
+any.  With help from Qualys.
+
+This is not exploitable due to the /dev traversal changes in sudo
+1.8.20p1 (thanks Solar!).
+
+--- a/src/ttyname.c
 b/src/ttyname.c
+@@ -447,26 +447,39 @@ done:
+ char *
+ get_process_ttyname(char *name, size_t namelen)
+ {
+-char path[PATH_MAX], *line = NULL;
++char path[PATH_MAX];
++char *cp, buf[1024];
+ char *ret = NULL;
+-size_t linesize = 0;
+ int serrno = errno;
+-ssize_t len;
+-FILE *fp;
++ssize_t nread;
++int fd;
+ debug_decl(get_process_ttyname, SUDO_DEBUG_UTIL)
+ 
+-/* Try to determine the tty from tty_nr in /proc/pid/stat. */
++/*
++ * Try to determine the tty from tty_nr in /proc/pid/stat.
++ * Ignore /proc/self/stat if it contains embedded NUL bytes.
++ */
+ snprintf(path, sizeof(path), "/proc/%u/stat", (unsigned int)getpid());
+-if ((fp = fopen(path, "r")) != NULL) {
+-  len = getline(, , fp);
+-  fclose(fp);
+-  if (len != -1) {
++if ((fd = open(path, O_RDONLY | O_NOFOLLOW)) != -1) {
++cp = buf;
++while ((nread = read(fd, cp, buf + sizeof(buf) - cp)) != 0) {
++if (nread == -1) {
++if (errno == EAGAIN || errno == EINTR)
++continue;
++break;
++}
++cp += nread;
++if (cp >= buf + sizeof(buf))
++break;
++}
++if (nread == 0 && memchr(buf, '\0', cp - buf) == NULL) {
+   /*
+* Field 7 is the tty dev (0 if no tty).
+-   * Since the process name at field 2 "(comm)" may include spaces,
+-   * start at the last ')' found.
++   * Since the process name at field 2 "(comm)" may include
++   * whitespace (including newlines), start at the last ')' found.
+*/
+-  char *cp = strrchr(line, ')');
++*cp = '\0';
++cp = strrchr(buf, ')');
+   if (cp != NULL) {
+   char *ep = cp;
+   const char *errstr;
+@@ -497,7 +510,8 @@ get_process_ttyname(char *name, size_t n
+ errno = ENOENT;
+ 
+ done:
+-free(line);
++if (fd != -1)
++  close(fd);
+ if (ret == NULL)
+   sudo_debug_printf(SUDO_DEBUG_WARN|SUDO_DEBUG_LINENO|SUDO_DEBUG_ERRNO,
+   "unable to resolve tty via %s", path);
diff -Nru sudo-1.8.19p1/debian/patches/series 
sudo-1.8.19p1/debian/patches/series
--- sudo-1.8.19p1/debian/patches/series 2017-05-31 06:35:01.0 +0200
+++ sudo-1.8.19p1/debian/patches/series 2017-06-05 06:19:37.0 +0200
@@ -1,3 +1,4 @@
 typo-in-classic-insults.diff
 paths-in-samples.diff
 CVE-2017-1000367.patch
+CVE-2017-1000368.patch

Bug#863897: sudo: Further issue in parsing /proc/[pid]/stat when process name contains newline

[Salvatore Bonaccorso]

Hi

Correct commit is obviously not the one posted, but rather

https://www.sudo.ws/repos/sudo/raw-rev/15a46f4007dd

Regards,
Salvatore

Bug#863897: sudo: Further issue in parsing /proc/[pid]/stat when process name contains newline

[Salvatore Bonaccorso]

Hi Bdale

Since time is pressing a bit for the release of stretch, any problem
in if I would prepare a NMU for both stretch (targetted) and sid for
this issue?

Regards,
Salvatore

Processed: Re: Bug#863897: sudo: Further issue in parsing /proc/[pid]/stat when process name contains newline

[Debian Bug Tracking System]

Processing control commands:

> severity -1 grave
Bug #863897 [src:sudo] sudo: Further issue in parsing /proc/[pid]/stat when 
process name contains newline
Severity set to 'grave' from 'important'

-- 
863897: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=863897
Debian Bug Tracking System
Contact ow...@bugs.debian.org with problems