Bug/Gross InEfficiency in HeathField's fgetline program

[Richard Heathfield]

Mark McIntyre said:

Personally I consider it the height of arrogance to believe
unequivocally that you cannot possibly be wrong.

Strawman. I haven't claimed that either that I cannot possibly be wrong or
that I *believe* I cannot possibly be wrong (and in fact I don't think
either of these statements is true). If you want to debate with me, fine,
but why bother to argue against something I haven't actually said?

Ok, if you prefer - I prefer to train a smart junior who's not afraid
to admit he has stuff to learn,

That is your prerogative, obviously...

than to try to persuade a senior, who
thinks he knows it all, to accept that he doesn't. .

....but this is just a continuation of your strawman argument.

<snip>

 

[Keith Thompson]

Unfortunately thats *precisely* the sort of arrogance that one can't
afford. If someone isn't open to having their mindset changed by
others, then they're probably useless as an employee. Imagine if you
hired someone who refused point blank to change to your in-house
coding standard, or to use your in-house libraries....

I think you're misunderstanding what Richard said. In fact, I suspect
that everyone in this discussion is in fundamental agreement about
most things; some of the concepts just aren't being communicated
clearly.

Nobody can change anyone else's mindset (barring the use of drugs or
other brainwashing techniques). The most you can possibly do is (try
to) convince someone that he needs to change his own mindset. I have
no doubt that Richard is perfectly willing to change his mind when he
feels it's appropriate to do so.

I don't believe that I'm right about everything, but for each thing I
believe, I believe that I'm right about that thing. If I didn't, I
would change my mind, and *then* I'd be right about it. That's not
arrogance; it's just a straightforward description of the universal
endeavor to understand things.

As for strncpy() the function itself is not fundamentally unsafe. Its
semantics are precisely defined by the standard, including the
specification of cases where its behavior is undefined. In contrast
to gets(), it *can* be used safely in well-written code.

It's true that it *can* be misused, but so can any function.

It's also true that, because of its misleading name, it's particularly
vulnerable to misuse by programmers who haven't bothered to read and
understand its specification (the standard, a man page, whatever).
Because of that, any use of strncpy() should IMHO be viewed with
suspicion (like any use of a cast operator) *unless* you're certain
that the author actually knows what he's doing.

I strongly suspect that most uses of strncpy() in production code are
incorrect.

But that's not the fault of strncpy() itself, which is a perfectly
innocent little function that does exactly what it's supposed to do.
It's the fault of the incorrect assumptions made by too many
programmers. And those assumptions are partly the fault of the way
strncpy() doesn't fit well into the context in which it's specified.

 

[Tor Rustad]

Tor Rustad said:

Richard Heathfield wrote:

[...]
[...]

Yesterday, I checked the latest update for OpenSSL, it included a
security fix for an off-by-one bug. Would you bet your life on, this
wasn't strncpy related?

I have no idea. But if it were, then clearly strncpy wasn't used properly.

If so, would you consider the OpenSSL maintainers sloppy and clueless?

You don't know about betting your life, but gladly bet e.g. $100 million
company money, that there isn't any usage pitfalls with strncpy?!

Yes, I can understand that - but on the other hand, obviously I don't think
there's anything wrong with my mindset and attitude, because otherwise I'd
change it myself. And *I* would change it. It's not your job to change my
mindset.

Old dogs and programmers... <g>

You simply didn't fit the profile of what I regard as *very* important
qualifications for a senior security officer. Get over it!

I'm not entirely sure this can be done in standard C, at least not the
obvious way, although I'd be glad to be proved wrong.

Hmm.. haven't written it before, my first try would be:

/* pre-condition: checking for overlapped objects */
N = strlen(s);
for (i=0; i<N; i++) {
for (j=0; j<N; j++)
assert(t + i != s + j);
}

Hey, that was your job to provide, not mine! *lol*

