Bug 1324759 (CVE-2016-4973) - CVE-2016-4973 gcc: Targets using libssp for SSP are missing -D_FORTIFY_SOURCE functionality
Summary: CVE-2016-4973 gcc: Targets using libssp for SSP are missing -D_FORTIFY_SOURCE...
Keywords:
Status: CLOSED WONTFIX
Alias: CVE-2016-4973
Product: Security Response
Classification: Other
Component: vulnerability
Version: unspecified
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Red Hat Product Security
QA Contact:
URL:
Whiteboard:
Depends On:
Blocks: 1324762
TreeView+ depends on / blocked
 
Reported: 2016-04-07 08:33 UTC by Adam Mariš
Modified: 2021-02-17 04:06 UTC (History)
30 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
It was found that targets using gcc's libssp library for Stack Smashing Protection (among others: Cygwin, MinGW, newlib, RTEMS; but not Glibc, Bionic, NetBSD which provide SSP in libc), are missing the Object Size Checking feature, even when explicitly requested with _FORTIFY_SOURCE. Vulnerable binaries compiled against such targets do not benefit of such protection, increasing the chances of success of a buffer overflow attack.
Clone Of:
Environment:
Last Closed: 2017-08-07 08:47:40 UTC
Embargoed:


Attachments (Terms of Use)
Patch for gcc-5.3.0 (5.79 KB, patch)
2016-04-07 13:50 UTC, Yaakov Selkowitz
no flags Details | Diff
Patch for gcc svn trunk (5.28 KB, patch)
2016-04-07 13:50 UTC, Yaakov Selkowitz
no flags Details | Diff
Link with -lssp when -D_FORTIFY_SOURCE=* is in use (needs testing) (377 bytes, patch)
2016-04-10 23:45 UTC, Yaakov Selkowitz
no flags Details | Diff
Cumulative patch for gcc svn trunk (9.70 KB, patch)
2016-06-07 02:51 UTC, Yaakov Selkowitz
no flags Details | Diff


Links
System ID Private Priority Status Summary Last Updated
GNU Compiler Collection 77409 0 None None None 2016-08-29 22:05:54 UTC

Description Adam Mariš 2016-04-07 08:33:52 UTC
It was reported that targets that use libssp for SSP (e.g. newlib, Cygwin, RTEMS, MinGW, but not e.g. Glibc, Bionic, NetBSD which provide SSP in libc) are mistakenly missing out on -D_FORTIFY_SOURCE functionality even when explicitly specified. The problem is in gcc libssp/Makefile.am:

libsubincludedir =
$(libdir)/gcc/$(target_noncanonical)/$(gcc_version)/include
nobase_libsubinclude_HEADERS = ssp/ssp.h ssp/string.h ssp/stdio.h
ssp/unistd.h

Headers are structured so that they should be in $(libsubincludedir), instead of $(libsubincludedir)/ssp where they are currently placed.

Demonstration:

$ cat fortify_test.c
/* example from https://gcc.gnu.org/bugzilla/show_bug.cgi?id=50460 [Open URL] */
#include <stdio.h>
#include <string.h>

const char *str1 = "JIHGFEDCBA";

int
main ()
{
struct A { char buf1[9]; char buf2[1]; } a;
strcpy (a.buf1 + (0 + 4), str1 + 5);
printf("%s %s\n", a.buf1, a.buf2);
return 0;
}

$ gcc -D_FORTIFY_SOURCE=2 -fstack-protector-strong -o fortify_test -O2
fortify_test.c
$ nm -C fortify_test | grep strcpy
U __strcpy_chk@@GLIBC_2.3.4

$ i686-w64-mingw32-gcc -D_FORTIFY_SOURCE=2 -fstack-protector-strong -o
fortify_test.exe -O2 fortify_test.c
$ i686-w64-mingw32-nm -C fortify_test.exe | grep strcpy
004061e8 I _imp__strcpy
00402624 T strcpy

If headers are moved, we can see:

