Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[IAST] Fix for VS Edit and Continue #6097

Merged
merged 2 commits into from
Oct 1, 2024
Merged

[IAST] Fix for VS Edit and Continue #6097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9da943d
Initial implementation
daniel-romano-DD Sep 27, 2024
082a93f
Added netfx variable
daniel-romano-DD Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tracer/src/Datadog.Tracer.Native/cor_profiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ HRESULT STDMETHODCALLTYPE CorProfiler::Initialize(IUnknown* cor_profiler_info_un

if (isIastEnabled || isRaspEnabled)
{
_dataflow = new iast::Dataflow(info_, rejit_handler);
_dataflow = new iast::Dataflow(info_, rejit_handler, runtime_information_);
if (FAILED(_dataflow->Init()))
{
Logger::Error("Callsite Dataflow failed to initialize");
Expand Down
3 changes: 1 addition & 2 deletions tracer/src/Datadog.Tracer.Native/cor_profiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,10 +170,10 @@ class CorProfiler : public CorProfilerBase
HRESULT STDMETHODCALLTYPE ProfilerDetachSucceeded() override;

HRESULT STDMETHODCALLTYPE JITInlining(FunctionID callerId, FunctionID calleeId, BOOL* pfShouldInline) override;

//
// ReJIT Methods
//

HRESULT STDMETHODCALLTYPE GetReJITParameters(ModuleID moduleId, mdMethodDef methodId,
ICorProfilerFunctionControl* pFunctionControl) override;

Expand Down Expand Up @@ -251,7 +251,6 @@ class CorProfiler : public CorProfilerBase
//
// Getters for exception filter
//

bool IsCallTargetBubbleUpExceptionTypeAvailable() const;
bool IsCallTargetBubbleUpFunctionAvailable() const;
};
Expand Down
6 changes: 6 additions & 0 deletions tracer/src/Datadog.Tracer.Native/environment_variables.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,12 @@ namespace environment
// Enables the workaround for dotnet issue 77973 (https://github.com/dotnet/runtime/issues/77973)
const shared::WSTRING internal_workaround_77973_enabled = WStr("DD_INTERNAL_WORKAROUND_77973_ENABLED");

// IDE Edit and Continue. If enabled, profiler behavior is modified slightly
const shared::WSTRING ide_edit_and_continue_core = WStr("COMPLUS_ForceEnc");

// IDE Edit and Continue. If enabled, profiler behavior is modified slightly
const shared::WSTRING ide_edit_and_continue_netfx = WStr("DOTNET_ForceEnc");

} // namespace environment
} // namespace trace

Expand Down
13 changes: 13 additions & 0 deletions tracer/src/Datadog.Tracer.Native/environment_variables_util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,4 +68,17 @@ bool IsRaspEnabled()
return IsRaspSettingEnabled() && IsAsmSettingEnabled();
}

bool IsEditAndContinueEnabledCore()
{
ToBooleanWithDefault(shared::GetEnvironmentValue(environment::ide_edit_and_continue_core), false);
}
bool IsEditAndContinueEnabledNetFx()
{
ToBooleanWithDefault(shared::GetEnvironmentValue(environment::ide_edit_and_continue_netfx), false);
}
bool IsEditAndContinueEnabled()
{
return IsEditAndContinueEnabledCore() || IsEditAndContinueEnabledNetFx();
}

} // namespace trace
1 change: 1 addition & 0 deletions tracer/src/Datadog.Tracer.Native/environment_variables_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ bool IsAzureFunctionsEnabled();
bool IsVersionCompatibilityEnabled();
bool IsIastEnabled();
bool IsRaspEnabled();
bool IsEditAndContinueEnabled();

} // namespace trace

Expand Down
39 changes: 26 additions & 13 deletions tracer/src/Datadog.Tracer.Native/iast/dataflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <fstream>
#include <chrono>
#include "../../../../shared/src/native-src/com_ptr.h"
#include "../environment_variables_util.h"

using namespace std::chrono;

