tarfs: Fix issues revealed by static analysis and testing.

[des]

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.

[des]

Fix size check, for real this time.

[allanjude]

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]

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.

[imp]

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.

[kib]

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.

[des]

sys/fs/tarfs/tarfs_io.c

633

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

[des]

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]

sys/fs/tarfs/tarfs_io.c

633

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

[des]

sys/fs/tarfs/tarfs_io.c

633

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

[mjg]

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.