Date: Thu, 9 Feb 2023 17:41:38 GMT From: =?utf-8?Q?Dag-Erling=20Sm=C3=B8rgrav?= <des@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: ce6a0c776b70 - main - tarfs: Fix issues revealed by static analysis and testing. Message-ID: <202302091741.319Hfcbd035309@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by des: URL: https://cgit.FreeBSD.org/src/commit/?id=ce6a0c776b702f063d4f200de34bfeaddcbb3cb7 commit ce6a0c776b702f063d4f200de34bfeaddcbb3cb7 Author: Dag-Erling Smørgrav <des@FreeBSD.org> AuthorDate: 2023-02-09 17:35:28 +0000 Commit: Dag-Erling Smørgrav <des@FreeBSD.org> CommitDate: 2023-02-09 17:35:47 +0000 tarfs: Fix issues revealed by static analysis and testing. * tarfs_alloc_mount(): Remove an unnecessary null check (CID 1504505) and an unused variable. * tarfs_alloc_one(): Verify that the file size is not negative (CID 1504506). While there, also validate the mode, owner and group. * tarfs_vget(), tarfs_zio_init(): Explicitly ignore return value from getnewvnode(), which cannot fail (CID 1504508) * tarfs_lookup_path(): Fix a case where a specially-crafted tarball could trigger a null pointer dereference by first descending into, and then backing out of, a previously unknown directory. (CID 1504515) * mktar: Construct a tarball that triggers the aforementioned null pointer dereference. Reported by: Coverity Sponsored by: Juniper Networks, Inc. Sponsored by: Klara, Inc. Reviewed by: imp, kib Differential Revision: https://reviews.freebsd.org/D38463 --- sys/fs/tarfs/tarfs_io.c | 2 +- sys/fs/tarfs/tarfs_vfsops.c | 88 +++++++++++++++++++++++++++------------------ sys/fs/tarfs/tarfs_vnops.c | 2 +- tests/sys/fs/tarfs/mktar.c | 43 ++++++++++++++++++++-- 4 files changed, 96 insertions(+), 39 deletions(-) diff --git a/sys/fs/tarfs/tarfs_io.c b/sys/fs/tarfs/tarfs_io.c index c185de8beef1..8837681ac5f0 100644 --- a/sys/fs/tarfs/tarfs_io.c +++ b/sys/fs/tarfs/tarfs_io.c @@ -630,7 +630,7 @@ tarfs_zio_init(struct tarfs_mount *tmp, off_t i, off_t o) zio->idx[zio->curidx].o = zio->opos = o; tmp->zio = zio; TARFS_DPF(ALLOC, "%s: allocated zio index\n", __func__); - getnewvnode("tarfsz", tmp->vfs, &tarfs_znodeops, &zvp); + (void)getnewvnode("tarfsz", tmp->vfs, &tarfs_znodeops, &zvp); zvp->v_data = zio; zvp->v_type = VREG; zvp->v_mount = tmp->vfs; diff --git a/sys/fs/tarfs/tarfs_vfsops.c b/sys/fs/tarfs/tarfs_vfsops.c index a45f005a2bd1..138a57c22e7f 100644 --- a/sys/fs/tarfs/tarfs_vfsops.c +++ b/sys/fs/tarfs/tarfs_vfsops.c @@ -289,7 +289,7 @@ tarfs_lookup_path(struct tarfs_mount *tmp, char *name, size_t namelen, char **endp, char **sepp, struct tarfs_node **retparent, struct tarfs_node **retnode, boolean_t create_dirs) { - struct componentname cn; + struct componentname cn = { }; struct tarfs_node *parent, *tnp; char *sep; size_t len; @@ -304,8 +304,6 @@ tarfs_lookup_path(struct tarfs_mount *tmp, char *name, size_t namelen, if (tnp == NULL) panic("%s: root node not yet created", __func__); - bzero(&cn, sizeof(cn)); - TARFS_DPF(LOOKUP, "%s: Full path: %.*s\n", __func__, (int)namelen, name); @@ -329,24 +327,21 @@ tarfs_lookup_path(struct tarfs_mount *tmp, char *name, size_t namelen, /* nothing */ ; /* check for . and .. */ - if (name[0] == '.' && len <= 2) { - if (len == 1) { - /* . */ - name += len; - namelen -= len; - continue; - } else if (name[1] == '.') { - /* .. */ - if (tnp == tmp->root) { - error = EINVAL; - break; - } - tnp = tnp->parent; - parent = tnp->parent; - name += len; - namelen -= len; - continue; + if (name[0] == '.' && len == 1) { + name += len; + namelen -= len; + continue; + } + if (name[0] == '.' && name[1] == '.' && len == 2) { + if (tnp == tmp->root) { + error = EINVAL; + break; } + tnp = parent; + parent = tnp->parent; + name += len; + namelen -= len; + continue; } /* create parent if necessary */ @@ -441,6 +436,7 @@ tarfs_alloc_one(struct tarfs_mount *tmp, off_t *blknump) struct sbuf *namebuf = NULL; char *exthdr = NULL, *name = NULL, *link = NULL; off_t blknum = *blknump; + int64_t num; int endmarker = 0; char *namep, *sep; struct tarfs_node *parent, *tnp; @@ -511,10 +507,41 @@ again: } /* get standard attributes */ - mode = tarfs_str2int64(hdrp->mode, sizeof(hdrp->mode)); - uid = tarfs_str2int64(hdrp->uid, sizeof(hdrp->uid)); - gid = tarfs_str2int64(hdrp->gid, sizeof(hdrp->gid)); - sz = tarfs_str2int64(hdrp->size, sizeof(hdrp->size)); + num = tarfs_str2int64(hdrp->mode, sizeof(hdrp->mode)); + if (num < 0 || num > ALLPERMS) { + TARFS_DPF(ALLOC, "%s: invalid file mode at %zu\n", + __func__, TARFS_BLOCKSIZE * (blknum - 1)); + mode = S_IRUSR; + } else { + mode = num; + } + num = tarfs_str2int64(hdrp->uid, sizeof(hdrp->uid)); + if (num < 0 || num > UID_MAX) { + TARFS_DPF(ALLOC, "%s: UID out of range at %zu\n", + __func__, TARFS_BLOCKSIZE * (blknum - 1)); + uid = tmp->root->uid; + mode &= ~S_ISUID; + } else { + uid = num; + } + num = tarfs_str2int64(hdrp->gid, sizeof(hdrp->gid)); + if (num < 0 || num > GID_MAX) { + TARFS_DPF(ALLOC, "%s: GID out of range at %zu\n", + __func__, TARFS_BLOCKSIZE * (blknum - 1)); + gid = tmp->root->gid; + mode &= ~S_ISGID; + } else { + gid = num; + } + num = tarfs_str2int64(hdrp->size, sizeof(hdrp->size)); + if (num < 0) { + TARFS_DPF(ALLOC, "%s: negative size at %zu\n", + __func__, TARFS_BLOCKSIZE * (blknum - 1)); + error = EINVAL; + goto bad; + } else { + sz = num; + } mtime = tarfs_str2int64(hdrp->mtime, sizeof(hdrp->mtime)); rdev = NODEV; TARFS_DPF(ALLOC, "%s: [%c] %zu @%jd %o %d:%d\n", __func__, @@ -777,7 +804,6 @@ tarfs_alloc_mount(struct mount *mp, struct vnode *vp, { struct vattr va; struct thread *td = curthread; - char *fullpath; struct tarfs_mount *tmp; struct tarfs_node *root; off_t blknum; @@ -788,7 +814,6 @@ tarfs_alloc_mount(struct mount *mp, struct vnode *vp, ASSERT_VOP_LOCKED(vp, __func__); tmp = NULL; - fullpath = NULL; TARFS_DPF(ALLOC, "%s: Allocating tarfs mount structure for vp %p\n", __func__, vp); @@ -802,8 +827,7 @@ tarfs_alloc_mount(struct mount *mp, struct vnode *vp, mtime = va.va_mtime.tv_sec; /* Allocate and initialize tarfs mount structure */ - tmp = (struct tarfs_mount *)malloc(sizeof(struct tarfs_mount), - M_TARFSMNT, M_WAITOK | M_ZERO); + tmp = malloc(sizeof(*tmp), M_TARFSMNT, M_WAITOK | M_ZERO); TARFS_DPF(ALLOC, "%s: Allocated mount structure\n", __func__); mp->mnt_data = tmp; @@ -848,9 +872,7 @@ tarfs_alloc_mount(struct mount *mp, struct vnode *vp, return (0); bad: - if (tmp != NULL) - tarfs_free_mount(tmp); - free(fullpath, M_TEMP); + tarfs_free_mount(tmp); return (error); } @@ -1104,9 +1126,7 @@ tarfs_vget(struct mount *mp, ino_t ino, int lkflags, struct vnode **vpp) if (tnp == NULL) return (ENOENT); - error = getnewvnode("tarfs", mp, &tarfs_vnodeops, &vp); - if (error != 0) - goto bad; + (void)getnewvnode("tarfs", mp, &tarfs_vnodeops, &vp); TARFS_DPF(FS, "%s: allocated vnode\n", __func__); vp->v_data = tnp; vp->v_type = tnp->type; diff --git a/sys/fs/tarfs/tarfs_vnops.c b/sys/fs/tarfs/tarfs_vnops.c index 266002bca7b2..99ff39d41271 100644 --- a/sys/fs/tarfs/tarfs_vnops.c +++ b/sys/fs/tarfs/tarfs_vnops.c @@ -250,7 +250,7 @@ tarfs_lookup(struct vop_cachedlookup_args *ap) static int tarfs_readdir(struct vop_readdir_args *ap) { - struct dirent cde; + struct dirent cde = { }; struct tarfs_node *current, *tnp; struct vnode *vp; struct uio *uio; diff --git a/tests/sys/fs/tarfs/mktar.c b/tests/sys/fs/tarfs/mktar.c index e1b1183af114..9b3d7910a12c 100644 --- a/tests/sys/fs/tarfs/mktar.c +++ b/tests/sys/fs/tarfs/mktar.c @@ -41,6 +41,8 @@ #define PROGNAME "mktar" #define SUBDIRNAME "directory" +#define EMPTYDIRNAME "empty" +#define NORMALFILENAME "file" #define SPARSEFILENAME "sparse_file" #define HARDLINKNAME "hard_link" #define SHORTLINKNAME "short_link" @@ -61,6 +63,25 @@ static void verbose(const char *fmt, ...) fprintf(stderr, "\n"); } +static void +mknormalfile(const char *filename, mode_t mode) +{ + char buf[512]; + ssize_t res; + int fd; + + if ((fd = open(filename, O_RDWR|O_CREAT|O_EXCL, mode)) < 0) + err(1, "%s", filename); + for (unsigned int i = 0; i < sizeof(buf); i++) + buf[i] = 32 + i % 64; + res = write(fd, buf, sizeof(buf)); + if (res < 0) + err(1, "%s", filename); + if (res != sizeof(buf)) + errx(1, "%s: short write", filename); + close(fd); +} + static void mksparsefile(const char *filename, mode_t mode) { @@ -68,7 +89,7 @@ mksparsefile(const char *filename, mode_t mode) ssize_t res; int fd; - if ((fd = open(filename, O_RDWR|O_CREAT|O_TRUNC, mode)) < 0) + if ((fd = open(filename, O_RDWR|O_CREAT|O_EXCL, mode)) < 0) err(1, "%s", filename); for (unsigned int i = 33; i <= 126; i++) { memset(buf, i, sizeof(buf)); @@ -106,6 +127,15 @@ mktar(void) if (mkdir(SUBDIRNAME, 0755) != 0) err(1, "%s", SUBDIRNAME); + /* create a second subdirectory which will remain empty */ + verbose("mkdir %s", EMPTYDIRNAME); + if (mkdir(EMPTYDIRNAME, 0755) != 0) + err(1, "%s", EMPTYDIRNAME); + + /* create a normal file */ + verbose("creating %s", NORMALFILENAME); + mknormalfile(NORMALFILENAME, 0644); + /* create a sparse file */ verbose("creating %s", SPARSEFILENAME); mksparsefile(SPARSEFILENAME, 0644); @@ -198,7 +228,12 @@ main(int argc, char *argv[]) #if 0 "--options", "zstd:frame-per-file", #endif - ".", + "./" EMPTYDIRNAME "/../" NORMALFILENAME, + "./" SPARSEFILENAME, + "./" HARDLINKNAME, + "./" SHORTLINKNAME, + "./" SUBDIRNAME, + "./" LONGLINKNAME, NULL); err(1, "execlp()"); } @@ -222,7 +257,9 @@ main(int argc, char *argv[]) (void)unlink(HARDLINKNAME); verbose("rm %s", SPARSEFILENAME); (void)unlink(SPARSEFILENAME); - verbose("rm %s", SUBDIRNAME); + verbose("rmdir %s", EMPTYDIRNAME); + (void)rmdir(EMPTYDIRNAME); + verbose("rmdir %s", SUBDIRNAME); (void)rmdir(SUBDIRNAME); verbose("cd -"); exit(0);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202302091741.319Hfcbd035309>