Date: Mon, 16 Dec 2013 14:31:30 +0100
From: Ingo Schwarze <schwarze(_at_)usta(_dot_)de>
Message-ID: <20131216133130(_dot_)GA31174(_at_)iris(_dot_)usta(_dot_)de>
| Again, the intention isn't "every single use is broken", but "almost
| every real-world codebase using it has at least some issues".
That maybe true, but the linker cannot know whether the particular code
it is linking has been validated or not - it is the wrong place to put
that kind of check (it is a different thing for the APIs that cannot be
used safely, like good old gets) - and the message is the wrong one to give.
| Besides, the above is not strdup(), which is usually implemented in
| terms of memcpy(), which is more efficient.
I didn't mean in terms of implementation, just the resulting operation.
(I just pointed that out so everyone wouldn't reply and say "it should use
strdup()" - which it couldn't when it was written, as strdup() didn't
exist, and once it was written, there's no real benefit in going back
and replacing it all.).
| Some old code can be improved - made more maintainable, secure, and/or
| more efficient - by modernizing it.
Of course. The question is whether doing that is worth the bother.
For nmh, probably not, mostly - it has been around a long time, used
by a lot of people (though, sadly, less and less as time passes) and
most likely isn't really worth any huge effort that isn't likely to
produce some measurable gains (and "feel good" isn't measurable enough).
| Are you going to fix the specific overflow i have shown,
| or do you consider it acceptable?
I think Ken probably will, I have never been an nmh developer (they
made some obnoxious decisions, related to the UI - not coding - way
back, and I could never reconcile myself to doing more than working
around it, and mostly ignoring them...)
But personally, no, I don't think that one was enough of a problem to
care about. Even back when BUFSIZ was just 512 (which is long long past)
domain names, whish are never supposed to be longer than 255 bytes,
were never going to overflow a BUFSIZ buffer, unless you're deliberately
trying to crash the code. Users crashing their own code isn't an
interesting issue - it would be different if incoming e-mail could make
it happen, but assuming that people will deliberately configure their
system with absurd values, just to make it fail, isn't a problem I'm
worried about.
And if I were, I'd fix it must the same as the way Paul Visie said he fixed
bind .. just test for things being too long, and abort - let the user
fix their config file so the strings there are reasonable lengths (anything
that is legitimate fits in BUFSIZ, easily) so there's no need to change the
structure of the code to fix that one, no genuine use will ever cause a
problem.
In practice, people don't use localdomain (the variable being strcat'd)
at all - the localname, either from the config file, or from gethostname()
(then passed through getaddrinfo()) contains the domain name already,
and no strcat is ever actually done.
The localdomain stuff was only ever there for people who want to get
gethostname() (reasonable, so the same config file can e shared over
multiple systems), don't have FQDN's in their hostname (dumb, but some
people seem to believe it is the right way) and use /etc/hosts for
hostname lookup, and have it configured with short names (not FQDNs)
as the primary hostname. These days I doubt you could find anyone
who uses it at all. Even way back when /etc/hosts was the primary
name resolution mechanism, only a small subset ever had this problem.
Only those people ever needed localdomain at all.
| Do you think there are more similar ones in the nmh code base?
There might be, some of the code used the "make the buffer big enough,
and it will never overflow" philosophy, which certainly isn't safe, but
in practice, never causes problems either - just as long as it is only
in locally configured data, not stuff that comes from random outsiders.
I think you'll find the code that processes messages is much more careful.
| Challenge taken; note that my point here is not to deliver a perfect
| or complete code audit
Of course.
| 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.
Technically no, but as above, if some user wants to crash some nmh
program by giving an absurdly long folder name as an arg to one of the
commands, let him - what harm is it doing? When they complain, we can
just say "don't do that". Typical folder names are a few tens of bytes
at max (often < 10) - those cause no problems at all. In almost 40 years
of MH usage (most of which time I've been on the mailing list) I don't
think I've ever heard a report of any MH program crashing because the
user gave it a folder name that was too long.
Sure, it would be nicer if that kind of problem didn't exist, but I doubt
it is worth it to actually fix (and certainly not fix in any way more
serious that telling the user he's an idiot, and to try again with more
reasonable args.)
| 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.
I have it installed already, so the test is simple .. I first botched it
by making the string way too long (about 1MB) which simply failed to exex
(arg list too long...) With a more rational arg length, I just a got
"file name too long" errpr. Maybe my string was truncated, but it certainly
didn't crash anything (it did append it to the Mail directory name, and it
was a string, as reported in the error, much longer than stdio.h's BUFSIZ,
which is 1024 on my system).
Actually (upon re-reading before sending this message) I just realised that
my test might not have been the greatest, as by long folder name was a long
flat (no /'s) name, which certainly would have exceeded the longest
rational filename. So I just tried again in a path that was full of /'s
(127 of them) and lots (but not too many) characters between, and that
behaved exactly the same way.
I believe I'm using a fairly pure nmh 1.5 for this (NetBSD has a few
patches, but nothing even close to anything related to these issues).
| Now if i were nasty, i'd boldly and sweepingly claim that nobody
| in nmh ever cared about correctness,
Practical correctness, yes - making it unbreakable, no - there has to be
a cost benefit tradeoff in everything, eventually the benefit from the
fix exceeds the cost of making it, then it is worth fixing, while the benefit
is too small, and the problem too irrelevant, and the cost too extreme, fixing
stuff, just because it can be done, is unjustifiable. Seeking perfection is
stupid - you can't ever achieve it, and the attempt just annoys everyone else.
The right thing to do is to make sure there are no dangerous, or commonly
encountered problems, and then worry about other stuff only if someone
actually falls over it. This one, never has caused a problem I'm aware
of (probably never ever has) so why would anyone worry?
| and that's why strcpy() is still in use after so many years,
Once more, there is absolutely nothing wrong with strcpy(). It just
needs to be used correctly. Sure, it isn't always, but neither is
anything else.
| But that's not my point. Actually, David Levine already agreed in
| private mail that an audit would make sense,
Of course it would - no-one would ever complain about someone volunteering
to look for bugs. It is possible there are some in there that are
actually real problems, rather than just irrelevant. In fact, it is
actually quite likely. But a simple minded "replace strcpy() with
strlcpy()" isn't going to fix any, and if not done very carefully, could
easily cause more problems than it cures .. in order to work, you need to
know how big the buffer is, which is fine for all those buffers that are
BUFSIZ - provided that the strlcpy() only ever starts at the beginning of
the buffer - it gets much messier with malloc'd buffers whose size might not
necessarily be passed down (irrelevant for strcpy, as provided the buffer
was malloc'd big enough, the size isn't needed - but strlcpy() then gets real
hard to use, certainly to use safely - I guess the obvious solution would
just be strlcpy(dst, src, 100000000) .... Your linker test won't object
to that one I think, and it is no better than strcpy().
| 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,
I disagree. I still use strcpy() all the time in code I write. I
believe I use it correctly, it just means being careful from the start.
I have on occasion used strncpy() when it has been appropriate (only
occasionally), I don't think I've ever used strlcpy() - and it is
available on the systems I use.
Using strcpy() properly just means checking all the string lengths, and
either realloc() if the longer string should work, or bailing with an
error, if it shouldn't, when the bounds are exceeded. It is manifestly
absurd to have strlcpy() checking the string lengths, when you have
already checked - and checking in advance is the only really correct way,
as it is the only way that allows the correct remedial action to be taken,
strlcpy() just avoids stupid code from overwriting its buffer - the code
doesn't get any smarter because of that, it is still stupid code. Good
code knows the buffer won't be overflowed, so doesn't need strlcpy().
| That sounds like a not so plausible theory. Why would "easier to
| audit" discourage anyone from actually doing the work?
You have already demonstrated... Code that is likely to have problems
is what everyone wants to hunt in, as there they're far more likely to
find problems. Would you have ever looked at the nmh code this way had
it been using strlcpy() instead of strcpy() ? It was only because you
were confident you'd find some problems that you looked at all.
| Wouldn't rather "hard to audit" deter people?
Not at all - that's the challenge. Sure, some lazy auditors might just
avoid looking, but that's a good thing, as that kind of person would
probably not do a very thorough check anyway. The people you'd want
checking the code are the ones who appreciate the task, and really go hunting
to see what they can find - and for that, a more complex task is more fun,
and certainly more rewarding when it is completed (emotionally rewarding,
financial is pretty much independent of any of this.)
| 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?
Probably it never has. Lots of other potential problems have probably
never been investigated either. Yet it is out there and works, and
while people have problems with nmh in lots of ways, none of them I've
ever heard of are the kinds of problems an audit would fix (more that
some of the user interface issues are weird, and nmh is still from the
dark ages of e-mail, and much modern stuff barely works, if at all).
But it has been a long time since I heard a report of any message giving
mh any problems - aside from the "how do I read this MIME stuff" or
"when I reply, what's all this junk in the message and how do I get rid
of it?" types.
That is, it is quite possible that there are lots of bugs that could be
fixed, but noone of them ever actually bother anyone (any legitimate
user, rather than someone just setting out to show the bug is there), so
why should anyone care?
kre
_______________________________________________
Nmh-workers mailing list
Nmh-workers(_at_)nongnu(_dot_)org
https://lists.nongnu.org/mailman/listinfo/nmh-workers