Unit tests never run the real write check in traversed_secure_open

[tanglearncode]

Hi,

I saw some cfg!(test) used in the code as a workaround for testability. I'm trying to see if you are interested my proposal to remove them and make tests easier. This approach can apply to any other places when needed, not only traversed_secure_open.

Disclosure: I maintain shimforge (MIT), the crate used below.

Current limitation

traversed_secure_open refuses a file if the user can write to any directory on the path. Under cargo test, that check is not the one that runs (src/system/audit.rs:416):

let user_has_write_perms = if cfg!(test) {
    // During testing we do a less comprehensive check as we don't have
    // permission to set the real user id to arbitrary users, but faccessat
    // looks at the real user id.
    perms & mode(Category::World, Op::Write) != 0
        || (perms & mode(Category::Group, Op::Write) != 0)
            && forbidden_user.in_group_by_gid(GroupId::new(meta.gid()))
        || (perms & mode(Category::Owner, Op::Write) != 0)
            && forbidden_user.uid.inner() == meta.uid()
} else {
    // Only works when forbidden_user is current user. ...
    faccess_at(file.as_fd(), c"", libc::W_OK, libc::AT_EMPTY_PATH).is_ok()
};

So test_traverse_secure_open_positive and test_traverse_secure_open_negative test a second copy of the rule, written in Rust, while the shipped code asks the kernel. Either side can be wrong without the other noticing. The compliance and e2e suites run the real code, but only as whole-binary scenarios in Docker.

The same reason put two more test-only pieces in this file: the parameter type differs under test (audit.rs:383) and sudo_call returns early under test (audit.rs:61).

Proposal

The test says what the kernel reports, by mocking libc::faccessat:

#[cfg(any(target_arch = "x86_64", target_arch = "aarch64"))]
#[test]
fn traverse_secure_open_refuses_a_path_the_user_can_write() {
    let user = CurrentUser::resolve().unwrap();
    let other_user = CurrentUser::fake(User { uid: UserId::new(1042), /* ... */ });

    // The kernel reports that the user may write to the first directory.
    let mut shim = Session::new();
    let faccessat = mock!(
        shim,
        libc::faccessat,
        unsafe extern "C" fn(
            libc::c_int,
            *const libc::c_char,
            libc::c_int,
            libc::c_int,
        ) -> libc::c_int
    );
    faccessat.expect().once().returns(0);

    let path = std::env::current_dir()
        .unwrap()
        .join("sudo-rs-test-file.txt");
    let error = traversed_secure_open(&path, &other_user, &user, &user.group()).unwrap_err();
    assert_eq!(error.kind(), ErrorKind::PermissionDenied);
}

Now in production code the if cfg!(test) block can be removed. Only else block is needed:

let user_has_write_perms =
    faccess_at(file.as_fd(), c"", libc::W_OK, libc::AT_EMPTY_PATH).is_ok();

See the proposed changes

Tried it

Green on a fork, on top of 4200478: run, cargo test --release --lib audit and again in debug. All five audit tests pass. Branch: test/shimforge-faccessat.

I'd like to discuss if this sounds interesting before opening a PR. Thanks!

[squell]

I think this by itself is an interesting approach, but right now the crates.io stats for shimforge show this is a very young crate, so for adoption in sudo-rs, I think I would like to see:

• More maturity / wider adoption (say, in a years time).
• Preferably two other spots where a cfg!(test) can be avoided (or brittle unit tests be made less brittle) by using this more implicit type of shimming so that it really pulls its weight.

(cf. criterium for adoption 5: "Dependencies must be kept to a minimum")