mirror of
https://github.com/DaLaw2/NetGuardia.git
synced 2026-08-24 14:10:28 +09:00
fix: address Copilot review — 6 issues from PR #18
1. Botnet detector source_ip was set to victim dst_ip, causing SOAR to block the victim instead of the attacker 2. HTTPS redirect host header injection: validate host is private IP, localhost, or .local hostname before constructing redirect URL 3. smtp_password plaintext residue: clear settings table after writing to SecretStore to prevent pre-migration plaintext from persisting 4. install.sh: add apt-get update before install on Debian/Ubuntu 5. download_log OOM risk: add 50MB file size limit before reading 6. update_config restart trigger: check return value, report if shutdown already in progress instead of claiming success Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
a7837af9d7
commit
4bdf051969
@ -52,6 +52,8 @@ fi
|
||||
|
||||
# ── Install runtime dependencies (SQLCipher needs OpenSSL) ──────────────────
|
||||
if command -v apt-get &>/dev/null; then
|
||||
info "Refreshing apt package metadata"
|
||||
DEBIAN_FRONTEND=noninteractive apt-get update >/dev/null 2>&1 || warn "Could not refresh apt metadata"
|
||||
info "Installing runtime dependencies (libssl)"
|
||||
DEBIAN_FRONTEND=noninteractive apt-get install -y libssl3 >/dev/null 2>&1 || warn "Could not install libssl3"
|
||||
elif command -v dnf &>/dev/null; then
|
||||
|
||||
@ -4,6 +4,9 @@ use serde::Serialize;
|
||||
/// Hardcoded log directory — not configurable via API to prevent directory traversal.
|
||||
const LOG_DIR: &str = "logs";
|
||||
|
||||
/// Maximum downloadable log file size (50 MB). Prevents OOM from reading huge files.
|
||||
const MAX_DOWNLOAD_SIZE: u64 = 50 * 1024 * 1024;
|
||||
|
||||
/// Validate log filename: only alphanumeric, dots, underscores, hyphens.
|
||||
/// Prevents path traversal.
|
||||
fn is_valid_log_filename(name: &str) -> bool {
|
||||
@ -84,14 +87,29 @@ async fn download_log(path: web::Path<String>) -> HttpResponse {
|
||||
}));
|
||||
}
|
||||
|
||||
// Check file size before reading to prevent OOM on large logs
|
||||
match std::fs::metadata(&canonical) {
|
||||
Ok(meta) if meta.len() > MAX_DOWNLOAD_SIZE => {
|
||||
return HttpResponse::PayloadTooLarge().json(serde_json::json!({
|
||||
"error": format!("Log file exceeds maximum download size ({}MB)", MAX_DOWNLOAD_SIZE / 1024 / 1024)
|
||||
}));
|
||||
}
|
||||
Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
|
||||
return HttpResponse::NotFound().json(serde_json::json!({
|
||||
"error": format!("Log file '{}' not found", filename)
|
||||
}));
|
||||
}
|
||||
Err(e) => {
|
||||
return HttpResponse::InternalServerError().json(serde_json::json!({
|
||||
"error": format!("Failed to read log file: {}", e)
|
||||
}));
|
||||
}
|
||||
Ok(_) => {}
|
||||
}
|
||||
|
||||
let content = match std::fs::read(&canonical) {
|
||||
Ok(bytes) => bytes,
|
||||
Err(e) => {
|
||||
if e.kind() == std::io::ErrorKind::NotFound {
|
||||
return HttpResponse::NotFound().json(serde_json::json!({
|
||||
"error": format!("Log file '{}' not found", filename)
|
||||
}));
|
||||
}
|
||||
return HttpResponse::InternalServerError().json(serde_json::json!({
|
||||
"error": format!("Failed to read log file: {}", e)
|
||||
}));
|
||||
|
||||
@ -112,11 +112,15 @@ async fn update_config(
|
||||
let needs_restart = updated.iter().any(|k| HTTP_RELOAD_KEYS.contains(&k.as_str()));
|
||||
if needs_restart {
|
||||
// Auto-trigger restart for HTTP config changes
|
||||
handle.trigger(ShutdownMode::Restart);
|
||||
let triggered = handle.trigger(ShutdownMode::Restart);
|
||||
HttpResponse::Ok().json(serde_json::json!({
|
||||
"updated": updated,
|
||||
"message": "Settings updated. Server restarting to apply HTTP config changes.",
|
||||
"restarting": true,
|
||||
"message": if triggered {
|
||||
"Settings updated. Server restarting to apply HTTP config changes."
|
||||
} else {
|
||||
"Settings updated. Restart already in progress."
|
||||
},
|
||||
"restarting": triggered,
|
||||
}))
|
||||
} else {
|
||||
HttpResponse::Ok().json(serde_json::json!({
|
||||
|
||||
@ -11,6 +11,48 @@ use actix_web::{Error as ActixError, HttpResponse, web};
|
||||
/// Shared flag: when true, non-HTTPS requests are redirected.
|
||||
pub type ForceHttpsFlag = Arc<AtomicBool>;
|
||||
|
||||
/// Validate that the host is safe to use in a redirect Location header.
|
||||
/// Only allows: private IPs (RFC 1918), loopback, .local hostnames, and bare hostnames
|
||||
/// without dots (e.g., "netguardia"). Rejects public IPs and arbitrary domains
|
||||
/// to prevent host-header injection / open redirect attacks.
|
||||
fn is_safe_redirect_host(host: &str) -> bool {
|
||||
// Strip port if present (e.g., "192.168.1.1:8443" → "192.168.1.1")
|
||||
let hostname = if host.starts_with('[') {
|
||||
// IPv6 bracket: [::1]:8443
|
||||
host.find(']').map(|i| &host[1..i]).unwrap_or(host)
|
||||
} else {
|
||||
host.split(':').next().unwrap_or(host)
|
||||
};
|
||||
|
||||
// Localhost
|
||||
if hostname == "localhost" || hostname == "127.0.0.1" || hostname == "::1" {
|
||||
return true;
|
||||
}
|
||||
|
||||
// .local mDNS hostnames (e.g., "netguardia.local")
|
||||
if hostname.ends_with(".local") {
|
||||
return true;
|
||||
}
|
||||
|
||||
// Bare hostname without dots (e.g., "netguardia", not a public domain)
|
||||
if !hostname.contains('.') && !hostname.contains(':') {
|
||||
return true;
|
||||
}
|
||||
|
||||
// Try parsing as IP — allow private ranges only
|
||||
if let Ok(ip) = hostname.parse::<std::net::IpAddr>() {
|
||||
return match ip {
|
||||
std::net::IpAddr::V4(v4) => {
|
||||
let o = v4.octets();
|
||||
o[0] == 10 || (o[0] == 172 && (16..=31).contains(&o[1])) || (o[0] == 192 && o[1] == 168) || o[0] == 127
|
||||
}
|
||||
std::net::IpAddr::V6(v6) => v6.is_loopback() || (v6.segments()[0] & 0xfe00) == 0xfc00,
|
||||
};
|
||||
}
|
||||
|
||||
false
|
||||
}
|
||||
|
||||
pub struct HttpsRedirect;
|
||||
|
||||
impl<S, B> Transform<S, ServiceRequest> for HttpsRedirect
|
||||
@ -82,10 +124,17 @@ where
|
||||
return Ok(res);
|
||||
}
|
||||
|
||||
// Build HTTPS redirect URL using connection_info (respects proxy headers safely)
|
||||
// Build HTTPS redirect URL.
|
||||
// Validate host to prevent host-header injection / open redirect:
|
||||
// only allow private IPs, localhost, and .local hostnames.
|
||||
let host = req.connection_info().host().to_string();
|
||||
let uri = req.uri().clone();
|
||||
|
||||
if !is_safe_redirect_host(&host) {
|
||||
let resp = HttpResponse::BadRequest().finish();
|
||||
return Ok(req.into_response(resp).map_into_right_body());
|
||||
}
|
||||
|
||||
let redirect_url = format!("https://{}{}", host, uri);
|
||||
let resp = HttpResponse::MovedPermanently()
|
||||
.insert_header((header::LOCATION, redirect_url))
|
||||
|
||||
@ -174,6 +174,9 @@ impl ConfigService {
|
||||
.and_then(json_value_as_string)
|
||||
{
|
||||
secrets.set_secret(key, &val)?;
|
||||
// Clear plaintext residue from settings table to prevent
|
||||
// pre-migration plaintext passwords from persisting.
|
||||
let _ = self.db.set_setting(key, "");
|
||||
updated.push(key.to_string());
|
||||
}
|
||||
}
|
||||
|
||||
@ -78,11 +78,13 @@ impl BotnetDetector {
|
||||
window_secs: BOTNET_WINDOW_SECS,
|
||||
});
|
||||
|
||||
// source_ip = the latest attacker; dest_ip = the victim being targeted.
|
||||
// SOAR blocks source_ip, so we must NOT put the victim here.
|
||||
let event = DetectionEvent {
|
||||
source: DetectionSource::Correlation,
|
||||
attack_type: "threat_detected".to_string(),
|
||||
confidence: 0.85,
|
||||
source_ip: key.clone(),
|
||||
source_ip: alert.src_ip.clone(),
|
||||
dest_ip: key.clone(),
|
||||
protocol: alert.protocol,
|
||||
packet_count: 0,
|
||||
@ -160,6 +162,9 @@ mod tests {
|
||||
detector.process(&alert, &tx);
|
||||
let event = rx.try_recv().expect("Should alert at threshold");
|
||||
assert_eq!(event.source, DetectionSource::Correlation);
|
||||
// source_ip must be the attacker, NOT the victim
|
||||
assert_eq!(event.source_ip, "10.0.0.9");
|
||||
assert_eq!(event.dest_ip, "192.168.1.1");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user