From 90e38dfd93e2eab866d5b6238f87a0ea4965b59a Mon Sep 17 00:00:00 2001 From: delphij Date: Wed, 5 Aug 2015 22:04:54 +0000 Subject: [PATCH 1/4] Fix shell injection vulnerability in patch(1) via ed(1) by tightening sanity check of the input. [1] While I'm there also replace ed(1) with red(1) because we do not need the unrestricted functionality. [2] Obtained from: Bitrig [1], DragonFly [2] Security: CVE-2015-1418 [1] --- usr.bin/patch/pathnames.h | 2 +- usr.bin/patch/pch.c | 18 +++++++++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/usr.bin/patch/pathnames.h b/usr.bin/patch/pathnames.h index d31300ea843a..79d8faea6285 100644 --- a/usr.bin/patch/pathnames.h +++ b/usr.bin/patch/pathnames.h @@ -9,4 +9,4 @@ #include -#define _PATH_ED "/bin/ed" +#define _PATH_RED "/bin/red" diff --git a/usr.bin/patch/pch.c b/usr.bin/patch/pch.c index e731ca1f9e8f..5fadf6292f67 100644 --- a/usr.bin/patch/pch.c +++ b/usr.bin/patch/pch.c @@ -1,4 +1,3 @@ - /*- * Copyright 1986, Larry Wall * @@ -1410,13 +1409,14 @@ do_ed_script(void) char *t; off_t beginning_of_this_line; FILE *pipefp = NULL; + int continuation; if (!skip_rest_of_patch) { if (copy_file(filearg[0], TMPOUTNAME) < 0) { unlink(TMPOUTNAME); fatal("can't create temp file %s", TMPOUTNAME); } - snprintf(buf, buf_size, "%s%s%s", _PATH_ED, + snprintf(buf, buf_size, "%s%s%s", _PATH_RED, verbose ? " " : " -s ", TMPOUTNAME); pipefp = popen(buf, "w"); } @@ -1434,7 +1434,19 @@ do_ed_script(void) (*t == 'a' || *t == 'c' || *t == 'd' || *t == 'i' || *t == 's')) { if (pipefp != NULL) fputs(buf, pipefp); - if (*t != 'd') { + if (*t == 's') { + for (;;) { + continuation = 0; + t = strchr(buf, '\0') - 1; + while (--t >= buf && *t == '\\') + continuation = !continuation; + if (!continuation || + pgets(true) == 0) + break; + if (pipefp != NULL) + fputs(buf, pipefp); + } + } else if (*t != 'd') { while (pgets(true)) { p_input_line++; if (pipefp != NULL) From 247f258761b884908a602fa331898ec778345877 Mon Sep 17 00:00:00 2001 From: delphij Date: Wed, 5 Aug 2015 22:04:56 +0000 Subject: [PATCH 2/4] Fix a bug which could make routed(8) daemon exit by sending a special RIP query from a remote machine, similar to SA-14:21.routed. Submitted by: hrs --- sbin/routed/input.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/sbin/routed/input.c b/sbin/routed/input.c index 895ef507db7e..bc030283e15f 100644 --- a/sbin/routed/input.c +++ b/sbin/routed/input.c @@ -160,6 +160,12 @@ input(struct sockaddr_in *from, /* received from this IP address */ trace_rip("Recv", "from", from, sifp, rip, cc); + if (sifp == 0) { + trace_pkt(" discard a request from an indirect router" + " (possibly an attack)"); + return; + } + if (rip->rip_vers == 0) { msglim(&bad_router, FROM_NADDR, "RIP version 0, cmd %d, packet received from %s", From 45b2a7c139b41e709a7e27fa21cf9656dfb2f825 Mon Sep 17 00:00:00 2001 From: mav Date: Wed, 5 Aug 2015 22:24:49 +0000 Subject: [PATCH 3/4] Pass SYNCHRONIZE CACHE command parameters to backends. At this point IMMED flag is translated to MNT_NOWAIT flag of VOP_FSYNC(), hoping that file system implements that (ZFS seems doesn't). MFC after: 2 weeks Sponsored by: iXsystems, Inc. --- sys/cam/ctl/ctl.c | 9 ++++ sys/cam/ctl/ctl_backend_block.c | 88 ++++++++++++++------------------- sys/cam/ctl/ctl_cmd_table.c | 4 +- 3 files changed, 49 insertions(+), 52 deletions(-) diff --git a/sys/cam/ctl/ctl.c b/sys/cam/ctl/ctl.c index 3d562d02c8f2..3889ca8eb642 100644 --- a/sys/cam/ctl/ctl.c +++ b/sys/cam/ctl/ctl.c @@ -5507,9 +5507,11 @@ ctl_sync_cache(struct ctl_scsiio *ctsio) { struct ctl_lun *lun; struct ctl_softc *softc; + struct ctl_lba_len_flags *lbalen; uint64_t starting_lba; uint32_t block_count; int retval; + uint8_t byte2; CTL_DEBUG_PRINT(("ctl_sync_cache\n")); @@ -5524,6 +5526,7 @@ ctl_sync_cache(struct ctl_scsiio *ctsio) starting_lba = scsi_4btoul(cdb->begin_lba); block_count = scsi_2btoul(cdb->lb_count); + byte2 = cdb->byte2; break; } case SYNCHRONIZE_CACHE_16: { @@ -5532,6 +5535,7 @@ ctl_sync_cache(struct ctl_scsiio *ctsio) starting_lba = scsi_8btou64(cdb->begin_lba); block_count = scsi_4btoul(cdb->lb_count); + byte2 = cdb->byte2; break; } default: @@ -5562,6 +5566,11 @@ ctl_sync_cache(struct ctl_scsiio *ctsio) goto bailout; } + lbalen = (struct ctl_lba_len_flags *)&ctsio->io_hdr.ctl_private[CTL_PRIV_LBA_LEN]; + lbalen->lba = starting_lba; + lbalen->len = block_count; + lbalen->flags = byte2; + /* * Check to see whether we're configured to send the SYNCHRONIZE * CACHE command directly to the back end. diff --git a/sys/cam/ctl/ctl_backend_block.c b/sys/cam/ctl/ctl_backend_block.c index 8ea52aa323df..5bb3121688a3 100644 --- a/sys/cam/ctl/ctl_backend_block.c +++ b/sys/cam/ctl/ctl_backend_block.c @@ -224,6 +224,7 @@ struct ctl_be_block_io { devstat_trans_flags ds_trans_type; uint64_t io_len; uint64_t io_offset; + int io_arg; struct ctl_be_block_softc *softc; struct ctl_be_block_lun *lun; void (*beio_cont)(struct ctl_be_block_io *beio); /* to continue processing */ @@ -581,7 +582,8 @@ ctl_be_block_flush_file(struct ctl_be_block_lun *be_lun, vn_lock(be_lun->vn, lock_flags | LK_RETRY); - error = VOP_FSYNC(be_lun->vn, MNT_WAIT, curthread); + error = VOP_FSYNC(be_lun->vn, beio->io_arg ? MNT_NOWAIT : MNT_WAIT, + curthread); VOP_UNLOCK(be_lun->vn, 0); vn_finished_write(mountpoint); @@ -674,9 +676,6 @@ ctl_be_block_dispatch_file(struct ctl_be_block_lun *be_lun, * ZFS pays attention to IO_SYNC (which translates into the * Solaris define FRSYNC for zfs_read()) for reads. It * attempts to sync the file before reading. - * - * So, to attempt to provide some barrier semantics in the - * BIO_ORDERED case, set both IO_DIRECT and IO_SYNC. */ error = VOP_READ(be_lun->vn, &xuio, flags, file_data->cred); @@ -711,9 +710,6 @@ ctl_be_block_dispatch_file(struct ctl_be_block_lun *be_lun, * ZFS pays attention to IO_SYNC (a.k.a. FSYNC or FRSYNC) * for writes. It will flush the transaction from the * cache before returning. - * - * So if we've got the BIO_ORDERED flag set, we want - * IO_SYNC in either the UFS or ZFS case. */ error = VOP_WRITE(be_lun->vn, &xuio, flags, file_data->cred); VOP_UNLOCK(be_lun->vn, 0); @@ -981,7 +977,6 @@ ctl_be_block_flush_dev(struct ctl_be_block_lun *be_lun, bio = g_alloc_bio(); bio->bio_cmd = BIO_FLUSH; - bio->bio_flags |= BIO_ORDERED; bio->bio_dev = dev_data->cdev; bio->bio_offset = 0; bio->bio_data = 0; @@ -1166,6 +1161,26 @@ ctl_be_block_getattr_dev(struct ctl_be_block_lun *be_lun, const char *attrname) return (arg.value.off); } +static void +ctl_be_block_cw_dispatch_sync(struct ctl_be_block_lun *be_lun, + union ctl_io *io) +{ + struct ctl_be_block_io *beio; + struct ctl_lba_len_flags *lbalen; + + DPRINTF("entered\n"); + beio = (struct ctl_be_block_io *)PRIV(io)->ptr; + lbalen = (struct ctl_lba_len_flags *)&io->io_hdr.ctl_private[CTL_PRIV_LBA_LEN]; + + beio->io_len = lbalen->len * be_lun->blocksize; + beio->io_offset = lbalen->lba * be_lun->blocksize; + beio->io_arg = (lbalen->flags & SSC_IMMED) != 0; + beio->bio_cmd = BIO_FLUSH; + beio->ds_trans_type = DEVSTAT_NO_DATA; + DPRINTF("SYNC\n"); + be_lun->lun_flush(be_lun, beio); +} + static void ctl_be_block_cw_done_ws(struct ctl_be_block_io *beio) { @@ -1188,7 +1203,6 @@ ctl_be_block_cw_dispatch_ws(struct ctl_be_block_lun *be_lun, union ctl_io *io) { struct ctl_be_block_io *beio; - struct ctl_be_block_softc *softc; struct ctl_lba_len_flags *lbalen; uint64_t len_left, lba; uint32_t pb, pbo, adj; @@ -1198,7 +1212,6 @@ ctl_be_block_cw_dispatch_ws(struct ctl_be_block_lun *be_lun, DPRINTF("entered\n"); beio = (struct ctl_be_block_io *)PRIV(io)->ptr; - softc = be_lun->softc; lbalen = ARGS(beio->io); if (lbalen->flags & ~(SWS_LBDATA | SWS_UNMAP | SWS_ANCHOR | SWS_NDOB) || @@ -1214,21 +1227,6 @@ ctl_be_block_cw_dispatch_ws(struct ctl_be_block_lun *be_lun, return; } - switch (io->scsiio.tag_type) { - case CTL_TAG_ORDERED: - beio->ds_tag_type = DEVSTAT_TAG_ORDERED; - break; - case CTL_TAG_HEAD_OF_QUEUE: - beio->ds_tag_type = DEVSTAT_TAG_HEAD; - break; - case CTL_TAG_UNTAGGED: - case CTL_TAG_SIMPLE: - case CTL_TAG_ACA: - default: - beio->ds_tag_type = DEVSTAT_TAG_SIMPLE; - break; - } - if (lbalen->flags & (SWS_UNMAP | SWS_ANCHOR)) { beio->io_offset = lbalen->lba * be_lun->blocksize; beio->io_len = (uint64_t)lbalen->len * be_lun->blocksize; @@ -1303,13 +1301,11 @@ ctl_be_block_cw_dispatch_unmap(struct ctl_be_block_lun *be_lun, union ctl_io *io) { struct ctl_be_block_io *beio; - struct ctl_be_block_softc *softc; struct ctl_ptr_len_flags *ptrlen; DPRINTF("entered\n"); beio = (struct ctl_be_block_io *)PRIV(io)->ptr; - softc = be_lun->softc; ptrlen = (struct ctl_ptr_len_flags *)&io->io_hdr.ctl_private[CTL_PRIV_LBA_LEN]; if ((ptrlen->flags & ~SU_ANCHOR) != 0 || be_lun->unmap == NULL) { @@ -1324,29 +1320,11 @@ ctl_be_block_cw_dispatch_unmap(struct ctl_be_block_lun *be_lun, return; } - switch (io->scsiio.tag_type) { - case CTL_TAG_ORDERED: - beio->ds_tag_type = DEVSTAT_TAG_ORDERED; - break; - case CTL_TAG_HEAD_OF_QUEUE: - beio->ds_tag_type = DEVSTAT_TAG_HEAD; - break; - case CTL_TAG_UNTAGGED: - case CTL_TAG_SIMPLE: - case CTL_TAG_ACA: - default: - beio->ds_tag_type = DEVSTAT_TAG_SIMPLE; - break; - } - beio->io_len = 0; beio->io_offset = -1; - beio->bio_cmd = BIO_DELETE; beio->ds_trans_type = DEVSTAT_FREE; - DPRINTF("UNMAP\n"); - be_lun->unmap(be_lun, beio); } @@ -1417,16 +1395,26 @@ ctl_be_block_cw_dispatch(struct ctl_be_block_lun *be_lun, beio->io = io; beio->lun = be_lun; beio->beio_cont = ctl_be_block_cw_done; + switch (io->scsiio.tag_type) { + case CTL_TAG_ORDERED: + beio->ds_tag_type = DEVSTAT_TAG_ORDERED; + break; + case CTL_TAG_HEAD_OF_QUEUE: + beio->ds_tag_type = DEVSTAT_TAG_HEAD; + break; + case CTL_TAG_UNTAGGED: + case CTL_TAG_SIMPLE: + case CTL_TAG_ACA: + default: + beio->ds_tag_type = DEVSTAT_TAG_SIMPLE; + break; + } PRIV(io)->ptr = (void *)beio; switch (io->scsiio.cdb[0]) { case SYNCHRONIZE_CACHE: case SYNCHRONIZE_CACHE_16: - beio->bio_cmd = BIO_FLUSH; - beio->ds_trans_type = DEVSTAT_NO_DATA; - beio->ds_tag_type = DEVSTAT_TAG_ORDERED; - beio->io_len = 0; - be_lun->lun_flush(be_lun, beio); + ctl_be_block_cw_dispatch_sync(be_lun, io); break; case WRITE_SAME_10: case WRITE_SAME_16: diff --git a/sys/cam/ctl/ctl_cmd_table.c b/sys/cam/ctl/ctl_cmd_table.c index fbf0d126f8b9..08ff88adefa9 100644 --- a/sys/cam/ctl/ctl_cmd_table.c +++ b/sys/cam/ctl/ctl_cmd_table.c @@ -788,7 +788,7 @@ const struct ctl_cmd_entry ctl_cmd_table[256] = {ctl_sync_cache, CTL_SERIDX_SYNC, CTL_CMD_FLAG_OK_ON_SLUN | CTL_FLAG_DATA_NONE, CTL_LUN_PAT_NONE, - 10, {0, 0xff, 0xff, 0xff, 0xff, 0, 0xff, 0xff, 0x07}}, + 10, {0x02, 0xff, 0xff, 0xff, 0xff, 0, 0xff, 0xff, 0x07}}, /* 36 LOCK UNLOCK CACHE(10) */ {NULL, CTL_SERIDX_INVLD, CTL_CMD_FLAG_NONE, CTL_LUN_PAT_NONE}, @@ -1141,7 +1141,7 @@ const struct ctl_cmd_entry ctl_cmd_table[256] = {ctl_sync_cache, CTL_SERIDX_SYNC, CTL_CMD_FLAG_OK_ON_SLUN | CTL_FLAG_DATA_NONE, CTL_LUN_PAT_NONE, - 16, {0, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 16, {0x02, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0, 0x07}}, /* 92 LOCK UNLOCK CACHE(16) */ From 3d8d5f23ac1a25b53b18e6399dc1e5b0322dd1c4 Mon Sep 17 00:00:00 2001 From: cem Date: Wed, 5 Aug 2015 22:27:30 +0000 Subject: [PATCH 4/4] nfsclient: Protest loudly when GETATTR responses are invalid BROKEN NFS SERVER OR MIDDLEWARE: Certain WAN "accelerators" attempt to cache NFS GETATTR traffic, but actually corrupt it (e.g., responding to requests with attributes for totally different files). Warn very verbosely when this is detected. Linux' NFS client has a similar warning. Adds a sysctl/tunable (vfs.nfs.fileid_maxwarnings) to configure the quantity of warnings; default to 10. (Zero disables; -1 is unlimited.) Adds a failpoint to aid in validating the warning / behavior with a non-broken server. Use something like: sysctl 'debug.fail_point.nfscl_force_fileid_warning=10%return(1)' Reviewed by: rmacklem Approved by: markj (mentor) Sponsored by: EMC / Isilon Storage Division Differential Revision: https://reviews.freebsd.org/D3304 --- sys/fs/nfsclient/nfs_clport.c | 85 ++++++++++++++++++++++++++++++++--- 1 file changed, 80 insertions(+), 5 deletions(-) diff --git a/sys/fs/nfsclient/nfs_clport.c b/sys/fs/nfsclient/nfs_clport.c index d3bac30bdc74..a6a216727133 100644 --- a/sys/fs/nfsclient/nfs_clport.c +++ b/sys/fs/nfsclient/nfs_clport.c @@ -42,7 +42,9 @@ __FBSDID("$FreeBSD$"); * generally, I don't like #includes inside .h files, but it seems to * be the easiest way to handle the port. */ +#include #include +#include #include #include #include @@ -83,6 +85,16 @@ NFSDLOCKMUTEX; extern void (*ncl_call_invalcaches)(struct vnode *); +SYSCTL_DECL(_vfs_nfs); +static int ncl_fileid_maxwarnings = 10; +SYSCTL_INT(_vfs_nfs, OID_AUTO, fileid_maxwarnings, CTLFLAG_RWTUN, + &ncl_fileid_maxwarnings, 0, + "Limit fileid corruption warnings; 0 is off; -1 is unlimited"); +static volatile int ncl_fileid_nwarnings; + +static void nfscl_warn_fileid(struct nfsmount *, struct nfsvattr *, + struct nfsvattr *); + /* * Comparison function for vfs_hash functions. */ @@ -343,6 +355,37 @@ nfscl_ngetreopen(struct mount *mntp, u_int8_t *fhp, int fhsize, return (EINVAL); } +static void +nfscl_warn_fileid(struct nfsmount *nmp, struct nfsvattr *oldnap, + struct nfsvattr *newnap) +{ + int off; + + if (ncl_fileid_maxwarnings >= 0 && + ncl_fileid_nwarnings >= ncl_fileid_maxwarnings) + return; + off = 0; + if (ncl_fileid_maxwarnings >= 0) { + if (++ncl_fileid_nwarnings >= ncl_fileid_maxwarnings) + off = 1; + } + + printf("newnfs: server '%s' error: fileid changed. " + "fsid %jx:%jx: expected fileid %#jx, got %#jx. " + "(BROKEN NFS SERVER OR MIDDLEWARE)\n", + nmp->nm_com.nmcom_hostname, + (uintmax_t)nmp->nm_fsid[0], + (uintmax_t)nmp->nm_fsid[1], + (uintmax_t)oldnap->na_fileid, + (uintmax_t)newnap->na_fileid); + + if (off) + printf("newnfs: Logged %d times about fileid corruption; " + "going quiet to avoid spamming logs excessively. (Limit " + "is: %d).\n", ncl_fileid_nwarnings, + ncl_fileid_maxwarnings); +} + /* * Load the attribute cache (that lives in the nfsnode entry) with * the attributes of the second argument and @@ -361,7 +404,11 @@ nfscl_loadattrcache(struct vnode **vpp, struct nfsvattr *nap, void *nvaper, struct nfsmount *nmp; struct timespec mtime_save; u_quad_t nsize; - int setnsize; + int setnsize, error, force_fid_err; + + error = 0; + setnsize = 0; + nsize = 0; /* * If v_type == VNON it is a new node, so fill in the v_type, @@ -389,6 +436,34 @@ nfscl_loadattrcache(struct vnode **vpp, struct nfsvattr *nap, void *nvaper, np->n_vattr.na_fsid = nap->na_fsid; np->n_vattr.na_mode = nap->na_mode; } else { + force_fid_err = 0; + KFAIL_POINT_ERROR(DEBUG_FP, nfscl_force_fileid_warning, + force_fid_err); + /* + * BROKEN NFS SERVER OR MIDDLEWARE + * + * Certain NFS servers (certain old proprietary filers ca. + * 2006) or broken middleboxes (e.g. WAN accelerator products) + * will respond to GETATTR requests with results for a + * different fileid. + * + * The WAN accelerator we've observed not only serves stale + * cache results for a given file, it also occasionally serves + * results for wholly different files. This causes surprising + * problems; for example the cached size attribute of a file + * may truncate down and then back up, resulting in zero + * regions in file contents read by applications. We observed + * this reliably with Clang and .c files during parallel build. + * A pcap revealed packet fragmentation and GETATTR RPC + * responses with wholly wrong fileids. + */ + if ((np->n_vattr.na_fileid != 0 && + np->n_vattr.na_fileid != nap->na_fileid) || + force_fid_err) { + nfscl_warn_fileid(nmp, &np->n_vattr, nap); + error = EIDRM; + goto out; + } NFSBCOPY((caddr_t)nap, (caddr_t)&np->n_vattr, sizeof (struct nfsvattr)); } @@ -419,8 +494,6 @@ nfscl_loadattrcache(struct vnode **vpp, struct nfsvattr *nap, void *nvaper, } else vap->va_fsid = vp->v_mount->mnt_stat.f_fsid.val[0]; np->n_attrstamp = time_second; - setnsize = 0; - nsize = 0; if (vap->va_size != np->n_size) { if (vap->va_type == VREG) { if (dontshrink && vap->va_size < np->n_size) { @@ -490,14 +563,16 @@ nfscl_loadattrcache(struct vnode **vpp, struct nfsvattr *nap, void *nvaper, vaper->va_mtime = np->n_mtim; } } + +out: #ifdef KDTRACE_HOOKS if (np->n_attrstamp != 0) - KDTRACE_NFS_ATTRCACHE_LOAD_DONE(vp, vap, 0); + KDTRACE_NFS_ATTRCACHE_LOAD_DONE(vp, vap, error); #endif NFSUNLOCKNODE(np); if (setnsize) vnode_pager_setsize(vp, nsize); - return (0); + return (error); } /*