Re: Unchecked sprintf/strcat?
* Wed May 24 2006 Rocco Rutte <pdmef@xxxxxxx>
> I think there're many more examples like this and if we want to go
> further in securing the code, we have to do two things (IMHO):
Thank you, I improved my patch. The latest patch is here:
http://www10.plala.or.jp/sanrinsha/tamo/patch-1.5.11cvs.tamo.secwarnings.5
> 1. Don't handle the errors in the wrapper functions but move it out
> to calling code as much as possible because only the caller can know
> what to do in case of an error. An a general abort() is just wrong.
Okay.
Because we have no way to detect overflow after safe_strcat, I make
safe_strcat return NULL when truncation occurs.
snprintf returns "the number of characters that would have been
output if the size were unlimited." - according to "man 3 snprintf."
So it's easy to detect overflow.
strncpy can't do a good job. I just check strlen before strfcpy.
(Well, I haven't tried
"#define safe_strfcpy(A,B,C) *(A)=0;safe_strcat(A,C,B)")
> 2. Really put these into a header and make them available inline. I
> didn't do any profiling yet, but at a point where nearly every libc
> call is wrapped up by a sanity wrapper, mutt could become slower by a
> measurable factor.
Thanks for your suggestion. safe_strfcpy and safe_snprintf are now
macros. (I don't like the printf macro, though.)
> I don't really see a problem with the code right now because it mostly
> ensures there're no buffer overflows (and thus remote code injection and
> all that) making the OS call abort(). We should instead just check
> errors more consequently, perhaps.
Yes, you are really right.
Mutt is good at handling remote data, but we can still improve
local data processing. (see bug #2205 - http://bugs.mutt.org/2205 )
--
tamo