Showing posts with label Security. Show all posts
Showing posts with label Security. Show all posts

Tuesday, 6 January 2015

GCC is wonderful: diagnostic options you should use

In my previous post, I explored how a few extensions to C provided by GCC (and Clang) can be used to implement a better ARRAY_SIZE macro, with type-safety. There are other wonderful things about GCC, though, including the set of compiler warning and error options that it provides. If you thought that ‘-Wall’ enabled all warnings, you may wish to take a closer look, as you’ll otherwise be missing out on many useful checks that could save you a lot of time and effort.

Of course, it’s not a simple case of enabling all the warning options, making sure nothing gets printed and hey presto (or ‘Abracadabra’), you have perfect code. I would liken using a compiler’s diagnostic options to tying up one’s shoelaces: it won’t guarantee that you won’t trip, and it certainly won’t protect you if you walk out into the street without looking, but it’s still a fairly sensible idea!

For the C language family, these diagnostic options can be divided into a few categories:
  • Warnings about the use of old-style ‘K&R’ or ‘traditional’ (pre-ANSI/ISO) C. Almost any C compiler worth caring about supports C90 (or more formally, ISO/IEC 9899:1990 and briefly ANSI X3.159-1989) or newer, these days. Unless you depend upon libraries which have traditional-style declarations in their headers (either accidentally or deliberately).
  • Warnings about poor practices that are simply best avoided.
  • Warnings about potential issues that one might not always choose to address, as these could sometimes be considered as false positives.
Quite a few of these tie in with common misconceptions about C, some of which are equally applicable to C++. GCC also has warning and error options for C++, Objective C and Objective C++ which are to be recommended, although I shall not cover them here.

Starting with GCC 4.2, any individual warning option can be promoted to an error. Prior to this, only a few specific warning options had an equivalent error option, although all enabled warnings could be turned into errors with a single option (-Werror), if desired. There are many situations where it is beneficial to allow false positives to be raised by the compiler without causing compilation to fail (such as many of the sub-options specified by -Wextra and a few by -Wall), and it’s certainly nice to avoid causing unnecessary pain to package maintainers and to users building your code from source, if yours is an open source project.

What follows is a run through of some of these options, together with some examples of the sorts of issues that they will help you to avoid. To begin with, I’ll focus on several options that you may wish to enable as early on in a project as possible, as they are the most likely ones to affect header files, and are the strongest candidates for turning into errors. I will then move on to discuss other useful warning options, before briefly mentioning one option that’s not quite as useful as one might otherwise imagine.

-Wwrite-strings

The -Wwrite-strings option isn’t actually a warning option at all, but masquerades as one: its effect is to modify the language, altering the type that is used for string literals in C from ‘char *’ to ‘const char *’, as is used by C++. No additional option is needed to produce warnings when implicitly dropping the const qualifier, as the messages that GCC emits for this are enabled by default.

You may wonder why on earth C uses ‘char *’ in the first place, especially given that string literals are not typically (at least on systems that use virtual memory) held in memory that is writeable. My suspicion here is that an awful lot of code had already been written by the time the const keyword was added, and it was deemed too big a task to modify all of it (there’d certainly be some precedence for this). To avoid getting stuck in the same mess, I would advise any new project to start off using this option.

If you’re writing a library, you should make sure at a minimum to use const anywhere in your headers where a string literal may be used, as you’ll make a mess for those writing applications in C++, otherwise. If your test code is written in C (and not C++), make sure it uses -Wwrite-strings to help check for missing const qualifiers.

In many established projects, you may find you get an awful lot of noise from a very small set of functions, which is likely to include wrappers around printf() and family and wrappers around write() or fwrite(). Once you’ve dealt with these, the warnings will start to relate a little more to potentially risky situations, and you might even find a situation where an attempt could be made to modify or call free() upon a string literal, perhaps in some of the less well-tested error paths of the codebase. Once you’ve got into the habit of using const throughout your codebase (to do otherwise would produce warnings when compiling your test code) you may find that it will help to catch a number of issues. Supposing we have a function for parsing IPv6 addresses, declared as:
bool read_ipv6_addr(uint8_t *ipv6_bin, const char *str);
We will have been forced into adding the const qualifier to str as shown above when writing our test code:
uint8_t ipv6_addr_buf[16];
static const uint8_t ipv6_addr_localhost[16] =
    { 0,0,0,0, 0,0,0,0, 0,0,0,0, 0,0,0,1 };
