Bug 77409 (CVE-2016-4973) - CVE-2016-4973 Targets using libssp for SSP are missing -D_FORTIFY_SOURCE functionality
Summary: CVE-2016-4973 Targets using libssp for SSP are missing -D_FORTIFY_SOURCE func...
Status: RESOLVED INVALID
Alias: CVE-2016-4973
Product: gcc
Classification: Unclassified
Component: other (show other bugs)
Version: 6.1.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-29 22:05 UTC by Yaakov Selkowitz
Modified: 2016-08-30 02:08 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Yaakov Selkowitz 2016-08-29 22:05:08 UTC
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 bug 50460 */
#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

Red Hat Product Security has assigned CVE-2016-4973 to this issue.

Further discussion: https://bugzilla.redhat.com/show_bug.cgi?id=1324759
Comment 1 Andrew Pinski 2016-08-30 01:38:12 UTC
I don't think this is a security hole at all.  In fact the security holes should be on the applications side rather than the library side.
Comment 2 Yaakov Selkowitz 2016-08-30 01:46:58 UTC
(In reply to Andrew Pinski from comment #1)
> I don't think this is a security hole at all.  In fact the security holes
> should be on the applications side rather than the library side.

The compiler is the cause and where this needs to be fixed first, therefore the CVE was assigned to gcc.
Comment 3 Andrew Pinski 2016-08-30 01:47:09 UTC
In fact this is by design.  NetBSD for an example has ssp/stdio.h where you use that to get the fority.
Comment 4 Andrew Pinski 2016-08-30 01:48:18 UTC
(In reply to Yaakov Selkowitz from comment #2)
> (In reply to Andrew Pinski from comment #1)
> > I don't think this is a security hole at all.  In fact the security holes
> > should be on the applications side rather than the library side.
> 
> The compiler is the cause and where this needs to be fixed first, therefore
> the CVE was assigned to gcc.

What I am trying to say is there is no security hole that this can cause, only the badly written applications where the security holes are located.

Also this looks like it was by design.
Comment 5 Andrew Pinski 2016-08-30 01:48:54 UTC
> NetBSD which provide SSP in libc

This statement is not true for older versions of netbsd.
Comment 6 Andrew Pinski 2016-08-30 01:51:28 UTC
(In reply to Andrew Pinski from comment #5)
> > NetBSD which provide SSP in libc
> 
> This statement is not true for older versions of netbsd.

And really not true even for the current version of netbsd but they do:
1.70      kristerw  596: #if _FORTIFY_SOURCE > 0
1.69      tls       597: #include <ssp/stdio.h>
1.70      kristerw  598: #endif

http://cvsweb.netbsd.org/bsdweb.cgi/src/include/stdio.h#rev1.69

:)

So again I don't see this as a bug but rather the person thinking -D_FORTIFY_SOURCE  works without including ssp/*.h on some targets.
Comment 7 Andrew Pinski 2016-08-30 01:53:43 UTC
This is a bug in newlib and MinGW (Cygwin and RTEMS uses newlib) rather than in GCC for not supporting -D_FORTIFY_SOURCE .  ssp should not be the one supplying this support (it just happens to does not mean it does for all things).
Comment 8 Yaakov Selkowitz 2016-08-30 01:55:38 UTC
(In reply to Andrew Pinski from comment #3)
> In fact this is by design.  NetBSD for an example has ssp/stdio.h where you
> use that to get the fority.

This does not apply where the libc provides its own SSP functionality.  In fact, part of the fix for this will be noconfigdirs+=target-libssp on such targets, as the libssp implementation conflicts.

(In reply to Andrew Pinski from comment #4)
> What I am trying to say is there is no security hole that this can cause,
> only the badly written applications where the security holes are located.

This was judged CVE worthy because a security barrier did not function as expected.
 
> Also this looks like it was by design.

Then the design is poor, as it does not work.


(In reply to Andrew Pinski from comment #6)
> And really not true even for the current version of netbsd but they do:
> 1.70      kristerw  596: #if _FORTIFY_SOURCE > 0
> 1.69      tls       597: #include <ssp/stdio.h>
> 1.70      kristerw  598: #endif
> 
> http://cvsweb.netbsd.org/bsdweb.cgi/src/include/stdio.h#rev1.69
> 
> So again I don't see this as a bug but rather the person thinking
> -D_FORTIFY_SOURCE  works without including ssp/*.h on some targets.

No, as documented, the only necessary intervention is to define _FORTIFY_SOURCE; the location of the headers is implementation specific.
Comment 9 Yaakov Selkowitz 2016-08-30 01:59:32 UTC
(In reply to Andrew Pinski from comment #7)
> This is a bug in newlib and MinGW (Cygwin and RTEMS uses newlib) rather than
> in GCC for not supporting -D_FORTIFY_SOURCE .  ssp should not be the one
> supplying this support (it just happens to does not mean it does for all
> things).

The entire point of libssp is to provide this support on systems whose libc does not include it, so that both -D_FORTIFY_SOURCE and -fstack-protector* functionality would be available cross-platform.

Also note that you can't say "just add it to libc" for either MinGW or commercial *NIXs.
Comment 10 Andrew Pinski 2016-08-30 02:04:54 UTC
https://bugzilla.redhat.com/show_bug.cgi?id=1324759#c6 and the others from Jakub Jelinek on why this is not a GCC bug.

And why you can't do what you want GCC to change it to do.

> But, if you tweak this upstream, then you break all the users that are installing gcc themselves.

THis is why any change to install them as normal headers is wrong even for targets where you think you should install them.

So again closing this as invalid.  This is a security bug in the applications thinking they get _FORTIFY_SOURCE support for mingw and cygwin, etc but they really need to include ssp library headers instead.  Not a GCC bug for someone including the wrong header :).
Comment 11 Andrew Pinski 2016-08-30 02:08:11 UTC
> The entire point of libssp is to provide this support on systems whose libc
> does not include it, so that both -D_FORTIFY_SOURCE and -fstack-protector*
> functionality would be available cross-platform.

Yes if you used the right headers and the right options.  You need to include the correct headers for those applications.  Use the option as Jakub mentioned in the redhat bug report and you get the behavior you want.  This is not designed to be included behind someone's back which is what you want.