Skip to content

Commit fc3a5d3

Browse files
[ASM] Fix access violation exception when reading WAF addresses (#6510)
## Summary of changes There are some errors detected in real customers that seem to be related to the method Waf.GetKnownAddresses() All the following errors seem to be caused by the same issue: memory corruption. The errors detected include: ``` exception:Type: System.AccessViolationException Message: Attempted to read or write protected memory. This is often an indication that other memory is corrupt. Unhandled Exception: System.ObjectDisposedException: Cannot access a disposed object. Object name: 'UnmanagedMemoryPool'. System.NullReferenceException at System.SpanHelpers.IndexOfNullByte(Byte* searchSpace) at System.String.Ctor(SByte* value) at System.Runtime.InteropServices.Marshal.PtrToStringAnsi(IntPtr ptr) at Datadog.Trace.AppSec.Waf.NativeBindings.WafLibraryInvoker.GetKnownAddresses(IntPtr wafHandle) ``` The stack traces suggest that the problem would be originated in the WAF dll: ``` System.AccessViolationException C:\\Windows\\SYSTEM32\\ntdll.dll!<unknown>+a38ee C:\\Program Files\\Datadog\\.NET Tracer\\win-x64\\ddwaf.dll!<unknown>+4f34e Datadog.Trace.dll!.IL_STUB_PInvoke Datadog.Trace.dll!Datadog.Trace.AppSec.Waf.NativeBindings.WafLibraryInvoker.GetKnownAddresses Datadog.Trace.dll!Datadog.Trace.AppSec.Security.UpdateActiveAddresses ``` A new unit test was created. This unit test launches 100 parallel calls to the WAF GetKnownAddresses method. The test was able to reproduce the error, crashing sometimes the testhost.exe process. It seems that the WAF builds a cache when receiving the first GetKnownAddresses request. Parallel requests seem to be causing a concurrency issue. A lock would prevent this issue. It would seem that we would only need to protect the GetKnownAddresses calls during the first call, but, when doing that, the unit test still failed. When locking all the calls to the GetKnownAddresses method, tests passed consistently. That's why a WriteLock has been used. Anyway, this method is only called when there are RC changes, so the performance impact of the lock should be minimal. ## Reason for change We were getting some of these exceptions logs from real customers. ## Implementation details ## Test coverage ## Other details <!-- Fixes #{issue} --> <!-- ⚠️ Note: where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. -->
1 parent afa3e03 commit fc3a5d3

File tree

3 files changed

+55
-27
lines changed

3 files changed

+55
-27
lines changed

tracer/src/Datadog.Trace/AppSec/Waf/NativeBindings/WafLibraryInvoker.cs

+13-26
Original file line numberDiff line numberDiff line change
@@ -243,37 +243,24 @@ internal void SetupLogging(bool wafDebugEnabled)
243243

244244
internal string[] GetKnownAddresses(IntPtr wafHandle)
245245
{
246-
try
247-
{
248-
if (!IsKnowAddressesSuported())
249-
{
250-
return Array.Empty<string>();
251-
}
252-
253-
uint size = 0;
254-
var result = _getKnownAddresses(wafHandle, ref size);
246+
uint size = 0;
247+
var result = _getKnownAddresses(wafHandle, ref size);
255248

256-
if (size == 0)
257-
{
258-
return Array.Empty<string>();
259-
}
260-
261-
string[] knownAddresses = new string[size];
249+
if (size == 0)
250+
{
251+
return Array.Empty<string>();
252+
}
262253

263-
for (uint i = 0; i < size; i++)
264-
{
265-
// Calculate the pointer to each string
266-
var stringPtr = Marshal.ReadIntPtr(result, (int)i * IntPtr.Size);
267-
knownAddresses[i] = Marshal.PtrToStringAnsi(stringPtr);
268-
}
254+
string[] knownAddresses = new string[size];
269255

270-
return knownAddresses;
271-
}
272-
catch (Exception ex)
256+
for (uint i = 0; i < size; i++)
273257
{
274-
Log.Error(ex, "Error while getting known addresses");
275-
return Array.Empty<string>();
258+
// Calculate the pointer to each string
259+
var stringPtr = Marshal.ReadIntPtr(result, (int)i * IntPtr.Size);
260+
knownAddresses[i] = Marshal.PtrToStringAnsi(stringPtr);
276261
}
262+
263+
return knownAddresses;
277264
}
278265

279266
internal bool IsKnowAddressesSuported(string libVersion = null)

tracer/src/Datadog.Trace/AppSec/Waf/Waf.cs

+27-1
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,33 @@ public bool IsKnowAddressesSuported()
113113

114114
public string[] GetKnownAddresses()
115115
{
116-
return _wafLibraryInvoker.GetKnownAddresses(_wafHandle);
116+
bool lockAcquired = false;
117+
try
118+
{
119+
if (_wafLocker.EnterWriteLock())
120+
{
121+
lockAcquired = true;
122+
123+
var result = _wafLibraryInvoker.GetKnownAddresses(_wafHandle);
124+
return result;
125+
}
126+
else
127+
{
128+
return Array.Empty<string>();
129+
}
130+
}
131+
catch (Exception ex)
132+
{
133+
Log.Error(ex, "Error while getting known addresses");
134+
return Array.Empty<string>();
135+
}
136+
finally
137+
{
138+
if (lockAcquired)
139+
{
140+
_wafLocker.ExitWriteLock();
141+
}
142+
}
117143
}
118144

119145
private unsafe UpdateResult UpdateWafAndDispose(IEncodeResult updateData)

tracer/test/Datadog.Trace.Security.Unit.Tests/RASP/RaspWafTests.cs

+15
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
using System;
77
using System.Collections.Generic;
88
using System.Linq;
9+
using System.Threading.Tasks;
910
using Datadog.Trace.AppSec;
1011
using Datadog.Trace.AppSec.Rcm;
1112
using Datadog.Trace.AppSec.Waf;
@@ -103,6 +104,20 @@ public void GivenARaspRule_WhenInsecureAccess_ThenBlock(object value, string par
103104
actionType);
104105
}
105106

107+
[Fact]
108+
public void GivenWafInstance_WhenGetKnownAddressesInParallel_ThenResultIsOk()
109+
{
110+
var args = CreateArgs("paramValue");
111+
var context = InitWaf(true, "rasp-rule-set.json", args, out var waf);
112+
113+
Parallel.For(0, 100, i =>
114+
{
115+
var addresses = waf.GetKnownAddresses();
116+
addresses.Should().NotBeNull();
117+
addresses.Count().Should().BeGreaterThan(0);
118+
});
119+
}
120+
106121
private void EnableDebugInfo(bool enable)
107122
{
108123
if (enable)

0 commit comments

Comments
 (0)