nevaforget/greetd-moongreet
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: audit findings — security, i18n, validation, dead code (v0.3.2)

Browse Source 
Quality:
- Q-5: Allow relative session commands (e.g. niri-session), greetd resolves PATH
- Q-3: Socket read+write timeouts with proper error logging
- Q-2: Remove unused PowerError::Timeout variant
- Q-M1: i18n for all login_worker error messages (new: unexpected_greetd_response)
- Q-M2: Explicit INVALID_LIST_POSITION check in session dropdown
- Q-M4: Log SVG loader.close() errors instead of silencing
- Q-M6: Testable persistence functions with proper roundtrip tests

Security:
- S-2: Validate GTK theme name (alphanumeric, _, -, +, . only)
- S-3: Log file created with mode 0o640
- S-4: Cache files (last-user, last-session) created with mode 0o600

Performance:
- P-3: Single symlink_metadata() call instead of exists() + is_symlink()
- P-4: Avoid Vec allocation in IPC send_message (two write_all calls)

Config:
- Update example GTK theme to Colloid-Catppuccin
This commit is contained in:
nevaforget
2026-03-28 00:37:35 +01:00
parent 4fa0dd0ead
commit 0d4a1b035a
8 changed files with 186 additions and 56 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/greeter.rs
+170 -37
View File
@@ -147,8 +147,16 @@ pub fn create_greeter_window(
// Apply GTK theme from config
if let Some(ref theme_name) = config.gtk_theme {
if let Some(settings) = gtk::Settings::default() {
settings.set_gtk_theme_name(Some(theme_name));
if !theme_name.is_empty()
&& theme_name
.chars()
.all(|c| c.is_ascii_alphanumeric() || matches!(c, '_' | '-' | '+' | '.'))
{
if let Some(settings) = gtk::Settings::default() {
settings.set_gtk_theme_name(Some(theme_name));
}
} else {
log::warn!("Ignoring invalid GTK theme name: {theme_name}");
}
}
@@ -606,7 +614,9 @@ fn set_default_avatar(
if let Ok(loader) = gdk_pixbuf::PixbufLoader::with_type("svg") {
loader.set_size(AVATAR_SIZE, AVATAR_SIZE);
if loader.write(svg_bytes).is_ok() {
let _ = loader.close();
if let Err(e) = loader.close() {
log::warn!("Failed to close SVG loader: {e}");
}
if let Some(pixbuf) = loader.pixbuf() {
let texture = gdk::Texture::for_pixbuf(&pixbuf);
state.borrow_mut().default_avatar_texture = Some(texture.clone());
@@ -629,8 +639,11 @@ fn get_selected_session(
if sessions.is_empty() {
return None;
}
let idx = dropdown.selected() as usize;
sessions.get(idx).cloned()
let idx = dropdown.selected();
if idx == gtk::INVALID_LIST_POSITION {
return None;
}
sessions.get(idx as usize).cloned()
}
/// Pre-select the last used session for a user in the dropdown.
@@ -795,6 +808,7 @@ fn attempt_login(
&sock_path,
&greetd_sock,
&login_cancelled,
strings,
)
})
.await;
@@ -869,14 +883,19 @@ fn login_worker(
sock_path: &str,
greetd_sock: &Arc<Mutex<Option<UnixStream>>>,
login_cancelled: &Arc<std::sync::atomic::AtomicBool>,
strings: &Strings,
) -> Result<LoginResult, String> {
if login_cancelled.load(std::sync::atomic::Ordering::SeqCst) {
return Ok(LoginResult::Cancelled);
}
let mut sock = UnixStream::connect(sock_path).map_err(|e| e.to_string())?;
sock.set_read_timeout(Some(std::time::Duration::from_secs(10)))
.ok();
if let Err(e) = sock.set_read_timeout(Some(std::time::Duration::from_secs(10))) {
log::warn!("Failed to set read timeout: {e}");
}
if let Err(e) = sock.set_write_timeout(Some(std::time::Duration::from_secs(10))) {
log::warn!("Failed to set write timeout: {e}");
}
{
let mut guard = greetd_sock.lock().map_err(|e| e.to_string())?;
*guard = Some(sock.try_clone().map_err(|e| e.to_string())?);
@@ -900,7 +919,7 @@ fn login_worker(
message: response
.get("description")
.and_then(|v| v.as_str())
.unwrap_or("Authentication failed")
.unwrap_or(strings.auth_failed)
.to_string(),
});
}
@@ -927,7 +946,7 @@ fn login_worker(
// Multi-stage auth is not supported
let _ = ipc::cancel_session(&mut sock);
return Ok(LoginResult::Error {
message: "Multi-stage authentication is not supported".to_string(),
message: strings.multi_stage_unsupported.to_string(),
});
}
}
@@ -939,17 +958,18 @@ fn login_worker(
_ => {
let _ = ipc::cancel_session(&mut sock);
return Ok(LoginResult::Error {
message: "Invalid session command".to_string(),
message: strings.invalid_session_command.to_string(),
});
}
};
// Validate: first token must be an absolute path to an existing file
let binary = std::path::Path::new(&cmd[0]);
if !binary.is_absolute() || !binary.is_file() {
// Validate: reject obviously invalid commands (empty, null bytes, path traversal)
// greetd resolves PATH for relative commands like "niri-session"
let first = &cmd[0];
if first.is_empty() || first.contains('\0') || first.contains("..") {
let _ = ipc::cancel_session(&mut sock);
return Ok(LoginResult::Error {
message: "Invalid session command".to_string(),
message: strings.invalid_session_command.to_string(),
});
}
@@ -969,14 +989,14 @@ fn login_worker(
message: response
.get("description")
.and_then(|v| v.as_str())
.unwrap_or("Failed to start session")
.unwrap_or(strings.session_start_failed)
.to_string(),
});
}
}
Ok(LoginResult::Error {
message: "Unexpected response from greetd".to_string(),
message: strings.unexpected_greetd_response.to_string(),
})
}
@@ -1012,7 +1032,11 @@ fn execute_power_action(
// -- Last user/session persistence --
fn load_last_user() -> Option<String> {
let content = std::fs::read_to_string(LAST_USER_PATH).ok()?;
load_last_user_from(Path::new(LAST_USER_PATH))
}
fn load_last_user_from(path: &Path) -> Option<String> {
let content = std::fs::read_to_string(path).ok()?;
let username = content.trim();
if is_valid_username(username) {
Some(username.to_string())
@@ -1022,15 +1046,29 @@ fn load_last_user() -> Option<String> {
}
fn save_last_user(username: &str) {
let path = Path::new(LAST_USER_PATH);
save_last_user_to(Path::new(LAST_USER_PATH), username);
}
fn save_last_user_to(path: &Path, username: &str) {
if let Some(parent) = path.parent() {
let _ = std::fs::create_dir_all(parent);
}
let _ = std::fs::write(path, username);
use std::os::unix::fs::OpenOptionsExt;
use std::io::Write;
let _ = std::fs::OpenOptions::new()
.create(true)
.write(true)
.truncate(true)
.mode(0o600)
.open(path)
.and_then(|mut f| f.write_all(username.as_bytes()));
}
fn load_last_session(username: &str) -> Option<String> {
let path = Path::new(LAST_SESSION_DIR).join(username);
load_last_session_from(&Path::new(LAST_SESSION_DIR).join(username))
}
fn load_last_session_from(path: &Path) -> Option<String> {
let content = std::fs::read_to_string(path).ok()?;
let name = content.trim();
if is_valid_session_name(name) {
@@ -1059,7 +1097,19 @@ fn save_last_session(username: &str, session_name: &str) {
}
let dir = Path::new(LAST_SESSION_DIR);
let _ = std::fs::create_dir_all(dir);
let _ = std::fs::write(dir.join(username), session_name);
save_last_session_to(&dir.join(username), session_name);
}
fn save_last_session_to(path: &Path, session_name: &str) {
use std::os::unix::fs::OpenOptionsExt;
use std::io::Write;
let _ = std::fs::OpenOptions::new()
.create(true)
.write(true)
.truncate(true)
.mode(0o600)
.open(path)
.and_then(|mut f| f.write_all(session_name.as_bytes()));
}
#[cfg(test)]
@@ -1103,26 +1153,71 @@ mod tests {
}
#[test]
fn last_user_persistence() {
fn last_user_roundtrip() {
let dir = tempfile::tempdir().unwrap();
let path = dir.path().join("last-user");
std::fs::write(&path, "alice").unwrap();
let content = std::fs::read_to_string(&path).unwrap();
let username = content.trim();
assert!(is_valid_username(username));
assert_eq!(username, "alice");
save_last_user_to(&path, "alice");
let loaded = load_last_user_from(&path);
assert_eq!(loaded, Some("alice".to_string()));
}
#[test]
fn last_session_persistence() {
fn last_user_rejects_invalid() {
let dir = tempfile::tempdir().unwrap();
let session_dir = dir.path().join("last-session");
std::fs::create_dir_all(&session_dir).unwrap();
std::fs::write(session_dir.join("alice"), "Niri").unwrap();
let path = dir.path().join("last-user");
save_last_user_to(&path, "../evil");
let loaded = load_last_user_from(&path);
assert_eq!(loaded, None);
}
let content = std::fs::read_to_string(session_dir.join("alice")).unwrap();
assert_eq!(content.trim(), "Niri");
#[test]
fn last_user_missing_file() {
let loaded = load_last_user_from(Path::new("/nonexistent/last-user"));
assert_eq!(loaded, None);
}
#[test]
fn last_session_roundtrip() {
let dir = tempfile::tempdir().unwrap();
let path = dir.path().join("alice");
save_last_session_to(&path, "Niri");
let loaded = load_last_session_from(&path);
assert_eq!(loaded, Some("Niri".to_string()));
}
#[test]
fn last_session_rejects_invalid() {
let dir = tempfile::tempdir().unwrap();
let path = dir.path().join("alice");
save_last_session_to(&path, "../../../etc/evil");
let loaded = load_last_session_from(&path);
assert_eq!(loaded, None);
}
#[test]
fn last_session_missing_file() {
let loaded = load_last_session_from(Path::new("/nonexistent/session"));
assert_eq!(loaded, None);
}
#[test]
fn last_user_file_permissions() {
let dir = tempfile::tempdir().unwrap();
let path = dir.path().join("last-user");
save_last_user_to(&path, "alice");
let meta = std::fs::metadata(&path).unwrap();
use std::os::unix::fs::PermissionsExt;
assert_eq!(meta.permissions().mode() & 0o777, 0o600);
}
#[test]
fn last_session_file_permissions() {
let dir = tempfile::tempdir().unwrap();
let path = dir.path().join("session");
save_last_session_to(&path, "Niri");
let meta = std::fs::metadata(&path).unwrap();
use std::os::unix::fs::PermissionsExt;
assert_eq!(meta.permissions().mode() & 0o777, 0o600);
}
// -- split_shell_words tests --
@@ -1259,6 +1354,7 @@ mod tests {
let result = login_worker(
"alice", "wrongpass", "/usr/bin/niri",
&sock_path, &default_greetd_sock(), &default_cancelled(),
load_strings(Some("en")),
);
let result = result.unwrap();
@@ -1300,6 +1396,7 @@ mod tests {
let result = login_worker(
"alice", "correct", "/usr/bin/bash",
&sock_path, &default_greetd_sock(), &default_cancelled(),
load_strings(Some("en")),
);
let result = result.unwrap();
@@ -1334,6 +1431,7 @@ mod tests {
let result = login_worker(
"alice", "pass", "/usr/bin/niri",
&sock_path, &default_greetd_sock(), &default_cancelled(),
load_strings(Some("en")),
);
let result = result.unwrap();
@@ -1370,6 +1468,7 @@ mod tests {
let result = login_worker(
"alice", "pass", "/usr/bin/bash",
&sock_path, &default_greetd_sock(), &default_cancelled(),
load_strings(Some("en")),
);
let result = result.unwrap();
@@ -1384,6 +1483,7 @@ mod tests {
let result = login_worker(
"alice", "pass", "/usr/bin/niri",
"/nonexistent/sock", &default_greetd_sock(), &cancelled,
load_strings(Some("en")),
);
let result = result.unwrap();
@@ -1405,19 +1505,52 @@ mod tests {
let _msg = ipc::recv_message(stream).unwrap();
ipc::send_message(stream, &serde_json::json!({"type": "success"})).unwrap();
// cancel_session (from invalid exec_cmd path)
// cancel_session (from invalid exec_cmd with path traversal)
let _msg = ipc::recv_message(stream).unwrap();
ipc::send_message(stream, &serde_json::json!({"type": "success"})).unwrap();
});
// Non-absolute exec_cmd
// exec_cmd with path traversal
let result = login_worker(
"alice", "pass", "relative-binary",
"alice", "pass", "../../../etc/evil",
&sock_path, &default_greetd_sock(), &default_cancelled(),
load_strings(Some("en")),
);
let result = result.unwrap();
assert!(matches!(result, LoginResult::Error { .. }));
handle.join().unwrap();
}
#[test]
fn login_worker_relative_exec_cmd_allowed() {
let (sock_path, handle) = fake_greetd(|stream| {
// create_session
let _msg = ipc::recv_message(stream).unwrap();
ipc::send_message(stream, &serde_json::json!({
"type": "auth_message",
"auth_message_type": "secret",
"auth_message": "Password: ",
})).unwrap();
// post_auth_response → success
let _msg = ipc::recv_message(stream).unwrap();
ipc::send_message(stream, &serde_json::json!({"type": "success"})).unwrap();
// start_session with relative command (e.g. niri-session)
let _msg = ipc::recv_message(stream).unwrap();
ipc::send_message(stream, &serde_json::json!({"type": "success"})).unwrap();
});
// Relative exec_cmd like "niri-session" should be allowed
let result = login_worker(
"alice", "pass", "niri-session",
&sock_path, &default_greetd_sock(), &default_cancelled(),
load_strings(Some("en")),
);
let result = result.unwrap();
assert!(matches!(result, LoginResult::Success { .. }));
handle.join().unwrap();
}
}