FreeAndNil opened a new pull request, #298: URL: https://github.com/apache/logging-log4net/pull/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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
