Bug 1324759 (CVE-2016-4973)

Acknowledgments:

Name: Yaakov Selkowitz (Red Hat)

Created attachment 1144750 [details]
Patch for gcc-5.3.0

Created attachment 1144751 [details]
Patch for gcc svn trunk

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.

(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.

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.

(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.

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.

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.

(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?

(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.

(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.

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.

(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.

(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.

Created attachment 1165477 [details]
Cumulative patch for gcc svn trunk

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

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.

Newlib 3.0.0 was released today including the builtin SSP implementation.

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.