Created attachment 248798 [details] corrupt tar image that triggers a tarfs use-after-free I've attached a tar image which, if mounted with tarfs on an INVARIANTS kernel, fails this MPASS() in tarfs_lookup_path(): MPASS(name != NULL && namelen != 0); On a non-INVARIANTS kernel, this image causes tarfs to use memory after freeing it. tarfs_alloc_mount() encounters an error from tarfs_alloc_one(), and calls tarfs_free_mount(). The latter frees everything on tmp->allnodes. But if the root tarfs_node is early on the list, then tarfs_free_node()'s recursive call case VDIR: ...; if (tnp->parent != NULL && tnp->parent != tnp) tarfs_free_node(tnp->parent); can use a tnp->parent that is the now-freed root tarfs_node. Without INVARIANTS: # uname -a FreeBSD 15.0-CURRENT FreeBSD 15.0-CURRENT #221 main-n250978-0a0623e9c824-dirty: Tue Feb 27 09:42:26 EST 2024 rtm@zika:/usr/obj/usr/rtm/symbsd/src/riscv.riscv64/sys/RTM riscv # mount -t tarfs tarfs2a.tar /mnt panic: pmap_kextract: No l2 KDB: stack backtrace: db_trace_self() at db_trace_self db_trace_self_wrapper() at db_trace_self_wrapper+0x36 kdb_backtrace() at kdb_backtrace+0x2c vpanic() at vpanic+0x120 panic() at panic+0x26 pmap_kextract() at pmap_kextract+0xfa free() at free+0x4e tarfs_free_node() at tarfs_free_node+0x8e tarfs_free_node() at tarfs_free_node+0x7a tarfs_alloc_mount() at tarfs_alloc_mount+0x100a tarfs_mount() at tarfs_mount+0x344 vfs_domount_first() at vfs_domount_first+0x20e vfs_domount() at vfs_domount+0x278 vfs_donmount() at vfs_donmount+0x89e sys_nmount() at sys_nmount+0x5a do_trap_user() at do_trap_user+0x23a cpu_exception_handler_user() at cpu_exception_handler_user+0x72 --- syscall (378, FreeBSD ELF64, nmount) With INVARIANTS: panic: Assertion name != NULL && namelen != 0 failed at /usr/src/sys/fs/tarfs/tarfs_vfsops.c:299 KDB: stack backtrace: db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe01255e72c0 vpanic() at vpanic+0x135/frame 0xfffffe01255e73f0 panic() at panic+0x43/frame 0xfffffe01255e7450 tarfs_lookup_path() at tarfs_lookup_path+0x362/frame 0xfffffe01255e74f0 tarfs_alloc_mount() at tarfs_alloc_mount+0x19c3/frame 0xfffffe01255e78d0 tarfs_mount() at tarfs_mount+0x407/frame 0xfffffe01255e7ad0 vfs_domount_first() at vfs_domount_first+0x258/frame 0xfffffe01255e7c10 vfs_domount() at vfs_domount+0x318/frame 0xfffffe01255e7d30 vfs_donmount() at vfs_donmount+0x8dd/frame 0xfffffe01255e7dc0 sys_nmount() at sys_nmount+0x6c/frame 0xfffffe01255e7e00 amd64_syscall() at amd64_syscall+0x153/frame 0xfffffe01255e7f30 fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe01255e7f30 --- syscall (378, FreeBSD ELF64, nmount), rip = 0x25b598c3f23a, rsp = 0x25b59668f6e8, rbp = 0x25b59668fc50 ---
Over to maintainer.
This uncovers two separate issues: The first issue is that we don't immediately reject a link entry with an empty target path. With INVARIANTS enabled, this triggers an assertion when we try to look up the empty path; without it, it improperly results in a link to the root directory. This can be fixed by checking that the target path is not empty before trying to look it up. The second issue is that if we find the link target but deem it invalid, we error out at a point where we've already pointed tnp->other at the target, but haven't incremented the target's link count, so when we later free our nodes, we free the target while the dangling link still holds a reference to it, then later try to follow that reference. This can be fixed by storing the result of the target path lookup in a temporary variable instead of directly in tnp->other.
I've attached a new image that, after applying patch D44161, causes this call to tarfs_lookup_path() in tarfs_alloc_one() to return errno=0, but other=NULL, causing other->type to crash: case TAR_TYPE_HARDLINK: if (link == NULL) { link = hdrp->linkname; linklen = strnlen(link, sizeof(hdrp->linkname)); } if (linklen == 0) { TARFS_DPF(ALLOC, "%s: %.*s: link without target\n", __func__, (int)namelen, name); error = EINVAL; goto bad; } error = tarfs_lookup_path(tmp, link, linklen, NULL, NULL, NULL, &other, false); if (error != 0) { goto bad; } if (other->type != VREG || other->other != NULL) {
Created attachment 248836 [details] cause tarfs_lookup_path() to return errno=0 but other=NULL
The new tarball gives me a checksum failure before it gets anywhere near tarfs_lookup_path(). Oddly, the tarsum tool which I wrote for the regression tests disagrees, but both GNU and BSD tar reject the tarball. But you're right that tarfs_lookup_path() fails to set errno to ENOENT when it can't find the requested path. I'll update the review in a moment.
See also https://reviews.freebsd.org/D44166 which improves validation of numeric fields.
See https://reviews.freebsd.org/D44226 for the reason why the checksum failed.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=38b3683592d4c20a74f52a6e8e29368e6fa61858 commit 38b3683592d4c20a74f52a6e8e29368e6fa61858 Author: Dag-Erling Smørgrav <des@FreeBSD.org> AuthorDate: 2024-03-06 16:13:42 +0000 Commit: Dag-Erling Smørgrav <des@FreeBSD.org> CommitDate: 2024-03-06 16:13:42 +0000 tarfs: Fix two input validation issues. * Reject hard or soft links with an empty target path. Currently, a debugging kernel will hit an assertion in tarfs_lookup_path() while a non-debugging kernel will happily create a link to the mount root. * Use a temporary variable to store the result of the link target path, and copy it to tnp->other only once we have found it to be valid. Otherwise we error out after creating a reference to the target but before incrementing the target's reference count, which results in a use-after-free situation in the cleanup code. * Correctly return ENOENT from tarfs_lookup_path() if the requested path was not found and create_dirs is false. Luckily, existing callers did not rely solely on the return value. MFC after: 3 days PR: 277360 Sponsored by: Juniper Networks, Inc. Sponsored by: Klara, Inc. Reviewed by: sjg Differential Revision: https://reviews.freebsd.org/D44161 sys/fs/tarfs/tarfs_vfsops.c | 37 +++++++---- tests/sys/fs/tarfs/Makefile | 2 +- tests/sys/fs/tarfs/tarfs_test.sh | 68 +++++++++++++++++++- tests/sys/fs/tarfs/tarsum.c (new) | 128 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 221 insertions(+), 14 deletions(-)
A commit in branch stable/14 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=08e799c0cc235b8c5a98921653127a915a985220 commit 08e799c0cc235b8c5a98921653127a915a985220 Author: Dag-Erling Smørgrav <des@FreeBSD.org> AuthorDate: 2024-03-06 16:13:42 +0000 Commit: Dag-Erling Smørgrav <des@FreeBSD.org> CommitDate: 2024-03-11 12:19:06 +0000 tarfs: Fix two input validation issues. * Reject hard or soft links with an empty target path. Currently, a debugging kernel will hit an assertion in tarfs_lookup_path() while a non-debugging kernel will happily create a link to the mount root. * Use a temporary variable to store the result of the link target path, and copy it to tnp->other only once we have found it to be valid. Otherwise we error out after creating a reference to the target but before incrementing the target's reference count, which results in a use-after-free situation in the cleanup code. * Correctly return ENOENT from tarfs_lookup_path() if the requested path was not found and create_dirs is false. Luckily, existing callers did not rely solely on the return value. MFC after: 3 days PR: 277360 Sponsored by: Juniper Networks, Inc. Sponsored by: Klara, Inc. Reviewed by: sjg Differential Revision: https://reviews.freebsd.org/D44161 (cherry picked from commit 38b3683592d4c20a74f52a6e8e29368e6fa61858) tarfs: Improve validation of numeric fields. MFC after: 3 days Sponsored by: Juniper Networks, Inc. Sponsored by: Klara, Inc. Reviewed by: sjg, allanjude Differential Revision: https://reviews.freebsd.org/D44166 (cherry picked from commit 8427d94ce05682abb6c75e2a27c8c497962c0dc5) tarfs: Avoid overflow in exthdr calculation. MFC after: 3 days PR: 277420 Sponsored by: Juniper Networks, Inc. Sponsored by: Klara, Inc. Reviewed by: kib Differential Revision: https://reviews.freebsd.org/D44202 (cherry picked from commit c291b7914e1db9469cc820abcb1f5dde7a6f7f28) tarfs: Remove unnecessary hack and obsolete comment. MFC after: 3 days Sponsored by: Juniper Networks, Inc. Sponsored by: Klara, Inc. Reviewed by: allanjude Differential Revision: https://reviews.freebsd.org/D44203 (cherry picked from commit e212f0c0666e7d3a24dce03b8c88920d14b80e47) tarfs: Fix checksum calculation. The checksum code assumed that struct ustar_header filled an entire block and calculcated the checksum based on the size of the structure. The header is in fact only 500 bytes long while the checksum covers the entire block (“logical record” in POSIX terms). Add padding and an assertion, and clean up the checksum code. MFC after: 3 days Sponsored by: Juniper Networks, Inc. Sponsored by: Klara, Inc. Reviewed by: imp Differential Revision: https://reviews.freebsd.org/D44226 (cherry picked from commit 0118b0c8e58a438a931a5ce1bf8d7ae6208cc61b) tarfs: Factor out common test code. MFC after: 3 days Sponsored by: Juniper Networks, Inc. Sponsored by: Klara, Inc. Reviewed by: allanjude Differential Revision: https://reviews.freebsd.org/D44227 (cherry picked from commit 32b8aac6f9b77a1c4326083472d634e5de427547) tarfs: Fix checksum on 32-bit platforms. MFC after: 3 days Fixes: b56872332e47786afc09515a4daaf1388da4d73c Sponsored by: Juniper Networks, Inc. Sponsored by: Klara, Inc. Reviewed by: bapt Differential Revision: https://reviews.freebsd.org/D44261 (cherry picked from commit cbddb2f02c7687d1039abcffd931e94e481c11a5) sys/fs/tarfs/tarfs_vfsops.c | 222 +++++++++++++++++++++----------------- tests/sys/fs/tarfs/Makefile | 2 +- tests/sys/fs/tarfs/tarfs_test.sh | 120 ++++++++++++++++++--- tests/sys/fs/tarfs/tarsum.c (new) | 128 ++++++++++++++++++++++ 4 files changed, 357 insertions(+), 115 deletions(-)