Mercurial > dovecot > original-hg > dovecot-1.2
annotate doc/securecoding.txt @ 7238:983c6ea05de0 HEAD
Compile fix with some compilers
| author | Timo Sirainen <tss@iki.fi> |
|---|---|
| date | Wed, 13 Feb 2008 20:08:11 +0200 |
| parents | 51406aaccb46 |
| children |
| rev | line source |
|---|---|
| 1787 | 1 Simplicity provides security. The more you have to remember to maintain |
| 2 security the easier it is to forget something. | |
| 3 | |
| 4 | |
| 5 Use Multiple Layers of Security | |
| 6 ------------------------------- | |
| 7 | |
| 8 Input validation is useful to prevent clients from taking too much server | |
| 9 resources. Add the restrictions only where it's useful. For example a | |
| 10 simple "maximum line length" will limit the length of pretty much all | |
| 11 possible client input. | |
| 12 | |
| 13 Don't rely on input validation. Maybe you missed something. Maybe someone | |
| 14 calls your function somewhere else where you didn't originally intend it. | |
| 15 Maybe someone makes the input validation less restrictive for some reason. | |
| 16 Point is, it's not an excuse to cause a security hole just because input | |
| 17 wasn't what you expected it to be. | |
| 18 | |
| 19 Don't trust memory. If code somewhere overflowed a buffer, don't make it | |
| 20 easier to exploit it. For example if you have code: | |
| 21 | |
| 22 static char staticbuf[100]; | |
| 23 .. | |
| 24 char stackbuf[100]; | |
| 25 strcpy(stackbuf, staticbuf); | |
| 26 | |
| 27 Just because staticbuf was declared as [100], it doesn't mean it couldn't | |
| 28 contain more data. Overflowing static buffers can't be directly exploited, | |
| 29 but the strcpy() overflowing stackbuf makes it possible. Always copy data | |
| 30 with bounds checking. | |
| 31 | |
| 32 | |
| 33 Prevent Buffer Overflows | |
| 34 ------------------------ | |
| 35 | |
| 36 Avoid writing to buffers directly. Write everything through buffer API | |
| 37 (lib/buffer.h) which guarantees protection against buffer overflows. | |
| 38 There are various safe string APIs as well (lib/str.h, lib/strfuncs.h). | |
|
3872
b054cd10ceaa
Small updates: Added note about array API and updates to data stack.
Timo Sirainen <tss@iki.fi>
parents:
1787
diff
changeset
|
39 Dovecot also provides a type safe array API (lib/array.h). |
| 1787 | 40 |
| 41 If you do write to buffers directly, mark the code with /* @UNSAFE */ | |
| 42 unless it's _obviously_ safe. Only obviously safe code is calling a | |
| 43 function with (buffer, sizeof(buffer)) parameters. If you do _any_ | |
| 44 calculations with buffer size, mark it unsafe. | |
| 45 | |
| 46 Use const with buffers whenever you can. It guarantees that you can't | |
| 47 accidentally modify it. | |
| 48 | |
| 49 Use "char *" only for NUL-terminated strings. Use "unsigned char *" | |
| 50 if it's not guaranteed to be NUL-terminated. | |
| 51 | |
| 52 | |
| 53 Avoid free() | |
| 54 ------------ | |
| 55 | |
| 56 Accessing freed memory is the most difficult problem to solve with C code. | |
| 57 Only real solution is to use garbage collector, but it's not possible to | |
| 58 write a portable GC without radical changes in how you write code. | |
| 59 | |
| 60 I've added support for Boehm GC, but it doesn't seem to be working very | |
| 61 well currently. In any case I'd rather not make it required. | |
| 62 | |
| 63 There are a few ways to avoid most free() calls however: data stack and | |
| 64 memory pools. | |
| 65 | |
| 66 Data stack works in somewhat similiar way to C's control stack. alloca() is | |
| 67 quite near to what it does, but there's one major difference: Stack frames | |
|
3872
b054cd10ceaa
Small updates: Added note about array API and updates to data stack.
Timo Sirainen <tss@iki.fi>
parents:
1787
diff
changeset
|
68 are explicitly defined, so functions can return values allocated from data |
|
b054cd10ceaa
Small updates: Added note about array API and updates to data stack.
Timo Sirainen <tss@iki.fi>
parents:
1787
diff
changeset
|
69 stack. t_strdup_printf() call is an excellent example of why this is |
|
b054cd10ceaa
Small updates: Added note about array API and updates to data stack.
Timo Sirainen <tss@iki.fi>
parents:
1787
diff
changeset
|
70 useful. Rather than creating some arbitrary sized buffer and using |
|
b054cd10ceaa
Small updates: Added note about array API and updates to data stack.
Timo Sirainen <tss@iki.fi>
parents:
1787
diff
changeset
|
71 snprintf() which may truncate the value, you can just use t_strdup_printf() |
|
b054cd10ceaa
Small updates: Added note about array API and updates to data stack.
Timo Sirainen <tss@iki.fi>
parents:
1787
diff
changeset
|
72 without worrying about buffer sizes being large enough. |
|
b054cd10ceaa
Small updates: Added note about array API and updates to data stack.
Timo Sirainen <tss@iki.fi>
parents:
1787
diff
changeset
|
73 |
|
b054cd10ceaa
Small updates: Added note about array API and updates to data stack.
Timo Sirainen <tss@iki.fi>
parents:
1787
diff
changeset
|
74 Try to keep the allocations from data stack small, since the data stack's |
|
b054cd10ceaa
Small updates: Added note about array API and updates to data stack.
Timo Sirainen <tss@iki.fi>
parents:
1787
diff
changeset
|
75 highest memory usage size is kept for the rest of the process's lifetime. |
|
b054cd10ceaa
Small updates: Added note about array API and updates to data stack.
Timo Sirainen <tss@iki.fi>
parents:
1787
diff
changeset
|
76 The initial data stack size is 32kB and it should be enough in normal use. |
|
b054cd10ceaa
Small updates: Added note about array API and updates to data stack.
Timo Sirainen <tss@iki.fi>
parents:
1787
diff
changeset
|
77 See lib/data-stack.h. |
| 1787 | 78 |
| 79 Memory pools are useful when you have to construct an object from multiple | |
| 80 pieces and you can free it all at once. Actually Dovecot's Memory Pool API | |
| 81 is just an abstract class for allocating memory. There's system_pool for | |
| 82 allocating memory with calloc(), realloc() and free() and you can create a | |
| 83 pool to allocate memory from data stack. If your function needs to allocate | |
| 84 memory for multiple objects, you may want to take struct pool as parameter | |
| 85 to allow caller to specify where the memory is allocated from. | |
| 86 See lib/mempool.h | |
| 87 | |
| 88 | |
|
3935
51406aaccb46
Added something about setting freed pointers to NULL.
Timo Sirainen <tss@iki.fi>
parents:
3872
diff
changeset
|
89 Deinitialize safely |
|
51406aaccb46
Added something about setting freed pointers to NULL.
Timo Sirainen <tss@iki.fi>
parents:
3872
diff
changeset
|
90 ------------------- |
|
51406aaccb46
Added something about setting freed pointers to NULL.
Timo Sirainen <tss@iki.fi>
parents:
3872
diff
changeset
|
91 |
|
51406aaccb46
Added something about setting freed pointers to NULL.
Timo Sirainen <tss@iki.fi>
parents:
3872
diff
changeset
|
92 Whenever you free a pointer, set it to NULL. That way if you accidentally |
|
51406aaccb46
Added something about setting freed pointers to NULL.
Timo Sirainen <tss@iki.fi>
parents:
3872
diff
changeset
|
93 try to free it again, it's less likely to cause a security hole. Dovecot |
|
51406aaccb46
Added something about setting freed pointers to NULL.
Timo Sirainen <tss@iki.fi>
parents:
3872
diff
changeset
|
94 does this automatically with most of its free() calls, but you should also |
|
51406aaccb46
Added something about setting freed pointers to NULL.
Timo Sirainen <tss@iki.fi>
parents:
3872
diff
changeset
|
95 make it a habit of making all your _destroy() functions take a |
|
51406aaccb46
Added something about setting freed pointers to NULL.
Timo Sirainen <tss@iki.fi>
parents:
3872
diff
changeset
|
96 pointer-to-pointer parameter which you set to NULL. |
|
51406aaccb46
Added something about setting freed pointers to NULL.
Timo Sirainen <tss@iki.fi>
parents:
3872
diff
changeset
|
97 |
| 1787 | 98 Don't Keep Secrets |
| 99 ------------------ | |
| 100 | |
| 101 We don't do anything special to protect ourself against read access buffer | |
| 102 overflows, so don't store anything sensitive in memory. We use multiple | |
| 103 processes to protect sensitive information between users. | |
| 104 | |
| 105 When dealing with passwords and such, erase them from memory after you | |
| 106 don't need it anymore. Note that such memset() may be optimized away by | |
| 107 compiler, use safe_memset(). | |
| 108 | |
| 109 | |
| 110 Use GCC Extensions | |
| 111 ------------------ | |
| 112 | |
| 113 GCC makes it easy to catch some potential errors: | |
| 114 | |
| 115 Format string vulnerabilities can be prevented by marking all functions | |
| 116 using format strings with __attr_format__() and __attr_format_arg__() | |
| 117 macros and using -Wformat=2 GCC option. | |
| 118 | |
| 119 -W option checks that you don't compare signed and unsigned variables. | |
| 120 | |
| 121 I hope GCC will later emit a warning whenever there's potential integer | |
| 122 truncation. -Wconversion kind of does that, but it's not really meant for | |
| 123 it and it gives too many other useless warnings. | |
| 124 | |
| 125 | |
| 126 Use union Safely | |
| 127 ---------------- | |
| 128 | |
| 129 Suppose there was code: | |
| 130 | |
| 131 union { | |
| 132 unsigned int number; | |
| 133 char *str; | |
| 134 } u; | |
| 135 | |
| 136 If it was possible for user to set number arbitrarily, but access the union | |
| 137 as string it'd be possible to read or write arbitrary memory locations. | |
| 138 | |
| 139 There's two ways to handle this. First would be to avoid union entirely and | |
| 140 use a struct instead. You don't really need the extra few bytes of memory | |
| 141 that union saves. | |
| 142 | |
| 143 Another way is to access the union only through macro that verifies that | |
| 144 you're accessing it correctly. See IMAP_ARG_*() macros in | |
| 145 lib-imap/imap-parser.h. |