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.