CU_ASSERT(read_ipv6_addr(ipv6_addr_buf, "::1"));
CU_ASSERT(!memcmp(ipv6_addr_buf, ipv6_addr_localhost));
Provided we otherwise maintain good discipline and introduce const qualifiers where we know a string will never need to be modified, we can entirely avoid ever accidentally transposing the arguments to read_ipv6_addr(), and the marathon of a debugging session that might otherwise arise from such a trivial mistake.

-Werror=implicit-function-declaration

My suggestion is to turn this into an error, although you could enable it just as a warning, as -Wimplicit-function-declaration. An implicit function declaration occurs at the earliest place where a function is called within a C source file, if the compiler has not already parsed a function declaration (or definition). What this typically means is that you forgot to include the header with the prototype for the function, so the compiler assumes you’ve called it correctly. It works out what the function’s parameters are from the arguments that have been passed to it, and then checks that the arguments of any subsequent calls within the file match the parameters of the implicit declaration. Either that, or you have included the correct header file, and the function prototype is entirely missing from it. In any case, the behaviour is the same: if the function takes a pointer and you’ve forgotten to pass it, the likelihood is one of an instant segfault, and if the code is called infrequently, there’s every chance that this will slip through testing.

Implicit function declarations are so nasty that GCC had an option to flag them as errors long before the release of GCC 4.2 (going back at least as far as GCC 2.95), although it was named -Werror-implicit-function-declaration, and this older name for the option is still supported in current versions of GCC. If you don’t care too much about GCC 4.1 and older, and if you plan on using any of the other -Werror= options described below, you may just as well use the newer syntax consistently.

C++ doesn’t allow implicit function declarations and that’s probably a fair indicator that you don’t want to allow them when using C, either. If you’re using a library function that someone forgot to include in the header files, you will just have to settle for declaring your own prototype, although it’s probably best to do this in a wrapper around the library’s own header file. This can get a little messy, but it’s still better than relying upon implicit declarations.

-Werror=strict-prototypes

This option will catch function declarations that are not ANSI C prototypes. If the compiler catches one of these, that means one of two things. Either the function takes no parameters but this has not been properly indicated where the function is declared, or worse, the function does take parameters but these have not been listed in the declaration. We’re basically back to the same behaviour as with implicit function declarations, where the compiler works on the basis of the earliest call to the function within the source file (or the function’s definition, if that happens to be found first).

This tends to throw C++ programmers who are used to thinking of C as a subset of C++, but yes, an empty parameter list in C means ‘don’t check’, whereas in C++ it is equivalent to writing void. If you specify void for the parameter list, the compiler can check to make sure you’re not mistakenly passing parameters to a function that takes none in the belief that it will use them. If the function does actually take parameters, the compiler will detect that the function’s definition (i.e. the function itself) does not match its prototype and will throw a compilation error.

Basically, unless you’re using C++:
int foo();     /* WRONG: K&R declaration, args not checked */
int foo(void); /* RIGHT: ANSI C prototype */
-Werror=missing-prototypes

This catches cases where a function has been defined globally (i.e. without the static keyword) but the prototype for the function has not been found. If the function is only called locally, you can simply mark it as static and move on. Otherwise, you have either forgotten to include the header in which it is declared, or you have included the correct header but again, it has not been declared within that header. Either way, the result is the same: the compiler cannot check that the function prototype and function definition actually match, and so it cannot provide any guarantee that the function is being called correctly.

-Wformat -Werror=format-security

Certain incorrect uses of the printf() family of functions can remarkably result in security vulnerabilities. Even if security is not a direct concern of yours (perhaps all input that your program will receive can be safely trusted), this will still help to catch some common mistakes. -Wformat itself will check that the argument list matches the format string, and is included as part of -Wall. However, these warnings can sometimes be benign, so I wouldn’t strictly recommend turning them into errors in all cases. The more severe issues to look out for are those where too few arguments are passed, where an integral type is passed but a char * is expected for use with %s, or the wrong type of pointer has been passed for use with the %s specifier.

-Werror=format-security is another matter, in that it specifically checks for situations where the format string is not a string literal, and no format arguments are passed. In other words, printf(str) has been used where printf("%s", str) or perhaps puts(str) would do just as well. In cases where str is not from trusted source, this can result in a uncontrolled format string vulnerability, allowing arbitrary code to be executed when a maliciously constructed string is passed to the program.

