GSoC 2015: Core support for _FORTIFY_SOURCE

[pfg]

Summary

Defining _FORTIFY_SOURCE will enable a set of bounds-checking functions
that act as replacement for regular functions: mostly string and memory
manipulation routines. This depends on compiler support for the
_buitin_object_size attribute, originating in GCC and supported
partially in clang.

FreeBSD_version will be bumped as some of the ports in the tree already
define this internally.

It should be a commit candidate after some small
problems detected by an exp-run in the ports tree are solved.

Most of the code was ported Oliver Pinter as his GSoC project; taken
Android with some influence from NetBSD. Documentation and support for
building "fortified" releases will come in the future.

Test Plan

This has been tested thoroughly in with the base clang and gcc (4.2.1).
Patches for a few ports were submitted.

[pfg]

Minor cleanups + initial attempt at providing a man page.

It's my first complete man page and it's still very rough
so be gentle ;).

[bjk]

Some thoughts from a first read through the man page.

share/man/man7/fortify_source.7

36

Please supply parameter names for the .Fn arguments. Thus,
.Fn __poll_chk "struct pollfd *fds" [...]

131

This "the" is probably not needed...

132

This may be better as .Dv; I'd have to check.

133

... particularly if this "for" is replaced with "in".

140

comma after warning.

146

cannot, or must not?

[pfg]

share/man/man7/fortify_source.7

36

While I could, it doesn't make much sense because these functions are not meant to be called, they only exist in underscored headers and will appear in bug reports.

I have to add symlinks for the real functions.

131

It was needed because I was later referring to the compiler flags.

133

Yes, this is a good change, Thanks!

146

cannot: we#error any attempt.

[pfg]

Rebase cdefs.h

We now give preference to GCC when checking for attributes.
It appears GCC doesn't like to __has_attributes and we
are generally checking for GCC specific features so asking
GCC first should help keep things cleaner.

The man page is still WIP.

[pfg]

Rebase again: underscore attribute name.

[pfg]

Small updates to the manpage: still a long way to go.

[wblock]

share/man/man7/fortify_source.7

133

Start new sentences on new lines.

137

Start new sentences on new lines.

145

s/adds/add/

149

The purpose of this colon is not clear. Should the sentence just be split here?

151

This sentence conflicts a bit with the previous ones. Might be a typo where "do" should be "do not". Or the sentence needs to point out that this is an exception, possibly replacing "do appear visible" with just "are visible":

However, checker functions are visible when debugging or when reporting an

155

Replace this colon by splitting this sentence in two. Add a comma after "reason":

optimization mode.
For this reason, disabling optimizations also involves

159

Replace colon by splitting sentence in two.

166

s/only/only a/
s/of/of the/

221

I would suggest:

.Nm
was introduced in
.Fx 11.0.``

[pfg]

As always, thanks for the feedback.

I took into accound all the suggetions and I re-did most of the manpage:
listing the private bounds-checking functions in the manpage doesn't make
much sense since the functions cannot be used directly in their programs.
Following the general questions on the web, people do wonder what those
*_chk functions are so I added symlinks to the fortify_source man page.

[pfg]

I suspect I am a man page away from something I can commit ;)..

[wblock]

share/man/man7/fortify_source.7

50

Can be simplified a bit:

Buffer overflows that are detected at compile time are reported by the compiler.
Otherwise, the checks are performed at runtime and produce an error message.

58

Um, conformant to what? Is this a warning that higher levels might cause false positives?

[pfg]

share/man/man7/fortify_source.7

58

TBH, I preserved the language used by the author of the GCC implementation.
My understanding is that indeed it will produce false positives and we have seen one such case when building world on MIPS (and only on MIPS!?).

[pfg]

Updated the manpage according to wblock's feedback.

[theraven]

I've done a partial review. This needs a lot more work before it's close to being ready to commit. I stopped after seeing the same logic errors repeated in many functions - there may be new kinds of error, but please fix the ones that are repeated everywhere first.

A few high-level comments:

• I don't like the use of secure for the header / directory names, as that implies that it's related to security. No system that fails open can be counted as a security measure.

