gets() is dead

[Army1987]

#include <stdio.h>
int main() { while (1) putchar( "!" ); return /* ha! */ 0; }

you meant putchar('!')?

 

[¬a\\/b]

¬a\/b wrote, On 27/04/07 08:56:

<snip more stuff based on this completely false statement>

Wrong. fgets is defined by the C standard as returning a null pointer on
failure or a pointer to the start of the buffer on success. I suggest
you go back to school.

i don't speak for fgets in the standard but how fgets could-should be

 

[Charles Richmond]

jacob navia wrote:

[...]

This is a very positive development. After all those discussions,
reason prevailed and we got rid of that wart.

These days, there should be no experienced C programmers using gets()
in security sensitive programs anyway, so I don't see the big fuzz about
this.

I have done a number of C code audits in safety-critical systems, and never
seen a single gets(), and didn't expect such a trivial bug either.

Who cares what students use?

I don't.

Hey, I use gets() all the time in "quick and dirty"
programs. I have some programs that take data in
flat files and create scripts of SQL statements to
add the data to a MySQL database. I just can *not*
see how gets() is going to cause me a security problem
here.

Also, in writing an adventure game or any other game
without secure access, that using gets() is okay.

But gcc still dogs me about using gets(). :-( I could
deal with a warning message, but gcc incorporates code
to print out a nasty message *every* time I run my code.
No fair... ;-)

 

[¬a\\/b]

jacob navia wrote:
[...]

This is a very positive development. After all those discussions,
reason prevailed and we got rid of that wart.

These days, there should be no experienced C programmers using gets()
in security sensitive programs anyway, so I don't see the big fuzz about
this.

I have done a number of C code audits in safety-critical systems, and never
seen a single gets(), and didn't expect such a trivial bug either.

Who cares what students use?

I don't.

Hey, I use gets() all the time in "quick and dirty"
programs. I have some programs that take data in
flat files and create scripts of SQL statements to
add the data to a MySQL database. I just can *not*
see how gets() is going to cause me a security problem
here.

Also, in writing an adventure game or any other game
without secure access, that using gets() is okay.

But gcc still dogs me about using gets(). :-( I could
deal with a warning message, but gcc incorporates code
to print out a nasty message *every* time I run my code.
No fair... ;-)

i not think like you on that

 

[Guest]

jacob navia wrote:

[...]

This is a very positive development. After all those discussions,
reason prevailed and we got rid of that wart.

These days, there should be no experienced C programmers using gets()
in security sensitive programs anyway, so I don't see the big fuzz about
this.

I have done a number of C code audits in safety-critical systems, and never
seen a single gets(), and didn't expect such a trivial bug either.

Who cares what students use?

I don't.

Hey, I use gets() all the time in "quick and dirty"
programs. I have some programs that take data in
flat files and create scripts of SQL statements to
add the data to a MySQL database. I just can *not*
see how gets() is going to cause me a security problem
here.

Also, in writing an adventure game or any other game
without secure access, that using gets() is okay.

But gcc still dogs me about using gets(). :-( I could
deal with a warning message, but gcc incorporates code
to print out a nasty message *every* time I run my code.
No fair... ;-)

gcc incorporates no such code, and if your particular standard C
library implementation does, it's broken. See if an upgrade is
available.

 

[Flash Gordon]

Harald van Dijk wrote, On 29/04/07 09:49:

Charles Richmond wrote:


gcc incorporates no such code, and if your particular standard C
library implementation does, it's broken. See if an upgrade is
available.

It is, however, broken in a very useful way IMHO.

I'm sure Charles knows the size of the buffers he is allocating, and he
obviously knows the maximum line length, so using fgets instead should
be easy enough. He can even use the GETS macro I published here in this
thread and his code will be much safer, or Chucks ggets, or any of the
other solutions which once implemented can be used as simply as gets.

 

[Guest]

Harald van Dijk wrote, On 29/04/07 09:49:

It is, however, broken in a very useful way IMHO.

I do not agree. If a build-time warning is issued, code using gets
will continue to function properly. A conforming implementation can do
this. If a build-time error is issued, code using gets will need to be
changed. A conforming implementation cannot do this, but for this I
might agree that it's broken in a somewhat useful way, though I would
still not use it. If a run-time warning is issued, code using gets
will continue to build, but run incorrectly. A conforming
implementation cannot do this, and the problem may well be caught too
late to be useful.

 

[Chris Dollin]

you meant putchar('!')?

I did. Well caught.

Erm ... It's the font. Yeah. The font. Or my glasses. Yeah, the
font. And my glasses. And the coffee. Well, the absence of the
coffee. The screen's dirty! The font, the glasses, the noncoffee,
the screen. Oh, look, a UFO!

(fx:hides)

 

[Chris Dollin]

Except it would be this:

What would be this?

int main()
{
for(i=0;i<(size_t) -1;i++)
putchar("!");
/* at this point sytem wraps round */
/* now we can put up to 127 bytes exploit code at an illegal point in
memory*/
printf("Uggleuggle^&*!!runme\n");
return 0;
}

