Hello,
I just wanted to double check if there is any additional feedback on the patch.
I am happy to make changes or discuss alternative approaches.
Thank you,
Stephen
On Tue, Oct 3, 2023 at 11:33 PM Stephen Fox <stephen.j.fox...@gmail.com> wrote:
>
> Hello,
>
> I received feedback from deraadt that the first two unveil(2) calls were
> unnecessary because pledge(2) automatically unveils "/usr/share/zoneinfo".
>
> This updated patch removes the unneeded unveil(2) calls.
>
> Best regards,
> Stephen
> ---
> usr.sbin/dhcpd/confpars.c | 41 ++++++++++++++++++++++++++-------------
> usr.sbin/dhcpd/db.c | 19 ++++++++++++++++++
> usr.sbin/dhcpd/dhcpd.c | 30 ++++++++++++++++++++++++++--
> usr.sbin/dhcpd/dhcpd.h | 2 ++
> 4 files changed, 76 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..7759f7839e0 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,34 @@ 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. These files are
> + * automatically unveiled by pledge(2). It also opens the
> + * /etc/ethers file using ether_hostton(3).
> + */
> + 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 */
