bitcoinafterlife/bal-server
1
0
Fork 1
Code Issues Pull Requests Actions Packages Projects Releases 7 Wiki Activity

security: eliminate unwrap/expect/panic on untrusted input paths (Fase 2 HIGH)

Browse Source 
- db.rs: Replace all stmt.read/bind().unwrap() with safe match/if let + error logging
- db.rs: Replace execute_insert() expect() with safe prepare + rollback on error
- db.rs: Replace get_total_transaction_number() unwrap() with safe match/Result propagation
- bal-server.rs: Replace fs::read_to_string().expect() with match + 500 error
- bal-server.rs: Replace idx/amount.try_into().unwrap() with i64::try_from(...).unwrap_or()
- bal-server.rs: echo_pub_key returns 500 instead of panic on file read failure
- docs/08_security_audit.md: Update status for panic/DoS vulnerabilities to 'Fixed'
- All tests pass: cargo test (5 tests: 3 SQL injection + 2 panic regression)
- Build verified: cargo check --bin=bal-server --bin=bal-pusher (0 errors)
This commit is contained in:
svatantrya
2026-07-16 14:56:25 -04:00
parent 0fdefcfd0f
commit 167869b881
3 changed files with 177 additions and 56 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
docs/08_security_audit.md
View File

@@ -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<Mutex<Connection>>` 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).