Am I not right in
saying that you can't "well-definedly" compare two pointers relationally
unless they point into the same object? If so, you'd have to compare using
s + i == t (or s + i != t, depending on which way you're going) for all i
< n, and t + i == s for all i < n as well. Not funny. But yes, it could be
done.

Brilliant answer, if someone had pulled that one off during an interview
session, I would have fallen off my chair!

Chances are good that it's a bug in the caller, but it depends on whether
you view n = 0 as a legitimate case or not. Some do, and some don't.
That's why it's arguable. (The Standard /does/ view it as a legitimate
case, of course.)

I hadn't thought of using assert(n > 0), since the standard allows it
and have never seen such a relevant bug. I do agree that the check might
be useful, at least in theory.

However, such a rule, need to be documented in the local C programming
standard. The non-security (aka application) programmers will for sure
object to it, so you better be prepared to argue your case.. I already
"hear" the laud complaints, from people like Pop and Kaz!

Each in-house rule sub-setting C, need to have a rationale behind it,
references to real bugs that happened, will make the job far easier.

 

[Richard Heathfield]

Tor Rustad said:

Tor Rustad said:

Richard Heathfield wrote:

[...]

There aren't any - strncpy is perfectly secure if used properly.

[...]

Yesterday, I checked the latest update for OpenSSL, it included a
security fix for an off-by-one bug. Would you bet your life on, this
wasn't strncpy related?

I have no idea. But if it were, then clearly strncpy wasn't used
properly.

If so, would you consider the OpenSSL maintainers sloppy and clueless?

If they've misused strncpy, I would consider them to be people capable of
misusing strncpy.

You don't know about betting your life, but gladly bet e.g. $100 million
company money, that there isn't any usage pitfalls with strncpy?!

I have no idea what point you're trying to make. Yes, strncpy can be
misused, just like soap, scissors, and salt can be misused. So what?

You simply didn't fit the profile of what I regard as *very* important
qualifications for a senior security officer. Get over it!

Over what? I don't *want* to be a security officer, senior or otherwise.

[...] you'd have to compare
using s + i == t (or s + i != t, depending on which way you're going)
for all i < n, and t + i == s for all i < n as well. Not funny. But yes,
it could be done.

Brilliant answer, if someone had pulled that one off during an interview
session, I would have fallen off my chair!

You lost me. I don't see what's so brilliant about it.

I hadn't thought of using assert(n > 0), since the standard allows it
and have never seen such a relevant bug. I do agree that the check might
be useful, at least in theory.

However, such a rule, need to be documented in the local C programming
standard. The non-security (aka application) programmers will for sure
object to it,

On what grounds? Why do they need to be able to copy up to, but no more
than, 0 bytes from one place to another? Why bother?

And, out of curiosity, why do you distinguish between "security" and
"non-security" programmers? All programmers are responsible for the
security of the code they write.

<snip>

 

[Malcolm McLean]

On what grounds? Why do they need to be able to copy up to, but no more
than, 0 bytes from one place to another? Why bother?

We've got a buffer of 32 bytes, and we want to jam in a Christian name and a
surname.
The correct algorithm is

/* assume database field with no necessary nul */
strcnpy(buff, christian, 32);
strncpy(buff + strlen(christian), surname, 32 - strlen(christian));

if christian happens not to fit in the buffer, we ask for a maximum of a
negative number of characters. That's legitimate.

 

[Richard Heathfield]

Malcolm McLean said:

We've got a buffer of 32 bytes, and we want to jam in a Christian name
and a surname.
The correct algorithm is

/* assume database field with no necessary nul */
strcnpy(buff, christian, 32);
strncpy(buff + strlen(christian), surname, 32 - strlen(christian));

Correct? Um, no, I don't think so.

Consider a system with 32-bit size_t, and a 40-byte Christian name. The
expression 32 - strlen(christian), on my system (which is fairly typical
of modern desktop sytems), evaluates to 4294967288, and you pass this
result to strncpy as a buffer limit, even though your buffer is only 32
bytes in size. That's one mother of a buffer overrun you have there.

 

[Francine.Neary]

We've got a buffer of 32 bytes, and we want to jam in a Christian name and a
surname.
The correct algorithm is

/* assume database field with no necessary nul */
strcnpy(buff, christian, 32);
strncpy(buff + strlen(christian), surname, 32 - strlen(christian));

if christian happens not to fit in the buffer, we ask for a maximum of a
negative number of characters. That's legitimate.

But strncpy takes a size_t argument, which can't be negative - you'll
actually get a spectacular buffer overflow!

 

[Francine.Neary]

But strncpy takes a size_t argument, which can't be negative - you'll
actually get a spectacular buffer overflow!

Oh dear, as soon as I sent this I spotted where this is going - brace
yourselves for another tedious tirade against size_t

 

[Mark McIntyre]

Mark McIntyre said:



Strawman.

Disingenuous attempt to divert attention from your error.

However this is entirely offtopic, and your responses are sadly
typical of your posts these days (ie happy to criticise others, but
instantly hyperdefensive when you're criticised, in fact everything
you complain about JN being) so I'm no sense continuing the debate.

...but this is just a continuation of your strawman argument.

.... but your comment is merely a continuation of your arrogance.

--
Mark McIntyre

"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it."
--Brian Kernighan

 

[Richard Heathfield]

Mark McIntyre said:

Disingenuous attempt to divert attention from your error.

No, Mark, it really really isn't. But now I remember why I plonked you.
Time for me to re-enable that filter, I think.

 

[Mark McIntyre]

I think you're misunderstanding what Richard said.

I understand it perfectly in fact. He's saying that in a literal
sense, only he can change his mind, since there's no known
(nonphysical) mechanism to alter the stored memory patterns in
someone's btain.

However this quite clearly wasn't what the original comment related
to, and RJH knew that perfectly too. His response was in my opinion an
example of the sort of 'smug cleverness' that some people complain the
regulars here exhibit too much of.

I tire of seeing such smart-arse posts here, especially when the
poster, when called on it, attempts to actually justify the sort of
behaviour that would, IRL, get them disciplined, sacked or socked.

As for strncpy() the function itself is not fundamentally unsafe. Its
semantics are precisely defined by the standard, including the
specification of cases where its behavior is undefined. In contrast
to gets(), it *can* be used safely in well-written code.

I agree.

I strongly suspect that most uses of strncpy() in production code are
incorrect.

Well, 'most' is probably a bit strong - I'd go with 'many'.
--
Mark McIntyre

"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it."
--Brian Kernighan

 

[Mark McIntyre]

Mark McIntyre said:


No, Mark, it really really isn't. But now I remember why I plonked you.
*sigh*

Time for me to re-enable that filter, I think.

Thats up to you. Perhaps if you stopped being so pompous and posting
such clever-clever smug posts, you'd be able to avoid that.
--
Mark McIntyre

"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it."
--Brian Kernighan

 

[Mark McIntyre]

No, Mark, it really really isn't. But now I remember why I plonked you.
Time for me to re-enable that filter, I think.

You might want to consider that my initial posts in this thread were
in your favour, and I've only commented adversely once you started
being a pompous twit.
--
Mark McIntyre

"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it."
--Brian Kernighan

 

[Ben Bacarisse]

On what grounds? Why do they need to be able to copy up to, but no more
than, 0 bytes from one place to another? Why bother?

We've got a buffer of 32 bytes, and we want to jam in a Christian name
and a surname.
The correct algorithm is

/* assume database field with no necessary nul */
strcnpy(buff, christian, 32);
strncpy(buff + strlen(christian), surname, 32 - strlen(christian));

if christian happens not to fit in the buffer, we ask for a maximum of
a negative number of characters. That's legitimate.[/QUOTE]

Others have commented on the third parameter but if, as you state, you
have a 32 byte buffer the first parameter invokes undefined behaviour
if the given name is longer than 32.

 

[¬a\\/b]

In data Sat, 13 Oct 2007 10:29:45 +0100, Malcolm McLean scrisse:

We've got a buffer of 32 bytes, and we want to jam in a Christian name and a
surname.
The correct algorithm is

/* assume database field with no necessary nul */
strcnpy(buff, christian, 32);
strncpy(buff + strlen(christian), surname, 32 - strlen(christian));

if christian happens not to fit in the buffer, we ask for a maximum of a
negative number of characters. That's legitimate.

hahhahaha and all this moviment for doing just a cat?
i not are a senior in C language so it is possible i wrong anhother
time but

i will write it like
string buff;
buff << name << surname;
if(buff==0) { errro:; ...}

or if i in the C day something like

char *buff;
int len=sprintf_m(0, 0, "%s%s", name, surname);

if(len<0) {error:; ...} /* ((unsigned)len)> INT_MAX */
buff=malloc(len+2);
if(buff==0) goto error;
len=sprintf_m(buf, len+1, "%s%s", name, surname);
if(len<0) goto error;

with sprintf_m and "<<" operator like an exercise for the reader to
code in assembly or in hex if you like ))

 

