From dcecbb562216f0d253c35be6645c5186f207ed5a Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Fri, 1 Apr 2022 11:12:36 -0700 Subject: [PATCH] tracing: disable regular expression matching in log filters [`tracing-subscriber` v0.3.10][1] introduces a new [builder API][2] for configuring the `EnvFilter` type. One of the configurations that can now be set using the builder is whether span field value filters for `fmt::Debug` fields are interpreted as precise string matching or as regular expressions. Disabling regular expressions is strongly recommended in cases where filter configurations can come from an untrusted source, as a malicious regular expression is a potential denial-of-service vector. In the proxy, we already implement a form of access control for setting the filter --- the `/admin/log-level` endpoint denies requests that did not originate from localhost, so it's only possible to set the log level when SSHed into the pod. However, it's probably wise to disable regex filters here as well, as a form of additional defense in depth. Therefore, this branch updates the `tracing-subscriber` dependency to v0.3.10, and disables regular expression filters. [1]: https://github.com/tokio-rs/tracing/releases/tag/tracing-subscriber-0.3.10 [2]: https://docs.rs/tracing-subscriber/latest/tracing_subscriber/filter/struct.Builder.html --- Cargo.lock | 4 ++-- linkerd/tracing/Cargo.toml | 2 +- linkerd/tracing/src/level.rs | 17 +++++++++++++++-- linkerd/tracing/src/lib.rs | 9 ++++++--- 4 files changed, 24 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 93f6c8d4ad..2bae249fd2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2632,9 +2632,9 @@ dependencies = [ [[package]] name = "tracing-subscriber" -version = "0.3.9" +version = "0.3.10" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9e0ab7bdc962035a87fba73f3acca9b8a8d0034c2e6f60b84aeaaddddc155dce" +checksum = "b9df98b037d039d03400d9dd06b0f8ce05486b5f25e9a2d7d36196e142ebbc52" dependencies = [ "ansi_term", "lazy_static", diff --git a/linkerd/tracing/Cargo.toml b/linkerd/tracing/Cargo.toml index 0f5216e48e..bc4642250f 100644 --- a/linkerd/tracing/Cargo.toml +++ b/linkerd/tracing/Cargo.toml @@ -17,6 +17,6 @@ tracing = "0.1" tracing-log = "0.1" [dependencies.tracing-subscriber] -version = "0.3" +version = "0.3.10" default-features = false features = ["env-filter", "fmt", "smallvec", "tracing-log", "json", "parking_lot", "registry"] diff --git a/linkerd/tracing/src/level.rs b/linkerd/tracing/src/level.rs index dca1d926f9..d015491413 100644 --- a/linkerd/tracing/src/level.rs +++ b/linkerd/tracing/src/level.rs @@ -1,10 +1,23 @@ use linkerd_error::Error; use tracing::trace; -use tracing_subscriber::{reload, EnvFilter, Registry}; +use tracing_subscriber::{ + filter::{self, EnvFilter, LevelFilter}, + reload, Registry, +}; #[derive(Clone)] pub struct Handle(reload::Handle); +/// Returns an `EnvFilter` builder with the configuration used for parsing new +/// filter strings. +pub(crate) fn filter_builder() -> filter::Builder { + EnvFilter::builder() + .with_default_directive(LevelFilter::WARN.into()) + // Disable regular expression matching for `fmt::Debug` fields, use + // exact string matching instead. + .with_regex(false) +} + impl Handle { pub(crate) fn new(handle: reload::Handle) -> Self { Self(handle) @@ -18,7 +31,7 @@ impl Handle { pub fn set_level(&self, level: impl AsRef) -> Result<(), Error> { let level = level.as_ref(); - let filter = level.parse::()?; + let filter = filter_builder().parse(level)?; self.0.reload(filter)?; tracing::info!(%level, "set new log level"); Ok(()) diff --git a/linkerd/tracing/src/lib.rs b/linkerd/tracing/src/lib.rs index 9f54d7d829..c7f89c3dc9 100644 --- a/linkerd/tracing/src/lib.rs +++ b/linkerd/tracing/src/lib.rs @@ -16,7 +16,7 @@ use linkerd_error::Error; use std::str; use tracing::{Dispatch, Subscriber}; use tracing_subscriber::{ - filter::{EnvFilter, FilterFn}, + filter::{FilterFn, LevelFilter}, fmt::format, prelude::*, registry::LookupSpan, @@ -60,7 +60,6 @@ pub fn init() -> Result { #[inline] pub(crate) fn update_max_level() { - use tracing::level_filters::LevelFilter; use tracing_log::{log, AsLog}; log::set_max_level(LevelFilter::current().as_log()); } @@ -167,7 +166,11 @@ impl Settings { pub fn build(self) -> (Dispatch, Handle) { let log_level = self.filter.as_deref().unwrap_or(DEFAULT_LOG_LEVEL); - let mut filter = EnvFilter::new(log_level); + let mut filter = level::filter_builder() + // When parsing the initial filter configuration from the + // environment variable, use `parse_lossy` to skip any invalid + // filter directives and print an error. + .parse_lossy(log_level); // If access logging is enabled, build the access log layer. let access_log = if let Some(format) = self.access_log {