Forked WithFiles.DisallowedFileSyscalls fails on FreeBSD 12 current

[svmhdvn]

The following assertion fails with EINVAL:

EXPECT_CAPMODE(mknod(TmpFile("capmode_mknod"), 0644, 0));

This fails with EINVAL because capability mode validation occurs after a few checks on mode and dev. It might not be a useful test on FreeBSD for this reason. Furthermore, if we changed the arguments to reflect the creation of an actual special file (e.g. a character device), we would require root access to run the test.

Relevant mknod functionality in FreeBSD 12 current from sys/kern/vfs_syscalls.c:

int
kern_mknodat(struct thread *td, int fd, char *path, enum uio_seg pathseg,
    int mode, dev_t dev)
{
    struct vnode *vp;
    struct mount *mp;
    struct vattr vattr;
    struct nameidata nd;
    cap_rights_t rights;
    int error, whiteout = 0;

    AUDIT_ARG_MODE(mode);
    AUDIT_ARG_DEV(dev);
    switch (mode & S_IFMT) {
    case S_IFCHR:
    case S_IFBLK:
        error = priv_check(td, PRIV_VFS_MKNOD_DEV);
        if (error == 0 && dev == VNOVAL)
            error = EINVAL;
        break;
    case S_IFMT:
        error = priv_check(td, PRIV_VFS_MKNOD_BAD);
        break;
    case S_IFWHT:
        error = priv_check(td, PRIV_VFS_MKNOD_WHT);
        break;
    case S_IFIFO:
        if (dev == 0)
            return (kern_mkfifoat(td, fd, path, pathseg, mode));
        /* FALLTHROUGH */
    default:
        error = EINVAL;
        break;
    }
    if (error != 0)
        return (error);
restart:
    bwillwrite();
    NDINIT_ATRIGHTS(&nd, CREATE, LOCKPARENT | SAVENAME | AUDITVNODE1 |
        NOCACHE, pathseg, path, fd, cap_rights_init(&rights, CAP_MKNODAT),
        td);
    if ((error = namei(&nd)) != 0)
        return (error);

[emaste]

As of https://github.com/freebsd/freebsd/commit/e75ba1d5c4c79376a78351c8544388491db49664 mknod(2) is actually a wrapper around mknodat(2):

int
mknod(const char *path, mode_t mode, dev_t dev)
{

    return (__sys_mknodat(AT_FDCWD, path, mode, dev));
}

so the syscall is permitted, and mode & privilege is checked first before the capability checks in namei can run.

We'd either need to force a call to __sys_mknod, test something that should succeed in non-capability mode (i.e., actually create a character special device as root, as @sivamahadevan mentioned above), or just #ifndef-away this test on FreeBSD, perhaps only if __FreeBSD_version >= 1200031

[daviddrysdale]

Could we shift the test to use EXPECT_CAPFAIL?

I guess not if the mknod -> mknodat(AT_FDCWD...) forwarding means it's now returning EINVAL, but I might have expected ENOTCAPABLE in that case.

[emaste]

I guess not if the mknod -> mknodat(AT_FDCWD...) forwarding means it's now returning EINVAL, but I might have expected ENOTCAPABLE in that case.

Exactly, it is now returning EINVAL. The problem is just that the uid and type checks are performed before checking for and disallowing AT_FDCWD. We could add an explicit check for AT_FDCWD prior to the uid/type checks, but I'm not convinced it's necessary; I'm not aware of any written guidance on errno precedence.

[emaste]

jilles@freebsd pointed out that "If more than one error occurs in processing a function call, any one of the possible errors may be returned, as the order of detection is undefined." http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_03.html

And a relevant link from an lwn discussion: https://old.lwn.net/Articles/726194/

The only things I've come across which actually cared [about the specific errno in the presence of multiple error conditions] have been LTP test cases which were trying to test for "correct error code for error condition A" but had bugs which meant they supplied arguments that also provoked error condition B, but happened to pass against a real kernel because the kernel checked A first. (The LTP developers accepted bugfix patches to the tests to make them not do this.)

I posted a link to this issue on cl-capsicum-discuss to allow others to comment but I am inclined to disable this test on FreeBSD, or make it require root and perform an otherwise-succeeding mknod.

[daviddrysdale]

Ah, I understand now -- I hadn't twigged about the EINVAL test ordering.

As @emaste pointed out on the Capsicum mailing list, if the EINVAL is purely triggered from the values of the arguments (i.e. it doesn't leak any information about what those arguments refer to), then it does seem that it shouldn't matter which of the possible errnos get returned.

So I think that means that the test is a little too artificial to be useful -- it should be an operation that would have worked if the process wasn't in capability mode. I guess that means it should be a REQUIRE_ROOT test?

[emaste]

I guess that means it should be a REQUIRE_ROOT test?

Yes, I think so, in combination with passing arguments to mknod that would otherwise succeed. So maybe EXPECT_CAPMODE(mknod(TmpFile("capmode_mknod"), S_IFWHT, 0));? (Does Linux have S_IFWHT?)

[daviddrysdale]

Not that I know of -- what does S_IFWHT do? The FreeBSD stat manpage doesn't help much, nor does a Google search ... (in fact the 4th link on the search comes back here!)

[emaste]

It's a "whiteout" - it forces a negative lookup result (e.g. when using union or other layered filesystems). Google turned up a patch posted to lkml but I don't know if it went in: http://lkml.iu.edu/hypermail/linux/kernel/0704.2/0502.html

I suggested it only because it has a known and unique value (I don't know what major/minor would be sensible to use if we wanted to actually create a character device).

[ngie-eign]

https://github.com/google/capsicum-test/pull/29 will fix this issue (it was falling through to the default case noted in the original bug report).