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 [email protected] https://lists.nongnu.org/mailman/listinfo/nmh-workers
