From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id A259275692 for ; Mon, 12 Jul 2021 18:30:55 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8ED9A18A9A for ; Mon, 12 Jul 2021 18:30:55 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 4413D18A8F for ; Mon, 12 Jul 2021 18:30:54 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 14FFD41016 for ; Mon, 12 Jul 2021 18:30:54 +0200 (CEST) From: Stefan Reiter To: pbs-devel@lists.proxmox.com Date: Mon, 12 Jul 2021 18:30:47 +0200 Message-Id: <20210712163047.3119673-1-s.reiter@proxmox.com> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.625 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pbs-devel] [PATCH proxmox-backup] auth: 'crypt' is not thread safe X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 12 Jul 2021 16:30:55 -0000 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 --- 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 { +// 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 { + #[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 { 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