Hi Robert,
Robert Elz wrote on Mon, Dec 16, 2013 at 01:41:19PM +0700:
Ingo Schwarze wrote on Mon, 16 Dec 2013 06:24:02 +0100:
[...]
| Well, the warning is not so much about "could" or "might" (in theory),
| but about *practical* experience of code auditors. It reads:
|
| warning: strcat() is almost always misused, please use strlcat()
"Almost always"??? specially when the function is strcpy, not strcat ?
Really - that suggests that it is almost never used correctly - and that's
nonsense.
Again, the intention isn't "every single use is broken", but "almost
every real-world codebase using it has at least some issues".
That's not nonsense - even if you might succeed in showing one
codebase, or a few, without any known issues.
I have no problem with a tool that checks for things like this ad produces
warnings, and allows develoers to check their code for correctness. To
be really useful, it should avoid wanrning about the obviously correct
uses, like ...
d = malloc(strlen(s) + 1);
if (d != 0)
strcpy(d, s);
return d;
And yes, know this is strdup() - but I would not be surprised if many
of the uses of strcpy() in nmh are just like that.
Some are, yes.
Besides, the above is not strdup(), which is usually implemented in
terms of memcpy(), which is more efficient.
nmh (and MH that preceded it) is old - much older than strdup().
Some old code can be improved - made more maintainable, secure, and/or
more efficient - by modernizing it. For example, the above could be
optimized by using memcpy() instead of strcpy() - not recommended -
or optimized *and* made easier to maintain by using strdup().
| In this case, overflow prevention is not even attempted, which is
| typical (rather than the exception) for strcat use in the wild.
I agree, strcat() is more often misused.
Are you going to fix the specific overflow i have shown,
or do you consider it acceptable?
Do you think there are more similar ones in the nmh code base?
But go back to the original
message - the complaint was about strcpy(). Feel free to let us know
which uses of strcpy() in nmh are errors ...
Challenge taken; note that my point here is not to deliver a perfect
or complete code audit (that would be asking a bit much of somebody
asked to comment on manual formatting issues to begin with) but to
show that even a moderately skilled code auditor can very quickly
find very suspicious instances of strcpy() use in the nmh codebase.
Even if you manage to prove that the first suspicious example i'm
showing below is actually correct, i guess the proof will be long
and tricky, and any potential code auditor is quite likely to
lose some hair.
For starters, many of the mh* utilities call sbr/path.c pluspath()
on command line arguments. So far, these are not even limited to
BUFSIZ. Pluspath passes the *name to sbr/path.c path(), that one
to sbr/path.c expath(), which uses snprintf to squeeze it into a
BUFSIZ buffer, silently truncating it (i think i heard somebody
criticise strlcat for silent truncation - which is unfounded when
used correctly - but so i thought some nmh workers don't like silent
truncation, do they?). The truncated buffer - which apparently can
really be BUFSIZ in length at this point - is handed to sbr/m_maildir.c
m_mailpath() which hands it on to sbr/maildir.c m_maildir() and then
to sbr/maildir.c exmaildir(), which does, more or less:
static char mailfold[BUFSIZ];
cp = mailfold;
sprintf (cp, "%s/", mypath); /* typically HOMEDIR iiuc */
cp += strlen (cp);
strcpy (cp, folder); /* boom? */
The latter appears to be completely unguarded, neither checked for
overflow nor for truncation.
Now if i were to perform an actual professional paid code audit,
i would install the software at this point and show that it can
really be crashed this way. As it stands, please merely consider
this as a candidate for a possible error.
That's in the fourth file i looked at, a dozen other or so to go.
Now if i were nasty, i'd boldly and sweepingly claim that nobody
in nmh ever cared about correctness, and that's why strcpy() is
still in use after so many years, excusing broken code with age.
Then i would bring out some popcorn and watch the fireworks.
But that's not my point. Actually, David Levine already agreed in
private mail that an audit would make sense, and i appreciate that.
My point is: In practice, if somebody decides to use or continue
to use strcpy(), it will almost always end up misused at some point,
so please use strlcat() or asprintf() on a case-by-case basis,
because it's so much easier to get that right and verify. Do *not*
do mechanical replacements, though!
ps: often "easier to audit" means "presumed better" which means
"investigated less stringently" which leads to "unsafe".
That sounds like a not so plausible theory. Why would "easier to
audit" discourage anyone from actually doing the work? Who would
be so naive as to think that just because good tools are used, a
piece of work is executed correctly? Wouldn't rather "hard to
audit" deter people? Or let's turn the argument around. If you
say that difficulty of a task helps to find somebody performing it,
how stringently has strcpy() usage been investigated in nmh in the
past? Who did the job, and when, to prove strcpy() usage correct?
There's no shortcut to actually doing the work.
That, i wholeheartedly agree with!
Yours,
Ingo
_______________________________________________
Nmh-workers mailing list
Nmh-workers(_at_)nongnu(_dot_)org
https://lists.nongnu.org/mailman/listinfo/nmh-workers