• Macros that wrap compiler builtins should have human-meaninful names.

• Please add tests. A number of the code paths are obviously broken when using the system compiler.

I'd also like to see some evidence of evaluation. It doesn't look as if this will catch anything that isn't trivially detectable by a simple static analyser. Are there any bugs caught by this that the clang analyser or coverity doesn't handle? Have you used DTrace's FBT on any real-world code to find what proportion of calls actually go through the _chk functions (with known object sizes)?

include/secure/_poll.h

60

These checks should not be globally !clang, they should be a new macro that is something like __FORTIFY_WORKING_BUITLIN_OBJECT_SIZE. Clang 3.8 has some patches that look as if they will fix these, and I'd hate to have to go and replace all of these with a clang version check. There should be a single version check in cdefs.h

77

__bos is a horrible name for this. Please change it to __object_size (i.e. something descriptive) or just use the builtin directly.

include/secure/_stdio.h

110

Why not use __builtin_choose_expr here and make it call the real one where the compiler doesn't know the bounds?

191

__bos0 is an even more horrible macro name, as it both exposes details of the underlying implementation and simultaneously fails to provide readers with useful information about what it does.

216

While I'm complaining about poor naming, most of these _bos variables could also have human-friendly names (e.g. _ptr_size).

include/secure/_string.h

96

Why are we adding branch prediction to a compile-time constant?

186

As above, why predict a compile-time constant?

See also every other place where this is repeated.

include/secure/_unistd.h

82

Please move this to the definition of __bos() using __builtin_choose_expr.

107

Why not make this (and all other similar checks) a _Static_assert? We know at compile time that it will abort at run time, so tell the user at compile time that their code is broken.

include/secure/security.h

64

mul_no_overflow is an identifier that is valid for user programs to use and so should not be used in a system header.

lib/libc/secure/__fread_chk.c

42

This function appears to duplicate the version in the header. Why does it exist?

lib/libc/secure/__fwrite_chk.c

45

Logic error. If we don't statically know the object size, then we skip the overflow checking. If there is an overflow, then we don't emit an error, we do exactly the same thing as if we'd not done the overflow checking.

46

All of these __predict_false() cases are going to make code slower if compiled with the system compiler (which, unconditionally, is setting this to __FORTIFY_UNKNOWN_SIZE).

lib/libc/secure/__memccpy_chk.c

46

Another logic error. If we don't know the size, then don't do overlap checks.

73

It would be more human-friendly to promote these to memmove with a warning.

lib/libc/secure/__memchr_chk.c

45

This is overcomplicated logic. It should be:

if (bos != __FORTIFY_UNKNOWN_SIZE && (n > bos))
  __fortify_chk_fail("memchr: prevented read past end of buffer");
return (memchr(s, c, n));

lib/libc/secure/__memmove_chk.c

45

These functions all look wrong. In the header wrappers, we're not trusting clang's __builtin_object_size, so we're eliding all of the checks, but then we're passing that value through unmodified to this function, which now does checks against it.

These functions should *never* be called with bos == __FORTIFY_UNKNOWN_SIZE and should have an assert to that effect.

I'm stopping individual reviews at this point, as every single function appears to reproduce the same logic errors.

[wblock]

share/man/man7/fortify_source.7

51

Needs a comma after "Otherwise", because there is a pause there.

57

s/may/might/ ("may" for permission, "might" for possibility: "Yes, you may use my spare video card. It might not work any more, though.")

[pfg]

Fix manpage comments. Thanks!

[pfg]

Update comments from regarding man page.
(I will let Oliver answer David's concerns.)

[pfg]

We will be abandoning this idea altogether:

FORTIFY_SOURCE has never been supported for clang, as the approach has been replaced by a combination of a static analyzer and a memory sanitizer. The strong stack protector also does a good job catching the bugs fortify used to find. The idea was good for times when we depended exclusively on GCC but as of now it would just add GCC-dependent bloat to our C library.

The project was successful in that it was good to run it at least once over the tree and it did find three bugs in base (which probably saved us some pain with the strong stack protector).