The branch main has been updated by des:
URL:
https://cgit.FreeBSD.org/src/commit/?id=ce6a0c776b702f063d4f200de34bfeaddcbb3cb7
commit ce6a0c776b702f063d4f200de34bfeaddcbb3cb7
Author: Dag-Erling Smørgrav <d...@freebsd.org>
AuthorDate: 2023-02-09 17:35:28 +0000
Commit: Dag-Erling Smørgrav <d...@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);
