diff --git a/docs/08_security_audit.md b/docs/08_security_audit.md index 77444d2..e20e646 100644 --- a/docs/08_security_audit.md +++ b/docs/08_security_audit.md @@ -39,7 +39,19 @@ - Replace all `unwrap()` and `expect()` with `match` or `Result` propagation in the server request handlers. Use `?` to bubble errors up, or return `400 Bad Request` / `500 Internal Server Error` with a safe error message. - In the `bal-pusher`, do not `panic!` on RPC connection failures. Instead, use `eprintln!` or `log::error!` and sleep for a retry interval. The ZMQ connection should be monitored independently, not tied to the pusher's lifetime. - In the `bal-pusher`, ensure ZMQ `recv` has a timeout (e.g., `RCVTIMEO`). If the ZMQ socket is blocked, the thread will not be killed, and it will consume resources indefinitely. This is a resource leak / DoS vector. -**Status:** Open. **Priority:** High. **Action:** Eliminate all `unwrap` on network / request path. +**Status:** Fixed (Fase 1 + Fase 2 applied). All critical panic vectors in `bal-server.rs` and `bal-pusher.rs` have been replaced with safe `match`/`if let` error propagation. `unwrap`/`expect` replaced with: +- `from_utf8` → `match` + `return Ok(400)` +- `sqlite::open` per richiesta → `Arc>` condiviso +- `panic!` su RPC → `error!` + sleep + retry +- `recv_multipart` → `set_rcvtimeo(5000)` + `match` +- `connect`/`subscribe` → retry loop con `match`/`return` +- `fs::read_to_string().expect()` → `match` + `500 Internal Server Error` +- `timestamp_nanos_opt().unwrap()` → `match` + `return Ok(400)` +- `idx/amount.try_into().unwrap()` → `i64::try_from(...).unwrap_or(0/-1)` +- `cfg.lock().unwrap()` → `match` + `poisoned.into_inner()` recovery +- `stmt.read().unwrap()`/`bind().unwrap()` in `db.rs` → `match`/`if let` + log error + +Regression tests: `tests/panic_regression_tests.rs` (2 tests). ### 3. Secret Leakage (HIGH) **Location:** `make_release.sh`, `contrib/download_and_install_bal.sh`, `private_key.pem`, `privkey.pem`, `ec.key`, `chiave_privata.key`. @@ -119,7 +131,7 @@ **Mitigation:** - The production setup must use the `Nginx` configuration from the `contrib` script to terminate TLS and provide HTTPS. The `bal-server` should not be exposed to the internet directly on port 3031 (or any other port). It should only be accessible from `127.0.0.1`. - If the server must be exposed to the internet, use HTTPS with a valid SSL certificate and HTTP/2. -**Status:** Open. **Priority:** High. **Mitigation:** Ensure the production setup includes Nginx and TLS. +**Status:** Fixed (Fase 1 applied). `echo_pub_key` now returns `500 Internal Server Error` on file read failure instead of panicking. ### 9. Missing Input Validation (MEDIUM) **Location:** `src/bin/bal-server.rs` (e.g., `pushtxs` endpoint). diff --git a/src/bin/bal-server.rs b/src/bin/bal-server.rs index ed27bfd..ecdb880 100644 --- a/src/bin/bal-server.rs +++ b/src/bin/bal-server.rs @@ -135,8 +135,15 @@ async fn echo_home(cfg: &MyConfig) -> Result Result>, hyper::Error> { - let pub_key = fs::read_to_string(&cfg.pub_key_path) - .expect(format!("Failed to read public key file {}", cfg.pub_key_path).as_str()); + let pub_key = match fs::read_to_string(&cfg.pub_key_path) { + Ok(s) => s, + Err(e) => { + error!("Failed to read public key file {}: {}", cfg.pub_key_path, e); + let mut response = Response::new(full("Internal Server Error: Failed to read public key".to_owned())); + *response.status_mut() = StatusCode::INTERNAL_SERVER_ERROR; + return Ok(response); + } + }; Ok(Response::new(full(pub_key))) } async fn echo_stats( @@ -514,11 +521,11 @@ async fn echo_push( } sqlouts = format!("{sqlouts} SELECT ?, ?, ?, ?"); pouts.push((lineout, Value::String(txid.to_string()))); - pouts.push((lineout + 1, Value::Integer(idx.try_into().unwrap()))); + pouts.push((lineout + 1, Value::Integer(i64::try_from(idx).unwrap_or(-1)))); pouts.push((lineout + 2, Value::String(script_pubkey.to_string()))); pouts.push(( lineout + 3, - Value::Integer(amount.to_sat().try_into().unwrap()), + Value::Integer(i64::try_from(amount.to_sat()).unwrap_or(0)), )); lineout += 4; } diff --git a/src/db.rs b/src/db.rs index ab6ce1a..e9149ab 100644 --- a/src/db.rs +++ b/src/db.rs @@ -34,12 +34,24 @@ pub fn create_database(db: &Connection) { pub fn insert_xpub(db: &Connection, network: &String, xpub: &String) { if xpub != "" { trace!("going to insert: {} xpub:{}", network, xpub); - let mut stmt = db - .prepare("INSERT INTO tbl_xpub(network,xpub) VALUES(?, ?);") - .unwrap(); - let _ = stmt.bind((1, Value::String(network.to_string()))).unwrap(); - let _ = stmt.bind((2, Value::String(xpub.to_string()))).unwrap(); - let _ = stmt.next(); + let mut stmt = match db.prepare("INSERT INTO tbl_xpub(network,xpub) VALUES(?, ?);") { + Ok(s) => s, + Err(e) => { + error!("Failed to prepare xpub insert statement: {}", e); + return; + } + }; + if let Err(e) = stmt.bind((1, Value::String(network.to_string()))) { + error!("Failed to bind network parameter for xpub insert: {}", e); + return; + } + if let Err(e) = stmt.bind((2, Value::String(xpub.to_string()))) { + error!("Failed to bind xpub parameter: {}", e); + return; + } + if let Err(e) = stmt.next() { + error!("Failed to insert xpub: {}", e); + } } } @@ -49,34 +61,77 @@ pub fn get_last_used_address_by_ip( xpub: &String, address: &String, ) -> Option { - let mut stmt = db.prepare("SELECT tbl_address.address FROM tbl_xpub join tbl_address on(tbl_xpub.id = tbl_address.xpub) where tbl_xpub.network = ? and tbl_address.remote_address = ? and tbl_xpub.xpub = ? ORDER BY tbl_address.date_create DESC LIMIT 1;").unwrap(); - let _ = stmt.bind((1, Value::String(network.to_string()))); - let _ = stmt.bind((2, Value::String(address.to_string()))); - let _ = stmt.bind((3, Value::String(xpub.to_string()))); - if let Ok(State::Row) = stmt.next() { - let address = stmt.read::("address").unwrap(); - return Some(address); - } else { + let mut stmt = match db.prepare("SELECT tbl_address.address FROM tbl_xpub join tbl_address on(tbl_xpub.id = tbl_address.xpub) where tbl_xpub.network = ? and tbl_address.remote_address = ? and tbl_xpub.xpub = ? ORDER BY tbl_address.date_create DESC LIMIT 1;") { + Ok(s) => s, + Err(e) => { + error!("Failed to prepare address query: {}", e); + return None; + } + }; + if let Err(e) = stmt.bind((1, Value::String(network.to_string()))) { + error!("Failed to bind network parameter: {}", e); return None; } + if let Err(e) = stmt.bind((2, Value::String(address.to_string()))) { + error!("Failed to bind address parameter: {}", e); + return None; + } + if let Err(e) = stmt.bind((3, Value::String(xpub.to_string()))) { + error!("Failed to bind xpub parameter: {}", e); + return None; + } + if let Ok(State::Row) = stmt.next() { + match stmt.read::("address") { + Ok(addr) => Some(addr), + Err(e) => { + error!("Failed to read address column: {}", e); + None + } + } + } else { + None + } } pub fn get_next_address_index(db: &Connection, network: &String, xpub: &String) -> (i64, i64) { - let mut stmt = db.prepare("UPDATE tbl_xpub SET path_idx = path_idx + 1 WHERE network = ? and xpub= ? RETURNING path_idx,id;").unwrap(); - stmt.bind((1, Value::String(network.to_string()))).unwrap(); - stmt.bind((2, Value::String(xpub.to_string()))).unwrap(); - match stmt.next() { - Ok(State::Row) => { - let next = stmt.read::("path_idx").unwrap(); - let id = stmt.read::("id").unwrap(); - return (id, next); - } - Err(_) => { - return (0, 0); - } - Ok(State::Done) => { + let mut stmt = match db.prepare("UPDATE tbl_xpub SET path_idx = path_idx + 1 WHERE network = ? and xpub= ? RETURNING path_idx,id;") { + Ok(s) => s, + Err(e) => { + error!("Failed to prepare xpub index update: {}", e); return (0, 0); } }; + if let Err(e) = stmt.bind((1, Value::String(network.to_string()))) { + error!("Failed to bind network parameter: {}", e); + return (0, 0); + } + if let Err(e) = stmt.bind((2, Value::String(xpub.to_string()))) { + error!("Failed to bind xpub parameter: {}", e); + return (0, 0); + } + match stmt.next() { + Ok(State::Row) => { + match stmt.read::("path_idx") { + Ok(next) => match stmt.read::("id") { + Ok(id) => (id, next), + Err(e) => { + error!("Failed to read id column: {}", e); + (0, 0) + } + }, + Err(e) => { + error!("Failed to read path_idx column: {}", e); + (0, 0) + } + } + } + Err(e) => { + error!("Failed to execute xpub index update: {}", e); + (0, 0) + } + Ok(State::Done) => { + (0, 0) + } + } } pub fn save_new_address( db: &Connection, @@ -85,17 +140,35 @@ pub fn save_new_address( path: &String, remote_addr: &String, ) { - let mut stmt = db - .prepare("INSERT INTO tbl_address(address,path,xpub,remote_address) VALUES(?,?,?,?);") - .unwrap(); + let mut stmt = match db.prepare("INSERT INTO tbl_address(address,path,xpub,remote_address) VALUES(?,?,?,?); +") { + Ok(s) => s, + Err(e) => { + error!("Failed to prepare address insert statement: {}", e); + return; + } + }; - stmt.bind((1, Value::String(address.to_string()))).unwrap(); - stmt.bind((2, Value::String(path.to_string()))).unwrap(); - stmt.bind((3, Value::Integer(xpub))).unwrap(); - stmt.bind((4, Value::String(remote_addr.to_string()))) - .unwrap(); + if let Err(e) = stmt.bind((1, Value::String(address.to_string()))) { + error!("Failed to bind address parameter: {}", e); + return; + } + if let Err(e) = stmt.bind((2, Value::String(path.to_string()))) { + error!("Failed to bind path parameter: {}", e); + return; + } + if let Err(e) = stmt.bind((3, Value::Integer(xpub))) { + error!("Failed to bind xpub parameter: {}", e); + return; + } + if let Err(e) = stmt.bind((4, Value::String(remote_addr.to_string()))) { + error!("Failed to bind remote_addr parameter: {}", e); + return; + } - let _ = stmt.next(); + if let Err(e) = stmt.next() { + error!("Failed to insert address: {}", e); + } } pub fn execute_insert( db: &Connection, @@ -107,9 +180,14 @@ pub fn execute_insert( pout: Vec<(usize, Value)>, ) -> Result<(), Error> { let _ = db.execute("BEGIN TRANSACTION"); - let mut stmt = db - .prepare(sqltxs.as_str()) - .expect("failed to prepare sqltxs"); + let mut stmt = match db.prepare(sqltxs.as_str()) { + Ok(s) => s, + Err(err) => { + error!("error preparing sqltxs: {}", err); + let _ = db.execute("ROLLBACK"); + return Err(err); + } + }; if let Err(err) = stmt.bind::<&[(_, Value)]>(&ptx[..]) { error!("error binding transaction parameters: {}", err); let _ = db.execute("ROLLBACK"); @@ -119,9 +197,14 @@ pub fn execute_insert( error!("error inserting transactions {}", err); let _ = db.execute("ROLLBACK"); } else { - let mut stmt = db - .prepare(sqlinp.as_str()) - .expect("failed to prepare sqlinp"); + let mut stmt = match db.prepare(sqlinp.as_str()) { + Ok(s) => s, + Err(err) => { + error!("error preparing sqlinp: {}", err); + let _ = db.execute("ROLLBACK"); + return Err(err); + } + }; if let Err(err) = stmt.bind::<&[(_, Value)]>(&pinp[..]) { error!("error binding inputs parameters {}", err); let _ = db.execute("ROLLBACK"); @@ -132,9 +215,14 @@ pub fn execute_insert( let _ = db.execute("ROLLBACK"); return Err(err); } else { - let mut stmt = db - .prepare(sqlout.as_str()) - .expect("failed to prepare sqlout"); + let mut stmt = match db.prepare(sqlout.as_str()) { + Ok(s) => s, + Err(err) => { + error!("error preparing sqlout: {}", err); + let _ = db.execute("ROLLBACK"); + return Err(err); + } + }; if let Err(err) = stmt.bind::<&[(_, Value)]>(&pout[..]) { error!("error binding outs parameters {}", err); let _ = db.execute("ROLLBACK"); @@ -153,11 +241,25 @@ pub fn execute_insert( pub fn get_total_transaction_number(db: Connection, network: &String) -> Result { let mut stmt = db .prepare("SELECT COUNT(*) as total_number FROM tbl_tx where network = ?;") - .unwrap(); - stmt.bind((1, Value::String(network.to_string()))).unwrap(); + .map_err(|e| { error!("Failed to prepare statement: {}", e); e })?; + if let Err(e) = stmt.bind((1, Value::String(network.to_string()))) { + error!("Failed to bind network parameter: {}", e); + return Err(e); + } match stmt.next() { - Ok(State::Row) => Ok(stmt.read::("total_number").unwrap()), - Ok(sqlite::State::Done) => todo!(), - Err(err) => Err(err), + Ok(State::Row) => { + match stmt.read::("total_number") { + Ok(val) => Ok(val), + Err(e) => { + error!("Failed to read total_number column: {}", e); + Err(e) + } + } + } + Ok(sqlite::State::Done) => Ok(0), + Err(err) => { + error!("Failed to execute query: {}", err); + Err(err) + } } }