Scott Rotondo's Weblog

June 2005 »
Sun	Mon	Tue	Wed	Thu	Fri	Sat
			1	2	3	4
5	6	7	8	9	10	11
12	13	14	15	16	17	18
19	20	21	22	23	24	25
26	27	28	29	30

Today

In honor of the release of OpenSolaris today, lots of Sun engineers have written blog entries about various things they implemented or fixed in Solaris. Check them out; there's lots of great technical content and some fascinating stories. Instead of writing about one specific part of the source code, I decided to describe something I did that involved small changes in dozens of source files.

About eighteen months ago, I changed the build process to make use of a new lint option that is designed to detect coding practices that could lead to security vulnerabilities. This checking is controlled by the -errsecurity option on the lint command line, which runs the security checks at one of three levels.

Changing the makefiles to add an option to each lint command line was trivial. All I had to do was add the following lines to usr/src/Makefile.master:

ALWAYS_LINT_DEFS +=   -errsecurity=$(SECLEVEL)
ALWAYS_LINT_DEFS +=   -erroff=E_SEC_CREAT_WITHOUT_EXCL
ALWAYS_LINT_DEFS +=   -erroff=E_SEC_FORBIDDEN_WARN_CREAT

SECLEVEL=     core

With these lines in Makefile.master, a simple make lint in any source directory will run lint with -errsecurity=core. To run the security checks at one of the other levels, just override the SECLEVEL variable on the command line; e.g., make lint SECLEVEL=standard.

Then the real work began. Adding this option caused lint to produce about 500 additional warnings. I needed to fix all of these before integrating my change to Makefile.master because we require the Solaris source to be lint-clean.¹ Interestingly, almost all of the problems detected by lint resulted in one of just three error messages. Since newly written code is likely to produce many of the same warnings, I thought it would be useful to describe what these messages mean and how to fix them.

1. variable format specifier to printf()

Format strings used with the printf family of functions should be fixed string literals. If they are variables derived from user input, several attacks are possible, particularly using the %n format specifier. Some techniques for fixing code like this:

Instead of printf(str) use printf("%s", str) or puts(str).
For functions that are wrappers around printf for debugging, error messages, etc., be sure the function is marked with the /*PRINTFLIKE*/ lint directive. This allows lint to verify that the arguments to the function match the format string, and it suppresses the warning when the variable format string is passed to vprintf.
Note that lint treats the strings returned from gettext and dgettext as constants, so it's OK to use them in place of string literals.
If you must use a variable format string (e.g. to select at runtime between two fixed format strings), declare the variable as a const char * to suppress the lint warning.

2. format argument to sprintf() contains an unbounded string specifier

A format string that contains %s can write an unlimited amount of output, possibly overflowing the fixed-size buffer passed to sprintf. Two simple techniques to fix this are:

Use snprintf instead of sprintf. The size argument places an upper bound on the number of characters that will be written to the buffer.
Change the format string to limit the number of characters written, e.g. %10s to write at most 10 characters. This is also the reason lint does not complain about sprintf calls that use fixed-size formats like %d.

3. format argument to scanf() contains an unbounded string specifier

Just as it does in the case above, a %s in a scanf format string can write an unlimited number of characters and possibly overflow the buffer supplied. For scanf, however, only one fix is available:

Change the format string to limit the number of characters. Important: The size given in the format string does not include the trailing null character, so a format of %10s requres an 11-byte buffer.

It would be nice to be able to pass the buffer size to scanf as an argument so you could use a manifest constant instead of a hard-coded string. Sadly, scanf doesn't share that bit of format-string syntax with printf. However, with a little preprocessor magic you can come pretty close; here's an example from usr/src/lib/libdevinfo/devfsmap.c:

/*
 *      Macros to produce a quoted string containing the value of a
 *      preprocessor macro. For example, if SIZE is defined to be 256,
 *      VAL2STR(SIZE) is "256". This is used to construct format
 *      strings for scanf-family functions below.
 */
#define QUOTE(x)        #x
#define VAL2STR(x)      QUOTE(x)

char ctd[MAXNAMELEN + 1];

while (fscanf(fp, "%" VAL2STR(MAXNAMELEN) "s", ctd) == 1) {
	...
}

1. To be more precise, a top-down build runs lint on only part of the source code, and we require that subset of the code to remain lint-clean. There is plenty of code that hasn't been made lint-clean yet, either because the code is old or because it comes from an external source and we don't want to diverge from the outside version. See usr/src/Makefile.lint to find out which source directories are linted.

Technorati Tag: OpenSolaris
Technorati Tag: Solaris