Date:         Wed, 12 Jul 2000 23:12:42 +0200
From: Pavel Kankovsky <peak@ARGO.TROJA.MFF.CUNI.CZ>
Subject:      ISC DHCP client v2 hole fixed...or not?
To: BUGTRAQ@SECURITYFOCUS.COM

Executive summary: the official fix for the recent ISC DHCP client
vulnerability is not as thorough as it should be.

If you diff version 2.0 and 2.0pl1 you can see the only substantial change
to the code happened in pretty_print_option() in common/options.c. The
function is used when data recieved from the server are saved to
dhclient.leases or passed to dhclient-script. Now, it escapes suspicious
characters when it formats text options. Good. (BTW: the code does not
follow the old "allow only characters known to be safe" rule, so some
problems might still lurk in the dark but this is not the point of my
mail.) Unfortunately, when you look at client/dhclient.c, you can see that
not every value is processed with pretty_print_option() or something
similar. Here is an example from script_write_params():

        if (lease -> filename) {
                fprintf (scriptFile, "%sfilename=\"%s\";\n",
                         prefix, lease -> filename);
                fprintf (scriptFile, "export %sfilename\n", prefix);
        }
        if (lease -> server_name) {
                fprintf (scriptFile, "%sserver_name=\"%s\";\n",
                         prefix, lease -> server_name);
                fprintf (scriptFile, "export %sserver_name\n", prefix);
        }

In fact, lease->filename and lease->server_name are used as they have come
from the network. Ergo, I conclude anyone controlling the DHCP/BOOTP
server or being able to spoof the replies can easily break into any
machine using ISC DHCP client of any version up to and including 2.0pl2
(unless it is a very recent OpenBSD version...see their cvsweb for
details).

P.S.1 I include a rather ugly patch that might (or might not) fix the bug.
Please note that 15 and 67 are more or less two arbitrary numbers to make
pretty_print_options() happy and use the right format.

P.S.2 I would report the bug to ISC folks directly if I could do it
without subscribing to one of their mailing lists.

--Pavel Kankovsky aka Peak  [ Boycott Microsoft--http://www.vcnet.com/bms ]
"Resistance is futile. Open your source code and prepare for assimilation."


-------------

--- client/dhclient.c.orig	Wed Jan 26 13:51:11 2000
+++ client/dhclient.c	Wed Jul 12 21:28:31 2000
@@ -902,7 +902,7 @@
 				break;
 		lease -> server_name = malloc (len + 1);
 		if (!lease -> server_name) {
-			warn ("dhcpoffer: no memory for filename.\n");
+			warn ("dhcpoffer: no memory for server name.\n");
 			free_client_lease (lease);
 			return (struct client_lease *)0;
 		} else {
@@ -1845,10 +1845,14 @@
 		 piaddr (lease -> address));
 	if (lease -> filename)
 		fprintf (leaseFile, "  filename \"%s\";\n",
-			 lease -> filename);
+			 pretty_print_option(67 /* bootfile-name option */,
+			 lease -> filename, strlen(lease -> filename),
+			 0, 0));
 	if (lease -> server_name)
 		fprintf (leaseFile, "  server-name \"%s\";\n",
-			 lease -> server_name);
+			 pretty_print_option(15 /* domain name option */,
+			 lease -> server_name, strlen(lease -> server_name),
+			 0, 0));
 	if (lease -> medium)
 		fprintf (leaseFile, "  medium \"%s\";\n",
 			 lease -> medium -> string);
@@ -1986,13 +1990,17 @@
 	}

 	if (lease -> filename) {
-		fprintf (scriptFile, "%sfilename=\"%s\";\n",
-			 prefix, lease -> filename);
+		fprintf (scriptFile, "%sfilename=\"%s\";\n", prefix,
+			 pretty_print_option(67 /* bootfile-name option */,
+			 lease -> filename, strlen(lease -> filename),
+			 0, 0));
 		fprintf (scriptFile, "export %sfilename\n", prefix);
 	}
 	if (lease -> server_name) {
-		fprintf (scriptFile, "%sserver_name=\"%s\";\n",
-			 prefix, lease -> server_name);
+		fprintf (scriptFile, "%sserver_name=\"%s\";\n", prefix,
+			 pretty_print_option(15 /* domain name option */,
+			 lease -> server_name, strlen(lease -> server_name),
+			 0, 0));
 		fprintf (scriptFile, "export %sserver_name\n", prefix);
 	}
 	for (i = 0; i < 256; i++) {