From 407409f83e9676d73dc23f4aeaa48593d9ed73cf Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Fri, 23 May 2025 14:17:23 +0200 Subject: [PATCH 1/6] Move apparmor::sys into the main apparmor module --- src/{apparmor/mod.rs => apparmor.rs} | 9 ++++++--- src/apparmor/sys.rs | 4 ---- 2 files changed, 6 insertions(+), 7 deletions(-) rename src/{apparmor/mod.rs => apparmor.rs} (84%) delete mode 100644 src/apparmor/sys.rs diff --git a/src/apparmor/mod.rs b/src/apparmor.rs similarity index 84% rename from src/apparmor/mod.rs rename to src/apparmor.rs index f9a5f8272..237872393 100644 --- a/src/apparmor/mod.rs +++ b/src/apparmor.rs @@ -2,8 +2,6 @@ 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()? { @@ -22,11 +20,16 @@ fn apparmor_is_enabled() -> std::io::Result { } } +#[link(name = "apparmor")] +extern "C" { + pub fn aa_change_onexec(profile: *const libc::c_char) -> libc::c_int; +} + /// 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()) })?; + cerr(unsafe { 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; -} From 629397330898fc6cb570f7d48df798b07b2bedb9 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Fri, 23 May 2025 14:19:02 +0200 Subject: [PATCH 2/6] Improve import organization in apparmor.rs --- src/apparmor.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/apparmor.rs b/src/apparmor.rs index 237872393..68931300c 100644 --- a/src/apparmor.rs +++ b/src/apparmor.rs @@ -1,9 +1,10 @@ -use std::{ffi::CString, io::ErrorKind}; +use std::ffi::CString; +use std::{fs, io}; 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) -> std::io::Result<()> { +pub fn set_profile_for_next_exec(profile_name: &str) -> io::Result<()> { if apparmor_is_enabled()? { apparmor_prepare_exec(profile_name) } else { @@ -12,10 +13,10 @@ pub fn set_profile_for_next_exec(profile_name: &str) -> std::io::Result<()> { } } -fn apparmor_is_enabled() -> std::io::Result { - match std::fs::read_to_string("/sys/module/apparmor/parameters/enabled") { +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() == ErrorKind::NotFound => Ok(false), + Err(e) if e.kind() == io::ErrorKind::NotFound => Ok(false), Err(e) => Err(e), } } @@ -26,7 +27,7 @@ extern "C" { } /// Switch the apparmor profile to the given profile on the next exec call -fn apparmor_prepare_exec(new_profile: &str) -> std::io::Result<()> { +fn apparmor_prepare_exec(new_profile: &str) -> io::Result<()> { 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()) })?; From b2131ff55066b96944f979a04aef79bb4fc7fab1 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Fri, 23 May 2025 14:05:34 +0200 Subject: [PATCH 3/6] Dlopen apparmor --- src/apparmor.rs | 44 ++++++++++++++++--- .../sudo-test/src/ours.linux.Dockerfile | 2 +- 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/src/apparmor.rs b/src/apparmor.rs index 68931300c..c020d6bff 100644 --- a/src/apparmor.rs +++ b/src/apparmor.rs @@ -1,5 +1,5 @@ -use std::ffi::CString; -use std::{fs, io}; +use std::ffi::{c_char, c_int, CStr, CString}; +use std::{fs, io, mem}; use crate::cutils::cerr; @@ -21,13 +21,43 @@ fn apparmor_is_enabled() -> io::Result { } } -#[link(name = "apparmor")] -extern "C" { - pub fn aa_change_onexec(profile: *const libc::c_char) -> libc::c_int; -} - /// 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. + // Option is guaranteed to represent a NULL value as None and any other value as Some. + let aa_change_onexec: Option c_int> = + unsafe { mem::transmute(libc::dlsym(handle, c"aa_change_onexec".as_ptr())) }; + let Some(aa_change_onexec) = aa_change_onexec else { + // SAFETY: Always safe to call + let err = unsafe { libc::dlerror() }; + 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. + return Err(io::Error::new( + io::ErrorKind::Other, + "aa_change_onexec symbol is a NULL pointer", + )); + } + // SAFETY: In case of an error, dlerror returns a valid C string. + return Err(io::Error::new(io::ErrorKind::NotFound, unsafe { + CStr::from_ptr(err).to_string_lossy().into_owned() + })); + }; + 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()) })?; 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 From e47d13fe34a7757c4a2de3bf86e42448e2fe155a Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Wed, 4 Jun 2025 15:49:24 +0200 Subject: [PATCH 4/6] Add test that LD_LIBRARY_PATH doesn't have any effect --- .github/workflows/ci.yaml | 2 +- test-framework/e2e-tests/Cargo.toml | 3 ++ test-framework/e2e-tests/src/lib.rs | 45 +++++++++++++++++++ .../src/sudo/apparmor.rs | 5 --- 4 files changed, 49 insertions(+), 6 deletions(-) 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/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"); From dc46a246665af1a83dcc0cb08fb516f192559607 Mon Sep 17 00:00:00 2001 From: Marc Schoolderman Date: Wed, 11 Jun 2025 14:16:15 +0200 Subject: [PATCH 5/6] address academic concerns with transmuting a void ptr to Option --- src/apparmor.rs | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/src/apparmor.rs b/src/apparmor.rs index c020d6bff..ff08d87f5 100644 --- a/src/apparmor.rs +++ b/src/apparmor.rs @@ -38,25 +38,29 @@ fn apparmor_prepare_exec(new_profile: &str) -> io::Result<()> { } // SAFETY: dlsym will either return a function pointer of the right signature or NULL. - // Option is guaranteed to represent a NULL value as None and any other value as Some. - let aa_change_onexec: Option c_int> = - unsafe { mem::transmute(libc::dlsym(handle, c"aa_change_onexec".as_ptr())) }; - let Some(aa_change_onexec) = aa_change_onexec else { + let aa_change_onexec = unsafe { libc::dlsym(handle, c"aa_change_onexec".as_ptr()) }; + + if aa_change_onexec.is_null() { // SAFETY: Always safe to call let err = unsafe { libc::dlerror() }; - if err.is_null() { + 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. - return Err(io::Error::new( + io::Error::new( io::ErrorKind::Other, "aa_change_onexec symbol is a NULL pointer", - )); - } - // SAFETY: In case of an error, dlerror returns a valid C string. - return Err(io::Error::new(io::ErrorKind::NotFound, unsafe { - CStr::from_ptr(err).to_string_lossy().into_owned() - })); - }; + ) + } 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 From e2d0395f45913259e0b358bcac1eb4cf3f7b8375 Mon Sep 17 00:00:00 2001 From: Marc Schoolderman Date: Wed, 11 Jun 2025 14:22:04 +0200 Subject: [PATCH 6/6] make code adhere to MSRV --- src/apparmor.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/apparmor.rs b/src/apparmor.rs index ff08d87f5..ed8525258 100644 --- a/src/apparmor.rs +++ b/src/apparmor.rs @@ -38,7 +38,7 @@ fn apparmor_prepare_exec(new_profile: &str) -> io::Result<()> { } // SAFETY: dlsym will either return a function pointer of the right signature or NULL. - let aa_change_onexec = unsafe { libc::dlsym(handle, c"aa_change_onexec".as_ptr()) }; + 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