diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 71cbb9f77..d34ce3b00 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -42,7 +42,7 @@ jobs: - name: Run all E2E tests working-directory: test-framework - run: cargo test -p e2e-tests + run: cargo test -p e2e-tests --features apparmor - name: prevent the cache from growing too large run: | diff --git a/src/apparmor.rs b/src/apparmor.rs new file mode 100644 index 000000000..ed8525258 --- /dev/null +++ b/src/apparmor.rs @@ -0,0 +1,70 @@ +use std::ffi::{c_char, c_int, CStr, CString}; +use std::{fs, io, mem}; + +use crate::cutils::cerr; + +/// Set the profile for the next exec call if AppArmor is enabled +pub fn set_profile_for_next_exec(profile_name: &str) -> io::Result<()> { + if apparmor_is_enabled()? { + apparmor_prepare_exec(profile_name) + } else { + // if the sysadmin doesn't have apparmor enabled, fail softly + Ok(()) + } +} + +fn apparmor_is_enabled() -> io::Result { + match fs::read_to_string("/sys/module/apparmor/parameters/enabled") { + Ok(enabled) => Ok(enabled.starts_with("Y")), + Err(e) if e.kind() == io::ErrorKind::NotFound => Ok(false), + Err(e) => Err(e), + } +} + +/// Switch the apparmor profile to the given profile on the next exec call +fn apparmor_prepare_exec(new_profile: &str) -> io::Result<()> { + // SAFETY: Always safe to call + unsafe { libc::dlerror() }; // Clear any existing error + + // SAFETY: Loading a known safe dylib. LD_LIBRARY_PATH is ignored because we are setuid. + let handle = unsafe { libc::dlopen(c"libapparmor.so.1".as_ptr(), libc::RTLD_NOW) }; + if handle.is_null() { + // SAFETY: In case of an error, dlerror returns a valid C string. + return Err(io::Error::new(io::ErrorKind::NotFound, unsafe { + CStr::from_ptr(libc::dlerror()) + .to_string_lossy() + .into_owned() + })); + } + + // SAFETY: dlsym will either return a function pointer of the right signature or NULL. + let aa_change_onexec = unsafe { libc::dlsym(handle, cstr!("aa_change_onexec").as_ptr()) }; + + if aa_change_onexec.is_null() { + // SAFETY: Always safe to call + let err = unsafe { libc::dlerror() }; + return Err(if err.is_null() { + // There was no error in dlsym, but the symbol itself was defined as NULL pointer. + // This is still an error for us, but dlerror will not return any error. + io::Error::new( + io::ErrorKind::Other, + "aa_change_onexec symbol is a NULL pointer", + ) + } else { + // SAFETY: In case of an error, dlerror returns a valid C string. + io::Error::new(io::ErrorKind::NotFound, unsafe { + CStr::from_ptr(err).to_string_lossy().into_owned() + }) + }); + } + + //SAFETY: aa_change_onexec is non-NULL, so we can cast it into a function pointer + let aa_change_onexec: unsafe extern "C" fn(*const c_char) -> c_int = + unsafe { mem::transmute(aa_change_onexec) }; + + let new_profile_cstr = CString::new(new_profile)?; + // SAFETY: new_profile_cstr provided by CString ensures a valid ptr + cerr(unsafe { aa_change_onexec(new_profile_cstr.as_ptr()) })?; + + Ok(()) +} diff --git a/src/apparmor/mod.rs b/src/apparmor/mod.rs deleted file mode 100644 index f9a5f8272..000000000 --- a/src/apparmor/mod.rs +++ /dev/null @@ -1,32 +0,0 @@ -use std::{ffi::CString, io::ErrorKind}; - -use crate::cutils::cerr; - -mod sys; - -/// Set the profile for the next exec call if AppArmor is enabled -pub fn set_profile_for_next_exec(profile_name: &str) -> std::io::Result<()> { - if apparmor_is_enabled()? { - apparmor_prepare_exec(profile_name) - } else { - // if the sysadmin doesn't have apparmor enabled, fail softly - Ok(()) - } -} - -fn apparmor_is_enabled() -> std::io::Result { - match std::fs::read_to_string("/sys/module/apparmor/parameters/enabled") { - Ok(enabled) => Ok(enabled.starts_with("Y")), - Err(e) if e.kind() == ErrorKind::NotFound => Ok(false), - Err(e) => Err(e), - } -} - -/// Switch the apparmor profile to the given profile on the next exec call -fn apparmor_prepare_exec(new_profile: &str) -> std::io::Result<()> { - let new_profile_cstr = CString::new(new_profile)?; - // SAFETY: new_profile_cstr provided by CString ensures a valid ptr - cerr(unsafe { sys::aa_change_onexec(new_profile_cstr.as_ptr()) })?; - - Ok(()) -} diff --git a/src/apparmor/sys.rs b/src/apparmor/sys.rs deleted file mode 100644 index d225aa7aa..000000000 --- a/src/apparmor/sys.rs +++ /dev/null @@ -1,4 +0,0 @@ -#[link(name = "apparmor")] -extern "C" { - pub fn aa_change_onexec(profile: *const libc::c_char) -> libc::c_int; -} diff --git a/test-framework/e2e-tests/Cargo.toml b/test-framework/e2e-tests/Cargo.toml index 29b02c583..fa7555768 100644 --- a/test-framework/e2e-tests/Cargo.toml +++ b/test-framework/e2e-tests/Cargo.toml @@ -6,3 +6,6 @@ version = "0.0.0" [dependencies] sudo-test.path = "../sudo-test" + +[features] +apparmor = ["sudo-test/apparmor"] diff --git a/test-framework/e2e-tests/src/lib.rs b/test-framework/e2e-tests/src/lib.rs index 35b5cab89..d4eb98595 100644 --- a/test-framework/e2e-tests/src/lib.rs +++ b/test-framework/e2e-tests/src/lib.rs @@ -13,3 +13,48 @@ fn sanity_check() { "you must set `SUDO_UNDER_TEST=ours` when running this test suite" ); } + +#[test] +#[cfg(feature = "apparmor")] +fn dlopen_apparmor_ignores_ld_library_path() -> Result<(), Box> { + use sudo_test::{Command, Env}; + + let env = Env("foo ALL=(ALL:ALL) APPARMOR_PROFILE=docker-default NOPASSWD: ALL") + .file( + "/tmp/crash_me.c", + "#include + +void __attribute__((constructor)) do_not_load() { + abort(); +} +", + ) + .user("foo") + .apparmor("unconfined") + .build(); + + Command::new("gcc") + .args(["/tmp/crash_me.c", "-shared", "-o", "/tmp/libapparmor.so.1"]) + .output(&env) + .assert_success(); + + let output = Command::new("sh") + .args([ + "-c", + "LD_LIBRARY_PATH=/tmp sudo -s cat /proc/\\$\\$/attr/current", + ]) + .as_user("foo") + .output(&env); + + output.assert_success(); + assert_eq!(output.stdout(), "docker-default (enforce)"); + + let output = Command::new("sh") + .args(["-c", "LD_PRELOAD=/tmp/libapparmor.so.1 ls"]) + .output(&env); + + output.assert_exit_code(134); // SIGABRT + assert_eq!(output.stderr(), "Aborted (core dumped)"); + + Ok(()) +} diff --git a/test-framework/sudo-compliance-tests/src/sudo/apparmor.rs b/test-framework/sudo-compliance-tests/src/sudo/apparmor.rs index dfe66ad1a..6904050ac 100644 --- a/test-framework/sudo-compliance-tests/src/sudo/apparmor.rs +++ b/test-framework/sudo-compliance-tests/src/sudo/apparmor.rs @@ -11,7 +11,6 @@ fn can_switch_the_apparmor_profile() -> Result<()> { let output = Command::new("sudo") .args(["-s", "cat", "/proc/$$/attr/current"]) .output(&env); - dbg!(&output); output.assert_success(); assert_eq!(output.stdout(), "docker-default (enforce)"); @@ -25,8 +24,6 @@ fn cannot_switch_to_nonexisting_profile() -> Result<()> { let output = Command::new("sudo").arg("true").output(&env); - dbg!(&output); - output.assert_exit_code(1); assert_contains!(output.stderr(), "unable to change AppArmor profile"); @@ -44,7 +41,6 @@ Defaults apparmor_profile=docker-default let output = Command::new("sudo") .args(["-s", "cat", "/proc/$$/attr/current"]) .output(&env); - dbg!(&output); output.assert_success(); assert_eq!(output.stdout(), "docker-default (enforce)"); @@ -63,7 +59,6 @@ Defaults apparmor_profile=docker-default let output = Command::new("sudo") .args(["-s", "cat", "/proc/$$/attr/current"]) .output(&env); - dbg!(&output); output.assert_success(); assert_eq!(output.stdout(), "unconfined"); diff --git a/test-framework/sudo-test/src/ours.linux.Dockerfile b/test-framework/sudo-test/src/ours.linux.Dockerfile index 60f31eca9..37c069cb5 100644 --- a/test-framework/sudo-test/src/ours.linux.Dockerfile +++ b/test-framework/sudo-test/src/ours.linux.Dockerfile @@ -1,6 +1,6 @@ FROM rust:1-slim-bookworm RUN apt-get update && \ - apt-get install -y --no-install-recommends apparmor libpam0g-dev libapparmor-dev procps sshpass rsyslog ca-certificates tzdata + apt-get install -y --no-install-recommends apparmor libpam0g-dev libapparmor1 procps sshpass rsyslog ca-certificates tzdata # cache the crates.io index in the image for faster local testing RUN cargo search sudo WORKDIR /usr/src/sudo