When -Wformat is used, GCC can also check that any wrappers for functions in the printf() family have been called correctly, provided their prototypes have been annotated with the format function attribute, which I shall demonstrate in a later post concerning the language extensions that GCC provides, including function, variable and type attributes.

-Wall

This is an alias for a whole load of other warning options which catch a whole load of common mistakes, and comes strongly recommended, although I would not suggest turning these into errors, simply because this option includes -Wmaybe-uninitialized which can produce false positives. Actually, the -Wmaybe-uninitialized option was only introduced in GCC 4.7, but in older versions these same false positives are produced with -Wuninitialized, which is also included in -Wall. With GCC 4.7 and later, you could perhaps use -Werror=all -Wno-error=maybe-uninitialized but there is a fair likelihood that it may be inconvenient for you to have GCC treat other warnings that are enabled by -Wall as errors. It it worth keeping in mind that the checking for certain options included in -Wall varies depending on the level of code optimisation in use, and that the precise details of this and the compiler’s propensity to report false positives will vary between versions.

-Werror=parentheses

This helps catch cases where missing brackets or braces affect the behaviour of a program, and is included (as a warning) by -Wall. This actually checks for quite a few different omissions, but the main concerns are the accidental use of the assignment operator (=) instead of the equality operator (==) where a truth value is expected, and common mistakes relating to C’s operator precedence, which as Dennis Ritchie had admitted is not ideal.

-Werror=undef

This causes preprocessing to fail if an undefined identifier is used within a ‘#if’ directive, rather than simply substituting 0 and carrying on as if nothing happened. This is almost always due to a missing include or a missing macro definition, which is probably not something you want ignoring. One great thing about this option is it allows you to switch away from using #ifdef with little confidence when checking a definition which you do not currently have enabled (where you could easily have made a typo, or failed to include the appropriate header) to using #if with complete assurance that if a feature is disabled, the checking of that feature is correct and an explicit value of ‘0’ has been set.

One option to avoid

The -Wshadow option is just one option that is somewhat less than useful. The tl;dr of the argument that Linus makes: do you really want a warning (or even compilation failure) whenever you define a local variable named index and you just happen to have included <strings.h>?

In closing

There are quite a few more options that you may wish to use, and my recommendation would be to read the Warning Options page of the GCC manual, if you want to know more.

glibc users may find that they get stricter warnings regarding use of the certain C library functions (including unused return values from read()) if they compile with -D_FORTIFY_SOURCE=2, although Ubuntu enables fortification by default.

If you’re using GCC 4.1, 4.2, 4.3, 4.4 or 4.5, you may also find the -fdiagnostics-show-option useful: at the end of each warning message that is printed, it will add the name of the warning option that controls that message, allowing you to look up the entry for that option in GCC’s manual if you wish, but also allowing you an easier way of determining which option to disable if you require. This became the default in GCC 4.6. (Also, ignore the typo in the documentation for it in GCC 4.1: the option name given here is the correct one.)

If you’re using GCC 4.9, you might chose to enable colourisation of GCC’s diagnostic messages. One way is to simply set the GCC_COLORS environment variable to ‘auto’ (meaning that colourisation will occur when GCC’s output is to a terminal). In GCC 5.0, ‘auto’ will be the default.

There are various static code analysis tools that are worth considering, such as Sparse and Coccinelle (with an appropriate set of semantic match files), and there’s a good deal of run-time analysis that can be helpful, too, e.g. AddressSanitizer, ThreadSanitizer and MemorySanitizer and the venerable Valgrind.

Monday, 29 December 2014

Shellshock and Bash

This has been discussed pretty widely already, but I include here my views on ShellShock that I made in a private post to Facebook on 26th September 2014. It might seem loosely worded in places but keep in mind that this is intended to be read by an audience of mixed technical ability. My own recollection is of quite a lot of panic and some amount of technical discussion of the problem, but very little discussion of the practices that allowed this to happen.

