openssh unix dev - Mar 2004 - Request for comment, logging patch

Greetings. 

Attached is a patch that provides more logging information 
for example: 
Mar 19 08:34:54 secosr5 sshd[7667]: Accepted publickey of? vix at wormhole for
root from 192.168.1.1 port 1256 ssh2
Mar 19 08:34:54 secosr5 sshd[7667]: executing command 'who' for? vix at
wormhole as user root
Mar 19 10:37:16 secosr5 sshd[7725]: Accepted publickey of? vix at wormhole for
root from 192.168.1.1 port 1257 ssh2
Mar 19 10:37:16 secosr5 sshd[7725]: executing command 'scp -f
/usr/udd1/dev/openssh-3.8p1.patch' for? vix at wormhole as user root

Can this code be reviewed and possibly added to the code base ? 
Please let me know what is incorrect with this code. 

Thanks 

Vikash 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: openssh-3.8p1.patch.gz
Type: application/x-gzip
Size: 1887 bytes
Desc: openssh-3.8p1.patch.gz
Url :
http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20040324/3a4207a0/attachment.bin

Vikash Badal - PCS wrote:
> Greetings. 
> 
> Attached is a patch that provides more logging information 
> for example: 
> Mar 19 08:34:54 secosr5 sshd[7667]: Accepted publickey of  vix at wormhole
for root from 192.168.1.1 port 1256 ssh2
> Mar 19 08:34:54 secosr5 sshd[7667]: executing command 'who' for 
vix at wormhole as user root
> Mar 19 10:37:16 secosr5 sshd[7725]: Accepted publickey of  vix at wormhole
for root from 192.168.1.1 port 1257 ssh2
> Mar 19 10:37:16 secosr5 sshd[7725]: executing command 'scp -f
/usr/udd1/dev/openssh-3.8p1.patch' for  vix at wormhole as user root
> 
> Can this code be reviewed and possibly added to the code base ? 
> Please let me know what is incorrect with this code. 
 > +extern char realname[64];

"realname" is populated with a copy of the address part of the comment
in the key.  Firstly, I'm not keen on logging too much user-controlled 
data, and there's no reason why the comment won't be longer than 64
chars...

If you must log it, you should probably run it through strnvis to escape 
any nasties.

 >               /* cp now points to the comment part. */
 > +             comment = cp;
 > +             commentlen = strlen(comment);
 > +             if (commentlen > 0 && comment[commentlen -1] ==
'\n')
 > +                comment[commentlen - 1] = '\0';

You're modifying the source string, although it looks like you're trying
not to (comment and cp are just pointers that point to the same chunk of 
memory).  You should probably use xstrdup (but see above).

+extern char user_name[16];
 > +     strncpy(user_name, authctxt->user, sizeof(user_name) -1 );

Is it really neccessary to keep another copy of the_authctxt->user?  And 
what guarantee is there that it's less than 16 chars?

-- 
Darren Tucker (dtucker at zip.com.au)
GPG key 8FF4FA69 / D9A3 86E9 7EEE AF4B B2D4  37C9 C982 80C7 8FF4 FA69
     Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.

Greetings,
> -----Original Message-----
> From: Darren Tucker [mailto:dtucker at zip.com.au]
> Sent: 24 March 2004 01:19
> To: Vikash Badal - PCS
> Cc: openssh-unix-dev at mindrot.org
> Subject: Re: Request for comment, logging patch
> 
> 
> Vikash Badal - PCS wrote:
> > Greetings. 
> > 
< ... SNIP>
> in the key.  Firstly, I'm not keen on logging too much 
> user-controlled 
> data, and there's no reason why the comment won't be longer 
> than 64 chars...
> 
In my haste to get this working, i took an arbitrary value
> If you must log it, you should probably run it through 
> strnvis to escape 
> any nasties.
will definitely learn more about this
> 
>  >               /* cp now points to the comment part. */
>  > +             comment = cp;
>  > +             commentlen = strlen(comment);
>  > +             if (commentlen > 0 && comment[commentlen -1]
== '\n')
>  > +                comment[commentlen - 1] = '\0';
> 
> You're modifying the source string, although it looks like 
> you're trying 
> not to (comment and cp are just pointers that point to the 
> same chunk of 
> memory).  You should probably use xstrdup (but see above).
> 
Thanks, I will work on this.
> +extern char user_name[16];
>  > +     strncpy(user_name, authctxt->user, sizeof(user_name) -1 );
> 
> Is it really neccessary to keep another copy of 
> the_authctxt->user?  And 
> what guarantee is there that it's less than 16 chars?
this part is from my ignorance of coding, sorry, still got a lot to learn.

Thanks for the feedback,

I will work with the comments you have provided and hopefully learn more.

I have no idea if this was a good idea or not, but in the environment
in which i administer systems, user accounts are shared ( i know
it's stupid, but i have no power over this).

Thanks again,
I have certainly gained some knowledge at the very least.

Vikash