From a51288aabbbc176a8a73a8b3cd56f79607db32cf Mon Sep 17 00:00:00 2001 From: Richard Yao Date: Tue, 27 Sep 2022 19:47:24 -0400 Subject: [PATCH] Fix unsafe string operations Coverity caught unsafe use of `strcpy()` in `ztest_dmu_objset_own()`, `nfs_init_tmpfile()` and `dump_snapshot()`. It also caught an unsafe use of `strlcat()` in `nfs_init_tmpfile()`. Inspired by this, I did an audit of every single usage of `strcpy()` and `strcat()` in the code. If I could not prove that the usage was safe, I changed the code to use either `strlcpy()` or `strlcat()`, depending on which function was originally used. In some cases, `snprintf()` was used to replace multiple uses of `strcat` because it was cleaner. Whenever I changed a function, I preferred to use `sizeof(dst)` when the compiler is able to provide the string size via that. When it could not because the string was passed by a caller, I checked the entire call tree of the function to find out how big the buffer was and hard coded it. Hardcoding is less than ideal, but it is safe unless someone shrinks the buffer sizes being passed. Additionally, Coverity reported three more string related issues: * It caught a case where we do an overlapping memory copy in a call to `snprintf()`. We fix that via `kmem_strdup()` and `kmem_strfree()`. * It caught `sizeof (buf)` being used instead of `buflen` in `zdb_nicenum()`'s call to `zfs_nicenum()`, which is passed to `snprintf()`. We change that to pass `buflen`. * It caught a theoretical unterminated string passed to `strcmp()`. This one is likely a false positive, but we have the information needed to do this more safely, so we change this to silence the false positive not just in coverity, but potentially other static analysis tools too. We switch to `strncmp()`. * There was a false positive in tests/zfs-tests/cmd/dir_rd_update.c. We suppress it by switching to `snprintf()` since other static analysis tools might complain about it too. Interestingly, there is a possible real bug there too, since it assumes that the passed directory path ends with '/'. We add a '/' to fix that potential bug. Reviewed-by: Brian Behlendorf Signed-off-by: Richard Yao Closes #13913 --- cmd/zdb/zdb.c | 2 +- cmd/zfs/zfs_main.c | 3 ++- cmd/zinject/translate.c | 8 ++++---- cmd/ztest.c | 10 +++++----- contrib/pam_zfs_key/pam_zfs_key.c | 5 ++--- lib/libshare/nfs.c | 4 ++-- lib/libspl/os/freebsd/mnttab.c | 2 +- lib/libzfs/libzfs_sendrecv.c | 28 ++++++++++++++++------------ lib/libzfs/os/linux/libzfs_util_os.c | 4 ++-- module/zfs/dmu_send.c | 3 ++- module/zfs/dsl_dataset.c | 3 +++ tests/zfs-tests/cmd/dir_rd_update.c | 5 ++--- tests/zfs-tests/cmd/mmap_sync.c | 3 +-- 13 files changed, 43 insertions(+), 37 deletions(-) diff --git a/cmd/zdb/zdb.c b/cmd/zdb/zdb.c index ba746e8a306..96db9c4b9a7 100644 --- a/cmd/zdb/zdb.c +++ b/cmd/zdb/zdb.c @@ -985,7 +985,7 @@ zdb_nicenum(uint64_t num, char *buf, size_t buflen) if (dump_opt['P']) (void) snprintf(buf, buflen, "%llu", (longlong_t)num); else - nicenum(num, buf, sizeof (buf)); + nicenum(num, buf, buflen); } static const char histo_stars[] = "****************************************"; diff --git a/cmd/zfs/zfs_main.c b/cmd/zfs/zfs_main.c index 74bdd796e88..7bda3f5292b 100644 --- a/cmd/zfs/zfs_main.c +++ b/cmd/zfs/zfs_main.c @@ -2459,7 +2459,8 @@ upgrade_set_callback(zfs_handle_t *zhp, void *data) cb->cb_numupgraded++; else cb->cb_numfailed++; - (void) strcpy(cb->cb_lastfs, zfs_get_name(zhp)); + (void) strlcpy(cb->cb_lastfs, zfs_get_name(zhp), + sizeof (cb->cb_lastfs)); } else if (version > cb->cb_version) { /* can't downgrade */ (void) printf(gettext("%s: can not be downgraded; " diff --git a/cmd/zinject/translate.c b/cmd/zinject/translate.c index cd4264bdc08..c63caedd158 100644 --- a/cmd/zinject/translate.c +++ b/cmd/zinject/translate.c @@ -115,12 +115,12 @@ parse_pathname(const char *inpath, char *dataset, char *relpath, return (-1); } - (void) strcpy(dataset, mp.mnt_special); + (void) strlcpy(dataset, mp.mnt_special, MAXNAMELEN); rel = fullpath + strlen(mp.mnt_mountp); if (rel[0] == '/') rel++; - (void) strcpy(relpath, rel); + (void) strlcpy(relpath, rel, MAXPATHLEN); return (0); } @@ -258,7 +258,7 @@ translate_record(err_type_t type, const char *object, const char *range, } dataset[0] = '\0'; - (void) strcpy(poolname, object); + (void) strlcpy(poolname, object, MAXNAMELEN); return (0); } @@ -298,7 +298,7 @@ translate_record(err_type_t type, const char *object, const char *range, /* * Copy the pool name */ - (void) strcpy(poolname, dataset); + (void) strlcpy(poolname, dataset, MAXNAMELEN); if ((slash = strchr(poolname, '/')) != NULL) *slash = '\0'; diff --git a/cmd/ztest.c b/cmd/ztest.c index b67c765a1aa..436f5c156ec 100644 --- a/cmd/ztest.c +++ b/cmd/ztest.c @@ -1544,7 +1544,7 @@ ztest_dmu_objset_own(const char *name, dmu_objset_type_t type, char *cp = NULL; char ddname[ZFS_MAX_DATASET_NAME_LEN]; - strcpy(ddname, name); + strlcpy(ddname, name, sizeof (ddname)); cp = strchr(ddname, '@'); if (cp != NULL) *cp = '\0'; @@ -3667,7 +3667,7 @@ ztest_vdev_attach_detach(ztest_ds_t *zd, uint64_t id) oldguid = oldvd->vdev_guid; oldsize = vdev_get_min_asize(oldvd); oldvd_is_log = oldvd->vdev_top->vdev_islog; - (void) strcpy(oldpath, oldvd->vdev_path); + (void) strlcpy(oldpath, oldvd->vdev_path, MAXPATHLEN); pvd = oldvd->vdev_parent; pguid = pvd->vdev_guid; @@ -3703,7 +3703,7 @@ ztest_vdev_attach_detach(ztest_ds_t *zd, uint64_t id) if (newvd->vdev_ops == &vdev_draid_spare_ops) newvd_is_dspare = B_TRUE; - (void) strcpy(newpath, newvd->vdev_path); + (void) strlcpy(newpath, newvd->vdev_path, MAXPATHLEN); } else { (void) snprintf(newpath, MAXPATHLEN, ztest_dev_template, ztest_opts.zo_dir, ztest_opts.zo_pool, @@ -6145,8 +6145,8 @@ ztest_fault_inject(ztest_ds_t *zd, uint64_t id) } vd0 = sav->sav_vdevs[ztest_random(sav->sav_count)]; guid0 = vd0->vdev_guid; - (void) strcpy(path0, vd0->vdev_path); - (void) strcpy(pathrand, vd0->vdev_path); + (void) strlcpy(path0, vd0->vdev_path, MAXPATHLEN); + (void) strlcpy(pathrand, vd0->vdev_path, MAXPATHLEN); leaf = 0; leaves = 1; diff --git a/contrib/pam_zfs_key/pam_zfs_key.c b/contrib/pam_zfs_key/pam_zfs_key.c index aaca670080b..e0bbd249afa 100644 --- a/contrib/pam_zfs_key/pam_zfs_key.c +++ b/contrib/pam_zfs_key/pam_zfs_key.c @@ -558,9 +558,8 @@ zfs_key_config_get_dataset(zfs_key_config_t *config) return (NULL); } ret[0] = 0; - strcat(ret, config->homes_prefix); - strcat(ret, "/"); - strcat(ret, config->username); + (void) snprintf(ret, len + 1, "%s/%s", config->homes_prefix, + config->username); return (ret); } diff --git a/lib/libshare/nfs.c b/lib/libshare/nfs.c index bbaea93fca5..3251913cba6 100644 --- a/lib/libshare/nfs.c +++ b/lib/libshare/nfs.c @@ -95,8 +95,8 @@ nfs_init_tmpfile(const char *prefix, const char *mdir, struct tmpfile *tmpf) return (B_FALSE); } - strcpy(tmpf->name, prefix); - strcat(tmpf->name, ".XXXXXXXX"); + strlcpy(tmpf->name, prefix, sizeof (tmpf->name)); + strlcat(tmpf->name, ".XXXXXXXX", sizeof (tmpf->name) - strlen(prefix)); int fd = mkostemp(tmpf->name, O_CLOEXEC); if (fd == -1) { diff --git a/lib/libspl/os/freebsd/mnttab.c b/lib/libspl/os/freebsd/mnttab.c index f0cc04d89de..a4673084ad5 100644 --- a/lib/libspl/os/freebsd/mnttab.c +++ b/lib/libspl/os/freebsd/mnttab.c @@ -74,7 +74,7 @@ hasmntopt(struct mnttab *mnt, const char *opt) if (mnt->mnt_mntopts == NULL) return (NULL); - (void) strcpy(opts, mnt->mnt_mntopts); + (void) strlcpy(opts, mnt->mnt_mntopts, MNT_LINE_MAX); f = mntopt(&opts); for (; *f; f = mntopt(&opts)) { if (strncmp(opt, f, strlen(opt)) == 0) diff --git a/lib/libzfs/libzfs_sendrecv.c b/lib/libzfs/libzfs_sendrecv.c index 9d514a37eeb..d63a9e1a4e0 100644 --- a/lib/libzfs/libzfs_sendrecv.c +++ b/lib/libzfs/libzfs_sendrecv.c @@ -1175,7 +1175,7 @@ dump_snapshot(zfs_handle_t *zhp, void *arg) return (-1); } - (void) strcpy(sdd->prevsnap, thissnap); + (void) strlcpy(sdd->prevsnap, thissnap, sizeof (sdd->prevsnap)); sdd->prevsnap_obj = zfs_prop_get_int(zhp, ZFS_PROP_OBJSETID); zfs_close(zhp); return (err); @@ -1736,7 +1736,7 @@ zfs_send_resume_impl_cb_impl(libzfs_handle_t *hdl, sendflags_t *flags, (void) nvlist_lookup_uint64(resume_nvl, "fromguid", &fromguid); if (flags->saved) { - (void) strcpy(name, toname); + (void) strlcpy(name, toname, sizeof (name)); } else { error = guid_to_name(hdl, toname, toguid, B_FALSE, name); if (error != 0) { @@ -2880,7 +2880,7 @@ recv_rename(libzfs_handle_t *hdl, const char *name, const char *tryname, goto out; if (tryname) { - (void) strcpy(newname, tryname); + (void) strlcpy(newname, tryname, ZFS_MAX_DATASET_NAME_LEN); if (flags->verbose) { (void) printf("attempting rename %s to %s\n", name, newname); @@ -4331,7 +4331,7 @@ zfs_receive_one(libzfs_handle_t *hdl, int infd, const char *tosnap, err = nvlist_lookup_string(rcvprops, zfs_prop_to_name(ZFS_PROP_KEYLOCATION), &keylocation); if (err == 0) { - strcpy(tmp_keylocation, keylocation); + strlcpy(tmp_keylocation, keylocation, MAXNAMELEN); (void) nvlist_remove_all(rcvprops, zfs_prop_to_name(ZFS_PROP_KEYLOCATION)); } @@ -4498,18 +4498,20 @@ zfs_receive_one(libzfs_handle_t *hdl, int infd, const char *tosnap, (void) snprintf(errbuf, sizeof (errbuf), dgettext(TEXT_DOMAIN, "cannot receive new filesystem stream")); - (void) strcpy(name, destsnap); + (void) strlcpy(name, destsnap, sizeof (name)); cp = strrchr(name, '/'); if (cp) *cp = '\0'; if (cp && !zfs_dataset_exists(hdl, name, ZFS_TYPE_DATASET)) { char suffix[ZFS_MAX_DATASET_NAME_LEN]; - (void) strcpy(suffix, strrchr(destsnap, '/')); + (void) strlcpy(suffix, strrchr(destsnap, '/'), + sizeof (suffix)); if (guid_to_name(hdl, name, parent_snapguid, B_FALSE, destsnap) == 0) { *strchr(destsnap, '@') = '\0'; - (void) strcat(destsnap, suffix); + (void) strlcat(destsnap, suffix, + sizeof (destsnap) - strlen(destsnap)); } } } else { @@ -4527,7 +4529,7 @@ zfs_receive_one(libzfs_handle_t *hdl, int infd, const char *tosnap, "cannot receive incremental stream")); } - (void) strcpy(name, destsnap); + (void) strlcpy(name, destsnap, sizeof (name)); *strchr(name, '@') = '\0'; /* @@ -4539,16 +4541,18 @@ zfs_receive_one(libzfs_handle_t *hdl, int infd, const char *tosnap, strlen(sendfs)) != '\0' && *chopprefix != '@')) && !zfs_dataset_exists(hdl, name, ZFS_TYPE_DATASET)) { char snap[ZFS_MAX_DATASET_NAME_LEN]; - (void) strcpy(snap, strchr(destsnap, '@')); + (void) strlcpy(snap, strchr(destsnap, '@'), + sizeof (snap)); if (guid_to_name(hdl, name, drrb->drr_fromguid, B_FALSE, destsnap) == 0) { *strchr(destsnap, '@') = '\0'; - (void) strcat(destsnap, snap); + (void) strlcat(destsnap, snap, + sizeof (destsnap) - strlen(destsnap)); } } } - (void) strcpy(name, destsnap); + (void) strlcpy(name, destsnap, sizeof (name)); *strchr(name, '@') = '\0'; redacted = DMU_GET_FEATUREFLAGS(drrb->drr_versioninfo) & @@ -4930,7 +4934,7 @@ zfs_receive_one(libzfs_handle_t *hdl, int infd, const char *tosnap, if (err == 0 && snapprops_nvlist) { zfs_cmd_t zc = {"\0"}; - (void) strcpy(zc.zc_name, destsnap); + (void) strlcpy(zc.zc_name, destsnap, sizeof (zc.zc_name)); zc.zc_cookie = B_TRUE; /* received */ zcmd_write_src_nvlist(hdl, &zc, snapprops_nvlist); (void) zfs_ioctl(hdl, ZFS_IOC_SET_PROP, &zc); diff --git a/lib/libzfs/os/linux/libzfs_util_os.c b/lib/libzfs/os/linux/libzfs_util_os.c index 4a44aa1f1d0..99faae66833 100644 --- a/lib/libzfs/os/linux/libzfs_util_os.c +++ b/lib/libzfs/os/linux/libzfs_util_os.c @@ -145,8 +145,8 @@ libzfs_load_module(void) if (pfds[0].revents & POLLIN) { verify(read(ino, ev, evsz) > sizeof (struct inotify_event)); - if (strcmp(ev->name, &ZFS_DEV[sizeof (ZFS_DEVDIR)]) - == 0) { + if (strncmp(ev->name, &ZFS_DEV[sizeof (ZFS_DEVDIR)], + ev->len) == 0) { ret = 0; break; } diff --git a/module/zfs/dmu_send.c b/module/zfs/dmu_send.c index 95377fa4660..4ee3ffc352b 100644 --- a/module/zfs/dmu_send.c +++ b/module/zfs/dmu_send.c @@ -3016,7 +3016,8 @@ dmu_send_estimate_fast(dsl_dataset_t *origds, dsl_dataset_t *fromds, dsl_dataset_name(origds, dsname); (void) strcat(dsname, "/"); - (void) strcat(dsname, recv_clone_name); + (void) strlcat(dsname, recv_clone_name, + sizeof (dsname) - strlen(dsname)); err = dsl_dataset_hold(origds->ds_dir->dd_pool, dsname, FTAG, &ds); diff --git a/module/zfs/dsl_dataset.c b/module/zfs/dsl_dataset.c index 64bc70c240e..c944db2c50b 100644 --- a/module/zfs/dsl_dataset.c +++ b/module/zfs/dsl_dataset.c @@ -2740,6 +2740,8 @@ dsl_get_mountpoint(dsl_dataset_t *ds, const char *dsname, char *value, relpath[0] != '\0')) mnt = value + 1; + mnt = kmem_strdup(mnt); + if (relpath[0] == '\0') { (void) snprintf(value, ZAP_MAXVALUELEN, "%s%s", root, mnt); @@ -2749,6 +2751,7 @@ dsl_get_mountpoint(dsl_dataset_t *ds, const char *dsname, char *value, relpath); } kmem_free(buf, ZAP_MAXVALUELEN); + kmem_strfree(mnt); } return (0); diff --git a/tests/zfs-tests/cmd/dir_rd_update.c b/tests/zfs-tests/cmd/dir_rd_update.c index 80c395cc62d..c0d283105a9 100644 --- a/tests/zfs-tests/cmd/dir_rd_update.c +++ b/tests/zfs-tests/cmd/dir_rd_update.c @@ -63,13 +63,12 @@ main(int argc, char **argv) } cp1 = argv[1]; - if (strlen(cp1) >= (sizeof (dirpath) - strlen("TMP_DIR"))) { + if (strlen(cp1) >= (sizeof (dirpath) - strlen("/TMP_DIR"))) { (void) printf("The string length of mount point is " "too large\n"); exit(-1); } - (void) strcpy(&dirpath[0], (const char *)cp1); - (void) strcat(&dirpath[strlen(dirpath)], "TMP_DIR"); + (void) snprintf(dirpath, sizeof (dirpath), "%s/TMP_DIR", cp1); ret = mkdir(dirpath, 0777); if (ret != 0) { diff --git a/tests/zfs-tests/cmd/mmap_sync.c b/tests/zfs-tests/cmd/mmap_sync.c index 618f1284aa7..e4d190aef1c 100644 --- a/tests/zfs-tests/cmd/mmap_sync.c +++ b/tests/zfs-tests/cmd/mmap_sync.c @@ -73,8 +73,7 @@ main(int argc, char *argv[]) filepath[0] = '\0'; char *file = &filepath[0]; - strcat(file, testdir); - strcat(file, "/msync_file"); + (void) snprintf(file, 512, "%s/msync_file", testdir); const int LEN = 8; cleanup(file);