Vincenzo Sciarra wrote:> I have just published a pre-alfa of a patch that has the goal to make
> OpenSSH aware with PMI.
>
> Reference site : http://nutmay.sourceforge.net
Out of curiosity I looked at the patch. I'm still none the wiser about
what PMI is. The patch makes interesting reading, though.
+char identity[80];
[...]
+ sprintf(identity,"NULL");
sprintf is generally frowned apon. This one is not really a problem
since it's a static string of known size but is still bad form. I
suggest strlcpy(identity, "NULL", sizeof(identity)) instead (if you
really need to have the literal string of "NULL", which I suspect you
don't).
This one, however:
+ type=packet_read();
+ if (type==SSH2_ACMSG_PATH) {
+ ACpath = packet_get_string(NULL);
[...]
+ char patholo[100];
+ sprintf(patholo,"%s",ACpath);
... reads an arbitrary string from the network the sprintf's it into
fixed-length string on the stack. This is not a great idea.
Moving along,
+ mfp=fopen("/etc/ssh/identity","r");
+ if (mfp==NULL)
+ debug3("No identity file found. Skipping Attribute
Authentication");
+
+ if (mfp!=NULL) {
if-else would make this easier to read.
+ debug3("Looking for your Attribute Identity");
+ debug3("/etc/ssh/identity opened");
+
+ while (!feof(mfp)) {
+ fscanf(mfp,"%s",mkey);
+ fscanf(mfp,"%s",temp);
mkey and temp are fixed length strings, so larger-than-expected contents
of /etc/ssh/identity will overflow them.
+ if (strcmp(fp,mkey) == 0) {
+ debug2("Client Attribute identity seems to be : %s ",&temp);
temp is a char array, so &temp is a pointer to the array. Surprisingly
this works, but gcc will generate a warning for such constructs. I
suggest you use -Wall or equivalent and fix anything in your code that
it complains about.
+ strcpy(options.ACid,temp);
strcpy bad too. Use strlcpy at least.
The code also doesn't fclose(mfp) and so leaks FILE structs.
The next bit is from auth2-pubkey.c, which is only used in sshd:
+ printf("\nLooking for your Attribute Certificate");
sshd is a daemon and normally runs without stdio connected. This will
write to fd 1 which, when daemonized, could be anything, but will
probably be the socket connected to the client. In that case it will
break protocol and the client will disconnect. Use debug() instead.
The following sequence is from server_loop2:
+ sprintf(buffer,"GET %s HTTP/1.0\n\n",patholo);
sprintf bad.
+ gethostname(protocol,80);
+ sockadd=socket_init(protocol,port);
This is confusing, but it looks like this is trying to connect to the
local host, reusing "protocol" for the hostname.
gethostname can fail and you're not checking for it. You would probably
be better off using getaddrinfo with a NULL host to talk to the loopback
interface.
+ write(sockadd,buffer,50);
What if there's more that 50 characters in the buffer?
I gave up at this point, but there's also declarations after code, which
some (pre C99) compilers won't accept, and many no-op changes in the
diff that only make it harder to read.
--
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.