Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changelog/reject-path-keystore-account-names.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
cast: patch
---

Rejected keystore account names that escape the keystore directory.
20 changes: 20 additions & 0 deletions crates/cast/src/cmd/wallet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1003,6 +1003,7 @@ flag to set your key via:
args.run()?;
}
Self::Remove { name, dir, unsafe_password } => {
ensure_account_name_available(&name)?;
let dir = if let Some(path) = dir {
Path::new(&path).to_path_buf()
} else {
Expand Down Expand Up @@ -1089,6 +1090,7 @@ flag to set your key via:
}
}
Self::DecryptKeystore { account_name, keystore_dir, unsafe_password } => {
ensure_account_name_available(&account_name)?;
// Set up keystore directory
let dir = if let Some(path) = keystore_dir {
Path::new(&path).to_path_buf()
Expand Down Expand Up @@ -1131,6 +1133,7 @@ flag to set your key via:
unsafe_password,
unsafe_new_password,
} => {
ensure_account_name_available(&account_name)?;
// Set up keystore directory
let dir = if let Some(path) = keystore_dir {
Path::new(&path).to_path_buf()
Expand Down Expand Up @@ -1321,6 +1324,10 @@ fn ensure_touch_id_available(touch_id: bool) -> Result<()> {
const TOUCH_ID_SIDECAR_SUFFIX: &str = ".touchid";

fn ensure_account_name_available(name: &str) -> Result<()> {
let file_name = Path::new(name).file_name().and_then(|s| s.to_str());
if name.is_empty() || name.contains('\\') || file_name != Some(name) {
eyre::bail!("account name must be a single path segment");
}
if name.ends_with(TOUCH_ID_SIDECAR_SUFFIX) {
eyre::bail!("account names ending in `{TOUCH_ID_SIDECAR_SUFFIX}` are reserved");
}
Expand Down Expand Up @@ -2325,4 +2332,17 @@ mod tests {
"expected error when both --nonce and --self-broadcast are provided"
);
}

#[test]
fn rejects_path_keystore_account_name() {
assert!(ensure_account_name_available("dev").is_ok());
assert!(ensure_account_name_available("testAccount").is_ok());
assert!(ensure_account_name_available("../pwned").is_err());
assert!(ensure_account_name_available("nested/alias").is_err());
assert!(ensure_account_name_available("foo/../bar").is_err());
assert!(ensure_account_name_available("..").is_err());
assert!(ensure_account_name_available(".").is_err());
assert!(ensure_account_name_available("").is_err());
assert!(ensure_account_name_available("foo\\bar").is_err());
}
}
30 changes: 30 additions & 0 deletions crates/cast/tests/cli/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1254,6 +1254,36 @@ Error: account names ending in `.touchid` are reserved
"#]]);
});

// `cast wallet import` treats ACCOUNT_NAME as a file name under the keystore dir.
// A path segment would write the encrypted keystore outside that directory.
casttest!(wallet_import_rejects_path_account_name, |prj, cmd| {
let keystore_dir = prj.root().join("keystore");
fs::create_dir_all(&keystore_dir).unwrap();
let escaped = prj.root().join("pwned_foundry_alias");

cmd.set_current_dir(prj.root());
cmd.args([
"wallet",
"import",
"../pwned_foundry_alias",
"--private-key",
"0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80",
"--keystore-dir",
keystore_dir.to_str().unwrap(),
"--unsafe-password",
"test",
])
.assert_failure()
.stdout_eq(str![""])
.stderr_eq(str![[r#"
Error: account name must be a single path segment

"#]]);

assert!(!escaped.exists());
assert!(!keystore_dir.join("../pwned_foundry_alias").exists());
});

// tests that `cast wallet sign message` outputs the expected signature
casttest!(wallet_sign_message_utf8_data, |_prj, cmd| {
let pk = "0x0000000000000000000000000000000000000000000000000000000000000001";
Expand Down
Loading