[pbs-devel] [PATCH proxmox-backup] auth: 'crypt' is not thread safe

[pbs-devel] [PATCH proxmox-backup] auth: 'crypt' is not thread safe

[Stefan Reiter]

According to crypt(3):
"crypt places its result in a static storage area, which will be
overwritten by subsequent calls to crypt. It is not safe to call crypt
from multiple threads simultaneously."

This means that multiple login calls as a PBS-realm user can collide and
produce intermittent authentication failures. A visible case is for
file-restore, where VMs with many disks lead to just as many auth-calls
at the same time, as the GUI tries to expand each tree element on load.

Instead, use the thread-safe variant 'crypt_r', which places the result
into a pre-allocated buffer of type 'crypt_data'. The C struct is laid
out according to 'lib/crypt.h.in' and the man page mentioned above.

Use the opportunity and make both arguments to the rust 'crypt' function
take a &[u8].

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

Easier solution would of course be to just wrap the call in a Mutex<()> or
similar, but that comes at the cost of locking...

 src/auth.rs | 57 +++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 42 insertions(+), 15 deletions(-)

diff --git a/src/auth.rs b/src/auth.rs
index 4845601a..aee183ee 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -4,7 +4,7 @@
 
 use std::process::{Command, Stdio};
 use std::io::Write;
-use std::ffi::{CString, CStr};
+use std::ffi::CStr;
 
 use anyhow::{bail, format_err, Error};
 use serde_json::json;
@@ -72,24 +72,51 @@ impl ProxmoxAuthenticator for PAM {
 
 pub struct PBS();
 
-pub fn crypt(password: &[u8], salt: &str) -> Result<String, Error> {
+// from libcrypt1, 'lib/crypt.h.in'
+const CRYPT_OUTPUT_SIZE: usize = 384;
+const CRYPT_MAX_PASSPHRASE_SIZE: usize = 512;
+const CRYPT_DATA_RESERVED_SIZE: usize = 767;
+const CRYPT_DATA_INTERNAL_SIZE: usize = 30720;
 
-    #[link(name="crypt")]
+#[repr(C)]
+struct crypt_data {
+    output: [libc::c_char; CRYPT_OUTPUT_SIZE],
+    setting: [libc::c_char; CRYPT_OUTPUT_SIZE],
+    input: [libc::c_char; CRYPT_MAX_PASSPHRASE_SIZE],
+    reserved: [libc::c_char; CRYPT_DATA_RESERVED_SIZE],
+    initialized: libc::c_char,
+    internal: [libc::c_char; CRYPT_DATA_INTERNAL_SIZE],
+}
+
+pub fn crypt(password: &[u8], salt: &[u8]) -> Result<String, Error> {
+    #[link(name = "crypt")]
     extern "C" {
-        #[link_name = "crypt"]
-        fn __crypt(key: *const libc::c_char, salt:  *const libc::c_char) -> * mut libc::c_char;
+        #[link_name = "crypt_r"]
+        fn __crypt_r(
+            key: *const libc::c_char,
+            salt: *const libc::c_char,
+            data: *mut crypt_data,
+        ) -> *mut libc::c_char;
     }
 
-    let salt = CString::new(salt)?;
-    let password = CString::new(password)?;
+    let mut data: crypt_data = unsafe { std::mem::zeroed() };
+    for (i, c) in salt.iter().take(data.setting.len() - 1).enumerate() {
+        data.setting[i] = *c as libc::c_char;
+    }
+    for (i, c) in password.iter().take(data.input.len() - 1).enumerate() {
+        data.input[i] = *c as libc::c_char;
+    }
 
     let res = unsafe {
-        CStr::from_ptr(
-            __crypt(
-                password.as_c_str().as_ptr(),
-                salt.as_c_str().as_ptr()
-            )
-        )
+        let status = __crypt_r(
+            &data.input as *const _,
+            &data.setting as *const _,
+            &mut data as *mut _,
+        );
+        if status.is_null() {
+            bail!("internal error: crypt_r returned null pointer");
+        }
+        CStr::from_ptr(&data.output as *const _)
     };
     Ok(String::from(res.to_str()?))
 }
@@ -100,11 +127,11 @@ pub fn encrypt_pw(password: &str) -> Result<String, Error> {
     let salt = proxmox::sys::linux::random_data(8)?;
     let salt = format!("$5${}$", base64::encode_config(&salt, base64::CRYPT));
 
-    crypt(password.as_bytes(), &salt)
+    crypt(password.as_bytes(), salt.as_bytes())
 }
 
 pub fn verify_crypt_pw(password: &str, enc_password: &str) -> Result<(), Error> {
-    let verify = crypt(password.as_bytes(), enc_password)?;
+    let verify = crypt(password.as_bytes(), enc_password.as_bytes())?;
     if verify != enc_password {
         bail!("invalid credentials");
     }
-- 
2.30.2

[pbs-devel] applied: [PATCH proxmox-backup] auth: 'crypt' is not thread safe

[Thomas Lamprecht]

On 12.07.21 18:30, Stefan Reiter wrote:
> According to crypt(3):
> "crypt places its result in a static storage area, which will be
> overwritten by subsequent calls to crypt. It is not safe to call crypt
> from multiple threads simultaneously."
> 
> This means that multiple login calls as a PBS-realm user can collide and
> produce intermittent authentication failures. A visible case is for
> file-restore, where VMs with many disks lead to just as many auth-calls
> at the same time, as the GUI tries to expand each tree element on load.
> 
> Instead, use the thread-safe variant 'crypt_r', which places the result
> into a pre-allocated buffer of type 'crypt_data'. The C struct is laid
> out according to 'lib/crypt.h.in' and the man page mentioned above.
> 
> Use the opportunity and make both arguments to the rust 'crypt' function
> take a &[u8].
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> Easier solution would of course be to just wrap the call in a Mutex<()> or
> similar, but that comes at the cost of locking...
> 
>  src/auth.rs | 57 +++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 42 insertions(+), 15 deletions(-)
> 
>

applied, thanks!