From 6c67e7853b1501aaceb785d1578c58744bed8865 Mon Sep 17 00:00:00 2001 From: Digiyang Date: Mon, 16 Feb 2026 12:14:23 +0100 Subject: [PATCH 1/2] Add input retry loops and default cipher selection Wrap each validation site in a retry loop that shows the error via show_input_popup and re-prompts instead of aborting the entire TUI flow. Default to Cv25519 when no cipher is selected, matching the existing default behavior for validity duration. --- src/sequoia_openpgp/certificate_manager.rs | 2 +- src/sequoia_openpgp/wrapper_fun.rs | 165 ++++++++++++--------- 2 files changed, 98 insertions(+), 69 deletions(-) diff --git a/src/sequoia_openpgp/certificate_manager.rs b/src/sequoia_openpgp/certificate_manager.rs index a63dcd6..5f358ab 100644 --- a/src/sequoia_openpgp/certificate_manager.rs +++ b/src/sequoia_openpgp/certificate_manager.rs @@ -319,7 +319,7 @@ impl CertificateManager { } match cipher.as_str() { - "1" => builder = builder.set_cipher_suite(CipherSuite::Cv25519), + "1" | "" => builder = builder.set_cipher_suite(CipherSuite::Cv25519), "2" => builder = builder.set_cipher_suite(CipherSuite::RSA2k), "3" => builder = builder.set_cipher_suite(CipherSuite::RSA3k), "4" => builder = builder.set_cipher_suite(CipherSuite::RSA4k), diff --git a/src/sequoia_openpgp/wrapper_fun.rs b/src/sequoia_openpgp/wrapper_fun.rs index 01da46a..ad7af00 100644 --- a/src/sequoia_openpgp/wrapper_fun.rs +++ b/src/sequoia_openpgp/wrapper_fun.rs @@ -8,7 +8,7 @@ use ratatui::{ }; use sequoia_openpgp::{crypto::Password, parse::Parse, Cert}; -use crate::app::ui::{draw_input_prompt, show_user_selection_popup}; +use crate::app::ui::{draw_input_prompt, show_input_popup, show_user_selection_popup}; use super::certificate_manager::CertificateManager; @@ -47,15 +47,19 @@ pub fn generate_keypair_tui( terminal: &mut Terminal>, ) -> Result<(), anyhow::Error> { let style = Style::default().fg(Color::Yellow); - let user_id = draw_input_prompt( - terminal, - &[Line::from(Span::styled( - "Enter user id (e.g 'Bob ):", - style, - ))], - true, - )?; - validate_not_empty(&user_id, "User ID")?; + let user_id_prompt = [Line::from(Span::styled( + "Enter user id (e.g 'Bob ):", + style, + ))]; + let user_id = loop { + let input = draw_input_prompt(terminal, &user_id_prompt, true)?; + match validate_not_empty(&input, "User ID") { + Ok(()) => break input, + Err(e) => { + show_input_popup(terminal, &e.to_string())?; + } + } + }; let prompt_validity = vec![ "Please specify a validity duration for your key:", @@ -72,7 +76,7 @@ pub fn generate_keypair_tui( .collect(); let prompt_cipher = vec![ - "Please select a cypher:", + "Please select a cypher (default: Cv25519):", " (1) Cv25519", " EdDSA and ECDH over Curve25519 with SHA512 and AES256", " (2) RSA2k", @@ -96,25 +100,28 @@ pub fn generate_keypair_tui( let validity = draw_input_prompt(terminal, &validity_spans, true)?; let cipher = draw_input_prompt(terminal, &cipher_spans, true)?; - let pw = draw_input_prompt( - terminal, - &[Line::from(Span::styled( - "Enter password (min. 8 chars, a passphrase is recommended):", - style, - ))], - false, - )?; - validate_password(&pw)?; - - let rpw = draw_input_prompt( - terminal, - &[Line::from(Span::styled("Repeat password:", style))], - false, - )?; - - if pw != rpw { - return Err(anyhow::anyhow!("Passwords do not match!")); - } + let pw_prompt = [Line::from(Span::styled( + "Enter password (min. 8 chars, a passphrase is recommended):", + style, + ))]; + let pw = loop { + let input = draw_input_prompt(terminal, &pw_prompt, false)?; + match validate_password(&input) { + Ok(()) => break input, + Err(e) => { + show_input_popup(terminal, &e.to_string())?; + } + } + }; + + let rpw_prompt = [Line::from(Span::styled("Repeat password:", style))]; + let rpw = loop { + let input = draw_input_prompt(terminal, &rpw_prompt, false)?; + if pw == input { + break input; + } + show_input_popup(terminal, "Passwords do not match!")?; + }; cert_manager.generate_keypair(user_id, validity, cipher, pw, rpw)?; Ok(()) @@ -136,14 +143,18 @@ pub fn export_certificate_tui( draw_input_prompt(terminal, &[Line::from("Enter key secret")], false)?.into(); match secret.decrypt_secret(&password) { Ok(_) => { - let file_name = draw_input_prompt( - terminal, - &[Line::from( - "Enter exported certificate name (e.g. name.certificate.pgp):", - )], - true, - )?; - validate_filename(&file_name)?; + let file_name_prompt = [Line::from( + "Enter exported certificate name (e.g. name.certificate.pgp):", + )]; + let file_name = loop { + let input = draw_input_prompt(terminal, &file_name_prompt, true)?; + match validate_filename(&input) { + Ok(()) => break input, + Err(e) => { + show_input_popup(terminal, &e.to_string())?; + } + } + }; cert_manager.export_certificate(cert_path, &file_name)?; } @@ -171,27 +182,33 @@ pub fn edit_password_tui( let original_pw = Password::from(original_pw_str); match secret.decrypt_secret(&original_pw) { Ok(_) => { - let new_password = draw_input_prompt( - terminal, - &[Line::from( - "Enter new password (min. 8 chars, a passphrase is recommended):", - )], - false, + let new_pw_prompt = [Line::from( + "Enter new password (min. 8 chars, a passphrase is recommended):", + )]; + let new_password = loop { + let input = draw_input_prompt(terminal, &new_pw_prompt, false)?; + match validate_password(&input) { + Ok(()) => break input, + Err(e) => { + show_input_popup(terminal, &e.to_string())?; + } + } + }; + + let repeat_pw_prompt = [Line::from("Repeat new password")]; + let repeat_password = loop { + let input = draw_input_prompt(terminal, &repeat_pw_prompt, false)?; + if new_password == input { + break input; + } + show_input_popup(terminal, "Passwords do not match!")?; + }; + cert_manager.edit_password( + certificate, + &original_pw, + new_password, + repeat_password, )?; - validate_password(&new_password)?; - - let repeat_password = - draw_input_prompt(terminal, &[Line::from("Repeat new password")], false)?; - if new_password == repeat_password { - cert_manager.edit_password( - certificate, - &original_pw, - new_password, - repeat_password, - )?; - } else { - return Err(anyhow::anyhow!("Passwords do not match!")); - } } Err(_) => { return Err(anyhow::anyhow!("Wrong password!")); @@ -259,8 +276,16 @@ pub fn add_user_tui( let original_pw = Password::from(original_pw_str); match secret.decrypt_secret(&original_pw) { Ok(_) => { - let new_user = draw_input_prompt(terminal, &[Line::from("Enter new user:")], true)?; - validate_not_empty(&new_user, "User ID")?; + let new_user_prompt = [Line::from("Enter new user:")]; + let new_user = loop { + let input = draw_input_prompt(terminal, &new_user_prompt, true)?; + match validate_not_empty(&input, "User ID") { + Ok(()) => break input, + Err(e) => { + show_input_popup(terminal, &e.to_string())?; + } + } + }; cert_manager.add_user(cert_path, &original_pw, new_user)?; } Err(_) => { @@ -285,14 +310,18 @@ pub fn split_users_tui( let should_continue = show_user_selection_popup(terminal, &mut users, &mut selected_items)?; if let Some(true) = should_continue { - let file_name = draw_input_prompt( - terminal, - &[Line::from( - "Enter exported certificate name (e.g. name.certificate.pgp):", - )], - true, - )?; - validate_filename(&file_name)?; + let file_name_prompt = [Line::from( + "Enter exported certificate name (e.g. name.certificate.pgp):", + )]; + let file_name = loop { + let input = draw_input_prompt(terminal, &file_name_prompt, true)?; + match validate_filename(&input) { + Ok(()) => break input, + Err(e) => { + show_input_popup(terminal, &e.to_string())?; + } + } + }; let selected_users: Vec = users .items From f29c7b21902c18d9d39b96f58776b6d5de8b96dd Mon Sep 17 00:00:00 2001 From: Digiyang Date: Mon, 16 Feb 2026 12:18:07 +0100 Subject: [PATCH 2/2] Sanitize User ID in file path construction Replace '..', '/' and '\' with '_' in the user ID before using it as a filename component in generate_keypair. This prevents a crafted User ID containing '../' from writing files outside ~/.pgpman/. (Issue #36) --- src/sequoia_openpgp/certificate_manager.rs | 14 ++-- tests/certificate_manager.rs | 77 +++++++++++++++++++++- 2 files changed, 84 insertions(+), 7 deletions(-) diff --git a/src/sequoia_openpgp/certificate_manager.rs b/src/sequoia_openpgp/certificate_manager.rs index 5f358ab..cf9f875 100644 --- a/src/sequoia_openpgp/certificate_manager.rs +++ b/src/sequoia_openpgp/certificate_manager.rs @@ -20,6 +20,12 @@ use crate::app::launch::clear_screen; use crate::utils::create_directory::{create_file, create_secret_file}; use crate::utils::parse_iso8601_duration::parse_iso8601_duration; +/// Sanitize a user ID for use as a filename component. +/// Replaces `..`, `/`, and `\` with `_` to prevent path traversal. +fn sanitize_for_path(input: &str) -> String { + input.replace("..", "_").replace(['/', '\\'], "_") +} + #[derive(Debug, Clone, Copy)] pub struct CertificateManager; @@ -294,7 +300,7 @@ impl CertificateManager { clear_screen(); let mut builder = CertBuilder::new(); - let uid_clone = user_id.clone(); + let safe_uid = sanitize_for_path(&user_id); builder = builder.add_userid(user_id); match validity.as_str() { @@ -352,11 +358,11 @@ impl CertificateManager { let home_dir = home::home_dir() .ok_or_else(|| anyhow::anyhow!("Could not determine home directory"))?; let key_path = - format!("{}/.pgpman/secrets/{}.pgp", &home_dir.display(), uid_clone).replace(" ", ""); + format!("{}/.pgpman/secrets/{}.pgp", &home_dir.display(), safe_uid).replace(" ", ""); let revcert_path = format!( "{}/.pgpman/revocation/{}.rev", &home_dir.display(), - uid_clone + safe_uid ) .replace(" ", ""); // export key to path @@ -381,7 +387,7 @@ impl CertificateManager { let cert_path = format!( "{}/.pgpman/certificates/{}.pgp", &home_dir.display(), - uid_clone + safe_uid ) .replace(" ", ""); { diff --git a/tests/certificate_manager.rs b/tests/certificate_manager.rs index 3f29aea..96ba2df 100644 --- a/tests/certificate_manager.rs +++ b/tests/certificate_manager.rs @@ -13,13 +13,19 @@ fn manager() -> CertificateManager { CertificateManager } +/// Mirror the sanitization logic from `certificate_manager::sanitize_for_path`. +fn sanitize_for_path(input: &str) -> String { + input.replace("..", "_").replace(['/', '\\'], "_") +} + /// Helper to compute the paths that `generate_keypair` writes to in ~/.pgpman/. fn pgpman_paths(uid: &str) -> (String, String, String) { let home = home::home_dir().unwrap(); + let safe = sanitize_for_path(uid); ( - format!("{}/.pgpman/secrets/{}.pgp", home.display(), uid).replace(' ', ""), - format!("{}/.pgpman/revocation/{}.rev", home.display(), uid).replace(' ', ""), - format!("{}/.pgpman/certificates/{}.pgp", home.display(), uid).replace(' ', ""), + format!("{}/.pgpman/secrets/{}.pgp", home.display(), safe).replace(' ', ""), + format!("{}/.pgpman/revocation/{}.rev", home.display(), safe).replace(' ', ""), + format!("{}/.pgpman/certificates/{}.pgp", home.display(), safe).replace(' ', ""), ) } @@ -301,3 +307,68 @@ fn generate_keypair_mismatched_passwords_fails() { ); assert!(result.is_err(), "mismatched passwords should fail"); } + +/// A User ID containing `../` must not escape ~/.pgpman/ directories. +#[test] +fn generate_keypair_sanitizes_path_traversal_in_uid() { + init_directory().unwrap(); + + let malicious_uid = "../evil "; + let (sk_expected, rev_expected, cert_expected) = pgpman_paths(malicious_uid); + + manager() + .generate_keypair( + malicious_uid.into(), + "1y".into(), + "1".into(), + "safepass".into(), + "safepass".into(), + ) + .unwrap(); + + // All files must land inside the expected ~/.pgpman/ subdirectories + let home = home::home_dir().unwrap(); + let secrets_dir = format!("{}/.pgpman/secrets", home.display()); + let revocation_dir = format!("{}/.pgpman/revocation", home.display()); + let certificates_dir = format!("{}/.pgpman/certificates", home.display()); + + assert!( + sk_expected.starts_with(&secrets_dir), + "secret key must be inside secrets dir" + ); + assert!( + rev_expected.starts_with(&revocation_dir), + "revocation cert must be inside revocation dir" + ); + assert!( + cert_expected.starts_with(&certificates_dir), + "public cert must be inside certificates dir" + ); + + // Files must actually exist at the sanitized paths + assert!(Path::new(&sk_expected).exists(), "secret key should exist"); + assert!( + Path::new(&rev_expected).exists(), + "revocation cert should exist" + ); + assert!( + Path::new(&cert_expected).exists(), + "public cert should exist" + ); + + // The filename component must not contain ".." + let sk_filename = Path::new(&sk_expected) + .file_name() + .unwrap() + .to_str() + .unwrap(); + assert!( + !sk_filename.contains(".."), + "filename must not contain '..'" + ); + + // Clean up + let _ = fs::remove_file(&sk_expected); + let _ = fs::remove_file(&rev_expected); + let _ = fs::remove_file(&cert_expected); +}