From db3c3f2929aeb72c2363a2cc88b8f8145731dd57 Mon Sep 17 00:00:00 2001 From: Pando85 Date: Sat, 2 May 2020 21:33:41 +0200 Subject: [PATCH 1/4] Fix audit issue logging by default peer address By default log format include remote address that is taken from headers. This is very easy to replace making log untrusted. Changing default log format value `%a` to peer address we are getting this trusted data always. Also, remote address option is maintianed and relegated to `%{r}a` value. Related kanidm/kanidm#191. --- src/info.rs | 18 ++++++++---- src/middleware/logger.rs | 60 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 70 insertions(+), 8 deletions(-) diff --git a/src/info.rs b/src/info.rs index c9a642b3645..4d3ee3d3f8f 100644 --- a/src/info.rs +++ b/src/info.rs @@ -30,7 +30,6 @@ impl ConnectionInfo { let mut host = None; let mut scheme = None; let mut remote = None; - let mut peer = None; // load forwarded header for hdr in req.headers.get_all(&header::FORWARDED) { @@ -106,6 +105,9 @@ impl ConnectionInfo { } } + // get peeraddr from socketaddr + let peer = req.peer_addr.map(|addr| format!("{}", addr)); + // remote addr if remote.is_none() { if let Some(h) = req @@ -116,10 +118,6 @@ impl ConnectionInfo { remote = h.split(',').next().map(|v| v.trim()); } } - if remote.is_none() { - // get peeraddr from socketaddr - peer = req.peer_addr.map(|addr| format!("{}", addr)); - } } ConnectionInfo { @@ -155,6 +153,16 @@ impl ConnectionInfo { &self.host } + /// Peer address of the request. + /// + /// Get peer address from socket address + pub fn peer(&self) -> Option<&str> { + if let Some(ref peer) = self.peer { + Some(peer) + } else { + None + } + } /// Remote socket addr of client initiated HTTP request. /// /// The addr is resolved through the following headers, in this order: diff --git a/src/middleware/logger.rs b/src/middleware/logger.rs index 7d1577c96a2..4bd02b03e89 100644 --- a/src/middleware/logger.rs +++ b/src/middleware/logger.rs @@ -55,7 +55,7 @@ use crate::HttpResponse; /// /// `%%` The percent sign /// -/// `%a` Remote IP-address (IP-address of proxy if using reverse proxy) +/// `%a` Client IP-address /// /// `%t` Time when the request was started to process (in rfc3339 format) /// @@ -72,12 +72,18 @@ use crate::HttpResponse; /// /// `%U` Request URL /// +/// `%{r}a` Remote IP-address (IP-address of proxy if using reverse proxy) **\*** +/// /// `%{FOO}i` request.headers['FOO'] /// /// `%{FOO}o` response.headers['FOO'] /// /// `%{FOO}e` os.environ['FOO'] /// +/// > **\* warning** It is critical to only use this value from intermediate +/// > hosts(proxies, etc) which are trusted by this server, since it is trivial +/// > for the remote client to simulate another client. +/// pub struct Logger(Rc); struct Inner { @@ -301,7 +307,7 @@ impl Format { /// Returns `None` if the format string syntax is incorrect. pub fn new(s: &str) -> Format { log::trace!("Access log format: {}", s); - let fmt = Regex::new(r"%(\{([A-Za-z0-9\-_]+)\}([ioe])|[atPrUsbTD]?)").unwrap(); + let fmt = Regex::new(r"%(\{([A-Za-z0-9\-_]+)\}([aioe])|[atPrUsbTD]?)").unwrap(); let mut idx = 0; let mut results = Vec::new(); @@ -315,6 +321,11 @@ impl Format { if let Some(key) = cap.get(2) { results.push(match cap.get(3).unwrap().as_str() { + "a" => if key.as_str() == "r" { + FormatText::RemoteAddr + } else { + unreachable!() + }, "i" => FormatText::RequestHeader( HeaderName::try_from(key.as_str()).unwrap(), ), @@ -328,7 +339,7 @@ impl Format { let m = cap.get(1).unwrap(); results.push(match m.as_str() { "%" => FormatText::Percent, - "a" => FormatText::RemoteAddr, + "a" => FormatText::ClientIP, "t" => FormatText::RequestTime, "r" => FormatText::RequestLine, "s" => FormatText::ResponseStatus, @@ -361,6 +372,7 @@ pub enum FormatText { ResponseSize, Time, TimeMillis, + ClientIP, RemoteAddr, UrlPath, RequestHeader(HeaderName), @@ -457,6 +469,14 @@ impl FormatText { }; *self = FormatText::Str(s.to_string()); } + FormatText::ClientIP => { + let s = if let Some(ref peer) = req.connection_info().peer() { + FormatText::Str(peer.to_string()) + } else { + FormatText::Str("-".to_string()) + }; + *self = s; + } FormatText::RemoteAddr => { let s = if let Some(remote) = req.connection_info().remote() { FormatText::Str(remote.to_string()) @@ -549,6 +569,7 @@ mod tests { header::USER_AGENT, header::HeaderValue::from_static("ACTIX-WEB"), ) + .peer_addr("127.0.0.1:8081".parse().unwrap()) .to_srv_request(); let now = OffsetDateTime::now_utc(); @@ -570,6 +591,7 @@ mod tests { }; let s = format!("{}", FormatDisplay(&render)); assert!(s.contains("GET / HTTP/1.1")); + assert!(s.contains("127.0.0.1")); assert!(s.contains("200 1024")); assert!(s.contains("ACTIX-WEB")); } @@ -598,4 +620,36 @@ mod tests { let s = format!("{}", FormatDisplay(&render)); assert!(s.contains(&format!("{}", now.format("%Y-%m-%dT%H:%M:%S")))); } + + #[actix_rt::test] + async fn test_remote_addr_format() { + let mut format = Format::new("%{r}a"); + + let req = TestRequest::with_header( + header::FORWARDED, + header::HeaderValue::from_static("for=192.0.2.60;proto=http;by=203.0.113.43"), + ) + .to_srv_request(); + + let now = OffsetDateTime::now_utc(); + for unit in &mut format.0 { + unit.render_request(now, &req); + } + + let resp = HttpResponse::build(StatusCode::OK).force_close().finish(); + for unit in &mut format.0 { + unit.render_response(&resp); + } + + let entry_time = OffsetDateTime::now_utc(); + let render = |fmt: &mut Formatter<'_>| { + for unit in &format.0 { + unit.render(fmt, 1024, entry_time)?; + } + Ok(()) + }; + let s = format!("{}", FormatDisplay(&render)); + println!("{}", s); + assert!(s.contains("192.0.2.60")); + } } From 6ebfbf0001dfddaf9ee94c0faab6a079c7b3d1d0 Mon Sep 17 00:00:00 2001 From: Pando85 Date: Mon, 4 May 2020 14:30:40 +0200 Subject: [PATCH 2/4] Rename peer/remote to remote_addr/realip_remote_addr Change names to avoid naming confusions. I choose this accord to Nginx variables and [ngx_http_realip_module](https://nginx.org/en/docs/http/ngx_http_realip_module.html). Add more specific documentation about security concerns of using Real IP in logger. --- src/info.rs | 53 ++++++++++++++++++++-------------------- src/middleware/logger.rs | 27 +++++++++++--------- 2 files changed, 41 insertions(+), 39 deletions(-) diff --git a/src/info.rs b/src/info.rs index 4d3ee3d3f8f..5b506d85add 100644 --- a/src/info.rs +++ b/src/info.rs @@ -12,8 +12,8 @@ const X_FORWARDED_PROTO: &[u8] = b"x-forwarded-proto"; pub struct ConnectionInfo { scheme: String, host: String, - remote: Option, - peer: Option, + realip_remote_addr: Option, + remote_addr: Option, } impl ConnectionInfo { @@ -29,7 +29,7 @@ impl ConnectionInfo { fn new(req: &RequestHead, cfg: &AppConfig) -> ConnectionInfo { let mut host = None; let mut scheme = None; - let mut remote = None; + let mut realip_remote_addr = None; // load forwarded header for hdr in req.headers.get_all(&header::FORWARDED) { @@ -41,8 +41,8 @@ impl ConnectionInfo { if let Some(val) = items.next() { match &name.to_lowercase() as &str { "for" => { - if remote.is_none() { - remote = Some(val.trim()); + if realip_remote_addr.is_none() { + realip_remote_addr = Some(val.trim()); } } "proto" => { @@ -105,26 +105,25 @@ impl ConnectionInfo { } } - // get peeraddr from socketaddr - let peer = req.peer_addr.map(|addr| format!("{}", addr)); + // get remote_addraddr from socketaddr + let remote_addr = req.peer_addr.map(|addr| format!("{}", addr)); - // remote addr - if remote.is_none() { + if realip_remote_addr.is_none() { if let Some(h) = req .headers .get(&HeaderName::from_lowercase(X_FORWARDED_FOR).unwrap()) { if let Ok(h) = h.to_str() { - remote = h.split(',').next().map(|v| v.trim()); + realip_remote_addr = h.split(',').next().map(|v| v.trim()); } } } ConnectionInfo { - peer, + remote_addr, scheme: scheme.unwrap_or("http").to_owned(), host: host.unwrap_or("localhost").to_owned(), - remote: remote.map(|s| s.to_owned()), + realip_remote_addr: realip_remote_addr.map(|s| s.to_owned()), } } @@ -153,23 +152,23 @@ impl ConnectionInfo { &self.host } - /// Peer address of the request. + /// remote_addr address of the request. /// - /// Get peer address from socket address - pub fn peer(&self) -> Option<&str> { - if let Some(ref peer) = self.peer { - Some(peer) + /// Get remote_addr address from socket address + pub fn remote_addr(&self) -> Option<&str> { + if let Some(ref remote_addr) = self.remote_addr { + Some(remote_addr) } else { None } } - /// Remote socket addr of client initiated HTTP request. + /// Real ip remote addr of client initiated HTTP request. /// /// The addr is resolved through the following headers, in this order: /// /// - Forwarded /// - X-Forwarded-For - /// - peer name of opened socket + /// - remote_addr name of opened socket /// /// # Security /// Do not use this function for security purposes, unless you can ensure the Forwarded and @@ -177,11 +176,11 @@ impl ConnectionInfo { /// address explicitly, use /// [`HttpRequest::peer_addr()`](../web/struct.HttpRequest.html#method.peer_addr) instead. #[inline] - pub fn remote(&self) -> Option<&str> { - if let Some(ref r) = self.remote { + pub fn realip_remote_addr(&self) -> Option<&str> { + if let Some(ref r) = self.realip_remote_addr { Some(r) - } else if let Some(ref peer) = self.peer { - Some(peer) + } else if let Some(ref remote_addr) = self.remote_addr { + Some(remote_addr) } else { None } @@ -210,7 +209,7 @@ mod tests { let info = req.connection_info(); assert_eq!(info.scheme(), "https"); assert_eq!(info.host(), "rust-lang.org"); - assert_eq!(info.remote(), Some("192.0.2.60")); + assert_eq!(info.realip_remote_addr(), Some("192.0.2.60")); let req = TestRequest::default() .header(header::HOST, "rust-lang.org") @@ -219,20 +218,20 @@ mod tests { let info = req.connection_info(); assert_eq!(info.scheme(), "http"); assert_eq!(info.host(), "rust-lang.org"); - assert_eq!(info.remote(), None); + assert_eq!(info.realip_remote_addr(), None); let req = TestRequest::default() .header(X_FORWARDED_FOR, "192.0.2.60") .to_http_request(); let info = req.connection_info(); - assert_eq!(info.remote(), Some("192.0.2.60")); + assert_eq!(info.realip_remote_addr(), Some("192.0.2.60")); let req = TestRequest::default() .header(X_FORWARDED_HOST, "192.0.2.60") .to_http_request(); let info = req.connection_info(); assert_eq!(info.host(), "192.0.2.60"); - assert_eq!(info.remote(), None); + assert_eq!(info.realip_remote_addr(), None); let req = TestRequest::default() .header(X_FORWARDED_PROTO, "https") diff --git a/src/middleware/logger.rs b/src/middleware/logger.rs index 4bd02b03e89..d4853a5bfbf 100644 --- a/src/middleware/logger.rs +++ b/src/middleware/logger.rs @@ -55,7 +55,7 @@ use crate::HttpResponse; /// /// `%%` The percent sign /// -/// `%a` Client IP-address +/// `%a` Remote IP-address (IP-address of proxy if using reverse proxy) /// /// `%t` Time when the request was started to process (in rfc3339 format) /// @@ -72,7 +72,7 @@ use crate::HttpResponse; /// /// `%U` Request URL /// -/// `%{r}a` Remote IP-address (IP-address of proxy if using reverse proxy) **\*** +/// `%{r}a` Real IP remote address **\*** /// /// `%{FOO}i` request.headers['FOO'] /// @@ -80,9 +80,12 @@ use crate::HttpResponse; /// /// `%{FOO}e` os.environ['FOO'] /// -/// > **\* warning** It is critical to only use this value from intermediate -/// > hosts(proxies, etc) which are trusted by this server, since it is trivial -/// > for the remote client to simulate another client. +/// ### **\* security warning** +/// It is calculated using +/// [`ConnectionInfo::realip_remote_addr()`](../dev/struct.ConnectionInfo.html#method.realip_remote_addr) +/// +/// If you use this value ensure that all requests come from trusted hosts, since it is trivial +/// for the remote client to simulate been another client. /// pub struct Logger(Rc); @@ -322,7 +325,7 @@ impl Format { if let Some(key) = cap.get(2) { results.push(match cap.get(3).unwrap().as_str() { "a" => if key.as_str() == "r" { - FormatText::RemoteAddr + FormatText::RealIPRemoteAddr } else { unreachable!() }, @@ -339,7 +342,7 @@ impl Format { let m = cap.get(1).unwrap(); results.push(match m.as_str() { "%" => FormatText::Percent, - "a" => FormatText::ClientIP, + "a" => FormatText::RemoteAddr, "t" => FormatText::RequestTime, "r" => FormatText::RequestLine, "s" => FormatText::ResponseStatus, @@ -372,8 +375,8 @@ pub enum FormatText { ResponseSize, Time, TimeMillis, - ClientIP, RemoteAddr, + RealIPRemoteAddr, UrlPath, RequestHeader(HeaderName), ResponseHeader(HeaderName), @@ -469,16 +472,16 @@ impl FormatText { }; *self = FormatText::Str(s.to_string()); } - FormatText::ClientIP => { - let s = if let Some(ref peer) = req.connection_info().peer() { + FormatText::RemoteAddr => { + let s = if let Some(ref peer) = req.connection_info().remote_addr() { FormatText::Str(peer.to_string()) } else { FormatText::Str("-".to_string()) }; *self = s; } - FormatText::RemoteAddr => { - let s = if let Some(remote) = req.connection_info().remote() { + FormatText::RealIPRemoteAddr => { + let s = if let Some(remote) = req.connection_info().realip_remote_addr() { FormatText::Str(remote.to_string()) } else { FormatText::Str("-".to_string()) From 9dfefb0648ef8acf8363294da575fba50cdcd5e9 Mon Sep 17 00:00:00 2001 From: Pando85 Date: Sat, 9 May 2020 08:14:15 +0200 Subject: [PATCH 3/4] Rename security advertise header in doc --- src/middleware/logger.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/middleware/logger.rs b/src/middleware/logger.rs index d4853a5bfbf..d6b931bb4b8 100644 --- a/src/middleware/logger.rs +++ b/src/middleware/logger.rs @@ -80,8 +80,8 @@ use crate::HttpResponse; /// /// `%{FOO}e` os.environ['FOO'] /// -/// ### **\* security warning** -/// It is calculated using +/// # Security +/// **\*** It is calculated using /// [`ConnectionInfo::realip_remote_addr()`](../dev/struct.ConnectionInfo.html#method.realip_remote_addr) /// /// If you use this value ensure that all requests come from trusted hosts, since it is trivial From 19398584f13d8c20a1319c9093d346a0f5c94c2e Mon Sep 17 00:00:00 2001 From: Pando85 Date: Sat, 9 May 2020 08:15:02 +0200 Subject: [PATCH 4/4] Add fix audit issue logging by default peer adress to changelog --- CHANGES.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index db2c8e8f5ad..b5ef6802574 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -7,9 +7,11 @@ * `{Resource,Scope}::default_service(f)` handlers now support app data extraction. [#1452] * Implement `std::error::Error` for our custom errors [#1422] * NormalizePath middleware now appends trailing / so that routes of form /example/ respond to /example requests. +* Fix audit issue logging by default peer address [#1485] [#1422]: https://github.com/actix/actix-web/pull/1422 [#1452]: https://github.com/actix/actix-web/pull/1452 +[#1485]: https://github.com/actix/actix-web/pull/1485 ## [3.0.0-alpha.1] - 2020-03-11