Page MenuHomeFreeBSD

tarfs: Fix issues revealed by static analysis and testing.
ClosedPublic

Authored by des on Feb 9 2023, 3:27 PM.
Tags
None
Referenced Files
Unknown Object (File)
Jan 19 2024, 4:41 AM
Unknown Object (File)
Dec 26 2023, 6:31 AM
Unknown Object (File)
Dec 23 2023, 1:09 AM
Unknown Object (File)
Oct 15 2023, 9:06 AM
Unknown Object (File)
Oct 15 2023, 9:06 AM
Unknown Object (File)
Aug 15 2023, 6:59 PM
Unknown Object (File)
Jul 4 2023, 1:03 PM
Unknown Object (File)
Jul 3 2023, 10:24 PM
Subscribers

Details

Summary
  • 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.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

des requested review of this revision.Feb 9 2023, 3:27 PM

Fix size check, for real this time.

allanjude added inline comments.
sys/fs/tarfs/tarfs_vfsops.c
335

this could stuff be an else if to avoid the extra test if we already matched len == 1?

des marked an inline comment as done.Feb 9 2023, 3:44 PM
des added inline comments.
sys/fs/tarfs/tarfs_vfsops.c
335

That's how it used to be, but it wasn't very readable. I trust the compiler to figure it out.

Just had two questions... I see it picked up the new coverity things... I was going to forward them to you, but you already have fixes.
The two questions are why you know its safe to ignore errors where you do in two places.

sys/fs/tarfs/tarfs_io.c
633

how do we know it's safe to ignore the return value here?

sys/fs/tarfs/tarfs_vfsops.c
1129

and here especially.

BTW, are there malloc()s calls in the code where sizes are controlled by the archive content? If yes (and from my casual reading there are) it is a significant issue.

This revision is now accepted and ready to land.Feb 9 2023, 4:03 PM
des marked 3 inline comments as done.Feb 9 2023, 4:04 PM
des added inline comments.
sys/fs/tarfs/tarfs_io.c
633

The last time getnewvnode() returned an error was in 1997.

des marked an inline comment as done.Feb 9 2023, 4:11 PM
In D38463#875225, @kib wrote:

BTW, are there malloc()s calls in the code where sizes are controlled by the archive content? If yes (and from my casual reading there are) it is a significant issue.

The only one I can think of is the block map for sparse files, in tarfs_load_blockmap() in sys/fs/tarfs/tarfs_subr.c. It's not unbounded, but you could craft a tarball that would cause it to grow very large. I suppose I can put a (tunable) limit on the number of map entries.

Still, at some point, the user has to take responsibility for what they do to their system.

imp added inline comments.
sys/fs/tarfs/tarfs_io.c
633

that's a good reason... why is it still int?

des marked an inline comment as done.Feb 9 2023, 4:59 PM
des added a subscriber: mjg.
des added inline comments.
sys/fs/tarfs/tarfs_io.c
633

Inertia, I guess? I'll leave it up to @kib or @mjg.

This revision was automatically updated to reflect the committed changes.
des marked an inline comment as done.
sys/fs/tarfs/tarfs_io.c
633

I don't know what the original author was thinking. However, it should be clear that ultimately getnewvnode must be able to return an error and have it handled.

The fact that not returning an error was introduced so long ago poses a massive problem fixing it.

I think the pragmatic thing to do is to introduce a variant which *can* fail and incrementally patch filesystems to use it.

Maybe i'll post smething next week. New filesystems should definitely NOT be ignoring the error.