[¬a\\/b]

In data Sat, 13 Oct 2007 00:59:07 +0200, Tor Rustad scrisse:

Tor Rustad said:

Richard Heathfield wrote: [...]
There aren't any - strncpy is perfectly secure if used properly. [...]
Yesterday, I checked the latest update for OpenSSL, it included a
security fix for an off-by-one bug. Would you bet your life on, this
wasn't strncpy related?

I have no idea. But if it were, then clearly strncpy wasn't used properly.

If so, would you consider the OpenSSL maintainers sloppy and clueless?

You don't know about betting your life, but gladly bet e.g. $100 million
company money, that there isn't any usage pitfalls with strncpy?!

Yes, I can understand that - but on the other hand, obviously I don't think
there's anything wrong with my mindset and attitude, because otherwise I'd
change it myself. And *I* would change it. It's not your job to change my
mindset.

Old dogs and programmers... <g>

You simply didn't fit the profile of what I regard as *very* important
qualifications for a senior security officer. Get over it!

I'm not entirely sure this can be done in standard C, at least not the
obvious way, although I'd be glad to be proved wrong.

Hmm.. haven't written it before, my first try would be:

/* pre-condition: checking for overlapped objects */
N = strlen(s);
for (i=0; i<N; i++) {
for (j=0; j<N; j++)
assert(t + i != s + j);
}