$ i686-w64-mingw32-gcc -D_FORTIFY_SOURCE=2 -fstack-protector-strong -o
fortify_test.exe -O2 fortify_test.c
$ i686-w64-mingw32-nm -C fortify_test.exe | grep strcpy
00406200 I _imp____strcpy_chk
00401590 T __strcpy_chk

Comment 1 Adam Mariš 2016-04-07 08:33:57 UTC
Acknowledgments:

Name: Yaakov Selkowitz (Red Hat)

Comment 2 Yaakov Selkowitz 2016-04-07 13:50:05 UTC
Created attachment 1144750 [details]
Patch for gcc-5.3.0

Comment 3 Yaakov Selkowitz 2016-04-07 13:50:54 UTC
Created attachment 1144751 [details]
Patch for gcc svn trunk

Comment 4 Yaakov Selkowitz 2016-04-10 23:45:01 UTC
Created attachment 1145808 [details]
Link with -lssp when -D_FORTIFY_SOURCE=* is in use (needs testing)

Note that a side-effect of having this feature actually work as intended is that the __*_chk symbols are required from libssp.  While gcc automatically adds -lssp if the -fstack-protector* flags are in use, it does not if only -D_FORTIFY_SOURCE=* is specified, resulting in undefined references at link time.

The attached patch, while not technically perfect -- there are several conditions without which -D_FORTIFY_SOURCE=* will have no effect, meaning this may "overlink" -- does appears to rectify that so far in limited testing.

