EPANET
EPANET copied to clipboard
Bound checking on string operations
Discussion on #133 suggested that string operations with sprintf
, strcat
and strcpy
should instead use snprintf
and strncat
and strncpy
. Using the 'n' functions protects against buffer overflow attacks by checking the bounds so that unlimited input is not allowed.
The strn***
set of functions do not always null-terminate, so you either have to assign my_str[len] = '\0'
or use the strlcpy
family of functions. I'm not sure about windows availability of these "strl***
" functions, but we could re-implement if they're not available.
Several different cases here. Overall, there may be some security advantages to making these changes but the changes feel risky without an existing set of tests to develop against. Thoughts?
printing
grep finds:
- 136 uses of sprintf. Looks like most of these print to a buffer of a known size so should be straightforward to use snprintf instead
- 0 uses of snprintf
copying
grep finds:
- 132 uses of strcpy The security team at CERN suggests using strlcpy if its available or defining it as below.
- 58 uses of strncpy. Seems like these should just be left alone.
#ifndef strlcpy
#define strlcpy(dst,src,sz) snprintf((dst), (sz), "%s", (src))
#endif
concatenation
grep finds:
- 30 uses of strcat. Most of these appear to build up lines for writing to the rpt file. Is there a simple #define for strlcat? Otherwise re-writing with strnat may be the best way.
- 0 uses of strncat