Re: [PATCH v1 00/10] Add Stack Smashing Protection and Object Size Checking

Re: [PATCH v1 00/10] Add Stack Smashing Protection and Object Size Checking

[Wilco Dijkstra]

Hi,

> In the process of overhauling our feature test macros, I discovered that
> GCC's libssp implementation of Object Size Checking (-D_FORTIFY_SOURCE=*) is
> completely broken and possibly unfixable (CVE-2016-4973).  Therefore, it
> seems the only way to make this work is to integrate it to Newlib itself like
> other libc's.

Wouldn't be better to implement a working -ffortify-string-functions feature
in GCC/LLVM so that the compiler can insert the correct checks?
Hacking all C libraries in the world still won't make the checks work -
as long as they rely on the broken __builtin_object_size implementation,
many cases won't be checked even when they should be, eg:

char s[100]; 
memcpy (s + 1, p, n);

The _chk variants also seem unnecessary, I don't understand their purpose.
All you want is to tell GCC to insert runtime checks when it detects the destination
is an array. You obviously want those checks to be inlined and optimized for
performance reasons.

Wilco

Re: [PATCH v1 00/10] Add Stack Smashing Protection and Object Size Checking

[Yaakov Selkowitz]

[-- Attachment #1.1: Type: text/plain, Size: 1701 bytes --]

On 2017-11-06 13:21, Wilco Dijkstra wrote:
>> In the process of overhauling our feature test macros, I discovered that
>> GCC's libssp implementation of Object Size Checking (-D_FORTIFY_SOURCE=*) is
>> completely broken and possibly unfixable (CVE-2016-4973).  Therefore, it
>> seems the only way to make this work is to integrate it to Newlib itself like
>> other libc's.
> 
> Wouldn't be better to implement a working -ffortify-string-functions feature
> in GCC/LLVM so that the compiler can insert the correct checks?

I have neither the time nor the interest in creating new
compiler/language extensions; I am simply trying to get the ones that
already exist working properly on our targets.

> Hacking all C libraries in the world still won't make the checks work -
> as long as they rely on the broken __builtin_object_size implementation,
> many cases won't be checked even when they should be

PTC?

> The _chk variants also seem unnecessary, I don't understand their purpose.

The __builtin__*_chk builtins (which are limited to the string.h and
basic stdio.h functions) expect corresponding __*_chk functions to be
present.  Also, some __*_chk functions are more extensive than others.

> All you want is to tell GCC to insert runtime checks when it detects the destination
> is an array. You obviously want those checks to be inlined and optimized for
> performance reasons.

Most of the dozens of other functions which have size-checking
implementations in glibc could be handled inline, as those for unistd.h
imported from NetBSD are.  I am already working on some of these
additions, but would like to get the basics into master first.

-- 
Yaakov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v1 00/10] Add Stack Smashing Protection and Object Size Checking

[Yaakov Selkowitz]

This is an initial draft; I am using the topic/ssp branch for development
of this feature.

In the process of overhauling our feature test macros, I discovered that
GCC's libssp implementation of Object Size Checking (-D_FORTIFY_SOURCE=*) is
completely broken and possibly unfixable (CVE-2016-4973).  Therefore, it
seems the only way to make this work is to integrate it to Newlib itself like
other libc's.