Comment 5 Yaakov Selkowitz 2016-04-11 01:45:35 UTC
(In reply to Yaakov Selkowitz from comment #4)
> Created attachment 1145808 [details]
> Link with -lssp when -D_FORTIFY_SOURCE=* is in use (needs testing)
> 
> The attached patch, while not technically perfect -- there are several
> conditions without which -D_FORTIFY_SOURCE=* will have no effect, meaning
> this may "overlink" -- does appears to rectify that so far in limited
> testing.

Unfortunately this doesn't cover the alternate form -Wp,-D_FORTIFY_SOURCE=*, so this will need further work.

Comment 6 Jakub Jelinek 2016-04-11 07:06:18 UTC
The original plan is that users on non-glibc systems would just add the -isystem /usr/include/ssp/ -lssp themselves in addition to -D_FORTIFY_SOURCE=*, or
-D_FORTIFY_SOURCE, or -D _FORTIFY_SOURCE=*, or -D _FORTIFY_SOURCE, or -Wp,-D -Wp,-_FORTIFY_SOURCE=*, or -Wp,-D_FORTIFY_SOURCE=*, or -Wp,-D,_FORTIFY_SOURCE, etc. (as you can see there are many different spellings).  So IMNSHO the change to install the headers elsewhere is just wrong, you should keep them as they were installed before, and if you can tweak the spec files for the targets that don't have _FORTIFY_SOURCE in their libc (it is not just glibc, I think e.g. Darwin and various others copied that), then do both the includes and link stuff in there, either through trying to match the most common forms of this option, or some spec helper function that covers all of them somehow.

Comment 7 Yaakov Selkowitz 2016-04-11 19:49:18 UTC
(In reply to Jakub Jelinek from comment #6)
> The original plan is that users on non-glibc systems would just add the
> -isystem /usr/include/ssp/ -lssp themselves 

This is not only extremely non-intuitive but also unprecedented:

* This is different from every other gcc feature which handles these details -- at least on the header side, if not also wrt linking -- automatically.

* The headers are not installed under /usr/include/ssp but under e.g. /usr/lib/gcc/$target/$version/include, IOW they are clearly an implementation detail of the compiler but there is also no consistent way to specify their location.

* Requiring a spec to handle the includedir where the header and preprocessor can handle this themselves doesn't make much sense.

The only situation where I could think that such a setup would be warranted is if you wanted to provide libssp as an alternative implementation even on systems that have their own support.  But besides being redundant, I'm not sure that the ssp headers' __*_chk declarations would not conflict with those in libc's headers.

Therefore, I was left with no other logical option but to conclude that this was simply a mistake.  Regardless, as the feature does not work OOTB as intended, it should be fixed so that it is actually used with -D_FORTIFY_SOURCE.

> in addition to -D_FORTIFY_SOURCE=*, or
> -D_FORTIFY_SOURCE, or -D _FORTIFY_SOURCE=*, or -D _FORTIFY_SOURCE, or -Wp,-D
> -Wp,-_FORTIFY_SOURCE=*, or -Wp,-D_FORTIFY_SOURCE=*, or
> -Wp,-D,_FORTIFY_SOURCE, etc. (as you can see there are many different
> spellings). 

Indeed, but I believe the space following -D did not affect the attempt in my spec patch, it's the -Wp, forms that aren't covered thereby.

> So IMNSHO the change to install the headers elsewhere is just
> wrong, you should keep them as they were installed before, and if you can
> tweak the spec files for the targets that don't have _FORTIFY_SOURCE in
> their libc (it is not just glibc, I think e.g. Darwin and various others
> copied that), then do both the includes and link stuff in there, either
> through trying to match the most common forms of this option, or some spec
> helper function that covers all of them somehow.

I don't know about Darwin, but otherwise AFAICS glibc, Bionic, and NetBSD are the only libc's with built-in support.  FreeBSD, DragonFlyBSD, and Musl have only SSP (__stack_chk_*) but not builtin object size checking (__*_chk), and uClibc has only such for bzero/memcpy/mempcpy.

As I said above, there is simply no need for the headers to be in a non-default directory, nor is there a reliable (or official) way to include their current location.  Therefore I still think the headers should be moved, and linking handled with a .spec file as you indicate.

Comment 8 Jakub Jelinek 2016-04-11 20:03:10 UTC
I don't care that much about the location of headers, as long as you make sure you DON"T install it when it should not be in a way (i.e. when the system libc supports fortify source natively).
And, because adding extra headers for standard headers in presence of #include_next and/or users using say -isystem /usr/include and similar (many users do that, don't ask me why), I'd just say that it would be better if the SSP headers even on systems where libc does not have fortify source support in it weren't by default in the way and only were added when using -D_FORTIFY_SOURCE*.

As for the reason why libssp fortify support has been added to gcc, it was just that we didn't want to submit a glibc only feature at that point to gcc, and allowed to handle something (a small subset, compared to what glibc ships) even on systems with other libcs.  That newlib and other libcs didn't add it natively after all those years is IMHO just a mistake of those libraries.

Comment 9 Jakub Jelinek 2016-04-11 20:12:48 UTC
Right now the ssp headers are installed into `gcc -print-file-name=include/ssp`
directory on all systems, including the glibc ones, and anyone is free to use it if they for whatever strange reason want that.  But, generally you really don't want to override the glibc stdio.h, string.h and unistd.h, not to mention that unistd.h will be installed even on systems which perhaps have no unistd.h header themselves and then this could just break some configure detection there.

Comment 10 Yaakov Selkowitz 2016-04-12 02:55:48 UTC
(In reply to Jakub Jelinek from comment #8)
> I don't care that much about the location of headers, as long as you make
> sure you DON"T install it when it should not be in a way (i.e. when the
> system libc supports fortify source natively).

See below.

> And, because adding extra headers for standard headers in presence of
> #include_next and/or users using say -isystem /usr/include and similar (many
> users do that, don't ask me why), I'd just say that it would be better if
> the SSP headers even on systems where libc does not have fortify source
> support in it weren't by default in the way and only were added when using
> -D_FORTIFY_SOURCE*.

FWIW several other GCC headers use #include_next too, so those too would be missed in such a case.  -I/usr/include is already ignored, can the same be done for -isystem /usr/include (at least w/o -notsdinc)?

> As for the reason why libssp fortify support has been added to gcc, it was
> just that we didn't want to submit a glibc only feature at that point to
> gcc, and allowed to handle something (a small subset, compared to what glibc
> ships) even on systems with other libcs.  That newlib and other libcs didn't
> add it natively after all those years is IMHO just a mistake of those
> libraries.

After this is disclosed we may very well have a discussion about enabling it in newlib (and hence Cygwin and RTEMS).  I obviously can't speak for the remaining BSDs nor musl.

However, there is also the matter of supporting object size checking on proprietary platforms where users have no control over their libc -- both the commercial *NIXes as well as MinGW.  At least the latter is completely reliant on libssp for this functionality.

(In reply to Jakub Jelinek from comment #9)
> Right now the ssp headers are installed into `gcc -print-file-name=include/ssp`
> directory on all systems, including the glibc ones, and anyone is free to
> use it if they for whatever strange reason want that. 

Only if libssp is enabled from the toplevel; AFAICS at least RHEL and Fedora gcc packages don't.  

> But, generally you really don't want to override the glibc stdio.h, string.h 
> and unistd.h

Agreed, and as I suspected, that won't even work:

$ gcc -O2 -D_FORTIFY_SOURCE=2 fortify_test.c -I /path/to/include/ssp
In file included from fortify_test.c:2:0:
/path/to/include/ssp/stdio.h:80:1: error: redefinition of ‘gets’
 gets (char *__str)
 ^
In file included from /usr/include/stdio.h:934:0,
                 from /path/to/include/ssp/stdio.h:39,
                 from fortify_test.c:2:
/usr/include/bits/stdio2.h:226:1: note: previous definition of ‘gets’ was here
 gets (char *__str)
 ^
In file included from fortify_test.c:2:0:
/path/to/include/ssp/stdio.h:92:1: error: redefinition of ‘fgets’
 fgets (char *__restrict__ __s, int __n, FILE *__restrict__ __stream)
 ^
In file included from /usr/include/stdio.h:934:0,
                 from /path/to/include/ssp/stdio.h:39,
                 from fortify_test.c:2:
/usr/include/bits/stdio2.h:245:1: note: previous definition of ‘fgets’ was here
 fgets (char *__restrict __s, int __n, FILE *__restrict __stream)

Should target-libssp just be added to noconfigdirs on SSP-and-object-size-checking-in-libc systems?

> not to mention that unistd.h will be installed even on systems which perhaps
> have no unistd.h header themselves and then this could just break some
> configure detection there.

Wouldn't the #include_next therein fail in that case, with the same result as if it weren't there at all?

Comment 11 Jakub Jelinek 2016-04-12 06:47:49 UTC
(In reply to Yaakov Selkowitz from comment #10)
> FWIW several other GCC headers use #include_next too, so those too would be
> missed in such a case.  -I/usr/include is already ignored, can the same be
> done for -isystem /usr/include (at least w/o -notsdinc)?

See e.g. http://gcc.gnu.org/PR69383 for an example of issues caused by include_next changes.  I don't think -I/usr/include is ignored, it shouldn't be, nor should -isystem behavior change.

> Only if libssp is enabled from the toplevel; AFAICS at least RHEL and Fedora
> gcc packages don't.  

No, in RHEL/Fedora packages it is actually installed and then the gcc.spec file removes them.  But, if you tweak this upstream, then you break all the users that are installing gcc themselves.

> Should target-libssp just be added to noconfigdirs on
> SSP-and-object-size-checking-in-libc systems?

Note, GCC 6 is going to be released really soon, you certainly don't want to make too big incompatible changes there now.

Comment 12 Yaakov Selkowitz 2016-05-20 07:13:53 UTC
(In reply to Jakub Jelinek from comment #11)
> Note, GCC 6 is going to be released really soon, you certainly don't want to
> make too big incompatible changes there now.

GCC 6 is out and trunk is on stage 1 again, so I'd like to find a way of moving this along.  

The problem linking with only -D_FORTIFY_SOURCE* (where -fstack-protector* is not used) is the -Wp, form, and this combination does happen in the real world (e.g. vim).  By definition that would seem to bypass the specs, leaving no way to make a conditional link with -lssp.  Even if there is a way, there's also the distinct possibility that the flag will not be passed during linking (it is a preprocessor flag, after all).

Therefore, it seems we would have to:

1) in toplevel configure, disable target-libssp for targets with SSP-in-libc.

This should be done regardless, as the libssp headers cannot be used.

But how?  Adding the gcc_cv_libc_provides_ssp test in gcc/configure.ac to toplevel (presumably as a config/*.m4 macro called by both) would result in a huge diff due to the test calling GCC_GLIBC_VERSION_GTE_IFELSE and target_header_dir.  The conditional tests therein are pretty old, can/should we just handle those targets unconditionally?

2) Add __gnu_inline__ to the extern inline SSP functions for C99 compatibility, and fix the declared return type of readline to ssize_t.

3) Move the SSP headers as originally proposed.

4) In gcc/gcc.c, change LINK_SSP_SPEC to unconditionally link with -lssp* when !TARGET_LIBC_PROVIDES_SSP, using LD{,_NO}_AS_NEEDED_OPTION where available.

I have preliminary patches for all of these.

Comment 13 Cedric Buissart 2016-05-26 12:59:25 UTC
My understanding from comment 9, 10 and 11 is that RHEL is not affected because we explicitly remove the SSP library out of gcc as shipped :

----8<----
rm -f %{buildroot}%{_prefix}/%{_lib}/libssp*
---->8----

Therefore there is no way a user could accidentally or willingly use the SSP library instead of glibc's ssp.

I checked from dist-git in the following gcc branches : 
rhel-7.3
rhel-6.9
rhel-5.11
devtoolset-4.1-rhel-7
devtoolset-4.1-rhel-6

And following packages :
compat-gcc-44 (rhel-6.9 & rhel-7.3)
gcc44

Featured appeared with 4.1, it seems. thus pre-4.1 aren't affected either.

Comment 15 Yaakov Selkowitz 2016-05-27 19:30:17 UTC
(In reply to Cedric Buissart from comment #13)
> My understanding from comment 9, 10 and 11 is that RHEL is not affected
> because we explicitly remove the SSP library out of gcc as shipped :

RHEL is not affected because the __*_chk functions are in libc itself and therefore always linked by default.

Comment 17 Yaakov Selkowitz 2016-05-27 19:35:02 UTC
(In reply to Yaakov Selkowitz from comment #15)
> (In reply to Cedric Buissart from comment #13)
> > My understanding from comment 9, 10 and 11 is that RHEL is not affected
> > because we explicitly remove the SSP library out of gcc as shipped :
> 
> RHEL is not affected because the __*_chk functions are in libc itself and
> therefore always linked by default.

Oh, and RHEL doesn't ship mingw*-gcc etc.

Comment 18 Yaakov Selkowitz 2016-06-07 02:51:04 UTC
Created attachment 1165477 [details]
Cumulative patch for gcc svn trunk

Here's what I have at the moment against svn trunk.

Comment 26 Yaakov Selkowitz 2017-11-30 01:25:15 UTC
For the record, SSP has been added to newlib git master as of today.  Consequently, the future Newlib 2.6.0 and Cygwin 2.10.0 releases (dates TBD), in conjunction with a gcc (re)built with --disable-libssp (and gcc_cv_libc_provides_ssp=yes in GCC 7 and earlier), will no longer have this issue.

Comment 27 Yaakov Selkowitz 2018-01-19 02:41:32 UTC
Newlib 3.0.0 was released today including the builtin SSP implementation.

Comment 28 Yaakov Selkowitz 2018-02-02 22:13:04 UTC
Cygwin 2.10.0 was released today, along with an update for the Cygwin distribution gcc (6.4.0-5) which uses the new builtin implementation.


Note You need to log in before you can comment on or make changes to this bug.