Firstly, from my initial posting upon finding out about the bug, a short comment on the state of the software industry:
This ‘ShellShock’ bug that’s got the net in a panic is amazing. It’s 22 years old, affects sites all over the Internet and it’s almost unthinkable that it could exist. The reason is the software that’s affected, Bash, is so cumbersome that hardly anyone has bothered to study it. I might start posting more about the sorry state of the software industry (and the surveillance industry), but I’ll try not to make it seem like ‘boring technical stuff’. This is about governance of infrastructure that affects us all. If your water, gas or electric supply were so badly mismanaged, you’d all be up in arms!
 Then, a look into Bash and Shellshock:
What is Bash? Well the name is from the appalling geek humour: “Bourne-Again SHell”. Steve Bourne was the person at Bell Labs who wrote the original shell, and Bash is a rewrite with some extra bits and pieces. The original was okay, but the rewrite has been widely criticised as overly complex and slow. There’s been some pressure to replace it, but this has been resisted by those preferring to take the “easy option”. Many will not have learnt from this that they were taking the hard option all along, and will continue to make the same mistakes.
In computing, a “shell” is just the part of the user interface that comes built into the system. In Windows this would include your Taskbar and Explorer and on a Mac, it’d include the Dock and Finder. This also includes the “open file”, “save file”, “print”, “are you sure?” boxes, and that sort of thing. 
The shell we’re talking about here is much simpler than this. You type the name of a program, press enter, and it runs it. From MS-DOS, maybe you’ve heard of “COMMAND.COM”, or perhaps you’ve seen “CMD.EXE” in Windows. They’re basically the same thing. 
That sounds like a pretty simple thing, right? Well, sure, but it doesn’t sound like a very friendly user interface, does it? Well, it isn’t. As a first step, we might want to run some sequence of programs and have that sequence stored in a list (such as “copy music from CD” and then “add files to music database”). Then again, we may not always want exactly the same sequence in every situation (maybe if I have my MP3 player attached, I want my music copying there, too). Before we even know it, we’ve ended up with what is really just a very badly designed programming language, and not the simple shell that was originally intended. 
This is where it all goes horribly wrong. Due to an almighty ‘oversight’, what is mainly used as a very simple program for running other programs will actually look for instructions to obey in any of the settings that are supplied to it. These settings (which we call “environment variables”) might well be something that a user has typed in to a web page before hitting “submit”, so where a web server just needs to run the shell to send off an email, which might need to be given NAME and ADDRESS settings to compose the mail, there might be nothing stopping the the user from entering their name as “() {:;}; rm -fr /” (define a subroutine that does nothing, and then delete all files on the system). 
We seem to be caught in a combination of two attitudes: 
  • Conservatism: “Don’t fix it if it’s working, even if it’s probably broken and we just don’t know it yet. We can’t afford to make changes that might have adverse effects [especially if this disrupts our customers at all]”.
  • Recklessness: “We need more cool features and we can always fix it later. Our programming isn’t confusing, you’re just not very good at understanding it”.
Two of the few systems that have replaced Bash with something simpler are Debian, which is a community-run project without commercial concerns, and Ubuntu, upon which it is based. It's interesting that they would take the plunge and advise some caution about upgrading, whereas Apple (Mac OS X), Red Hat, openSUSE and various other systems have not made this decision. 
Much of the problem with Bash would have been easily avoided if its extra behaviour was disabled unless specifically asked for, as we could then have switched to something simpler with much greater confidence. In fact, some means of allowing Bash to throw up warnings when this extra behaviour is used might not be a bad thing to have. I’ve had to get rid of it from systems during my career and it has been frustrating to remove the ‘Bashisms’ that programmers leave in without knowing or caring. 
An especially frustrating part was dealing with software given to us by suppliers, knowing full well that my counterparts in other companies would be going through the same frustrations, not being allowed to spare them the frustration by working with the supplier and knowing that the supplier would not be terribly interested or responsive in dealing with this themselves or even in accepting changes that I would provide to them.
A friend of mine pointed out that the BSDs now avoid using Bash as a system shell, but that only newer versions (and not older versions) of OS X use Bash. My view: “To change their system shell to Bash seems a moronic thing for Apple to have done.”

As an aside, anyone who has even the slightest level of familiarity with C who hasn’t seen the original Bourne Shell source code should read about it now — but make sure that you have some Optrex and Mindbleach to hand, first, though! Steve Bourne used C macros to #define C into something that can at best be described as a cross between C, shell script and Algol. I suppose somebody had to do it first so that the rest of us could learn not to.