Bug#895268: libtickit: Fix out-of-bounds write with FD_SET()

[James McCoy]

On Mon, Apr 09, 2018 at 12:21:32AM -0700, Steve Langasek wrote:
> Hi James,

Hey,

> You filed https://bugs.launchpad.net/libtickit/+bug/1744933 about tests
> reporting a buffer overflow in libtickit.  It seems you worked around this
> by disabling the hardening flags

Yes, I was hoping to get time to look into _why_ the -1 was being
returned in that scenario, since that should likely be fixed.

> - or at least attempting to, which was
> ineffective in Ubuntu because -D_FORTIFY_SOURCE=2 is a compiler built-in in
> Ubuntu; which is how I noticed this, because the package still failed to
> build in Ubuntu.

Good to know.

> I dug into the build failure, and this looks like a genuine out-of-bounds
> write in the use of FD_SET() in src/term.c (i.e. the source, not the
> tests).  An attacker can likely only cause the fd to be set to -1 rather
> than to an arbitrary value, so it's not necessarily exploitable, but the
> code does currently allow for scribbling into memory where it shouldn't, so
> that's not good.

Thanks.  I'll send the patch upstream, since the defensive measures are
useful.  I'll also see if Paul has some time to look into the root
cause.

Cheers,
-- 
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7  2D23 DFE6 91AE 331B A3DB

Bug#895268: libtickit: Fix out-of-bounds write with FD_SET()

[Steve Langasek]

Package: libtickit
Version: 0.2-2
Severity: important
Tags: patch
User: ubuntu-de...@lists.ubuntu.com
Usertags: origin-ubuntu bionic ubuntu-patch

Hi James,

You filed https://bugs.launchpad.net/libtickit/+bug/1744933 about tests
reporting a buffer overflow in libtickit.  It seems you worked around this
by disabling the hardening flags - or at least attempting to, which was
ineffective in Ubuntu because -D_FORTIFY_SOURCE=2 is a compiler built-in in
Ubuntu; which is how I noticed this, because the package still failed to
build in Ubuntu.

I dug into the build failure, and this looks like a genuine out-of-bounds
write in the use of FD_SET() in src/term.c (i.e. the source, not the
tests).  An attacker can likely only cause the fd to be set to -1 rather
than to an arbitrary value, so it's not necessarily exploitable, but the
code does currently allow for scribbling into memory where it shouldn't, so
that's not good.

I've pushed the attached patch to Ubuntu, which fixes the build failure and
also drops the override of -fortify from the hardening flags.

Hope that helps,
-- 
Steve Langasek   Give me a lever long enough and a Free OS
Debian Developer   to set it on, and I can move the world.
Ubuntu Developerhttp://www.debian.org/
slanga...@ubuntu.com vor...@debian.org
diff -Nru libtickit-0.2/debian/patches/cflags-for-tests.patch 
libtickit-0.2/debian/patches/cflags-for-tests.patch
--- libtickit-0.2/debian/patches/cflags-for-tests.patch 1969-12-31 
16:00:00.0 -0800
+++ libtickit-0.2/debian/patches/cflags-for-tests.patch 2018-04-08 
22:17:54.0 -0700
@@ -0,0 +1,20 @@
+Description: include configured compiler flags when building test cases
+ The test suite fails on Ubuntu because the default buildflags there cause
+ a _FORTIFY_SOURCE failure.  Pass $CFLAGS and $LDFLAGS when building the
+ test cases, to correct this flag mismatch.
+Author: Steve Langasek 
+Last-Modified: 2018-04-08 
+
+Index: libtickit-0.2/Makefile
+===
+--- libtickit-0.2.orig/Makefile
 libtickit-0.2/Makefile
+@@ -73,7 +73,7 @@
+   perl $^ > $@
+ 
+ t/%.t: t/%.c $(LIBRARY) t/taplib.lo t/mockterm.lo t/taplib-tickit.lo
+-  $(LIBTOOL) --mode=link --tag=CC gcc -o $@ -Iinclude -std=c99 -ggdb $^
++  $(LIBTOOL) --mode=link --tag=CC gcc $(CFLAGS) $(LDFLAGS) -o $@ 
-Iinclude -std=c99 -ggdb $^
+ 
+ t/%.lo: t/%.c
+   $(LIBTOOL) --mode=compile --tag=CC gcc $(CFLAGS) -o $@ -c $^
diff -Nru libtickit-0.2/debian/patches/fix-buffer-overflow.patch 
libtickit-0.2/debian/patches/fix-buffer-overflow.patch
--- libtickit-0.2/debian/patches/fix-buffer-overflow.patch  1969-12-31 
16:00:00.0 -0800
+++ libtickit-0.2/debian/patches/fix-buffer-overflow.patch  2018-04-09 
00:07:35.0 -0700
@@ -0,0 +1,21 @@
+Description: fix out-of-bounds write with fd_set[-1]
+ If termkey_get_fd() fails, we shouldn't try to call select for an fd of -1.
+ Just return instead of crashing on undefined behavior.
+Author: Steve Langasek 
+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1744933
+Last-Modified: 2018-04-09
+
+Index: libtickit-0.2/src/term.c
+===
+--- libtickit-0.2.orig/src/term.c
 libtickit-0.2/src/term.c
+@@ -679,7 +679,9 @@
+   }
+ 
+   fd_set readfds;
++  FD_ZERO();
+   int fd = termkey_get_fd(tk);
++  if (fd < 0 || fd >= FD_SETSIZE) return;
+   FD_SET(fd, );
+   int ret = select(fd + 1, , NULL, NULL, msec > -1 ?  : NULL);
+ 
diff -Nru libtickit-0.2/debian/patches/series 
libtickit-0.2/debian/patches/series
--- libtickit-0.2/debian/patches/series 1969-12-31 16:00:00.0 -0800
+++ libtickit-0.2/debian/patches/series 2018-04-08 23:56:27.0 -0700
@@ -0,0 +1,2 @@
+cflags-for-tests.patch
+fix-buffer-overflow.patch
diff -Nru libtickit-0.2/debian/rules libtickit-0.2/debian/rules
--- libtickit-0.2/debian/rules  2018-02-05 05:06:30.0 -0800
+++ libtickit-0.2/debian/rules  2018-04-09 00:07:43.0 -0700
@@ -1,7 +1,7 @@
 #!/usr/bin/make -f
 # -*- makefile -*-
 
-export DEB_BUILD_MAINT_OPTIONS=hardening=+all,-fortify
+export DEB_BUILD_MAINT_OPTIONS=hardening=+all
 
 include /usr/share/dpkg/default.mk