[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: think twice before enabling -D_FORTIFY_SOURCE=2 for C projects without thorough build-time testing

Thorsten Glaser <tg@mirbsd.de> writes:
> Russ Allbery <rra <at> debian.org> writes:

>> (consider resource exhaustion errors in the crypt implementation, for

> No, the standard said it would either always fail or never, but
> independent on the input data.

I believe that just because POSIX only documents one specific error
condition for a function doesn't mean that's the only error that it is
allowed to return.  In particular, in glibc, many functions return several
of the generic error codes, like EINTR, even if they're not explicitly
enumerated in POSIX.  My understanding is that's why the error section is
phrased as "shall fail if" not "may only fail if."

The important distinction is whether a function can fail or not.  If it
can fail, it can potentially fail for other reasons than those explicitly
stated.  If it can't fail, it should return void, and then you know that
you don't have to check the return status.

If POSIX says somewhere that this is not the case, I'd appreciate a
pointer, since I've been wondering about that for some time and have
followed several past discussions on that topic on libc-alpha.

Programs that don't check the return status of functions that they think
won't ever fail are a bit of a pet peeve of mine, in part because it would
make a lot of sense for localtime() to be able to fail when the question
it was asked is undefined.  But no one ever checks the return status of
localtime() for much the same reason that you spell out for not checking
the return status of crypt(), which means that localtime() is required by
all this legacy code to return arbitrary nonsense instead of an error.

>> Your proposed solution on libc-alpha was ingenious, but I think it breaks
>> the crypt contract in even more serious ways, since it means that crypt
>> could now return a string matching the disabled password field in

> No, it cannot, because you pass the “password field” as seed, hence the
> comparison: if the password field is disabled by ‘*’ you get ‘x’, if
> it’s disabled by ‘x’ got get ‘*’. It always returns something not
> matching.

Sure, that's what you're *supposed* to do.  You're also supposed to check
the return status of crypt().  :)  People use crypt() for things other than
processing /etc/shadow files, and may or may not handle the seed in the
way that the UNIX local password mechanisms do.  Nothing in the POSIX
documentation of the function says that it has to be used that way.

> And prevents serious security issues in legacy code (which, by the way,
> often predates the standard and thus is allowed to not follow it).

That excuse ran out about five years ago, IMO.  Also, I think it's a bit
of a stretch to call a DoS attack a serious security issue.

The most appealing part of this change for me is that, if software gets
fixed to handle NULL returns from crypt properly, it can be possible to
meaningfully disable use of DES globally.

So I guess your general statement in your previous message is correct: I
don't care that this change broke a bunch of things.  In fact, I agree
with it.

Russ Allbery (rra@debian.org)               <http://www.eyrie.org/~eagle/>

Reply to: