Re: Security in a shell that starts ssh
Thanks for the feedback, I'll respond to both your replies at once.
On Wed, Jun 13, 2001 at 08:24:32PM +0400, Daniel Ginsburg <dg@warpsolutions.com> wrote:
> On Wed, Jun 13, 2001 at 10:57:08AM -0500, Steve Greenland wrote:
> > Tim, good fixups, a few C coding/style nitpicks:
> >
> > On 12-Jun-01, 17:57 (CDT), Tim van Erven <tripudium@chello.nl> wrote:
> > > #include <stdio.h>
> >
> > #include <unistd.h> /* For execlp */
> > #include <stdlib.h> /* For exit */
> >
> > > int main()
> >
> > int main(void) /* () != (void) in C */
The comp.lang.c faq (http://www.faqs.org/faqs/C-faq/faq/) says it's ok.
> > > {
> > > 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.
> > > printf("Login as: ");
> > > fflush(stdout);
> > >
> > > if(fgets(name, 21, stdin)) {
> > > /* if(name[strlen(name) - 1] != '\n') */
> >
> > if(name[strlen(name) - 1] != '\n') {
Yes, this does look slightly better.
> >
>
> 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, fgets will return 0 if that happens.
> > > fprintf(stderr, "Username to long.\n");
> > > /* else { */
> >
> > } else {
> > > name[strlen(name) - 1] = '\0';
> > > execlp("/usr/bin/ssh", "ssh", "-l", name, "foo.foo.es", (char *)0);
> > > }
> > > }
> > >
> > > /* return 0; */
> >
> > exit(EXIT_SUCCESS); /* return doesn't call atexit() registered functions,
> > which doesn't apply in this case, but it's a good
> > habit to get into */
> >
>
> Wrong comment. Returning from main _does_ call atexit() registered
> functions.
>
> > > }
> >
> >
> > You also should should make sure name doesn't contain any spaces: as
> > written I can pass additional options to ssh. Also, for this kind of
> > application you really ought to be checking the error conditions for
> > *every* library call.
> >
>
> Spaces and other shell metacharecters are irrelevant in this case, since
> executed command won't undergo shell interpretation.
No comments about my spelling mistake ('to long')? Personally I
considered that the worst offence. Btw, I should start posting all my
code to this list. That would *really* fix-up my coding habbits. :)
Miquel, if you're not comfortable integrating all this feedback, mail
me directly (off the list) and I'd be happy to do it for you.
Regards,
--
Tim van Erven
tripudium@chello.nl
talerven@wins.uva.nl
Reply to: