PMI patch for OpenSSH 4.4p1

[Brian Hamon (brhamon)]

I also browsed that code and found that new messages were being added to
the protocol that would allow an unauthenticated client to provide the
hostname and URL that the ssh server host would then connect to and read
2K into a buffer. Ignoring the buffer overflow issues Darren raised, it
is almost never a good idea to allow a client to drive that kind of
behavior. It opens up a terrible denial of service capability, as the
vast installed base of OpenSSH servers could be turned into a widely
distributed DoS attack engine that some nefarious person could direct
against a single web server.

At that point I stopped reading. I do think there is a correct way to
provide a distributed authentication mechanism, perhaps by leveraging
LDAP or secure DNS. The domain for these discussions would be on within
the LDAP, sDNS or Kerberos development communities. 

-----Original Message-----
From: openssh-unix-dev-bounces+brhamon=cisco.com at mindrot.org
[mailto:openssh-unix-dev-bounces+brhamon=cisco.com at mindrot.org] On
Behalf Of Darren Tucker
Sent: Friday, January 19, 2007 8:30 PM
To: Vincenzo Sciarra
Cc: openssh-unix-dev at mindrot.org
Subject: Re: PMI patch for OpenSSH 4.4p1

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.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev at mindrot.org
http://lists.mindrot.org/mailman/listinfo/openssh-unix-dev