in the little i know overlap is not a problem in a x86 cpu
because the C instruction of assignament is done trhu a register or
trhu push pop; so where is the problem?

 

[Tor Rustad]

In data Sat, 13 Oct 2007 00:59:07 +0200, Tor Rustad scrisse:

Richard Heathfield wrote:

[...]

Hmm.. haven't written it before, my first try would be:

/* pre-condition: checking for overlapped objects */
N = strlen(s);
for (i=0; i<N; i++) {
for (j=0; j<N; j++)
assert(t + i != s + j);
}

in the little i know overlap is not a problem in a x86 cpu
because the C instruction of assignament is done trhu a register or
trhu push pop; so where is the problem?

Assuming, you want the job, I say provide good answers to the questions
you get.

 

[Tor Rustad]

Tor Rustad said:

Tor Rustad said:

Richard Heathfield wrote: [...]

There aren't any - strncpy is perfectly secure if used properly. [...]

Yesterday, I checked the latest update for OpenSSL, it included a
security fix for an off-by-one bug. Would you bet your life on, this
wasn't strncpy related?
I have no idea. But if it were, then clearly strncpy wasn't used
properly.

If so, would you consider the OpenSSL maintainers sloppy and clueless?

If they've misused strncpy, I would consider them to be people capable of
misusing strncpy.

You did not answer the question.

I have no idea what point you're trying to make. Yes, strncpy can be
misused

The very simple point is, if the claim "no usage pitfalls with strncpy"
is correct, then it should follow that at least the *experts*, almost
never misuse the function, and that *advanced* programmers very rarely
do so too. Agreed?

My own expectations, would be that the OpenSSL maintainers was in the
advanced+ to the expert category. Also, my guess would be that the
OpenSSL bug in question, would unlikely be strncpy() related.

You OTOH, are unwilling to make a stand, it appears your "no usage
pitfalls", is worth nothing.

You lost me. I don't see what's so brilliant about it.

I be surprised, if the level of expertise at job interviews, is that
much different in UK.

On what grounds? Why do they need to be able to copy up to, but no more
than, 0 bytes from one place to another? Why bother?

On the same grounds, as you defend strncpy(). However, their case would
be better, as I cannot see assert(n>0) trapping common errors.

And, out of curiosity, why do you distinguish between "security" and
"non-security" programmers? All programmers are responsible for the
security of the code they write.

Only a clueless, would use an application developer without special
training, for security engineering.

 

[Richard Heathfield]

Tor Rustad said:

Tor Rustad said:

[...] would you consider the OpenSSL maintainers sloppy and clueless?

If they've misused strncpy, I would consider them to be people capable
of misusing strncpy.

You did not answer the question.

It was a loaded question, so I engaged the safety catch.

<snip>

 

[Tor Rustad]

Tor Rustad said:
[...]

You did not answer the question.

It was a loaded question, so I engaged the safety catch.

I won this on a bluff then, they didn't misuse strncpy.