Safety comments: PR1 (#860)

Cargo.toml

@@ -29,7 +29,7 @@ name = "visudo"
 path = "bin/visudo.rs"
 
 [dependencies]
-libc = "0.2.127"
+libc = "0.2.149"
 glob = "0.3.0"
 log = { version = "0.4.11", features = ["std"] }
 

src/cutils/mod.rs

@@ -24,11 +24,13 @@ extern "C" {
 }
 
 pub fn set_errno(no: libc::c_int) {
+    // SAFETY: errno_location is a thread-local pointer to an integer, so we are the only writers
     unsafe { *errno_location() = no };
 }
 
 pub fn sysconf(name: libc::c_int) -> Option<libc::c_long> {
     set_errno(0);
+    // SAFETY: sysconf will always respond with 0 or -1 for every input
     cerr(unsafe { libc::sysconf(name) }).ok()
 }
 
@@ -43,6 +45,7 @@ pub unsafe fn string_from_ptr(ptr: *const libc::c_char) -> String {
     if ptr.is_null() {
         String::new()
     } else {
+        // SAFETY: the function contract says that CStr::from_ptr is safe
         let cstr = unsafe { CStr::from_ptr(ptr) };
         cstr.to_string_lossy().to_string()
     }
@@ -57,6 +60,7 @@ pub unsafe fn os_string_from_ptr(ptr: *const libc::c_char) -> OsString {
     if ptr.is_null() {
         OsString::new()
     } else {
+        // SAFETY: the function contract says that CStr::from_ptr is safe
         let cstr = unsafe { CStr::from_ptr(ptr) };
         OsStr::from_bytes(cstr.to_bytes()).to_owned()
     }
@@ -69,13 +73,17 @@ pub fn safe_isatty(fildes: libc::c_int) -> bool {
     // The Rust standard library doesn't have FileTypeExt on Std{in,out,err}, so we
     // can't just use FileTypeExt::is_char_device and have to resort to libc::fstat.
     let mut maybe_stat = std::mem::MaybeUninit::<libc::stat>::uninit();
+
+    // SAFETY: we are passing fstat a pointer to valid memory
     if unsafe { libc::fstat(fildes, maybe_stat.as_mut_ptr()) } == 0 {
+        // SAFETY: if `fstat` returned 0, maybe_stat will be initialized
         let mode = unsafe { maybe_stat.assume_init() }.st_mode;
 
         // To complicate matters further, the S_ISCHR macro isn't in libc as well.
         let is_char_device = (mode & libc::S_IFMT) == libc::S_IFCHR;
 
         if is_char_device {
+            // SAFETY: isatty will return 0 or 1
             unsafe { libc::isatty(fildes) != 0 }
         } else {
             false

src/exec/event.rs

@@ -178,6 +178,9 @@ impl<T: Process> EventRegistry<T> {
             return Ok(ids);
         }
 
+        // SAFETY: `poll` expects a pointer to an array of file descriptors (first argument),
+        // the length of which is indicated by the second argument; the third argument being -1
+        // denotes an infinite timeout.
         // FIXME: we should set either a timeout or use ppoll when available.
         cerr(unsafe { libc::poll(fds.as_mut_ptr(), fds.len() as _, -1) })?;
 

src/pam/converse.rs

@@ -203,8 +203,13 @@ pub(super) unsafe extern "C" fn converse<C: Converser>(
             messages: Vec::with_capacity(num_msg as usize),
         };
         for i in 0..num_msg as isize {
+            // SAFETY: the PAM contract ensures that `num_msg` does not exceed the amount
+            // of messages presented to this function in `msg`, and that it is not being
+            // written to at the same time as we are reading it. Note that the reference
+            // we create does not escape this loopy body.
             let message: &pam_message = unsafe { &**msg.offset(i) };
 
+            // SAFETY: PAM ensures that the messages passed are properly null-terminated
             let msg = unsafe { string_from_ptr(message.msg) };
             let style = if let Some(style) = PamMessageStyle::from_int(message.msg_style) {
                 style
@@ -221,6 +226,7 @@ pub(super) unsafe extern "C" fn converse<C: Converser>(
         }
 
         // send the conversation of to the Rust part
+        // SAFETY: appdata_ptr contains the `*mut ConverserData` that is untouched by PAM
         let app_data = unsafe { &mut *(appdata_ptr as *mut ConverserData<C>) };
         if app_data
             .converser
@@ -232,6 +238,8 @@ pub(super) unsafe extern "C" fn converse<C: Converser>(
 
         // Conversation should now contain response messages
         // allocate enough memory for the responses, set it to zero
+        // SAFETY: this will either allocate the required amount of (initialized) bytes,
+        // or return a null pointer.
         let temp_resp = unsafe {
             libc::calloc(
                 num_msg as libc::size_t,
@@ -244,6 +252,9 @@ pub(super) unsafe extern "C" fn converse<C: Converser>(
 
         // Store the responses
         for (i, msg) in conversation.messages.into_iter().enumerate() {
+            // SAFETY: `i` will not exceed `num_msg` by the way `conversation_messages`
+            // is constructed, so `temp_resp` will have allocated-and-initialized data at
+            // the required offset that only we have a writable pointer to.
             let response: &mut pam_response = unsafe { &mut *(temp_resp.add(i)) };
 
             if let Some(secbuf) = msg.response {
@@ -252,6 +263,7 @@ pub(super) unsafe extern "C" fn converse<C: Converser>(
         }
 
         // Set the responses
+        // SAFETY: PAM contract says that we are passed a valid, non-null, writeable pointer here.
         unsafe { *response = temp_resp };
 
         PamErrorType::Success
@@ -262,6 +274,7 @@ pub(super) unsafe extern "C" fn converse<C: Converser>(
         Ok(r) => r,
         Err(_) => {
             // notify caller that a panic has occured
+            // SAFETY: appdata_ptr contains the `*mut ConverserData` that is untouched by PAM
             let app_data = unsafe { &mut *(appdata_ptr as *mut ConverserData<C>) };
             app_data.panicked = true;
 

src/pam/error.rs

@@ -154,13 +154,14 @@ impl PamErrorType {
     }
 
     fn get_err_msg(&self) -> String {
-        // pam_strerror technically takes a pam handle as the first argument,
+        // SAFETY: pam_strerror technically takes a pam handle as the first argument,
         // but we do not know of any implementation that actually uses the pamh
         // argument. See also the netbsd man page for `pam_strerror`.
         let data = unsafe { pam_strerror(std::ptr::null_mut(), self.as_int()) };
         if data.is_null() {
             String::from("Error unresolved by PAM")
         } else {
+            // SAFETY: pam_strerror returns a pointer to a null-terminated string
             unsafe { string_from_ptr(data) }
         }
     }

src/pam/mod.rs

@@ -57,6 +57,8 @@ impl<C: Converser> PamContextBuilder<C> {
             }));
 
             let mut pamh = std::ptr::null_mut();
+            // SAFETY: we are passing the required fields to `pam_start`; in particular, the value
+            // of `pamh` set above is not used, but will be overwritten by `pam_start`.
             let res = unsafe {
                 pam_start(
                     c_service_name.as_ptr(),
@@ -161,6 +163,7 @@ impl<C: Converser> PamContext<C> {
         flags |= self.silent_flag();
         flags |= self.disallow_null_auth_token_flag();
 
+        // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`)
         pam_err(unsafe { pam_authenticate(self.pamh, flags) })?;
 
         if self.has_panicked() {
@@ -175,6 +178,7 @@ impl<C: Converser> PamContext<C> {
         flags |= self.silent_flag();
         flags |= self.disallow_null_auth_token_flag();
 
+        // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`)
         pam_err(unsafe { pam_acct_mgmt(self.pamh, flags) })
     }
 
@@ -195,6 +199,8 @@ impl<C: Converser> PamContext<C> {
     /// Set the user that will be authenticated.
     pub fn set_user(&mut self, user: &str) -> PamResult<()> {
         let c_user = CString::new(user)?;
+        // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`); furthermore,
+        // `c_user.as_ptr()` will point to a correct null-terminated string.
         pam_err(unsafe {
             pam_set_item(self.pamh, PAM_USER, c_user.as_ptr() as *const libc::c_void)
         })
@@ -203,14 +209,17 @@ impl<C: Converser> PamContext<C> {
     /// Get the user that is currently active in the PAM handle
     pub fn get_user(&mut self) -> PamResult<String> {
         let mut data = std::ptr::null();
+        // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`)
         pam_err(unsafe { pam_get_item(self.pamh, PAM_USER, &mut data) })?;
 
-        // safety check to make sure that we do not ready a null ptr into a cstr
+        // safety check to make sure that we were not passed a null pointer by PAM,
+        // or that in fact PAM did not write to `data` at all.
         if data.is_null() {
             return Err(PamError::InvalidState);
         }
 
-        // unsafe conversion to cstr
+        // SAFETY: the contract for `pam_get_item` ensures that if `data` was touched by
+        // `pam_get_item`, it will point to a valid null-terminated string.
         let cstr = unsafe { CStr::from_ptr(data as *const c_char) };
 
         Ok(cstr.to_str()?.to_owned())
@@ -219,12 +228,16 @@ impl<C: Converser> PamContext<C> {
     /// Set the TTY path for the current TTY that this PAM session started from.
     pub fn set_tty<P: AsRef<OsStr>>(&mut self, tty_path: P) -> PamResult<()> {
         let data = CString::new(tty_path.as_ref().as_bytes())?;
+        // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`); furthermore,
+        // `data.as_ptr()` will point to a correct null-terminated string.
         pam_err(unsafe { pam_set_item(self.pamh, PAM_TTY, data.as_ptr() as *const libc::c_void) })
     }
 
     // Set the user that requested the actions in this PAM instance.
     pub fn set_requesting_user(&mut self, user: &str) -> PamResult<()> {
         let data = CString::new(user.as_bytes())?;
+        // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`); furthermore,
+        // `data.as_ptr()` will point to a correct null-terminated string.
         pam_err(unsafe { pam_set_item(self.pamh, PAM_RUSER, data.as_ptr() as *const libc::c_void) })
     }
 
@@ -238,6 +251,7 @@ impl<C: Converser> PamContext<C> {
         let mut flags = action;
         flags |= self.silent_flag();
 
+        // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`).
         pam_err(unsafe { pam_setcred(self.pamh, flags) })
     }
 
@@ -252,12 +266,14 @@ impl<C: Converser> PamContext<C> {
         if expired_only {
             flags |= PAM_CHANGE_EXPIRED_AUTHTOK;
         }
+        // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`).
         pam_err(unsafe { pam_chauthtok(self.pamh, flags) })
     }
 
     /// Start a user session for the authenticated user.
     pub fn open_session(&mut self) -> PamResult<()> {
         if !self.session_started {
+            // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`).
             pam_err(unsafe { pam_open_session(self.pamh, self.silent_flag()) })?;
             self.session_started = true;
             Ok(())
@@ -269,6 +285,7 @@ impl<C: Converser> PamContext<C> {
     /// End the user session.
     pub fn close_session(&mut self) -> PamResult<()> {
         if self.session_started {
+            // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`).
             pam_err(unsafe { pam_close_session(self.pamh, self.silent_flag()) })?;
             self.session_started = false;
             Ok(())
@@ -280,13 +297,25 @@ impl<C: Converser> PamContext<C> {
     /// Get a full listing of the current PAM environment
     pub fn env(&mut self) -> PamResult<HashMap<OsString, OsString>> {
         let mut res = HashMap::new();
+        // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`).
+        // The man page for pam_getenvlist states that:
+        //    The format of the memory is a malloc()'d array of char pointers, the last element
+        //    of which is set to NULL. Each of the non-NULL entries in this array point to a
+        //    NUL terminated and malloc()'d char string of the form: "name=value".
+        //
+        //    The pam_getenvlist function returns NULL on failure.
         let envs = unsafe { pam_getenvlist(self.pamh) };
         if envs.is_null() {
             return Err(PamError::EnvListFailure);
         }
         let mut curr_env = envs;
+        // SAFETY: the loop invariant is as follows:
+        // - `curr_env` itself is always a valid pointer to an array of valid (possibly NULL) pointers
+        // - if `curr_env` points to a pointer that is not-null, that data is a c-string allocated by malloc()
+        // - `curr_env` points to NULL if and only if it is the final element in the array
         while let Some(curr_str) = NonNull::new(unsafe { curr_env.read() }) {
             let data = {
+                // SAFETY: `curr_str` points to a valid null-terminated string per the above
                 let cstr = unsafe { CStr::from_ptr(curr_str.as_ptr()) };
                 let bytes = cstr.to_bytes();
                 if let Some(pos) = bytes.iter().position(|b| *b == b'=') {
@@ -301,19 +330,24 @@ impl<C: Converser> PamContext<C> {
                 res.insert(k, v);
             }
 
-            // free the current string and move to the next one
+            // SAFETY: curr_str was obtained via libc::malloc() so we are responsible for freeing it.
+            // At this point, curr_str is also the only remaining pointer/reference to that allocated data
+            // (the data was copied above), so it can be deallocated without risk of use-after-free errors.
             unsafe { libc::free(curr_str.as_ptr().cast()) };
+            // SAFETY: curr_env was not NULL, so it was not the last element in the list and so PAM
+            // ensures that the next offset also is a valid pointer, and points to valid data.
             curr_env = unsafe { curr_env.offset(1) };
         }
 
-        // free the entire array
+        // SAFETY: `envs` itself was obtained by malloc(), so we are reponsible for freeing it.
         unsafe { libc::free(envs.cast()) };
 
         Ok(res)
     }
 
     /// Check if anything panicked since the last call.
     pub fn has_panicked(&self) -> bool {
+        // SAFETY: self.data_ptr was created by Box::into_raw
         unsafe { (*self.data_ptr).panicked }
     }
 }
@@ -336,13 +370,15 @@ impl PamContext<CLIConverser> {
 impl<C: Converser> Drop for PamContext<C> {
     fn drop(&mut self) {
         // data_ptr's pointee is de-allocated in this scope
+        // SAFETY: self.data_ptr was created by Box::into_raw
         let _data = unsafe { Box::from_raw(self.data_ptr) };
         let _ = self.close_session();
 
         // It looks like PAM_DATA_SILENT is important to set for our sudo context, but
         // it is unclear what it really does and does not do, other than the vague
         // documentation description to 'not take the call to seriously'
         // Also see https://github.com/systemd/systemd/issues/22318
+        // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`)
         unsafe {
             pam_end(
                 self.pamh,

src/pam/rpassword.rs

@@ -51,6 +51,7 @@ impl HiddenInput {
         term.c_lflag |= ECHONL;
 
         // Save the settings for now.
+        // SAFETY: we are passing tcsetattr a valid file descriptor and pointer-to-struct
         cerr(unsafe { tcsetattr(fd, TCSANOW, &term) })?;
 
         Ok(Some(HiddenInput { tty, term_orig }))
@@ -60,6 +61,7 @@ impl HiddenInput {
 impl Drop for HiddenInput {
     fn drop(&mut self) {
         // Set the the mode back to normal
+        // SAFETY: we are passing tcsetattr a valid file descriptor and pointer-to-struct
         unsafe {
             tcsetattr(self.tty.as_raw_fd(), TCSANOW, &self.term_orig);
         }
@@ -68,7 +70,9 @@ impl Drop for HiddenInput {
 
 fn safe_tcgetattr(fd: RawFd) -> io::Result<termios> {
     let mut term = mem::MaybeUninit::<termios>::uninit();
+    // SAFETY: we are passing tcgetattr a pointer to valid memory
     cerr(unsafe { ::libc::tcgetattr(fd, term.as_mut_ptr()) })?;
+    // SAFETY: if the previous call was a success, `tcgetattr` has initialized `term`
     Ok(unsafe { term.assume_init() })
 }
 

src/pam/securemem.rs

@@ -42,6 +42,8 @@ impl PamBuffer {
 
 impl Default for PamBuffer {
     fn default() -> Self {
+        // SAFETY: `calloc` returns either a cleared, allocated chunk of `SIZE` bytes
+        // or NULL to indicate that the allocation request failed
         let res = unsafe { libc::calloc(1, SIZE) };
         if let Some(nn) = NonNull::new(res) {
             PamBuffer(nn.cast())
@@ -55,20 +57,26 @@ impl std::ops::Deref for PamBuffer {
     type Target = [u8];
 
     fn deref(&self) -> &[u8] {
-        // make the slice one less in size to guarantee the existence of a terminating NUL
+        // SAFETY: `self.0.as_ptr()` is non-null, aligned, and initialized, and points to `SIZE` bytes.
+        // The lifetime of the slice does not exceed that of `self`.
+        //
+        // We make the slice one less in size to guarantee the existence of a terminating NUL.
         unsafe { slice::from_raw_parts(self.0.as_ptr().cast(), SIZE - 1) }
     }
 }
 
 impl std::ops::DerefMut for PamBuffer {
     fn deref_mut(&mut self) -> &mut [u8] {
+        // SAFETY: see above
         unsafe { slice::from_raw_parts_mut(self.0.as_ptr().cast(), SIZE - 1) }
     }
 }
 
 impl Drop for PamBuffer {
     fn drop(&mut self) {
+        // SAFETY: same as for `deref()` and `deref_mut()`
         wipe_memory(unsafe { self.0.as_mut() });
+        // SAFETY: `self.0.as_ptr()` was obtained via `calloc`, so calling `free` is proper.
         unsafe { libc::free(self.0.as_ptr().cast()) }
     }
 }
@@ -80,6 +88,7 @@ fn wipe_memory(memory: &mut [u8]) {
 
     let nonsense: u8 = 0x55;
     for c in memory {
+        // SAFETY: `c` is safe for writes (it comes from a &mut reference)
         unsafe { std::ptr::write_volatile(c, nonsense) };
     }
 

src/system/file/chown.rs

@@ -13,6 +13,8 @@ impl Chown for File {
     fn chown(&self, owner: UserId, group: GroupId) -> io::Result<()> {
         let fd = self.as_raw_fd();
 
+        // SAFETY: `fchown` is passed a proper file descriptor; and even if the user/group id
+        // is invalid, it will not cause UB.
         cerr(unsafe { libc::fchown(fd, owner, group) }).map(|_| ())
     }
 }

src/system/file/lock.rs

@@ -52,6 +52,8 @@ fn flock(fd: RawFd, action: LockOp, nonblocking: bool) -> Result<()> {
         operation |= libc::LOCK_NB;
     }
 
+    // SAFETY: even if `fd` would not be a valid file descriptor, that would merely
+    // result in an error condition, not UB
     cerr(unsafe { libc::flock(fd, operation) })?;
     Ok(())
 }