PMI patch for OpenSSH 4.4p1

[Darren Tucker]

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.