EPANET icon indicating copy to clipboard operation
EPANET copied to clipboard

Bound checking on string operations

Open bradleyjeck opened this issue 7 years ago • 2 comments

Discussion on #133 suggested that string operations with sprintf, strcat and strcpyshould 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.

bradleyjeck avatar Nov 09 '17 14:11 bradleyjeck

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.

samhatchett avatar Nov 09 '17 15:11 samhatchett

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

bradleyjeck avatar Nov 09 '17 23:11 bradleyjeck