auth: 'crypt' is not thread safe

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>
This commit is contained in:
Stefan Reiter 2021-07-12 18:30:47 +02:00 committed by Thomas Lamprecht
parent 0ed40b19c7
commit c4c4b5a3ef
1 changed files with 42 additions and 15 deletions

View File

@ -4,7 +4,7 @@
use std::process::{Command, Stdio}; use std::process::{Command, Stdio};
use std::io::Write; use std::io::Write;
use std::ffi::{CString, CStr}; use std::ffi::CStr;
use anyhow::{bail, format_err, Error}; use anyhow::{bail, format_err, Error};
use serde_json::json; use serde_json::json;
@ -72,24 +72,51 @@ impl ProxmoxAuthenticator for PAM {
pub struct PBS(); 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)]
extern "C" { struct crypt_data {
#[link_name = "crypt"] output: [libc::c_char; CRYPT_OUTPUT_SIZE],
fn __crypt(key: *const libc::c_char, salt: *const libc::c_char) -> * mut libc::c_char; 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],
} }
let salt = CString::new(salt)?; pub fn crypt(password: &[u8], salt: &[u8]) -> Result<String, Error> {
let password = CString::new(password)?; #[link(name = "crypt")]
extern "C" {
#[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 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 { let res = unsafe {
CStr::from_ptr( let status = __crypt_r(
__crypt( &data.input as *const _,
password.as_c_str().as_ptr(), &data.setting as *const _,
salt.as_c_str().as_ptr() &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()?)) 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 = proxmox::sys::linux::random_data(8)?;
let salt = format!("$5${}$", base64::encode_config(&salt, base64::CRYPT)); 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> { 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 { if verify != enc_password {
bail!("invalid credentials"); bail!("invalid credentials");
} }