I can't imagine why you think something would be that. (Well, I can,
but the presented options are inappropriate for this newsgroup; I'm
already worried enough about SCORPION STARE.)

 

[Malcolm McLean]

What would be this?


I can't imagine why you think something would be that. (Well, I can,
but the presented options are inappropriate for this newsgroup; I'm
already worried enough about SCORPION STARE.)

And fgets() is in practise too difficult for any but the best programmers
to
use safely.

Here's the code.

That certainly means I am unable to use fgets() safely,
and since I just wrote this function which reads from stdin
using fgets() I thought I would to like to hear any opinions
on my code from the people here.
Although it may not be the best way to handle memory, I'm
mostly interested to know if this code is correct, and safe.
Thanks!

/Michael Brennan


#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define BUFSZ 128
char *readstr(void)
{
char buffer[BUFSZ];
char *input;
size_t size;
char *tmp;

input = NULL;
size = 0;
do {
tmp = realloc(input, size + BUFSZ);
if (tmp == NULL) {
free(input);
return NULL;
}
input = tmp;

if (fgets(buffer, sizeof buffer, stdin) == NULL) {
free(input);
return NULL;
}

if (size == 0)
strcpy(input, buffer);
else
strcat(input, buffer);

size = strlen(input);
} while (strrchr(buffer, '\n') == NULL);

input[strcspn(input, "\n")] = '\0';

return input;
}

It is pretty good stab at using fgets() safely, but in fact it isn't right.
When we supply a line of greater that SIZE_MAX bytes the "size" variable
will wrap.
On most machines SIZE_MAX also indicates the total memory available to
the machine, so malloc() can only fail long before this point is reached,
and the function is OK, as long as the caller remembers to flush the input
on fail to deal with the half-read data. However if that is not the case it
asks for what is probably a block of zero bytes, then write a string to it,
i.e. undefined behaviour. Depending on the realloc internals, by putting
strategically-placed NULs in the initial input, which fgets() will then read
(another problem with the code) we can then direct a 127-byte exploit string
to where we want in memory.
So the answer Michael Brennan's question is, no, the code isn't safe,
although it is acceptable for everyday use in a non-security non-portable
environment.

 

[Flash Gordon]

Malcolm McLean wrote, On 29/04/07 18:26:

It is pretty good stab at using fgets() safely, but in fact it isn't right.
When we supply a line of greater that SIZE_MAX bytes the "size" variable
will wrap.
On most machines SIZE_MAX also indicates the total memory available
to the machine, so malloc() can only fail long before this point is

I think you meant to say that on most machines SIZE_MAX is far greater
than the amount of memory the OS will make available to any given
process, which is true but should not be relied upon you are correct.

reached, and the function is OK, as long as the caller remembers to
flush the input on fail to deal with the half-read data. However if that
is not the case it asks for what is probably a block of zero bytes, then
write a string to it, i.e. undefined behaviour. Depending on the realloc
internals, by putting strategically-placed NULs in the initial input,
which fgets() will then read (another problem with the code) we can then
direct a 127-byte exploit string to where we want in memory.

The embedded null character will mean the function will loose the rest
of the "block" that contained the null character, it will not cause it
to try to execute it or anything like that. So all an attacker does by
embedding null characters is get some of the input to be effectively
ignored. Since the attacker is providing the input, that is not IMHO a
major flaw. It may cause the next line to be appended to the current
line of course, but it is still the attackers input being mangled, not
code being executed.

So the answer Michael Brennan's question is, no, the code isn't safe,
although it is acceptable for everyday use in a non-security
non-portable environment.

I would say that ANY process which has an effectively unbounded buffer
growth is not secure, since it can be used for a denial of service
attack. However, the correct action on too-large input depends on the
situation, so it is important for it to be left in the hands of the
programmer. For example, Apache will log too long a line and not process
it, with too long being configurable IIRC. In other situations you might
want to pick up the rest of the line and process it, for example if you
are just copying the text file. This is something that needs to be
determined as part of the requirements analysis, and then as it is a
requirement it will be tested (any organisation doing serious
security-related work worth its salt will have formal procedures to
check such things, and pedantic nit-picking abstrads to ensure it is
done properly).

In any case, one programmer not getting it quite right is not proof that
only the best programmers can use fgets safely.

 

[Malcolm McLean]

Malcolm McLean wrote, On 29/04/07 18:26:

I would say that ANY process which has an effectively unbounded buffer
growth is not secure, since it can be used for a denial of service attack.

And in this case it could on some platforms lead to UB and possibly open a
hole for exploit code to be introduced.
The denial of service could be more severe than that. Since he grows by the
length of the string we can stall the process for as long as we want by
passing it nul bytes. So we can grab as much memory as we want and then keep
it for as long as we want.

In any case, one programmer not getting it quite right is not proof that
only the best programmers can use fgets safely.

No it doesn't. It's one data point.

 

[Keith Thompson]

I do not agree. If a build-time warning is issued, code using gets
will continue to function properly. A conforming implementation can do
this. If a build-time error is issued, code using gets will need to be
changed. A conforming implementation cannot do this, but for this I
might agree that it's broken in a somewhat useful way, though I would
still not use it. If a run-time warning is issued, code using gets
will continue to build, but run incorrectly. A conforming
implementation cannot do this, and the problem may well be caught too
late to be useful.