Expand Down Expand Up @@ -153,23 +154,26 @@ AspectFilter* ModuleAspects::GetFilter(DataflowAspectFilterValue filterValue)

//--------------------

Dataflow::Dataflow(ICorProfilerInfo* profiler, std::shared_ptr<RejitHandler> rejitHandler) :
Dataflow::Dataflow(ICorProfilerInfo* profiler, std::shared_ptr<RejitHandler> rejitHandler,
const RuntimeInformation& runtimeInfo) :
Rejitter(rejitHandler, RejitterPriority::Low)
{
HRESULT hr = profiler->QueryInterface(__uuidof(ICorProfilerInfo3), (void**) &_profiler);
if (_profiler != nullptr)
m_runtimeType = runtimeInfo.runtime_type;
m_runtimeVersion = VersionInfo{runtimeInfo.major_version, runtimeInfo.minor_version, runtimeInfo.build_version, 0};
trace::Logger::Info("Dataflow::Dataflow -> Detected runtime version : ", m_runtimeVersion.ToString());

this->_setILOnJit = trace::IsEditAndContinueEnabled();
if (this->_setILOnJit)
{
USHORT major;
USHORT minor;
USHORT build;
trace::Logger::Info("Dataflow detected Edit and Continue feature (COMPLUS_ForceEnc != 0) : Enabling SetILCode in JIT event.");
}

if (SUCCEEDED(_profiler->GetRuntimeInformation(nullptr, &m_runtimeType, &major, &minor, &build, nullptr, 0,
nullptr, nullptr)))
{
m_runtimeVersion = VersionInfo{major, minor, build, 0};
}
HRESULT hr = profiler->QueryInterface(__uuidof(ICorProfilerInfo3), (void**) &_profiler);
if (FAILED(hr))
{
_profiler = nullptr;
trace::Logger::Error("Dataflow::Dataflow -> Something very wrong happened, as QI on ICorProfilerInfo3 failed. Disabling Dataflow. HRESULT : ", Hex(hr));
}
trace::Logger::Info("Dataflow::Dataflow -> Detected runtime version : ", m_runtimeVersion.ToString());
}
Dataflow::~Dataflow()
{
Expand All @@ -182,6 +186,10 @@ HRESULT Dataflow::Init()
{
return S_FALSE;
}
if (_profiler == nullptr)
{
return E_FAIL;
}
HRESULT hr = S_OK;
try
{
Expand Down Expand Up @@ -654,8 +662,9 @@ HRESULT Dataflow::RewriteMethod(MethodInfo* method, trace::FunctionControlWrappe
}
}
}
if (!pFunctionControl && written)
if (!pFunctionControl && written && !_setILOnJit)
{
// We are in JIT event. If _setILOnJit is false, we abort the commit and request a rejit
context.aborted = true;
}
method->SetInstrumented(written);
Expand All @@ -668,6 +677,10 @@ HRESULT Dataflow::RewriteMethod(MethodInfo* method, trace::FunctionControlWrappe
{
hr = method->ApplyFinalInstrumentation((ICorProfilerFunctionControl*) pFunctionControl);
}
else if (_setILOnJit)
{
hr = method->ApplyFinalInstrumentation();
}
else
{
std::vector<ModuleID> modulesVector = {module->_id};
Expand Down
3 changes: 2 additions & 1 deletion tracer/src/Datadog.Tracer.Native/iast/dataflow.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ namespace iast
friend class ModuleInfo;
friend class ModuleAspects;
public:
Dataflow(ICorProfilerInfo* profiler, std::shared_ptr<RejitHandler> rejitHandler);
Dataflow(ICorProfilerInfo* profiler, std::shared_ptr<RejitHandler> rejitHandler, const RuntimeInformation& runtimeInfo);
virtual ~Dataflow();
private:
CS _cs;
Expand All @@ -75,6 +75,7 @@ namespace iast
protected:
bool _initialized = false;
bool _loaded = false;
bool _setILOnJit = false;

std::vector<DataflowAspectClass*> _aspectClasses;
std::vector<DataflowAspect*> _aspects;
Expand Down
2 changes: 1 addition & 1 deletion tracer/test/Datadog.Trace.Security.IntegrationTests/AspNetBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ protected IImmutableList<MockSpan> WaitForSpans(MockTracerAgent agent, int expec
agent.SpanFilters.Add(s => s.Tags.ContainsKey("http.url") && s.Tags["http.url"].IndexOf(url, StringComparison.InvariantCultureIgnoreCase) > -1);
}

var spans = agent.WaitForSpans(expectedSpans, minDateTime: minDateTime);
var spans = agent.WaitForSpans(expectedSpans, minDateTime: minDateTime, assertExpectedCount: false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably not disable this by default 🤔 It was a big effort to enable it as we had a large number of cases where we were failing and we didn't know it 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can reenable it, but the reason for disabling it was that when not all the spans are received, we do not know which ones are missing, because the snapshot difference is skipped, and it is an inconvenience.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I seem to remember we came up with a way assert, keep a log of it, but not fail the test? Is it possible to do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get your point here. If there are fewer spans verify will fail and show it in the snapshot difference

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get your point here. If there are fewer spans verify will fail and show it in the snapshot difference

Unfortunately that assumes

  1. Every test is using Verify
  2. The test does not filter the spans, before sending them to Verify

As for how to do both, you can create an assertion scope, but it's probably not the right option here. Unfortunately, there's not really a great option right now.

if (spans.Count != expectedSpans)
{
Output?.WriteLine($"spans.Count: {spans.Count} != expectedSpans: {expectedSpans}, this is phase: {phase}");
Expand Down
102 changes: 60 additions & 42 deletions tracer/test/Datadog.Trace.Security.IntegrationTests/IAST/AspNetCore5IastTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ await VerifyHelper.VerifySpans(spansFiltered, settings)
}
}

// Class to test particular features
// Classes to test particular features
public class AspNetCore5IastTestsStackTraces : AspNetCore5IastTests
{
public AspNetCore5IastTestsStackTraces(AspNetCoreTestFixture fixture, ITestOutputHelper outputHelper)
Expand Down Expand Up @@ -406,6 +406,65 @@ await VerifyHelper.VerifySpans(spans, settings)
}
}

public class AspNetCore5IastTestsCompatEditAndContinue : AspNetCore5IastTests
{
public AspNetCore5IastTestsCompatEditAndContinue(AspNetCoreTestFixture fixture, ITestOutputHelper outputHelper)
: base(fixture, outputHelper, enableIast: true, testName: "AspNetCore5IastTestsCompat", samplingRate: 100, isIastDeduplicationEnabled: false, vulnerabilitiesPerRequest: 200, redactionEnabled: true)
{
SetEnvironmentVariable("COMPLUS_ForceEnc", "1");
}

[SkippableFact]
[Trait("RunOnWindows", "True")]
public async Task TestIastCustomSpanRequestManual()
{
var filename = "Iast.CustomManual.AspNetCore5." + (IastEnabled ? "IastEnabled" : "IastDisabled");
if (RedactionEnabled is true) { filename += ".RedactionEnabled"; }
var url = "/Iast/CustomManual?userName=Vicent";
IncludeAllHttpSpans = true;
await TryStartApp();
var agent = Fixture.Agent;
var spans = await SendRequestsAsync(agent, 3, new string[] { url });
var spansFiltered = spans.Where(s => !s.Resource.StartsWith("CREATE TABLE") && !s.Resource.StartsWith("INSERT INTO")).ToList();

var settings = VerifyHelper.GetSpanVerifierSettings();
settings.AddIastScrubbing();
await VerifyHelper.VerifySpans(spansFiltered, settings)
.UseFileName(filename)
.DisableRequireUniquePrefix();
}
}

public class AspNetCore5IastTestsCompat : AspNetCore5IastTestsCompatEditAndContinue
{
public AspNetCore5IastTestsCompat(AspNetCoreTestFixture fixture, ITestOutputHelper outputHelper)
: base(fixture, outputHelper)
{
SetEnvironmentVariable("COMPLUS_ForceEnc", "0");
}

[SkippableFact]
[Trait("RunOnWindows", "True")]
public async Task TestIastCustomSpanRequestAttribute()
{
var filename = "Iast.CustomAttribute.AspNetCore5." + (IastEnabled ? "IastEnabled" : "IastDisabled");
if (RedactionEnabled is true) { filename += ".RedactionEnabled"; }
var url = "/Iast/CustomAttribute?userName=Vicent";
IncludeAllHttpSpans = true;
await TryStartApp();
var agent = Fixture.Agent;
var spans = await SendRequestsAsync(agent, 2, new string[] { url });
var spansFiltered = spans.Where(s => !s.Resource.StartsWith("CREATE TABLE") && !s.Resource.StartsWith("INSERT INTO")).ToList();

var settings = VerifyHelper.GetSpanVerifierSettings();
settings.AddIastScrubbing();
settings.AddRegexScrubber(new Regex(@"_dd.agent_psr: .{1,3},"), string.Empty);
await VerifyHelper.VerifySpans(spansFiltered, settings)
.UseFileName(filename)
.DisableRequireUniquePrefix();
}
}

public class AspNetCore5IastTestsSpanTelemetryIastEnabled : AspNetCore5IastTests
{
public AspNetCore5IastTestsSpanTelemetryIastEnabled(AspNetCoreTestFixture fixture, ITestOutputHelper outputHelper)
Expand Down Expand Up @@ -1169,47 +1228,6 @@ await VerifyHelper.VerifySpans(spansFiltered, settings)
.DisableRequireUniquePrefix();
}

[SkippableFact]
[Trait("RunOnWindows", "True")]
public async Task TestIastCustomSpanRequestAttribute()
{
var filename = "Iast.CustomAttribute.AspNetCore5." + (IastEnabled ? "IastEnabled" : "IastDisabled");
if (RedactionEnabled is true) { filename += ".RedactionEnabled"; }
var url = "/Iast/CustomAttribute?userName=Vicent";
IncludeAllHttpSpans = true;
await TryStartApp();
var agent = Fixture.Agent;
var spans = await SendRequestsAsync(agent, 3, new string[] { url });
var spansFiltered = spans.Where(s => !s.Resource.StartsWith("CREATE TABLE") && !s.Resource.StartsWith("INSERT INTO")).ToList();

var settings = VerifyHelper.GetSpanVerifierSettings();
settings.AddIastScrubbing();
settings.AddRegexScrubber(new Regex(@"_dd.agent_psr: .{1,3},"), string.Empty);
await VerifyHelper.VerifySpans(spansFiltered, settings)
.UseFileName(filename)
.DisableRequireUniquePrefix();
}

[SkippableFact]
[Trait("RunOnWindows", "True")]
public async Task TestIastCustomSpanRequestManual()
{
var filename = "Iast.CustomManual.AspNetCore5." + (IastEnabled ? "IastEnabled" : "IastDisabled");
if (RedactionEnabled is true) { filename += ".RedactionEnabled"; }
var url = "/Iast/CustomManual?userName=Vicent";
IncludeAllHttpSpans = true;
await TryStartApp();
var agent = Fixture.Agent;
var spans = await SendRequestsAsync(agent, 3, new string[] { url });
var spansFiltered = spans.Where(s => !s.Resource.StartsWith("CREATE TABLE") && !s.Resource.StartsWith("INSERT INTO")).ToList();

var settings = VerifyHelper.GetSpanVerifierSettings();
settings.AddIastScrubbing();
await VerifyHelper.VerifySpans(spansFiltered, settings)
.UseFileName(filename)
.DisableRequireUniquePrefix();
}

[SkippableFact]
[Trait("RunOnWindows", "True")]
public async Task TestNHibernateSqlInjection()
Expand Down
Loading