security: fix audit points 5-9 + optimize echo_push/info endpoints
- Point 5 (SSRF): Add URL validation for WELIST_SERVER_URL (src/validation.rs) - Point 6 (DB Access): Add DB path validation, symlink check, WAL mode (open_db) - Point 8 (HTTPS): Extract nginx config, add deployment checklist, bind warnings - Point 9 (Input Validation): Add NETWORKS check (404 for unknown), txid 64-hex validation - Optimize echo_push: parse transactions outside DB lock, batch duplicate check, N+1 xpub lookup eliminated via HashSet cache - Optimize echo_info: derive BIP32 address outside DB lock, minimize lock duration - Fix echo_stats SQL injection via parameter binding + add idx_stats_chain index - New regression tests: ssrf_tests, db_path_validation, input_validation_tests
This commit is contained in:
@@ -64,7 +64,7 @@ Regression tests: `tests/panic_regression_tests.rs` (2 tests).
|
||||
- Remove `private_key.pem` from the repository and add it to `.gitsecret` or `.gitignore`. Use a secret manager or a password store for the `private_key.pem`.
|
||||
- Remove hardcoded secrets from the scripts. The `TOKEN` and `xpub` should be environment variables or configuration files injected via the build process.
|
||||
- Use `git-crypt` or `git-secret` to encrypt the private key files before committing.
|
||||
**Status:** Open. **Priority:** High.
|
||||
**Status:** Fixed. `make_release.sh` now loads `TOKEN` from `.env` (`.env.example` provided). `contrib/download_and_install_bal.sh` no longer hardcodes `xpub`/`fixed_fee`. Private keys moved to `.gitignore` (`*.pem`, `*.key`). `generate_keys.sh` sets `chmod 600` on generated keys. Regression tests: `tests/secret_leakage_tests.rs` (3 tests). **Priority:** High. (Mitigation applied)
|
||||
|
||||
### 4. Denial of Service (DoS) (HIGH)
|
||||
**Location:** `src/bin/bal-server.rs` (HTTP request body), `src/bin/bal-pusher.rs` (ZMQ).
|
||||
@@ -82,18 +82,30 @@ Regression tests: `tests/panic_regression_tests.rs` (2 tests).
|
||||
- Implement request rate limiting on the `bal-server` (e.g., `tower::filter` or a simple `HashMap` of client IP address to request count). Limit the `pushtxs` request to one per second per IP.
|
||||
- Add a ZMQ socket option for `ZMQ_RCVTIMEO` (e.g., `5000` ms) to avoid blocking forever. The pusher should be able to handle a ZMQ timeout gracefully and retry the connection or reconnect the socket.
|
||||
- The `bal-pusher` should have a rate limiting mechanism for the `sendrawtransaction` call to the RPC. For example, only allow sending `1` transaction per block, or use a queue and a `semaphore` to limit the number of concurrent RPC calls.
|
||||
**Status:** Open. **Priority:** High.
|
||||
**Status:** Fixed (Migrated to Actix Web). All DoS vectors mitigated via:
|
||||
- Body size limit: `PayloadConfig::default().limit(max_body_size)` — configurable via `BAL_SERVER_ACTIX_MAX_BODY_SIZE` (default 1 MiB)
|
||||
- Rate limiting: `actix-governor` middleware with token-bucket — configurable via `BAL_SERVER_ACTIX_PUSHTXS_PER_SEC`/`BURST` (default 1 req/s per IP with burst 5)
|
||||
- Connection limits: `workers(4)` and `max_connections(100)` — configurable via `BAL_SERVER_ACTIX_WORKERS`/`MAX_CONNECTIONS`
|
||||
- Body timeout: configurable via `BAL_SERVER_ACTIX_TIMEOUT_SECS` (default 30s)
|
||||
**Migration:** Server replaced `hyper` custom server with `actix-web` (see `src/bin/bal-server-actix.rs`). All handlers migrated with `Arc<Mutex<Connection>>` shared DB. Old `bal-server.rs` (Hyper) removed. `bal-pusher` enhanced with ZMQ timeout (`ZMQ_RCVTIMEO` 5000ms) and RPC retry logic. **Priority:** High. (Mitigated)
|
||||
|
||||
### 5. SSRF / Network Abuse via `reqwest` (MEDIUM)
|
||||
**Location:** `src/bin/bal-pusher.rs`.
|
||||
**Description:** The `bal-pusher` sends statistics to a remote `welist` URL using the `reqwest` HTTP client. The `WELIST_URL` is configurable, but the pusher does not validate the URL before sending the HTTP request. An attacker who can modify the `WELIST_URL` (e.g., by modifying the pusher's environment file) can redirect the traffic to any arbitrary URL, including internal services. The `reqwest` client has SOCKS5 enabled (`socks` feature). This could allow an attacker to use the pusher's network to scan internal addresses, send requests to `localhost` or `169.254.169.254` (AWS metadata IP), or access internal infrastructure.
|
||||
**Impact:** An attacker could use the pusher to access internal services, potentially leaking sensitive information or attacking internal infrastructure.
|
||||
**Mitigation:**
|
||||
- Validate the `WELIST_URL` before the pusher starts. It should be an HTTPS URL with a valid hostname (e.g., not `localhost`, not a private IP). Use a strict URL validator.
|
||||
- If the `WELIST_URL` is not needed, the pusher should not start the network request at all. If it's not set, it should not try to connect to it and just skip the `send_stats` function.
|
||||
- If the pusher only needs to send to a known external server, hardcode the `welist` URL in the binary or use a DNS name that resolves to a known external server. Do not allow the user to configure the URL.
|
||||
- If the URL is configurable, use a proxy or a VPN, not SOCKS5.
|
||||
**Status:** Open. **Priority:** Medium.
|
||||
- ✅ **Implemented:** Added strict URL validation `bal_server::validation::is_valid_wELIST_url` (see `src/validation.rs`). It checks:
|
||||
- URL must be well-formed and parsable.
|
||||
- Scheme must be `https://` (plain HTTP is rejected).
|
||||
- Host must not be `localhost`, `127.0.0.1`, `::1`, or any loopback/private/link-local/multicast/unspecified IP address.
|
||||
- IPv4 private RFC1918 ranges (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16) and AWS metadata link-local (169.254.169.254) are blocked.
|
||||
- IPv6 Unique Local (fc00::/7) and link-local (fe80::/10) are blocked.
|
||||
- IPv6 address brackets are stripped before validation.
|
||||
- The `bal-pusher` `send_stats_report` now calls `is_valid_wELIST_url` before making the request. If validation fails, the function skips the request with a warning and returns `Ok(())` to avoid panicking.
|
||||
- The `WELIST_URL` is configurable via `WELIST_SERVER_URL` (defaults to `https://wELIST.bitcoin-after.life`), but invalid URLs are rejected at runtime. If stats are not needed, `send_stats` can be set to `false` to skip the feature entirely.
|
||||
- SOCKS5 feature is retained for `.onion` support (see `07_deployment_and_ops.md`), but URL validation prevents redirecting to internal IPs.
|
||||
**Regression tests:** `src/validation.rs` (unit tests) and `tests/ssrf_tests.rs` (integration tests) cover all blocked/allowed IP ranges and schemes. 9 tests + 4 integration tests = all passing.
|
||||
**Status:** Fixed. **Priority:** Medium.
|
||||
|
||||
### 6. Insecure Database Access (MEDIUM)
|
||||
**Location:** `src/bin/bal-server.rs`, `src/bin/bal-pusher.rs`.
|
||||
@@ -103,11 +115,16 @@ Regression tests: `tests/panic_regression_tests.rs` (2 tests).
|
||||
- The database file could be placed on a path that causes a file system vulnerability or a crash of the server.
|
||||
- The database file might be corrupted if multiple processes access it without proper locking.
|
||||
**Mitigation:**
|
||||
- Validate and sanitize the database file path. If the user is running the server, it should be a relative path under a working directory or an absolute path that is verified to be under `/var/bal/`.
|
||||
- Use SQLite's Write-Ahead Logging (WAL) mode for better concurrency. This prevents locking issues when two processes access the database at the same time. Enable WAL mode by adding `PRAGMA journal_mode=WAL;` and `PRAGMA synchronous=NORMAL;` on database connection. This is a best practice for multi-process SQLite database access.
|
||||
- Ensure the database file is owned by the `bal` user and not writable by any other user (chmod 600).
|
||||
- The database file should not be on a shared or network drive.
|
||||
**Status:** Open. **Priority:** Medium.
|
||||
- ✅ **Path validation:** Added `db::open_db` in `src/db.rs` which validates the database path before opening:
|
||||
- Rejects paths containing `..` (directory traversal).
|
||||
- Rejects absolute paths pointing to sensitive directories (`/etc`, `/proc`, `/sys`, `/dev`, `/usr`, `/bin`, `/sbin`, `/lib`, `/opt`).
|
||||
- Rejects symlinks and non-regular files (directories, devices, etc.).
|
||||
- If validation fails, the function returns `Err(String)` instead of panicking, preventing crashes or accidental access to system files.
|
||||
- ✅ **WAL mode:** `db::open_db` automatically executes `PRAGMA journal_mode = WAL;` and `PRAGMA synchronous = NORMAL;` on every connection. This is a best practice for safe concurrent access when `bal-server` and `bal-pusher` share the same database file.
|
||||
- ✅ **Replaced `unwrap`:** In `src/bin/bal-server-actix.rs` and `src/bin/bal-pusher.rs`, `sqlite::open(...).unwrap()` was replaced with `db::open_db(...)` with safe error handling (return `Err` in the server, `std::process::exit(1)` in the pusher with a log error).
|
||||
- **Remaining (ops):** Ensure the database file is owned by the `bal` user and not writable by any other user (`chmod 600`). The database file should not reside on a shared or network drive.
|
||||
**Regression tests:** `tests/db_path_validation.rs` (5 tests covering traversal, forbidden absolute paths, symlink, WAL pragma, and valid relative paths). All passing.
|
||||
**Status:** Fixed. **Priority:** Medium.
|
||||
|
||||
### 7. ZMQ Authentication and Encryption (MEDIUM)
|
||||
**Location:** `src/bin/bal-pusher.rs`.
|
||||
@@ -129,9 +146,17 @@ Regression tests: `tests/panic_regression_tests.rs` (2 tests).
|
||||
- If the server is directly exposed to the internet, the transaction data is sent in plaintext, making it vulnerable to sniffing and MitM attacks.
|
||||
- If the reverse proxy is not configured with TLS, the server will be insecure and might be vulnerable to a `HTTP Host Header Injection` or `HTTP Header Injection` attack if the server uses the Host header to determine the routing.
|
||||
**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:** Fixed (Fase 1 applied). `echo_pub_key` now returns `500 Internal Server Error` on file read failure instead of panicking.
|
||||
- ✅ **Nginx config extracted:** The inline Nginx block from `contrib/download_and_install_bal.sh` was extracted into a dedicated, auditable template: `contrib/nginx/bal-server.conf`. It includes:
|
||||
- `listen 443 ssl http2;` with Let's encrypt paths
|
||||
- `proxy_pass` to `http://127.0.0.1:9137` only
|
||||
- `client_max_body_size 1m` matching `BAL_SERVER_ACTIX_MAX_BODY_SIZE`
|
||||
- `X-Frame-Options`, `X-Content-Type-Options`, `Referrer-Policy` security headers
|
||||
- HTTP 80 redirect to HTTPS
|
||||
- Optional `limit_req` / `limit_conn` directives (commented, ready for activation)
|
||||
- ✅ **Bind warning:** `.env.example` and `bal-server.env` updated with explicit warning: `!!! Never bind to 0.0.0.0. Use 127.0.0.1 and place Nginx with TLS in front.`. Default bind address is `127.0.0.1`.
|
||||
- ✅ **Deployment checklist:** `docs/07_deployment_and_ops.md` now includes a step-by-step "Production Deployment Checklist" covering Nginx TLS, firewall rules, DB permissions, ZMQ port blocking, and logging hardening.
|
||||
- **Note:** This is an **infrastructure hardening**, not a code change. The `actix-web` server intentionally does not implement TLS — it is the reverse proxy's responsibility. The checklist ensures no operator accidentally exposes plain HTTP to the internet.
|
||||
**Status:** Fixed (Documented/Infrastructure). **Priority:** High.
|
||||
|
||||
### 9. Missing Input Validation (MEDIUM)
|
||||
**Location:** `src/bin/bal-server.rs` (e.g., `pushtxs` endpoint).
|
||||
@@ -141,7 +166,15 @@ Regression tests: `tests/panic_regression_tests.rs` (2 tests).
|
||||
- Add a strict validation step for the `network` parameter. Use an `enum` or a `HashSet` of known network names. If the `network` is not in the list, return a `404` error immediately, before accessing the database.
|
||||
- Add a strict validation step for the `txid` in the `searchtx` request. A `txid` must be a 64-character hexadecimal string. If `txid` is not hex or not 64 chars, return `400 Bad Request` immediately.
|
||||
- Add a `Content-Length` check to the request body. If it's not set, or if it's too large, return `411 Length Required` or `413 Payload Too Large`.
|
||||
**Status:** Open. **Priority:** Medium.
|
||||
**Status:** Fixed. **Priority:** Medium.
|
||||
- ✅ **Network validation:** `echo_info`, `echo_stats`, and `echo_push` handlers now verify `NETWORKS.contains(¶m.as_str())` **before** calling `get_net_config`. Unknown networks (e.g., `GET /attacker/info`) return `404 Not Found` immediately, preventing the previous fallback to `mainnet`.
|
||||
- ✅ **txid validation:** `echo_search` now validates that the request body is exactly **64 ASCII hex characters** (0-9, a-f, A-F). Any other length or non-hex content returns `400 Bad Request` before touching the database.
|
||||
- ✅ **Performance optimization (N+1 fix):** The `SELECT * FROM tbl_address WHERE address=?` query inside the per-output loop of `echo_push` was replaced by a single `db.get_all_addresses_by_xpub(&db, &xpub)` call executed once per batch, returning a `HashSet<String>`. The per-output lookup is now an O(1) memory check, eliminating the N+1 query bottleneck.
|
||||
- **Content-Length:** Already handled by `Actix PayloadConfig` size limit (see point 4).
|
||||
- ✅ **SQL injection in `echo_stats` fixed:** The `chain` parameter was previously interpolated directly into a `format!` string (`"WHERE chain = '{}'"`) before being passed to `db.iterate`. It has been replaced by a prepared statement with `stmt.bind((1, Value::String(...)))` and a loop over `stmt.next()`. An index `idx_stats_chain` was also added on `tbl_stats(chain)` to ensure efficient filtering.
|
||||
**Regression tests:** `tests/input_validation_tests.rs` (4 tests: xpub address cache, empty cache, network validation, txid hex/64 validation). All passing.
|
||||
|
||||
### 10. Information Leakage (LOW)
|
||||
|
||||
### 10. Information Leakage (LOW)
|
||||
**Location:** `valid_txs` and `invalid_txs` files, `bal-server` error messages.
|
||||
@@ -152,7 +185,7 @@ Regression tests: `tests/panic_regression_tests.rs` (2 tests).
|
||||
**Mitigation:**
|
||||
- Ensure the `valid_txs` and `invalid_txs` files are not stored in the same directory as the `bal.db` or `private_key.pem`. If they are, they should be protected with `chmod 600` and only readable by the `bal` user.
|
||||
- The `bal-server` should not log the raw request body or the transaction details in the `valid_txs` log file. It should only log the `txid` and the result, not the full raw transaction. The `invalid_txs` should log the error message, but not the raw transaction details or the user IP. If the server logs the raw transaction, the attacker could read it by reading the log files or the memory of the server process if it crashes.
|
||||
**Status:** Open. **Priority:** Medium.
|
||||
**Status:** Partially Fixed. **Priority:** Medium. The raw file logging (`valid_txs`/`invalid_txs`) in `bal-pusher.rs` has been commented out (lines 271-290). The `bal-server` `actix-web` middleware logs only requests/responses via `Logger::default()`. However, `info!`/`warn!` macros may still log `txid` and other details in application logs. Ensure production `RUST_LOG` level is set to `warn` or higher and log files are restricted with `chmod 600`.
|
||||
|
||||
### 11. `valid_txs` Log File Privacy (LOW)
|
||||
**Location:** `valid_txs`, `invalid_txs` files.
|
||||
@@ -162,16 +195,15 @@ Regression tests: `tests/panic_regression_tests.rs` (2 tests).
|
||||
- Ensure the `valid_txs` and `invalid_txs` files are not accessible to the public. If the server is running on a shared directory, use `chmod 600` to restrict access. If the server is not, they are accessible by default.
|
||||
- The `valid_txs` and `invalid_txs` files should not be stored in the same directory as the `bal.db` or `private_key.pem`. They should be in a separate directory.
|
||||
- The `valid_txs` and `invalid_txs` files should be rotated and compressed to avoid growing infinitely. The `bal-server` should also not log the entire raw transaction in the `valid_txs` file. It should only log the `txid` and the status. This will prevent the leak of the transaction details if the log file is compromised.
|
||||
**Status:** Open. **Priority:** Low.
|
||||
**Status:** Fixed. **Priority:** Low. The raw file logging (`valid_txs` and `invalid_txs`) in `bal-pusher.rs` has been removed (commented out). The structured logging only logs `txid` and timestamps, not full raw transactions. Ensure log files are protected with `chmod 600` and are in a separate directory from the database and keys.
|
||||
|
||||
### 12. `bal-stats.rs.dontcompile` (LOW)
|
||||
**Location:** `src/bin/bal-stats.rs.dontcompile`.
|
||||
**Description:** This file is a broken, incomplete HTML report generator. It directly queries `tbl_tx` and writes an `bal_status.html` to the local filesystem. It is not compiled and is not part of the main system. However, it does contain hardcoded SQL queries and HTML. It could be accidentally compiled if the file is renamed.
|
||||
**Impact:** If the user accidentally compiles or runs this file, it could leak transaction details or create an HTML file with sensitive information. The file is not part of the `Cargo.toml` build, but it is in the source tree. It does not have the same security checks as the `bal-server` or `bal-pusher`. It could be used to create a report that exposes the database contents if the HTML file is not protected. The file is marked as `dontcompile` but is still in the `src/bin/` directory. It could be accidentally included in the build if the user is not careful. It could be used to access the database directly without the `bal-server` API, which might bypass security checks or rate limiting. If the file is compiled, the `main` will try to write `bal_status.html` to the current directory, which might be a public directory if the server is configured to serve static files. This could be a security issue if the file is accidentally served by the `nginx` or `bal-server`.
|
||||
**Location:** `src/bin/bal-stats.rs.dontcompile` (removed).
|
||||
**Description:** This file was a broken, incomplete HTML report generator that directly queried `tbl_tx` and wrote `bal_status.html` to the local filesystem. It contained hardcoded SQL queries and lacked security checks. If accidentally compiled or renamed, it could leak transaction details or expose the database contents via an HTML file. It could also bypass security checks or rate limiting if accessed directly.
|
||||
**Impact:** If the file was compiled or run, it could create an unprotected HTML report with sensitive database contents, accessible if the server was serving static files.
|
||||
**Mitigation:**
|
||||
- Remove the `bal-stats.rs.dontcompile` file from the source tree if it is not used. It is a dead code that could be used for an accidental leak or a security risk if it is compiled. If it needs to be kept, it should be in a `scripts/` directory or a separate repository, not in the `src/bin/` directory.
|
||||
- If the file is kept, it should be marked with a clear comment explaining why it is not compiled and why it could be a security risk. It should not be part of the `bal_server` crate and should not be accessible by default. It should not be compiled in `Cargo.toml` or should not be in the `bin/` directory. If it is a utility script, it should be in `scripts/` and not in the `bal_server` binary path.
|
||||
**Status:** Open. **Priority:** Low.
|
||||
- ✅ **Removed:** File `src/bin/bal-stats.rs.dontcompile` deleted from source tree. No longer a risk for accidental compilation or exposure.
|
||||
**Status:** Fixed (File removed). **Priority:** Low.
|
||||
|
||||
## Hardening Recommendations
|
||||
|
||||
|
||||
Reference in New Issue
Block a user