From f365498188260f1d3f03f19d75cf664b4b6503da Mon Sep 17 00:00:00 2001 From: "Nicholas J. Kain" Date: Thu, 20 Mar 2014 04:07:12 -0400 Subject: [PATCH] Convert the remaining strnk* calls to use snprintf instead, and make sure to detect truncations and failures in all cases. --- ndhc/duiaid.c | 1 - ndhc/ifchd-parse.rl | 36 +++++++++++++++++++++++++++--------- ndhc/ifset.c | 2 +- ndhc/leasefile.c | 18 +++++++++++++----- ndhc/ndhc.c | 38 ++++++++++++++++++++++++++------------ 5 files changed, 67 insertions(+), 28 deletions(-) diff --git a/ndhc/duiaid.c b/ndhc/duiaid.c index 5aa0fb5..ce75495 100644 --- a/ndhc/duiaid.c +++ b/ndhc/duiaid.c @@ -36,7 +36,6 @@ #include #include #include "duiaid.h" -#include "strl.h" #include "log.h" #include "random.h" #include "io.h" diff --git a/ndhc/ifchd-parse.rl b/ndhc/ifchd-parse.rl index 107dee9..697a84b 100644 --- a/ndhc/ifchd-parse.rl +++ b/ndhc/ifchd-parse.rl @@ -27,14 +27,16 @@ */ #include +#include +#include #include #include #include "ifchd-parse.h" #include "ifchd.h" #include "log.h" -#include "strl.h" #include "ifset.h" +#include "ndhc.h" %%{ machine ipv4set_parser; @@ -183,11 +185,15 @@ int execute_buffer(char *newbuf) char buf[MAX_BUF * 2]; char tb[MAX_BUF]; - if (strnkcpy(buf, cl.ibuf, sizeof buf)) - goto buftooshort; - if (strnkcat(buf, newbuf, sizeof buf)) { -buftooshort: - log_line("error: input is too long for buffer"); + ssize_t buflen = snprintf(buf, sizeof buf, "%s%s", cl.ibuf, newbuf); + if (buflen < 0) { + log_error("%s: (%s) snprintf1 failed; your system is broken?", + client_config.interface, __func__); + return -1; + } + if ((size_t)buflen >= sizeof buf) { + log_error("%s: (%s) input is too long for buffer", + client_config.interface, __func__); return -1; } @@ -204,14 +210,26 @@ buftooshort: size_t bytes_left = pe - p; if (bytes_left > 0) { size_t taken = init_siz - bytes_left; - strnkcpy(cl.ibuf, buf + taken, MAX_BUF); + ssize_t ilen = snprintf(cl.ibuf, sizeof cl.ibuf, "%s", buf + taken); + if (ilen < 0) { + log_error("%s: (%s) snprintf2 failed; your system is broken?", + client_config.interface, __func__); + return -1; + } + if ((size_t)ilen >= sizeof buf) { + log_error("%s: (%s) unconsumed input too long for buffer", + client_config.interface, __func__); + return -1; + } } if (cs < ifchd_parser_first_final) { - log_line("error: received invalid commands"); + log_error("%s: ifch received invalid commands", + client_config.interface); return -1; } - log_line("Commands received and successfully executed."); + log_line("%s: Commands received and successfully executed.", + client_config.interface); return 0; } diff --git a/ndhc/ifset.c b/ndhc/ifset.c index 11221ca..2d25a4c 100644 --- a/ndhc/ifset.c +++ b/ndhc/ifset.c @@ -27,6 +27,7 @@ */ #include +#include #include #include #include @@ -51,7 +52,6 @@ #include "ifchd.h" #include "ndhc.h" #include "log.h" -#include "strl.h" #include "nl.h" static uint32_t ifset_nl_seq = 1; diff --git a/ndhc/leasefile.c b/ndhc/leasefile.c index dfd0c61..3d6ee6b 100644 --- a/ndhc/leasefile.c +++ b/ndhc/leasefile.c @@ -40,7 +40,6 @@ #include "leasefile.h" #include "ndhc.h" #include "log.h" -#include "strl.h" #include "io.h" #include "defines.h" @@ -76,12 +75,21 @@ void open_leasefile(void) void write_leasefile(struct in_addr ipnum) { - char ip[INET_ADDRSTRLEN+2]; + char ip[INET_ADDRSTRLEN]; + char out[INET_ADDRSTRLEN*2]; int ret; - if (leasefilefd < 0) + if (leasefilefd < 0) { + log_error("%s: (%s) leasefile fd < 0; no leasefile will be written", + client_config.interface, __func__); return; + } inet_ntop(AF_INET, &ipnum, ip, sizeof ip); - strnkcat(ip, "\n", sizeof ip); + ssize_t olen = snprintf(out, sizeof out, "%s\n", ip); + if (olen < 0 || (size_t)olen >= sizeof ip) { + log_error("%s: (%s) snprintf failed; return=%d", + client_config.interface, __func__, olen); + return; + } retry_trunc: ret = ftruncate(leasefilefd, 0); switch (ret) { @@ -94,7 +102,7 @@ void write_leasefile(struct in_addr ipnum) return; } lseek(leasefilefd, 0, SEEK_SET); - ret = safe_write(leasefilefd, ip, strlen(ip)); + ret = safe_write(leasefilefd, out, strlen(out)); if (ret == -1) log_warning("%s: Failed to write ip to lease file.", client_config.interface); diff --git a/ndhc/ndhc.c b/ndhc/ndhc.c index a1f6087..09f5237 100644 --- a/ndhc/ndhc.c +++ b/ndhc/ndhc.c @@ -65,7 +65,6 @@ #include "log.h" #include "chroot.h" #include "cap.h" -#include "strl.h" #include "pidfile.h" #include "io.h" #include "seccomp.h" @@ -419,6 +418,19 @@ void background(void) write_pid(pidfile); } +static void copy_cmdarg(char *dest, char *src, size_t destlen, char *argname) +{ + ssize_t olen = snprintf(dest, destlen, "%s", src); + if (olen < 0) { + log_error("snprintf failed on %s; your system is broken?", argname); + exit(EXIT_FAILURE); + } + if ((size_t)olen >= destlen) { + log_error("snprintf would truncate %s arg; it's too long", argname); + exit(EXIT_FAILURE); + } +} + int main(int argc, char **argv) { static const struct option arg_options[] = { @@ -470,18 +482,19 @@ int main(int argc, char **argv) gflags_detach = 1; break; case 'p': - strnkcpy(pidfile, optarg, sizeof pidfile); + copy_cmdarg(pidfile, optarg, sizeof pidfile, "pidfile"); break; case 'P': - strnkcpy(pidfile_ifch, optarg, sizeof pidfile_ifch); + copy_cmdarg(pidfile_ifch, optarg, sizeof pidfile_ifch, + "ifch-pidfile"); break; case 'h': - strnkcpy(client_config.hostname, optarg, - sizeof client_config.hostname); + copy_cmdarg(client_config.hostname, optarg, + sizeof client_config.hostname, "hostname"); break; case 'i': - strnkcpy(client_config.interface, optarg, - sizeof client_config.interface); + copy_cmdarg(client_config.interface, optarg, + sizeof client_config.interface, "interface"); break; case 'n': client_config.abort_if_no_lease = 1; @@ -527,10 +540,10 @@ int main(int argc, char **argv) break; } case 'C': - strnkcpy(chroot_dir, optarg, sizeof chroot_dir); + copy_cmdarg(chroot_dir, optarg, sizeof chroot_dir, "chroot"); break; case 's': - strnkcpy(state_dir, optarg, sizeof state_dir); + copy_cmdarg(state_dir, optarg, sizeof state_dir, "state-dir"); break; case 'S': seccomp_enforce = true; @@ -588,8 +601,8 @@ int main(int argc, char **argv) exit(EXIT_SUCCESS); break; case 'V': - strnkcpy(client_config.vendor, optarg, - sizeof client_config.vendor); + copy_cmdarg(client_config.vendor, optarg, + sizeof client_config.vendor, "vendorid"); break; case 't': { char *p; @@ -609,7 +622,8 @@ int main(int argc, char **argv) break; } case 'R': - strnkcpy(resolv_conf_d, optarg, sizeof resolv_conf_d); + copy_cmdarg(resolv_conf_d, optarg, sizeof resolv_conf_d, + "resolv-conf"); break; case 'H': allow_hostname = 1;