This is an automated email from the ASF dual-hosted git repository. FreeAndNil pushed a commit to branch Feature/security-audit-hardening in repository https://gitbox.apache.org/repos/asf/logging-log4net.git
commit 394fd3dc8316de38bf013b23b86fa07fbfdf0e71 Author: Jan Friedrich <[email protected]> AuthorDate: Mon Aug 17 22:41:45 2026 +0200 bound regular expression matching in the string match filters RegexToMatch was compiled with Regex.InfiniteMatchTimeout, and the match runs while the appender lock is held, so a pattern that backtracks could stall everything logging through the appender on some inputs. Matching now stops after MatchTimeoutMillis, 1000 by default, and 0 restores the previous unbounded behaviour. An abandoned match counts as no match, so the event is left to the rest of the filter chain rather than having its decision changed. The pattern comes from configuration and is trusted, so this is hardening against a pattern that turns out to be expensive, not protection against untrusted input. StringMatchFilter and PropertyFilter both matched the regex themselves, so the handling lives in one protected IsRegexMatch used by both, which also covers MdcFilter and NdcFilter. It reports an abandoned match once per filter rather than once per event, since a warning per event would be a problem of its own. regexToMatch was missing from the manual entirely and is documented now. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- .../3.4.0/309-bound-filter-regex-matching.xml | 17 +++ src/log4net.Tests/Filter/StringMatchFilterTest.cs | 128 +++++++++++++++++++++ src/log4net/Filter/PropertyFilter.cs | 2 +- src/log4net/Filter/StringMatchFilter.cs | 87 +++++++++++++- .../ROOT/pages/manual/configuration/filters.adoc | 23 ++++ 5 files changed, 252 insertions(+), 5 deletions(-) diff --git a/src/changelog/3.4.0/309-bound-filter-regex-matching.xml b/src/changelog/3.4.0/309-bound-filter-regex-matching.xml new file mode 100644 index 00000000..5b721997 --- /dev/null +++ b/src/changelog/3.4.0/309-bound-filter-regex-matching.xml @@ -0,0 +1,17 @@ +<?xml version="1.0" encoding="UTF-8"?> +<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xmlns="https://logging.apache.org/xml/ns"; + xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"; + type="changed"> + <issue id="309" link="https://github.com/apache/logging-log4net/pull/309"/> + <description format="asciidoc"> + give `RegexToMatch` matching a deadline in `StringMatchFilter` and the filters deriving from it, + `PropertyFilter`, `MdcFilter` and `NdcFilter`. The pattern was matched with + `Regex.InfiniteMatchTimeout` while the appender lock was held, so a pattern that backtracks could + stall everything logging through the appender on some inputs. Matching now stops after + `MatchTimeoutMillis`, 1000 by default, and an abandoned match is reported once and leaves the event + to the rest of the filter chain; 0 restores unbounded matching. The pattern comes from + configuration and is trusted, so this is hardening rather than a vulnerability fix + (audit 1231d72-f013) + </description> +</entry> diff --git a/src/log4net.Tests/Filter/StringMatchFilterTest.cs b/src/log4net.Tests/Filter/StringMatchFilterTest.cs new file mode 100644 index 00000000..ff3eced6 --- /dev/null +++ b/src/log4net.Tests/Filter/StringMatchFilterTest.cs @@ -0,0 +1,128 @@ +#region Apache License +// +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to you under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +#endregion + +using System; +using System.Collections.Generic; +using System.Diagnostics; + +using log4net.Core; +using log4net.Filter; +using log4net.Util; + +using NUnit.Framework; + +namespace log4net.Tests.Filter; + +/// <summary> +/// Tests for <see cref="StringMatchFilter"/> +/// </summary> +[TestFixture] +public class StringMatchFilterTest +{ + /// <summary> + /// A pattern that backtracks, with an input that makes it do so. Matching this without a deadline + /// runs for longer than any test would wait. + /// </summary> + private const string CatastrophicPattern = "^(a+)+$"; + + private const string CraftedMessage = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa!"; + + /// <summary> + /// The match runs while the appender lock is held, so one that backtracks has to be abandoned + /// rather than holding the thread. The event is then left to the rest of the filter chain. + /// </summary> + [Test] + public void AMatchThatBacktracksIsAbandoned() + { + StringMatchFilter filter = new() { RegexToMatch = CatastrophicPattern, MatchTimeoutMillis = 100 }; + filter.ActivateOptions(); + + Stopwatch stopwatch = Stopwatch.StartNew(); + FilterDecision decision = FilterDecision.Accept; + LogLog.ExecuteWithoutEmittingInternalMessages(() => decision = filter.Decide(CreateEvent(CraftedMessage))); + stopwatch.Stop(); + + Assert.That(decision, Is.EqualTo(FilterDecision.Neutral)); + Assert.That(stopwatch.Elapsed, Is.LessThan(TimeSpan.FromSeconds(30))); + } + + /// <summary> + /// Abandoning the match must not be silent, but it must also not report once per event: the + /// condition repeats for every event that reaches the filter. + /// </summary> + [Test] + [NonParallelizable] + public void AnAbandonedMatchIsReportedOnce() + { + StringMatchFilter filter = new() { RegexToMatch = CatastrophicPattern, MatchTimeoutMillis = 100 }; + filter.ActivateOptions(); + + List<LogLog> messages = []; + LogLog.ExecuteWithoutEmittingInternalMessages(() => + { + using LogLog.LogReceivedAdapter _ = new(messages); + filter.Decide(CreateEvent(CraftedMessage)); + filter.Decide(CreateEvent(CraftedMessage)); + filter.Decide(CreateEvent(CraftedMessage)); + }); + + Assert.That( + messages.ConvertAll(m => m.Message).FindAll(m => m.IndexOf("was abandoned", StringComparison.Ordinal) >= 0), + Has.Count.EqualTo(1)); + } + + /// <summary> + /// A pattern that does not backtrack has to keep working, with the decision unchanged. + /// </summary> + [Test] + public void AMatchingPatternStillDecides() + { + StringMatchFilter filter = new() { RegexToMatch = "cat", AcceptOnMatch = true }; + filter.ActivateOptions(); + + Assert.That(filter.Decide(CreateEvent("the cat sat")), Is.EqualTo(FilterDecision.Accept)); + Assert.That(filter.Decide(CreateEvent("the dog sat")), Is.EqualTo(FilterDecision.Neutral)); + } + + /// <summary> + /// The deadline has to be finite by default, so that an expensive pattern cannot hold the + /// appender lock indefinitely without the operator having opted into that. + /// </summary> + [Test] + public void MatchTimeoutMillisDefaultsToAFiniteValue() + => Assert.That(new StringMatchFilter().MatchTimeoutMillis, Is.EqualTo(1000)); + + /// <summary> + /// 0 is the documented opt-out that restores unbounded matching; a negative deadline has no + /// meaning and is rejected rather than being reinterpreted. + /// </summary> + [Test] + public void MatchTimeoutMillisRejectsNegativeValuesButAllowsZero() + { + StringMatchFilter filter = new(); + + Assert.That(() => filter.MatchTimeoutMillis = -1, Throws.TypeOf<ArgumentOutOfRangeException>()); + + filter.MatchTimeoutMillis = 0; + Assert.That(filter.MatchTimeoutMillis, Is.EqualTo(0)); + } + + private static LoggingEvent CreateEvent(string message) + => new(new LoggingEventData { Level = Level.Info, Message = message, LoggerName = "TestLogger" }); +} diff --git a/src/log4net/Filter/PropertyFilter.cs b/src/log4net/Filter/PropertyFilter.cs index cef49c75..c487682b 100644 --- a/src/log4net/Filter/PropertyFilter.cs +++ b/src/log4net/Filter/PropertyFilter.cs @@ -95,7 +95,7 @@ public override FilterDecision Decide(LoggingEvent loggingEvent) if (m_regexToMatch is not null) { // Check the regex - if (m_regexToMatch.Match(msg).Success == false) + if (!IsRegexMatch(msg)) { // No match, continue processing return FilterDecision.Neutral; diff --git a/src/log4net/Filter/StringMatchFilter.cs b/src/log4net/Filter/StringMatchFilter.cs index e7819913..d0c79c42 100644 --- a/src/log4net/Filter/StringMatchFilter.cs +++ b/src/log4net/Filter/StringMatchFilter.cs @@ -17,6 +17,7 @@ // #endregion +using System; using System.Text.RegularExpressions; using log4net.Core; @@ -53,14 +54,92 @@ public class StringMatchFilter : FilterSkeleton /// <see cref="ActivateOptions"/> must be called again. /// </para> /// </remarks> - public override void ActivateOptions() + public override void ActivateOptions() { if (RegexToMatch is not null) { - m_regexToMatch = new(RegexToMatch, RegexOptions.Compiled); + m_regexToMatch = new(RegexToMatch, RegexOptions.Compiled, + _matchTimeoutMillis == 0 + ? Regex.InfiniteMatchTimeout + : TimeSpan.FromMilliseconds(_matchTimeoutMillis)); } } + /// <summary> + /// Gets or sets the time, in milliseconds, that matching <see cref="RegexToMatch"/> against a + /// single event may take before the match is abandoned. + /// </summary> + /// <value> + /// A positive number of milliseconds, or 0 to let a match run for as long as it takes. + /// </value> + /// <remarks> + /// <para> + /// A regular expression that backtracks can take a very long time on some inputs. The pattern is + /// matched while the appender lock is held, so an unbounded match would stall everything logging + /// through the appender, and matching is therefore given a deadline. A match that reaches it is + /// treated as no match, leaving the rest of the filter chain to decide. + /// </para> + /// <para> + /// The pattern comes from configuration and is trusted, so this is a guard against a pattern that + /// turns out to be expensive rather than protection against untrusted input. + /// </para> + /// <para> + /// The default value is 1000 (one second). Setting the value to 0 restores unbounded matching and + /// is not recommended. Changing it takes effect when <see cref="ActivateOptions"/> is called. + /// </para> + /// </remarks> + /// <exception cref="ArgumentOutOfRangeException">The value specified is negative.</exception> + public int MatchTimeoutMillis + { + get => _matchTimeoutMillis; + set + { + if (value < 0) + { + throw SystemInfo.CreateArgumentOutOfRangeException(nameof(value), value, + "The value specified for MatchTimeoutMillis is negative."); + } + _matchTimeoutMillis = value; + } + } + + private int _matchTimeoutMillis = 1000; + private bool _matchTimeoutReported; + + /// <summary> + /// Matches <paramref name="value"/> against <see cref="m_regexToMatch"/>. + /// </summary> + /// <param name="value">The text to match.</param> + /// <returns> + /// <see langword="true"/> when the pattern matches, and <see langword="false"/> when it does not + /// or when matching took longer than <see cref="MatchTimeoutMillis"/>. + /// </returns> + protected bool IsRegexMatch(string value) + { + try + { + return m_regexToMatch!.IsMatch(value); + } + catch (RegexMatchTimeoutException) + { + if (!_matchTimeoutReported) + { + // Once per filter. The condition repeats for every event that reaches it, and a warning + // per event would be a denial of service of its own. + _matchTimeoutReported = true; + LogLog.Warn(_declaringType, + $"Matching the pattern [{RegexToMatch}] took longer than {MatchTimeoutMillis}ms and was abandoned, so the event was not filtered by it. " + + "A pattern that backtracks can take arbitrarily long on some inputs; consider rewriting it or raising MatchTimeoutMillis."); + } + return false; + } + } + + /// <summary> + /// The fully qualified type of the <see cref="StringMatchFilter"/> class. + /// </summary> + private static readonly Type _declaringType = typeof(StringMatchFilter); + /// <summary> /// <see cref="FilterDecision.Accept"/> when matching <see cref="StringToMatch"/> or <see cref="RegexToMatch"/> /// </summary> @@ -144,11 +223,11 @@ public override FilterDecision Decide(LoggingEvent loggingEvent) if (m_regexToMatch is not null) { // Check the regex - if (m_regexToMatch.Match(msg).Success == false) + if (!IsRegexMatch(msg)) { // No match, continue processing return FilterDecision.Neutral; - } + } // we've got a match if (AcceptOnMatch) diff --git a/src/site/antora/modules/ROOT/pages/manual/configuration/filters.adoc b/src/site/antora/modules/ROOT/pages/manual/configuration/filters.adoc index 5f2e07a6..0c34f84c 100644 --- a/src/site/antora/modules/ROOT/pages/manual/configuration/filters.adoc +++ b/src/site/antora/modules/ROOT/pages/manual/configuration/filters.adoc @@ -94,3 +94,26 @@ The following filters are defined in the log4net package: |log4net.Filter.StringMatchFilter |Matches events containing a specific substring in the message. |=== + +[#filters-regex-timeout] +== Matching with a regular expression + +`StringMatchFilter`, `PropertyFilter`, `MdcFilter` and `NdcFilter` accept a `regexToMatch` instead +of a `stringToMatch`. + +A regular expression that backtracks can take a very long time on some inputs, and the match runs +while the appender lock is held. +Matching is therefore given a deadline of one second, configurable with `matchTimeoutMillis`. +A match that reaches the deadline is abandoned, the filter reports it once and returns `Neutral`, +and the remaining filters decide the event. +Prefer a pattern that cannot backtrack; the deadline is a safety net, not a substitute. + +[source,xml] +---- +<filter type="log4net.Filter.StringMatchFilter"> + <regexToMatch value="user=[a-z0-9._-]+@example\.com" /> + <matchTimeoutMillis value="1000" /> +</filter> +---- + +Setting `matchTimeoutMillis` to `0` lets a match run for as long as it takes and is not recommended.
