Re: Security in a shell that starts ssh
[some text omitted for brevity]
On Wed, 13 Jun 2001, Tim van Erven wrote:
[...]
> > > > {
> > > > char name[21]; /* Should be macro (#define NAMELEN 21) */
> Possibly, but the name that can be entered is at most 20 chars long, so
> NAMELEN should arguably be defined to 20 and the declaration for name
> changed to char name[NAMELEN + 1];. I don't think it adds anything to
> readability in this case, though.
It's not readability, but maintainability. If a future maintainer
updates the parameter to fgets() but not the array size, then you may
have a buffer overflow bug. Using a single #define means there's
only one thing to change.
[...]
> > > > if(fgets(name, 21, stdin)) {
Better yet, use sizeof name instead of 21. This prevents having to
worry about the buffer overflow.
> > > if(name[strlen(name) - 1] != '\n') {
[...]
> > Possible access to unallocated memory if "\0\n" supplied as input.
> Only if strlen(name) = 0 and besides from being hard to achieve when
> entering data on stdin,
Maybe, but is there any other mechanism in this program to prevent a
file from being piped to it?
> fgets will return 0 if that happens.
fgets() returns the string you passed it on success, and NULL on error
or when the end of file is reached before any characters have been
read.
> > > > fprintf(stderr, "Username to long.\n");
The format string of printf() and friends should be treated with
great respect. This form suggests that it's just a string, and an
unsuspecting maintainer who adds a percent sign somewhere in the
string can break things. Better:
fputs("Username too long.\n", stderr);
In general, use the least powerful function in the library that
can achieve what you want.
Cheers.
Steven
Reply to: