Hello,
While reading dhcpd's code, I noticed it parses the lease database
file ("/var/db/dhcpd.leases") while the process is running as root.
This happens prior to switching to the "_dhcp" user and calling
chroot(2) / pledge(2).
This is potentially unsafe because the lease database file contains
attacker-controlled data (such as client hostnames). I really like
the thoughtful security measures that the daemon takes on startup.
It felt worthwhile to ensure that the parsing logic executes after
those measures are applied.
This patch attempts to split the lease database file logic apart,
making dhcpd behave like this:
1. Open read-only and write-only file descriptors to the lease DB
2. Drop privileges / apply security policy
3. Read data from the read-only file descriptor and parse the data
The tricky part about this change is that the lease parsing logic
opens and reads from the following files:
- /usr/share/zoneinfo/GMT (Due to date string with timezone)
- /usr/share/zoneinfo/posixrules (Due to date string with timezone)
- /etc/ethers (Because of call to ether_hostton(3))
Although the first two files are covered by pledge(2) "stdio", the
ethers file is not. Another complication is that chroot(2) call
makes those files unavailable to the process.
To handle this complication, I decided to replace the chroot(2) call
with unveil(2). This required the addition of the "rpath" promise to
read ethers. I am not the biggest fan of the pledge(2) code I added.
Perhaps there is a cleaner / more graceful way to drop the
"rpath" privilege?
Best regards,
Stephen
---
usr.sbin/dhcpd/confpars.c | 41 ++++++++++++++++++++++++++-------------
usr.sbin/dhcpd/db.c | 19 ++++++++++++++++++
usr.sbin/dhcpd/dhcpd.c | 33 +++++++++++++++++++++++++++++--
usr.sbin/dhcpd/dhcpd.h | 2 ++
4 files changed, 79 insertions(+), 16 deletions(-)
diff --git usr.sbin/dhcpd/confpars.c usr.sbin/dhcpd/confpars.c
index 1bdffb38842..bb245fc663e 100644
--- usr.sbin/dhcpd/confpars.c
+++ usr.sbin/dhcpd/confpars.c
@@ -57,6 +57,8 @@
#include "dhctoken.h"
#include "log.h"
+FILE *db_file_ro;
+
int parse_cidr(FILE *, unsigned char *, unsigned char *);
/*
@@ -106,18 +108,11 @@ readconf(void)
}
/*
- * lease-file :== lease-declarations EOF
- * lease-statments :== <nil>
- * | lease-declaration
- * | lease-declarations lease-declaration
+ * Open and retain a read-only file descriptor to the lease database file.
*/
void
-read_leases(void)
+open_leases(void)
{
- FILE *cfile;
- char *val;
- int token;
-
new_parse(path_dhcpd_db);
/*
@@ -131,23 +126,40 @@ read_leases(void)
* thinking that no leases have been assigned to anybody, which
* could create severe network chaos.
*/
- if ((cfile = fopen(path_dhcpd_db, "r")) == NULL) {
+ if ((db_file_ro = fopen(path_dhcpd_db, "r")) == NULL) {
log_warn("Can't open lease database (%s)", path_dhcpd_db);
log_warnx("check for failed database rewrite attempt!");
log_warnx("Please read the dhcpd.leases manual page if you");
fatalx("don't know what to do about this.");
}
+}
+
+/*
+ * lease-file :== lease-declarations EOF
+ * lease-statments :== <nil>
+ * | lease-declaration
+ * | lease-declarations lease-declaration
+ */
+void
+read_leases(void)
+{
+ char *val;
+ int token;
+
+ if (!db_file_ro) {
+ fatalx("db_file_ro is NULL");
+ }
do {
- token = next_token(&val, cfile);
+ token = next_token(&val, db_file_ro);
if (token == EOF)
break;
if (token != TOK_LEASE) {
log_warnx("Corrupt lease file - possible data loss!");
- skip_to_semi(cfile);
+ skip_to_semi(db_file_ro);
} else {
struct lease *lease;
- lease = parse_lease_declaration(cfile);
+ lease = parse_lease_declaration(db_file_ro);
if (lease)
enter_lease(lease);
else
@@ -155,7 +167,8 @@ read_leases(void)
}
} while (1);
- fclose(cfile);
+ fclose(db_file_ro);
+ db_file_ro = NULL;
}
/*
diff --git usr.sbin/dhcpd/db.c usr.sbin/dhcpd/db.c
index 295e522b1ce..634ec8a593a 100644
--- usr.sbin/dhcpd/db.c
+++ usr.sbin/dhcpd/db.c
@@ -190,6 +190,16 @@ commit_leases(void)
return (1);
}
+/*
+ * Open and retain two file descriptors to the lease database file:
+ *
+ * - A read-only fd for learning existing leases
+ * - A write-only fd for writing new leases
+ *
+ * Reading and parsing data from the read-only fd is done separately.
+ * This allows privilege drop to happen *before* parsing untrusted
+ * client data from the lease database file.
+ */
void
db_startup(void)
{
@@ -202,6 +212,15 @@ db_startup(void)
if ((db_file = fdopen(db_fd, "w")) == NULL)
fatalx("Can't fdopen new lease file!");
+ open_leases();
+}
+
+/*
+ * Read and parse the existing leases from the lease database file.
+ */
+void
+db_parse(void)
+{
/* Read in the existing lease file... */
read_leases();
time(&write_time);
diff --git usr.sbin/dhcpd/dhcpd.c usr.sbin/dhcpd/dhcpd.c
index b3562dce34f..d7006b628c3 100644
--- usr.sbin/dhcpd/dhcpd.c
+++ usr.sbin/dhcpd/dhcpd.c
@@ -244,8 +244,6 @@ main(int argc, char *argv[])
icmp_startup(1, lease_pinged);
- if (chroot(pw->pw_dir) == -1)
- fatal("chroot %s", pw->pw_dir);
if (chdir("/") == -1)
fatal("chdir(\"/\")");
if (setgroups(1, &pw->pw_gid) ||
@@ -253,6 +251,37 @@ main(int argc, char *argv[])
setresuid(pw->pw_uid, pw->pw_uid, pw->pw_uid))
fatal("can't drop privileges");
+ /*
+ * The lease database parsing logic needs the zoneinfo files
+ * for the lease begin / end date strings. It also opens the
+ * /etc/ethers file using ether_hostton(3).
+ */
+ if (unveil("/usr/share/zoneinfo/GMT", "r") == -1)
+ err(1, "unveil /usr/share/zoneinfo/GMT");
+ if (unveil("/usr/share/zoneinfo/posixrules", "r") == -1)
+ err(1, "unveil /usr/share/zoneinfo/posixrules");
+ if (unveil("/etc/ethers", "r") == -1)
+ err(1, "unveil /etc/ethers");
+ if (unveil(NULL, NULL) == -1)
+ err(1, "unveil");
+
+ /*
+ * Apply initial pledge(2) promises - we will drop "rpath" after
+ * parsing the lease database (we need it for ether_hostton(3)).
+ */
+ if (udpsockmode) {
+ if (pledge("rpath stdio inet route sendfd", NULL) == -1)
+ err(1, "pledge init");
+ } else {
+ if (pledge("rpath stdio inet sendfd", NULL) == -1)
+ err(1, "pledge init");
+ }
+
+ db_parse();
+
+ /*
+ * Drop the "rpath" promise.
+ */
if (udpsockmode) {
if (pledge("stdio inet route sendfd", NULL) == -1)
err(1, "pledge");
diff --git usr.sbin/dhcpd/dhcpd.h usr.sbin/dhcpd/dhcpd.h
index 0903c244ddd..8c52b6603fc 100644
--- usr.sbin/dhcpd/dhcpd.h
+++ usr.sbin/dhcpd/dhcpd.h
@@ -343,6 +343,7 @@ int peek_token(char **, FILE *);
/* confpars.c */
int readconf(void);
+void open_leases(void);
void read_leases(void);
int parse_statement(FILE *, struct group *, int, struct host_decl *, int);
void parse_allow_deny(FILE *, struct group *, int);
@@ -489,6 +490,7 @@ char *piaddr(struct iaddr);
int write_lease(struct lease *);
int commit_leases(void);
void db_startup(void);
+void db_parse(void);
void new_lease_file(void);
/* packet.c */
