diff options
Diffstat (limited to 'doc/securecoding.txt')
-rw-r--r-- | doc/securecoding.txt | 142 |
1 files changed, 142 insertions, 0 deletions
diff --git a/doc/securecoding.txt b/doc/securecoding.txt new file mode 100644 index 0000000..72489ae --- /dev/null +++ b/doc/securecoding.txt @@ -0,0 +1,142 @@ +Simplicity provides security. The more you have to remember to maintain +security the easier it is to forget something. + + +Use Multiple Layers of Security +------------------------------- + +Input validation is useful to prevent clients from taking too much server +resources. Add the restrictions only where it's useful. For example a +simple "maximum line length" will limit the length of pretty much all +possible client input. + +Don't rely on input validation. Maybe you missed something. Maybe someone +calls your function somewhere else where you didn't originally intend it. +Maybe someone makes the input validation less restrictive for some reason. +Point is, it's not an excuse to cause a security hole just because input +wasn't what you expected it to be. + +Don't trust memory. If code somewhere overflowed a buffer, don't make it +easier to exploit it. For example if you have code: + + static char staticbuf[100]; + .. + char stackbuf[100]; + strcpy(stackbuf, staticbuf); + +Just because staticbuf was declared as [100], it doesn't mean it couldn't +contain more data. Overflowing static buffers can't be directly exploited, +but the strcpy() overflowing stackbuf makes it possible. Always copy data +with bounds checking. + + +Prevent Buffer Overflows +------------------------ + +Avoid writing to buffers directly. Write everything through buffer API +(lib/buffer.h) which guarantees protection against buffer overflows. +There are various safe string APIs as well (lib/str.h, lib/strfuncs.h). +Dovecot also provides a type safe array API (lib/array.h). + +If you do write to buffers directly, mark the code with /* @UNSAFE */ +unless it's _obviously_ safe. Only obviously safe code is calling a +function with (buffer, sizeof(buffer)) parameters. If you do _any_ +calculations with buffer size, mark it unsafe. + +Use const with buffers whenever you can. It guarantees that you can't +accidentally modify it. + +Use "char *" only for NUL-terminated strings. Use "unsigned char *" +if it's not guaranteed to be NUL-terminated. + + +Avoid free() +------------ + +Accessing freed memory is the most difficult problem to solve with C code. +Only real solution is to use garbage collector, but it's not possible to +write a portable GC without radical changes in how you write code. + +There are a few ways to avoid most free() calls however: data stack and +memory pools. + +Data stack works in somewhat similar way to C's control stack. alloca() is +quite near to what it does, but there's one major difference: Stack frames +are explicitly defined, so functions can return values allocated from data +stack. t_strdup_printf() call is an excellent example of why this is +useful. Rather than creating some arbitrary sized buffer and using +snprintf() which may truncate the value, you can just use t_strdup_printf() +without worrying about buffer sizes being large enough. + +Try to keep the allocations from data stack small, since the data stack's +highest memory usage size is kept for the rest of the process's lifetime. +The initial data stack size is 32kB and it should be enough in normal use. +See lib/data-stack.h. + +Memory pools are useful when you have to construct an object from multiple +pieces and you can free it all at once. Actually Dovecot's Memory Pool API +is just an abstract class for allocating memory. There's system_pool for +allocating memory with calloc(), realloc() and free() and you can create a +pool to allocate memory from data stack. If your function needs to allocate +memory for multiple objects, you may want to take struct pool as parameter +to allow caller to specify where the memory is allocated from. +See lib/mempool.h + + +Deinitialize safely +------------------- + +Whenever you free a pointer, set it to NULL. That way if you accidentally +try to free it again, it's less likely to cause a security hole. Dovecot +does this automatically with most of its free() calls, but you should also +make it a habit of making all your _destroy() functions take a +pointer-to-pointer parameter which you set to NULL. + +Don't Keep Secrets +------------------ + +We don't do anything special to protect ourself against read access buffer +overflows, so don't store anything sensitive in memory. We use multiple +processes to protect sensitive information between users. + +When dealing with passwords and such, erase them from memory after you +don't need it anymore. Note that such memset() may be optimized away by +compiler, use safe_memset(). + + +Use GCC Extensions +------------------ + +GCC makes it easy to catch some potential errors: + +Format string vulnerabilities can be prevented by marking all functions +using format strings with __attr_format__() and __attr_format_arg__() +macros and using -Wformat=2 GCC option. + +-W option checks that you don't compare signed and unsigned variables. + +I hope GCC will later emit a warning whenever there's potential integer +truncation. -Wconversion kind of does that, but it's not really meant for +it and it gives too many other useless warnings. + + +Use union Safely +---------------- + +Suppose there was code: + +union { + unsigned int number; + char *str; +} u; + +If it was possible for user to set number arbitrarily, but access the union +as string it'd be possible to read or write arbitrary memory locations. + +There's two ways to handle this. First would be to avoid union entirely and +use a struct instead. You don't really need the extra few bytes of memory +that union saves. + +Another way is to access the union only through macro that verifies that +you're accessing it correctly. See IMAP_ARG_*() macros in +lib-imap/imap-parser.h. |