This is an automated email from the ASF dual-hosted git repository. FreeAndNil pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/logging-log4net.git
commit 0cd39cb1165c3bbb4e86d5f26b113d7deb7cd4ba Merge: e633f774 2a0a1901 Author: Jan Friedrich <[email protected]> AuthorDate: Mon Jun 22 13:08:33 2026 +0200 Fixes after AVSS scanning #298 Fixes for https://github.com/apache/tooling-agents/blob/main/ASVS/reports/logging-log4net/f57d7b3/issues.md --- ## Issue: FINDING-001 - Filter chain modification methods lack synchronization, creating potential race with FilterEvent under active logging **Labels:** bug, security, priority:low **Description:** ### Summary The `AddFilter` and `ClearFilters` methods in `AppenderSkeleton.cs` lack proper synchronization, creating a race condition with `FilterEvent` during active logging operations. This can lead to inconsistent filter chain state, potentially causing filters to be skipped, `NullReferenceException`, or lost filter entries. ### Details **CWE:** CWE-362 (Concurrent Execution using Shared Resource with Improper Synchronization) **ASVS:** 15.4.1 (L3) **Data Flow:** - `AddFilter` (no lock) → modifies `FilterHead`/`_tailFilter`/`filter.Next` - `FilterEvent` (under `LockObj` in `DoAppend`) reads `FilterHead` and traverses `f.Next` **Attack Vector:** In-process code within the trust boundary calling `AddFilter`/`ClearFilters` concurrently with active logging—for example, during dynamic reconfiguration while the appender is receiving log events. **Impact:** Inconsistent filter chain state during traversal in `FilterEvent`, resulting in: - Filters being skipped during evaluation - `NullReferenceException` during chain traversal - Lost filter entries ### Remediation Add `lock(LockObj)` to both `AddFilter` and `ClearFilters` methods to synchronize with the `DoAppend` hot path and ensure thread-safe filter chain modifications. ### Acceptance Criteria - [x] Fixed: `AddFilter` method wrapped with `lock(LockObj)` - [x] Fixed: `ClearFilters` method wrapped with `lock(LockObj)` - [x] Test added: Concurrent filter modification during active logging ### References - File: `src/log4net/Appender/AppenderSkeleton.cs` - Source Report: 15.4.1.md ### Priority **Low** - Requires in-process code with concurrent reconfiguration during active logging. Limited to availability/integrity impact within the logging subsystem. --- ## Issue: FINDING-002 - InterProcessLock Mutex Not Released When Underlying File Stream Is Null **Labels:** bug, security, priority:low **Description:** ### Summary When `InterProcessLock.AcquireLock()` is called and the underlying `_stream` is null (due to a prior file open failure), the named Mutex is acquired but never released. This causes a resource leak that blocks other processes attempting to use InterProcessLock on the same file, potentially leading to deadlock or resource exhaustion. ### Details **CWE:** CWE-772 (Missing Release of Resource after Effective Lifetime) **ASVS:** 1.4.3 (L2) **Data Flow:** 1. `InterProcessLock.AcquireLock()` called with `_stream == null` 2. `_mutex.WaitOne()` acquires the named Mutex 3. `_recursiveWatch` is incremented 4. Method returns null without releasing the mutex 5. Caller (`FileAppender.Append`) does not enter try/finally block 6. `ReleaseLock()` is never called 7. Named system Mutex remains held indefinitely **Attack Vector:** Not directly exploitable by external attackers. Requires environmental file open failure (e.g., permissions, disk full, file locked by another process). **Impact:** - Named Mutex remains held indefinitely - Other processes using InterProcessLock on the same file are blocked - Potential deadlock across processes - Resource exhaustion if multiple locks are leaked ### Remediation Release the named Mutex immediately when `AcquireLock()` detects that `_stream` is null: 1. Decrement `_recursiveWatch` 2. Call `_mutex.ReleaseMutex()` 3. Return null Ensure all code paths that acquire the mutex properly release it, even in error conditions. ### Acceptance Criteria - [x] Fixed: Mutex released when `_stream` is null in `AcquireLock()` - [x] Fixed: `_recursiveWatch` properly decremented in error path - [x] Test added: Verify mutex released when file stream is null ### References - File: `src/log4net/Appender/FileAppender.cs` - Source Report: 1.4.3.md ### Priority **Low** - Requires environmental file system failure. Impact limited to inter-process synchronization and resource exhaustion within logging subsystem. --- ## Issue: FINDING-003 - Finalizer path lacks exception protection, risking process termination **Labels:** bug, security, priority:low **Description:** ### Summary The `~AppenderSkeleton()` finalizer calls `Close()` which in turn calls `OnClose()` without exception protection. An unhandled exception on the finalizer thread will terminate the entire process in .NET Framework 2.0+ and .NET Core/5+. ### Details **CWE:** CWE-755 (Improper Handling of Exceptional Conditions) **ASVS:** 16.5.4 (L3) **Data Flow:** GC finalizer thread → `~AppenderSkeleton()` → `Close()` → `OnClose()` (subclass implementation) → unhandled exception → **process termination** **Attack Vector:** If a subclass implementation of `OnClose()` throws an unhandled exception during finalization (e.g., due to resource cleanup failure, network timeout, or malformed state), the finalizer thread will propagate the exception and terminate the entire application process. **Impact:** - Complete application/service termination - Denial of service - Loss of in-flight data - Ungraceful shutdown without proper cleanup ### Remediation 1. Wrap the finalizer's call to `Close()` in a try-catch block: ```csharp catch (Exception ex) when (!ex.IsFatal()) { // Log if possible, otherwise suppress } ``` 2. Consider protecting `Close()` itself with exception handling 3. Ensure `_isClosed` is set in a finally block to prevent repeated finalization attempts ### Acceptance Criteria - [x] Fixed: Finalizer wrapped with try-catch for non-fatal exceptions - [x] Fixed: `_isClosed` flag set in finally block - [x] Code review: Verify fatal exceptions (OutOfMemoryException, StackOverflowException) are not caught ### References - File: `src/log4net/Appender/AppenderSkeleton.cs` - Source Report: 16.5.4.md ### Priority **Low** - Requires specific failure conditions during finalization. However, impact is severe (process termination) when triggered. Recommend prioritizing fix despite low likelihood. ...8-fix-appender-filter-chain-synchronization.xml | 11 +++ ...8-fix-appender-skeleton-finalizer-exception.xml | 11 +++ .../3.3.2/298-fix-interprocesslock-mutex-leak.xml | 11 +++ src/log4net.Tests/Appender/AppenderSkeletonTest.cs | 88 ++++++++++++++++++++++ src/log4net.Tests/Appender/FileAppenderTest.cs | 32 ++++++++ src/log4net/Appender/AppenderSkeleton.cs | 54 +++++++++---- src/log4net/Appender/FileAppender.cs | 3 + 7 files changed, 195 insertions(+), 15 deletions(-)
