TODO
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Per bde's comments before on #ifdef foo vs #if defined(foo), (and the !defined(foo) idiom), the shorter versions should be used.
On a more relevant note -- why are the ssp testcases causing issues?
| include/secure/_socket.h | ||
|---|---|---|
| 74 | Why not just use s like the manpage states? RECV(2) FreeBSD System Calls Manual RECV(2)
NAME
recv, recvfrom, recvmsg - receive a message from a socket
LIBRARY
Standard C Library (libc, -lc)
SYNOPSIS
#include <sys/types.h>
#include <sys/socket.h>
ssize_t
recv(int s, void *buf, size_t len, int flags);
ssize_t
recvfrom(int s, void *buf, size_t len, int flags,
struct sockaddr * restrict from, socklen_t * restrict fromlen);
ssize_t
recvmsg(int s, struct msghdr *msg, int flags); | |
| include/secure/_stdio.h | ||
| 2–5 | This seems out of order. Shouldn't the RCS tags be below the licensing text? | |
| include/secure/_unistd.h | ||
| 3–4 | This seems chronologically backwards | |
| include/secure/security.h | ||
| 3 | Olivér Pintér's name has accents here, but not elsewhere. Also, I thought convention was to omit email addresses. | |
Comment Actions
Also, csh scripts for building HardenedBSD? Ow...
I make it an effort not to install tcsh scripts on my systems. If you need help converting the scripts over, I can assist.
Comment Actions
No issues, just thought we may want to enable them in our local tests: In NetBSD FORTIFY_SOURCE is part of SSP.
This said.. according to the logs the tests do not work on NetBSD either.
Comment Actions
Yeah.. ignore those. It's the first thing I remove every time I test the patches.
and thanks for the notes on #ifdef, I prefer the short form always but I thought it was not mandatory.
| include/secure/_socket.h | ||
|---|---|---|
| 74 | Nice catch, I will convert them. | |
| include/secure/_unistd.h | ||
| 3–4 | There are examples both these and both the reverse order in the tree: https://github.com/freebsd/freebsd/blob/master/sys/kern/imgact_elf.c | |
Comment Actions
FBSD FORTIFY: add WITHOUT_FORTIFY knob description FBSD FORTIFY: rename parameter names, requested by ngie FBSD FORTIFY: move the RCS IDs below the license, requested by ngie FBSD FORTIFY: add accent to my name, and remove mail address from copyright
Comment Actions
Oliver Pinter (11):
FBSD FORTIFY: blacklist boot/powerpc/uboot from fortified build
FBSD FORTIFY: call the original functions from __vs{,n}printf_chk, and not the __*_real version
FBSD FORTIFY: drop __gets_chk, discussed with Pedro
FBSD FORTIFY: added __getcwd_chk.cpp from bionic
FBSD FORTIFY: adapt __getcwd_chk
FBSD FORTIFY: cleanup _unistd.h
FBSD FORTIFY: take __fread_chk.cpp and __fwrite_chk.cpp from android
FBSD FORTIFY: adapt __f{read,write}_chk
FBSD: fix expression in _stdio.h, this change should be a no-op
FBSD FORTIFY: change ifdef style
FBSD FORTIFY: one more round of style change| include/secure/security.h | ||
|---|---|---|
| 44 | This should be __GNUC_PREREQ__(5, 0) for consistency. | |
Comment Actions
Seen on MIPS and POWERPC (gcc-4.2 from base)
> lib/libc/tests/stdio (all)
fmemopen2_test.o: In function `atfu_test_data_length_body':
fmemopen2_test.c:(.text+0xfa4): undefined reference to `__fread_too_big_error'
- fmemopen2_test ---
- [fmemopen2_test] Error code 1
| include/secure/_stdio.h | ||
|---|---|---|
| 192 | It looks like we should also restrict this from some gcc versions. We are using __fread_too_big_error in this path but this causing linking erros with gcc 4.2. | |
| include/secure/security.h | ||
|---|---|---|
| 37 | Better: | |
| include/secure/security.h | ||
|---|---|---|
| 37 | Bah.. not so easy.. but there is something wrong there :( | |
Comment Actions
>>> World build completed on Thu Aug 13 21:30:52 UTC 2015
mips.mips64 buildworld completed on Thu Aug 13 21:30:52 UTC 2015
mips.mips buildworld completed on Thu Aug 13 21:30:52 UTC 2015
mips.mipsn32 buildworld completed on Thu Aug 13 21:30:52 UTC 2015
mips.mipsel buildworld completed on Thu Aug 13 21:30:55 UTC 2015
...
Great!
| include/secure/security.h | ||
|---|---|---|
| 37 | #if __GNUC_PREREQ__(4, 3) works here. | |
| tools/build/options/WITHOUT_FORTIFY | ||
|---|---|---|
| 4 ↗ | (On Diff #7932) | FORTIFY_SOURCE `is a libc feature that provides primitive support for detecting buffer overflows in various functions which operate on memory areas and strings. A limited set of buffer overflows can be detected with this feature, but it provides an additional level of validation for some functions which are the origin of buffer overflow flaws. (remove the "potential" term: These are buffer overflows, there is not such thing as a false positive at least at level 1.) |
Comment Actions
- FBSD FORTIFY: fix undefined reference error with gcc-4.2 on MIPS
- FBSD FORTIFY: fix a typo error_attr -> errordecl
- FBSD FORTIFY: fix build failure GNUC_PREREQ -> __GNUC_PREREQ
- rebased against recent HEAD
Comment Actions
Add some more reviewers: It is still WIP, and it needs tmore testing with newer GCC but there are many new files and it's easy for minor details to go unnoticed if I am the only reviewer.
I think the functionality is important as it found 3 bugs already, and the feature is widely available (glibc, Android's bionic, NetBSD, Apple MacOSX), so I am planning to bring it to the tree in small steps: new functionality in libc first, support for buildworld and option knobs afterwards.
Also note: while testing it has is convenient to build the functionality ON by default but I don't plan to commit this with such aggressiveness. NetBSD and others build world with this by default but that is a decision that corresponds elsewhere. Also pending is how ports may want to use it.
| lib/libc/secure/Symbol.map | ||
|---|---|---|
| 20 | Not sure if it makes sense to carry these empty references. | |
Comment Actions
The diff is enormous and it pollutes a lot of unrelated places. E.g. the loader Makefiles changes or the forced undef fortify in libc/strings/stdio, as I understand, to avoid recursion.
Can the pollution be minimized somehow ? Can the patch be split into digestable pieces ?
| lib/libc/secure/__builtin_object_size.3 | ||
|---|---|---|
| 48 | What does it mean that the object does not have any side effects ? Is this a typo ? Or does it indeed means that the object must be PDO (doubt it) ? | |
| lib/libc/secure/fortify_source.c | ||
| 47 | You do not need four comparisions, two are enough. Look at sys/kern/kern_rangelock.c:ranges_overlap() to see how to do it. | |
Comment Actions
It is huge. The undefs are necessary to build a fortified libc, otherwise there will be the above mentioned recursion problem.
Can the pollution be minimized somehow ? Can the patch be split into digestable pieces ?
Yes: we can create a new revision with only the changes in include/ and libc/secure/ (plus mtree), which is the core and is mostly new files.
The support for building fortified builds should be in an independent review and I had no plans to commit it in a first pass anyways.
For now it is/was convenient to have both together for testing purposes but as soon as we pass the build tests with modern gcc we can split them functionally.
| lib/libc/secure/__builtin_object_size.3 | ||
|---|---|---|
| 51 | the "side effects" concept is surely vague: this manpage comes from NetBSD and it looks like it came from the GCC documentation: https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html It doesn't really make sense for us to document compiler features. It is preferable to create a new fortify_source(7) man page explaining the effect of -D_FORTIFY_SOURCE. | |
| lib/libc/secure/Symbol.map | ||
|---|---|---|
| 64 | The _chk functions clearly need to be exported, but does __secure_fail need to be exported? | |
| lib/libc/secure/__builtin_object_size.3 | ||
| 51 | I think "side effects" are things like function calls and writes to variables in the ptr expression. However, I think this manual page documenting a pure compiler feature should not be added. | |
| lib/libc/secure/__memccpy_chk.c | ||
| 61 | should have else
len = n; | |
| 64 | Nothing was prevented, since this code is after the memccpy() call. Therefore, the error message should be adjusted, since it is valid to have an overlap between src..src+n-1 and dest..dest+n-1 if an occurrence of c prevents an actual overlap. | |
| lib/libc/secure/__strlen_chk.c | ||
| 65 | Nothing was prevented here, so the message should probably read strlen: detected read past end of buffer | |
| lib/libc/secure/fortify_source.c | ||
| 47 | Additionally, casts to const char * should be added before adding len to avoid depending on a GCC extension, and casts to uintptr_t should be added before comparing to avoid undefined behaviour from comparing (</<=/>/>=) pointers from different arrays. Also, this should be an inline function. | |
| lib/libc/secure/secure_common.c | ||
| 42 | Please place #include "namespace.h" and #include "un-namespace.h" around the other includes (except libc_private.h which should go after). You will need to use the _ versions of some functions. This reduces possible interposition and PLT usage in this function. | |
Comment Actions
Partly the boot loader pollution is like the pollution for SSP. I do wonder why FORTIFY conditionals were added to some places that currently do not have SSP conditionals.
I think compilation features that do not work in boot loader environments will continue to exist and grow, so a more general framework for turning them off may be interesting.
Comment Actions
The problem is that libstand and libkern have similar interfaces to libc and try to reuse the libc headers: this pulls in references to the *_chk functions so FORTIFY_SOURCE has to be disabled.
I think compilation features that do not work in boot loader environments will continue to exist and grow, so a more general framework for turning them off may be interesting.
NetBSD and others (linux mostly) ship with most of the OS fortified but it would be really nice to have the flexibility to choose other alternatives as well.
I have discussed with Oliver about having a single framework to activate things like the memory or address sanitizers currently in clang.
Thanks for all this feedback!
| lib/libc/secure/__builtin_object_size.3 | ||
|---|---|---|
| 51 | I have been rethinking this and documenting __builtin_object_size only makes sense if we document the difference between GCC and clang. Of course this is to be done in the context of a wider fortify_source documentation. | |
Comment Actions
The makefile changes mostly correlate with -ffreestanding. I'm thinking about make the MK_SSP and MK_FORTIRY on freestanding. I plan to factor out all of the CFLAGS+= -freestanding to new "make option".
The undefs are needed, if we could commit them before the other parts of the work, than we able to split out and create a separate patch. But in testing phase I and Pedro like to keep the patch in one patch to easier test.
Comment Actions
That is a good idea.
The undefs are needed, if we could commit them before the other parts of the work, than we able to split out and create a separate patch. But in testing phase I and Pedro like to keep the patch in one patch to easier test.
Actually the undefs are only needed if someone specifies -D_FORTIFY_SOURCE`. At least of first, this should be off by default so technically the undefs are part of the build support.
I think a good "final" test for the framework would be to build world with a recent gcc: we have already tested the rest of the paths with gcc-4.2.1 and clang. After that is done we can separate the build and basic support parts.
Comment Actions
Some of them added libstand as build dependency, but do not added the -ffreestanding to the CFLAGS. One example is sys/boot/i386/libi386/Makefile.
| lib/libc/secure/fortify_source.c | ||
|---|---|---|
| 47 | jilles: casting of len to (const char *) does not work with clang: op@opn /tmp> make test
cc -O2 -pipe -pipe -DHARDENEDBSD test.c -o test
test.c:51:37: error: invalid operands to binary expression ('const char *' and 'const char *')
if ((uintptr_t)(a) < (uintptr_t)(b + (const char *)l) &&
~ ^ ~~~~~~~~~~~~~~~
test.c:52:20: error: invalid operands to binary expression ('const char *' and 'const char *')
(uintptr_t)(a + (const char *)l) > (uintptr_t)(b))
~ ^ ~~~~~~~~~~~~~~~
2 errors generated.
*** Error code 1 | |
Comment Actions
FBSD FORTIFY: remove unneeded man page, this page documented a compiler feature instead of the fortify source
FBSD FORTIFY: fix jilles comments and add a comment about overlapping
FBSD FORTIFY: fix jilles comment in __strlen_chk.c
FBSD FORTIFY: optimize and move __fortify_chk_overlap
FBSD FORITIFY: undef _FORTIFY_SOURCE in secure_common.c to avoid recursion
FBSD FORTIFY: added {,un-}namespace.h around includes in secure_common.c
FBSD FORTIFY: fix the build
FBSD FORTIFY: change signatureComment Actions
Note some of the issues detected by the exp-run:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=202311
| include/secure/_string.h | ||
|---|---|---|
| 307 | From the net-p2p/namecoin port log (using clang and libc++): ... ^ /usr/include/secure/_string.h:306:1: note: previous definition is here (This builds with FORTIFY_SOURCE=2 enabled by the upstream sources). | |
| 331 | From the net-p2p/namecoin port log (using clang and libc++): ^ /usr/include/secure/_string.h:330:1: note: previous definition is here | |
| sys/sys/cdefs.h | ||
| 565 | Perhaps a parenthesis around __optimize__ >0 is pertinent here. In file included from /usr/include/stdio.h:39: defined(__OPTIMIZE__) && __OPTIMIZE__ > 0 && !defined(lint)
^1 error generated. | |
| 567 | Note that clang-3.8 will have fixes for higher levels of FORTIFY. (3?) so we may want to consider that case here too. | |
| include/secure/_stdio.h | ||
|---|---|---|
| 80 | You'd have to ask the local C standard experts but I don't think declaring a standard function inline is standard compliant. What you should do instead is declare the function normally and then define a macro with the same name that performs the necessary checks. If you don't want to put too much code into the macro you can put it into a "static inline __always_inline" function instead and call this function from the macro. I think this will solve the problems you are seeing in some ports. | |
| include/secure/_string.h | ||
|---|---|---|
| 307 | The strlcpy isn't a standard function, so we could disable them, when we building c++ programs or if only a small set of the ports failed, we should fix these ports instead. I just looked into namecoins source, and they reimplement unconditionally the strlcpy function, regardless of existing on the system or not, and blindly define BSD or BSD_VISIBLE on their headers. | |
| 331 | Same as the above. | |
Comment Actions
As a side-note: the exp-run is a regular ports build on a "fortified" system.
It doesn't force a fortification option on the ports.
In general I consider any failure there to be a commit-stopper, however
it would be understandable that some ports (like namecoins apparently)
assume that _FORTIFY_SOURCE lets them use linuxisms and in such cases we should try to clean those.
Luckily there are not many cases to worry about.
| include/secure/_string.h | ||
|---|---|---|
| 307 | FORTIFY_SOURCE depends on the system headers being used. If only those are affected, it sounds like something that should be fixed in the port (and upstreamed). | |
| sys/sys/cdefs.h | ||
|---|---|---|
| 565 | And seems like this is again a bug in the ports: root@nyi-01:/usr/ports/mail/ifile/work/ifile-1.3.8 # grep -R __OPTIMIZE__ * argp/argp-fs-xinl.c:#undef __OPTIMIZE__ argp/argp-fs-xinl.c:#define __OPTIMIZE__ argp/argp-xinl.c:#undef __OPTIMIZE__ argp/argp-xinl.c:#define __OPTIMIZE__ argp/argp.h:#ifdef __OPTIMIZE__ argp/argp.h:#endif /* __OPTIMIZE__ */ argp/argp-fmtstream.h:#ifdef __OPTIMIZE__ argp/argp-fmtstream.h:#endif /* __OPTIMIZE__ */ argp/argp.h.orig:#ifdef __OPTIMIZE__ argp/argp.h.orig:#endif /* __OPTIMIZE__ */ root@nyi-01:/usr/ports/mail/ifile/work/ifile-1.3.8 # argp/argp-fs-xinl.c: ...
24
25 #define ARGP_FS_EI
26 #undef __OPTIMIZE__
27 #define __OPTIMIZE__
28 #include "argp-fmtstream.h"
...they redefine the OPTIMIZE macro with empty value. So we should add MK_FORTIFY= no to the port's Makefile. | |
| sys/sys/cdefs.h | ||
|---|---|---|
| 565 | Not an option: MK_FORTIFY may not be available after all. Especially not in 10.x or 9.x. I think upstream treats __OPTIMIZE__ as boolean in some case, which is against sensible style. Change the port to #define __OPTIMIZE__ 0 that should be more portable, | |
Comment Actions
Oliver Pinter (4):
FBSD FORTIFY: fix spelling in WITHOUT_FORTIFY FBSD FORTIFY: update gcc build script FBSD FORTIFY: add the ability do disable FORTIFY_SOURCE per compiler FBSD FORTIFY: blacklist from fortified build in gcc case the libbfd, objdump and libsqlite3
Comment Actions
I did a detailed review, which Phabricator appears to have eaten. I'll try to summarise here, as there was some talk of committing this, and it is a long way away from being ready to go in the tree:
- All of the #ifdef clang should be changed to something like __FORTIFY_WORKING_OBJECT_SIZE which can be changed in one place for clang 3.8 (which appears to contain fixes for the relevant bugs).
- The implementations in the .c files are full of logic errors. If the size is not known at compile time, you're skipping doing overflow checks, even though these checks don't depend on these things. The overflow checks should be done unconditionally in the underlying functions anyway, irrespective of whether FORTIFY_SOURCE is defined.
- __bos is not a human-friendly name. __bos0 is both not human-friendly and exposes implementation details of the underlying builtin. Please rename these to something human friendly.
- Many of these checks could be _Static_asserts, triggering at compile time (it's braindead to abort at run time for something that the compiler knows broken at compile time).
- There are a number of __predict_false for compile-time constants. These imply that the person who wrote the code doesn't understand what it does and will confuse the reader.
- There is no evaluation of the performance impact of these and there are no tests for correctness. The latter is important, as it would have found some of these bugs.
I only got half way down the diff before I got tired of seeing the same mistakes repeated. It needs at least one more round of detailed review before it is ready to go in the tree.
Comment Actions
Hi David;
I was waiting for Oliver to comment but he has been really busy all this week.
Rest assured that your comments are being taken seriously.
From my side, I will of course not commit anything until it's ready and all comments have been addressed..
I was considering a commit before your review but given the code is big I was waiting for more review.
BTW, to Oliver: please close all the other code review requests you opened it is a mess already.
- All of the #ifdef clang should be changed to something like __FORTIFY_WORKING_OBJECT_SIZE which can be changed in one place for clang 3.8 (which appears to contain fixes for the relevant bugs).
Indeed, we have been in touch with Google Chrome developers, and we pointed them to that bug in particular, which is the reason why the issue is being worked on for clang-3.8. They also pointed back at us other three bugs and it's not clear they will be fixed.
I don't like such a long name for a macro. Please propose something shorter that still makes sense
- The implementations in the .c files are full of logic errors. If the size is not known at compile time, you're skipping doing overflow checks, even though these checks don't depend on these things. The overflow checks should be done unconditionally in the underlying functions anyway, irrespective of whether FORTIFY_SOURCE is defined.
This will certainly have to be reviewed, however we want FORTIFY_SOURCE to be non-invasive: we will not touch the base headers or code and the replacement functions may have to repeat some of the functionality..
- __bos is not a human-friendly name. __bos0 is both not human-friendly and exposes implementation details of the underlying builtin. Please rename these to something human friendly.
These are likely to be controversial no matter what we choose: after the bikeshed leading to __result_use_check I got request to go back to the original __wur and other BSDs have something completely different . In general we like to have this similar to other implementations: bionic has added new checks during all the summer and we would like to make them easier to port them.
I don't really care much about it. Let's get settled on a name : __object_size and __object_size_type0`` ?
- Many of these checks could be _Static_asserts, triggering at compile time (it's braindead to abort at run time for something that the compiler knows broken at compile time).
- There are a number of __predict_false for compile-time constants. These imply that the person who wrote the code doesn't understand what it does and will confuse the reader.
These great ideas, and the reason why I had wanted you to mentor this project :). These all came from either Google or NetBSD, I don't recall where.
There was at least one undocumented issue used to workaround clang inlining bugs. I think I have been thinking about waiting for the clang update before revisiting that issue.
- There is no evaluation of the performance impact of these and there are no tests for correctness. The latter is important, as it would have found some of these bugs.
We do have some tests from NetBSD that haven't been integrated. I honestly am only concerned about correctness at this time. My idea from the start has been to have this disabled and consider it only as an alternative to the sanitizers.
We ran all base through this and then an exp-run over the ports tree. Some overal comments are:
- The FORTIFY_SOURCE compile time checks work well only with GCC, and we carry the alloc_size attributes that are really useful for newer versions but not for GCC-4.2.1.
- With GCC-4.2.1 we found two bugs: one in libdtrace and another in an snmpd module. Both were also identified by Coverity, which doesn't take merit from the checks because
- Not everyone has access to Coverity.
- While we have access to Coverity, the bugs were there and were real. So it's clearly good
to have another tool that can find such issues.
- The runtime checks with clang found one bug, which was not detected by Coverity but was real. In order to find more similar bugs we need to exercise the code, probably through a fuzzing tool.
From the exp-run over ports:
- It appears we don't have any buffer overflow within the toolchain, at least nothing that appears during normal operation. That is quite notable, IMHO.
- A test that appeared to be simple and maybe even useless, namely umask_chk, found a bug in GNU emacs that was not detected by FORTIFY_SOURCE in linux.
I only got half way down the diff before I got tired of seeing the same mistakes repeated. It needs at least one more round of detailed review before it is ready to go in the tree.
I have no hurry to get this in the tree. I feel like we may not be far but I have had that
feeling for a long while and I can live with a longer test-fix-review cycle. As long as we have valuable code review (thanks David) the code will only keep improving.
Comment Actions
hmm, apparently this sat in my account unsubmitted. No idea is it still useful.
| lib/libc/secure/Symbol.map | ||
|---|---|---|
| 15 | The version_gen.awk does not need empty symbol lists AFAIR. The Symbol.map files are not used as the direct linker input, they are merged together, directed by the Version.def, using the awk script. The Version.def determines the versions used and the inheritance relations. This is done to make some scalability and locality for the versioning. That said, all used versions must appear in Version.def, but individual Symbol.map files only must define actual symbol's versions. | |
Comment Actions
The review yes, the code is not.
This is basically what is left out of GSoC 2015 project referred to here:
https://wiki.freebsd.org/SummerOfCode2015/FreeBSDLibcSecurityExtensions
and while it's incomplete and outdated it is important that it doesn't disappear completely.