Skip to content

Commit 1bb8bda

Browse files
committed
security hardening 2: safer way to store api keys and log files via unix permissions, validate file paths
1 parent a4a8a99 commit 1bb8bda

7 files changed

Lines changed: 150 additions & 9 deletions

File tree

src/cache.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,10 @@ impl ResponseCache {
3232
return Ok(());
3333
}
3434
let dir = Self::storage_dir()?;
35-
fs::create_dir_all(&dir)
35+
crate::fsutil::create_private_dir(&dir)
3636
.with_context(|| format!("creating cache directory {}", dir.display()))?;
37-
fs::write(self.path_for(key)?, value).context("writing response cache entry")?;
37+
crate::fsutil::write_private(&self.path_for(key)?, value)
38+
.context("writing response cache entry")?;
3839
self.prune(&dir)
3940
}
4041

src/chat.rs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ impl ChatSession {
3030
/// shell_gpt's count-based truncation.
3131
pub fn save(&self, chat_id: &str, max_length: usize) -> Result<()> {
3232
let dir = Self::storage_dir()?;
33-
fs::create_dir_all(&dir)
33+
crate::fsutil::create_private_dir(&dir)
3434
.with_context(|| format!("creating chat directory {}", dir.display()))?;
3535

3636
let messages = if self.messages.len() > max_length {
@@ -48,7 +48,8 @@ impl ChatSession {
4848
messages,
4949
})
5050
.context("serializing chat session")?;
51-
fs::write(&path, contents).with_context(|| format!("writing chat file {}", path.display()))
51+
crate::fsutil::write_private(&path, &contents)
52+
.with_context(|| format!("writing chat file {}", path.display()))
5253
}
5354

5455
pub fn delete(chat_id: &str) -> Result<()> {
@@ -79,6 +80,10 @@ impl ChatSession {
7980
}
8081

8182
fn path(chat_id: &str) -> Result<PathBuf> {
83+
// `chat_id` comes from argv and is used verbatim as a filename; reject
84+
// path separators / traversal so load, save, and delete can't escape
85+
// the chat directory (e.g. `--show-chat ../../../../etc/passwd`).
86+
crate::fsutil::validate_identifier("chat", chat_id)?;
8287
Ok(Self::storage_dir()?.join(chat_id))
8388
}
8489

src/config.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ impl Config {
7575

7676
fn bootstrap(path: PathBuf, skip_api_key_prompt: bool) -> Result<Config> {
7777
if let Some(parent) = path.parent() {
78-
fs::create_dir_all(parent)
78+
crate::fsutil::create_private_dir(parent)
7979
.with_context(|| format!("creating config directory {}", parent.display()))?;
8080
}
8181

@@ -103,7 +103,9 @@ impl Config {
103103
contents.push_str(value);
104104
contents.push('\n');
105105
}
106-
fs::write(&self.path, contents)
106+
// The config holds the API key in plaintext; write it owner-only (0600)
107+
// so other local users can't read it.
108+
crate::fsutil::write_private(&self.path, &contents)
107109
.with_context(|| format!("writing config file {}", self.path.display()))
108110
}
109111

src/debug.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,9 @@ impl DebugLog {
2525
let path = std::env::current_dir()
2626
.unwrap_or_default()
2727
.join(format!("rgpt-debug-{ts}.log"));
28-
let file = File::create(&path)
28+
// The log captures full prompt/response content (including piped file
29+
// data); create it owner-only so it isn't world-readable.
30+
let file = crate::fsutil::create_private_file(&path)
2931
.with_context(|| format!("creating debug log file {}", path.display()))?;
3032
eprintln!("debug: logging to {}", path.display());
3133
Ok(Self {

src/fsutil.rs

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
use std::fs;
2+
use std::io::Write;
3+
use std::path::Path;
4+
5+
use anyhow::{Context, Result, bail};
6+
7+
/// Creates a directory (and any missing parents) that only the owner can
8+
/// access: mode 0700 on Unix, so other local users can't list saved chats,
9+
/// roles, cached responses, or the config that holds the API key. A no-op for
10+
/// permissions on non-Unix targets, which don't have this permission model.
11+
pub fn create_private_dir(path: &Path) -> Result<()> {
12+
fs::create_dir_all(path).with_context(|| format!("creating directory {}", path.display()))?;
13+
set_private_dir_permissions(path)
14+
}
15+
16+
/// Writes `contents` to `path` with owner-only permissions (mode 0600 on Unix).
17+
/// The file is created 0600 from the start — not created world-readable and
18+
/// then narrowed — so a secret (the API key) is never briefly exposed. An
19+
/// existing file is also re-narrowed, migrating configs written by older
20+
/// versions that predate this hardening.
21+
pub fn write_private(path: &Path, contents: &str) -> Result<()> {
22+
let mut options = fs::OpenOptions::new();
23+
options.write(true).create(true).truncate(true);
24+
#[cfg(unix)]
25+
{
26+
use std::os::unix::fs::OpenOptionsExt;
27+
options.mode(0o600);
28+
}
29+
let mut file = options
30+
.open(path)
31+
.with_context(|| format!("opening file {}", path.display()))?;
32+
file.write_all(contents.as_bytes())
33+
.with_context(|| format!("writing file {}", path.display()))?;
34+
// `mode()` only applies when the file is newly created; enforce 0600 on a
35+
// pre-existing (possibly world-readable) file too.
36+
set_private_file_permissions(path)
37+
}
38+
39+
/// Creates (truncating) a file for writing with owner-only permissions (mode
40+
/// 0600 on Unix) and returns the open handle, for callers that append to it
41+
/// over time rather than writing once. Like [`write_private`], the file is
42+
/// created 0600 from the start and any pre-existing file is re-narrowed.
43+
pub fn create_private_file(path: &Path) -> Result<fs::File> {
44+
let mut options = fs::OpenOptions::new();
45+
options.write(true).create(true).truncate(true);
46+
#[cfg(unix)]
47+
{
48+
use std::os::unix::fs::OpenOptionsExt;
49+
options.mode(0o600);
50+
}
51+
let file = options
52+
.open(path)
53+
.with_context(|| format!("creating file {}", path.display()))?;
54+
set_private_file_permissions(path)?;
55+
Ok(file)
56+
}
57+
58+
/// Rejects an identifier that is used verbatim as a filename under a storage
59+
/// directory. Chat ids and role names arrive from argv and are joined directly
60+
/// onto a base directory, so a value like `../../etc/passwd` or an absolute
61+
/// path would otherwise let a read/write/delete escape that directory.
62+
pub fn validate_identifier(kind: &str, name: &str) -> Result<()> {
63+
if name.is_empty() {
64+
bail!("{kind} name must not be empty");
65+
}
66+
if name == "." || name == ".." {
67+
bail!("{kind} name {name:?} is not allowed");
68+
}
69+
if name.contains('/') || name.contains('\\') || name.contains('\0') {
70+
bail!("{kind} name must not contain path separators or null bytes: {name:?}");
71+
}
72+
Ok(())
73+
}
74+
75+
#[cfg(unix)]
76+
fn set_private_file_permissions(path: &Path) -> Result<()> {
77+
use std::os::unix::fs::PermissionsExt;
78+
fs::set_permissions(path, fs::Permissions::from_mode(0o600))
79+
.with_context(|| format!("setting permissions on {}", path.display()))
80+
}
81+
82+
#[cfg(unix)]
83+
fn set_private_dir_permissions(path: &Path) -> Result<()> {
84+
use std::os::unix::fs::PermissionsExt;
85+
fs::set_permissions(path, fs::Permissions::from_mode(0o700))
86+
.with_context(|| format!("setting permissions on {}", path.display()))
87+
}
88+
89+
#[cfg(not(unix))]
90+
fn set_private_file_permissions(_path: &Path) -> Result<()> {
91+
Ok(())
92+
}
93+
94+
#[cfg(not(unix))]
95+
fn set_private_dir_permissions(_path: &Path) -> Result<()> {
96+
Ok(())
97+
}
98+
99+
#[cfg(test)]
100+
mod tests {
101+
use super::*;
102+
103+
#[test]
104+
fn rejects_traversal_and_separators() {
105+
for bad in ["", ".", "..", "../etc", "a/b", "a\\b", "x\0y", "/abs"] {
106+
assert!(validate_identifier("chat", bad).is_err(), "{bad:?}");
107+
}
108+
}
109+
110+
#[test]
111+
fn accepts_ordinary_names() {
112+
for ok in ["temp", "my-chat", "ShellGPT", "Shell Command Generator", "a..b"] {
113+
assert!(validate_identifier("chat", ok).is_ok(), "{ok:?}");
114+
}
115+
}
116+
117+
#[cfg(unix)]
118+
#[test]
119+
fn written_file_is_owner_only() {
120+
use std::os::unix::fs::PermissionsExt;
121+
let path = std::env::temp_dir().join(format!("rgpt-fsutil-{}", std::process::id()));
122+
write_private(&path, "secret").unwrap();
123+
let mode = std::fs::metadata(&path).unwrap().permissions().mode();
124+
assert_eq!(mode & 0o777, 0o600);
125+
std::fs::remove_file(&path).ok();
126+
}
127+
}

src/main.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ mod client;
55
mod config;
66
mod debug;
77
mod editor;
8+
mod fsutil;
89
mod handler;
910
mod render;
1011
mod role;

src/role.rs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ impl SystemRole {
5959
/// Safe to call on every startup.
6060
pub fn ensure_defaults(config: &Config) -> Result<()> {
6161
let dir = Self::storage_dir()?;
62-
fs::create_dir_all(&dir)
62+
crate::fsutil::create_private_dir(&dir)
6363
.with_context(|| format!("creating role directory {}", dir.display()))?;
6464

6565
let shell = shell_name(config);
@@ -83,6 +83,7 @@ impl SystemRole {
8383
}
8484

8585
pub fn get(name: &str) -> Result<Self> {
86+
crate::fsutil::validate_identifier("role", name)?;
8687
let path = Self::storage_dir()?.join(format!("{name}.json"));
8788
if !path.exists() {
8889
bail!("Role \"{name}\" not found.");
@@ -130,6 +131,7 @@ impl SystemRole {
130131
}
131132

132133
fn save(&self, confirm_overwrite: bool) -> Result<()> {
134+
crate::fsutil::validate_identifier("role", &self.name)?;
133135
let path = Self::storage_dir()?.join(format!("{}.json", self.name));
134136

135137
if confirm_overwrite && path.exists() {
@@ -148,7 +150,8 @@ impl SystemRole {
148150
}
149151

150152
let contents = serde_json::to_string(self).context("serializing role")?;
151-
fs::write(&path, contents).with_context(|| format!("writing role file {}", path.display()))
153+
crate::fsutil::write_private(&path, &contents)
154+
.with_context(|| format!("writing role file {}", path.display()))
152155
}
153156
}
154157

0 commit comments

Comments
 (0)