A build-time warning does not violate the standard. An implementation
can warn about anything it likes.

As for a run-time warning, it's certainly true that such an
implementation is non-conforming. Flash didn't say otherwise; he
merely said that it's *usefully* non-conforming.

 

[Keith Thompson]

Hey, I use gets() all the time in "quick and dirty"
programs. I have some programs that take data in
flat files and create scripts of SQL statements to
add the data to a MySQL database. I just can *not*
see how gets() is going to cause me a security problem
here.

Presumably you're using gets() to read lines from your flat files
(with stdin redirected to read from a file). How do you *know* that
no line in your input file exceeds the size of your buffer? Why do
you want to eliminate the possibility of detecting such an error?

Also, in writing an adventure game or any other game
without secure access, that using gets() is okay.

So it's "okay" if a user accidentally or intentionally holds down the
space key long enough to overflow your input buffer, and the program
misbehaves in some arbitrarily bad way? Why isn't it better to use
fgets() and *check* for overly long input lines?

But gcc still dogs me about using gets(). :-( I could
deal with a warning message, but gcc incorporates code
to print out a nasty message *every* time I run my code.
No fair... ;-)

gcc does not do this. Your runtime library might.

But there's a really good workaround: don't use gets().

 

[Guest]

A build-time warning does not violate the standard. An implementation
can warn about anything it likes.

As for a run-time warning, it's certainly true that such an
implementation is non-conforming. Flash didn't say otherwise; he
merely said that it's *usefully* non-conforming.

I know, I explained why I do not think this is useful. I'm sorry if I
didn't word my message clearly enough, but I meant that I would rather
have an implementation not provide gets() at all than include a
defective one.

 

[Flash Gordon]

Keith Thompson wrote, On 29/04/07 20:16:

A build-time warning does not violate the standard. An implementation
can warn about anything it likes.

As for a run-time warning, it's certainly true that such an
implementation is non-conforming. Flash didn't say otherwise; he
merely said that it's *usefully* non-conforming.

Keith is entirely correct, both about what I said and more importantly
what I meant.

In fact, if I knew how to enable such a message on my systems I *would*
enable it and remove any SW that used gets. I would especially like such
a warning enabled on the various servers I have some control over, since
I take security of servers very seriously and getting rid of
fundamentally unsafe SW is a good step towards security.

 

[Flash Gordon]

Malcolm McLean wrote, On 29/04/07 20:04:

And in this case it could on some platforms lead to UB and possibly open
a hole for exploit code to be introduced.

We both agree that the code by someone else you posted can invoke UB but
is unlikely to because malloc would almost certainly fail first.

The denial of service could be more severe than that. Since he grows by
the length of the string we can stall the process for as long as we want
by passing it nul bytes. So we can grab as much memory as we want and
then keep it for as long as we want.

Agreed. Once I've rejected a piece of code as not suitable for purpose I
don't always bother finding all the problems since I will not use it anyway.

However, as I also said, in any SW shop worth its salt there are a
number of points at which the problem would be detected, starting with
the requirements analysis where it would be pointed out that you need to
specify in the requirements what the SW is to do in the case of bad data.

As I also pointed out the input routines in a number of other languages
are as broken or more so since they do not allow you to specify how much
memory they are allowed to grab.

No it doesn't. It's one data point.

True. However given a complete specification of the required behaviour
of a program on good/bad input I believe a number of people here who do
not claim to be experts could produce code using fgets which is safe or,
if fgets was not suitable given the specification identify that and
produce correct code not using it. I also believe that an incomplete
specification would be spotted. Even more importantly, since the code
would be reviewed (as happens in decent SW shops) any errors would be
spotted, so at the end of the day a safe input routine would be produced.

 

[Guest]

Keith Thompson wrote, On 29/04/07 20:16:

Keith is entirely correct, both about what I said and more importantly
what I meant.

In fact, if I knew how to enable such a message on my systems I *would*
enable it and remove any SW that used gets. I would especially like such
a warning enabled on the various servers I have some control over, since
I take security of servers very seriously and getting rid of
fundamentally unsafe SW is a good step towards security.

<OT>
Depending on your system, it may be possible to build a special libc
which links against the system libc to provide all standard symbols,
but provides its own implementation of gets(). This would let you get
the warning if and only if you want it, by changing the searched
library path.
</OT>

 

[Flash Gordon]

Harald van Dijk wrote, On 29/04/07 20:57:

I know, I explained why I do not think this is useful. I'm sorry if I
didn't word my message clearly enough, but I meant that I would rather
have an implementation not provide gets() at all than include a
defective one.

An implementation not providing gets is just as broken as far as the
standard is concerned as an implementation providing a broken one.

 

[Guest]

Harald van Dijk wrote, On 29/04/07 20:57:

An implementation not providing gets is just as broken as far as the
standard is concerned as an implementation providing a broken one.

Right, but as I tried to explain, this could actually be useful.
Programs using gets() would not run at all, rather than run with
incorrect behaviour, and it's the running with incorrect behaviour
that I mainly object to.