I used NetBSD as the basis for much of this.  While relatively limited in
coverage compared to glibc (which we can't take from), it should provide the
foundation needed to add more coverage in the future.

This does require some minor changes in configuring GCC because its libssp
would conflict with this (as it similarly conflicts with glibc), as noted in
the commit messages.

There is probably a more portable way of getting a random canary for the
benefit of bare metal targets (since arc4random required getentropy), but
the terminator canary does work (tested with mmix target).

Yaakov Selkowitz (10):
  ssp: add APIs for Stack Smashing Protection (-fstack-protector*)
  ssp: add Object Size Checking for basic string functions
  ssp: add Object Size Checking for bcopy, bzero
  ssp: add Object Size Checking for basic stdio functions
  ssp: add Object Size Checking for basic unistd.h functions
  ssp: document _FORTIFY_SOURCE with the feature test macros
  ssp: add build infrastructure
  ssp: install headers
  cygwin: export SSP functions
  cygwin: create libssp compatibility import library

 newlib/Makefile.am                     |   4 +
 newlib/Makefile.in                     |   4 +
 newlib/libc/Makefile.am                |   4 +-
 newlib/libc/Makefile.in                |  15 +-
 newlib/libc/configure                  |   3 +-
 newlib/libc/configure.in               |   2 +-
 newlib/libc/include/ssp/ssp.h          |  93 +++++
 newlib/libc/include/ssp/stdio.h        |  74 ++++
 newlib/libc/include/ssp/string.h       | 112 ++++++
 newlib/libc/include/ssp/strings.h      |  48 +++
 newlib/libc/include/ssp/unistd.h       |  51 +++
 newlib/libc/include/stdio.h            |   4 +
 newlib/libc/include/string.h           |   4 +
 newlib/libc/include/strings.h          |   4 +
 newlib/libc/include/sys/features.h     |   7 +-
 newlib/libc/include/sys/unistd.h       |  10 +
 newlib/libc/ssp/Makefile.am            |  71 ++++
 newlib/libc/ssp/Makefile.in            | 714 +++++++++++++++++++++++++++++++++
 newlib/libc/ssp/chk_fail.c             |  13 +
 newlib/libc/ssp/fgets_chk.c            |  55 +++
 newlib/libc/ssp/gets_chk.c             |  78 ++++
 newlib/libc/ssp/memcpy_chk.c           |  54 +++
 newlib/libc/ssp/memmove_chk.c          |  50 +++
 newlib/libc/ssp/mempcpy_chk.c          |  21 +
 newlib/libc/ssp/memset_chk.c           |  49 +++
 newlib/libc/ssp/snprintf_chk.c         |  59 +++
 newlib/libc/ssp/sprintf_chk.c          |  63 +++
 newlib/libc/ssp/stack_protector.c      |  46 +++
 newlib/libc/ssp/stpcpy_chk.c           |  58 +++
 newlib/libc/ssp/stpncpy_chk.c          |  56 +++
 newlib/libc/ssp/strcat_chk.c           |  62 +++
 newlib/libc/ssp/strcpy_chk.c           |  55 +++
 newlib/libc/ssp/strncat_chk.c          |  73 ++++
 newlib/libc/ssp/strncpy_chk.c          |  55 +++
 newlib/libc/ssp/vsnprintf_chk.c        |  51 +++
 newlib/libc/ssp/vsprintf_chk.c         |  60 +++
 winsup/cygwin/Makefile.in              |   5 +-
 winsup/cygwin/common.din               |  20 +
 winsup/cygwin/include/cygwin/version.h |   7 +-
 39 files changed, 2202 insertions(+), 12 deletions(-)
 create mode 100644 newlib/libc/include/ssp/ssp.h
 create mode 100644 newlib/libc/include/ssp/stdio.h
 create mode 100644 newlib/libc/include/ssp/string.h
 create mode 100644 newlib/libc/include/ssp/strings.h
 create mode 100644 newlib/libc/include/ssp/unistd.h
 create mode 100644 newlib/libc/ssp/Makefile.am
 create mode 100644 newlib/libc/ssp/Makefile.in
 create mode 100644 newlib/libc/ssp/chk_fail.c
 create mode 100644 newlib/libc/ssp/fgets_chk.c
 create mode 100644 newlib/libc/ssp/gets_chk.c
 create mode 100644 newlib/libc/ssp/memcpy_chk.c
 create mode 100644 newlib/libc/ssp/memmove_chk.c
 create mode 100644 newlib/libc/ssp/mempcpy_chk.c
 create mode 100644 newlib/libc/ssp/memset_chk.c
 create mode 100644 newlib/libc/ssp/snprintf_chk.c
 create mode 100644 newlib/libc/ssp/sprintf_chk.c
 create mode 100644 newlib/libc/ssp/stack_protector.c
 create mode 100644 newlib/libc/ssp/stpcpy_chk.c
 create mode 100644 newlib/libc/ssp/stpncpy_chk.c
 create mode 100644 newlib/libc/ssp/strcat_chk.c
 create mode 100644 newlib/libc/ssp/strcpy_chk.c
 create mode 100644 newlib/libc/ssp/strncat_chk.c
 create mode 100644 newlib/libc/ssp/strncpy_chk.c
 create mode 100644 newlib/libc/ssp/vsnprintf_chk.c
 create mode 100644 newlib/libc/ssp/vsprintf_chk.c

-- 
2.14.3

Re: [PATCH v1 00/10] Add Stack Smashing Protection and Object Size Checking

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 369 bytes --]

On Oct 31 23:52, Yaakov Selkowitz wrote:
> This is an initial draft; I am using the topic/ssp branch for development
> of this feature.

This looks good.  Is that basically the final state for the first patch
submission as you see it?  And how do you coordinate the required GCC
change?


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v1 00/10] Add Stack Smashing Protection and Object Size Checking

[Yaakov Selkowitz]

[-- Attachment #1.1: Type: text/plain, Size: 982 bytes --]

On 2017-11-02 09:25, Corinna Vinschen wrote:
> On Oct 31 23:52, Yaakov Selkowitz wrote:
>> This is an initial draft; I am using the topic/ssp branch for development
>> of this feature.
> 
> This looks good.  Is that basically the final state for the first patch
> submission as you see it? 

The topic/ssp branch now contains patchset v2, and I'm considering a few
more minor changes which shouldn't take long.

> And how do you coordinate the required GCC change?

I think the gcc change will need to be conditional on the presence of
$target_header_dir/ssp/ssp.h, in order to continue to support older
Newlib/Cygwin/RTEMS releases.  If so, then this will have to go in
first, then I submit the change to GCC and get it pulled into supported
stable branches.  Toolchain builders will need to configure with
--disable-libssp permanently (like on other targets with their own
implementation) and carry a small patch until releases are available.

-- 
Yaakov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]