From e7fd68009347c362b771a4589e963fc8d147394b Mon Sep 17 00:00:00 2001 From: Daniel Romano <108014683+daniel-romano-DD@users.noreply.github.com> Date: Tue, 29 Aug 2023 14:21:30 +0200 Subject: [PATCH] [ASM] DecompileDelegate lib bugfix (#4554) * Initial flawed fix * Working fix * Minor improvement * Linux compilation fix * Added more tests --- .../Datadog.Tracer.Native/iast/dataflow.cpp | 25 ++++++- .../src/Datadog.Tracer.Native/iast/dataflow.h | 7 +- .../iast/method_info.cpp | 59 +++++++++++++++ .../Datadog.Tracer.Native/iast/method_info.h | 19 +++++ .../iast/module_info.cpp | 74 +++++++++++++++++++ .../Datadog.Tracer.Native/iast/module_info.h | 9 +++ .../Samples.InstrumentedTests.csproj | 1 + .../SqlInjection/DabaseHelpers/TestClasses.cs | 18 +++++ .../Vulnerabilities/SqlInjection/EFTests.cs | 46 ++++++++++++ 9 files changed, 255 insertions(+), 3 deletions(-) diff --git a/tracer/src/Datadog.Tracer.Native/iast/dataflow.cpp b/tracer/src/Datadog.Tracer.Native/iast/dataflow.cpp index eeb5c1ebf889..7af0fa8664b7 100644 --- a/tracer/src/Datadog.Tracer.Native/iast/dataflow.cpp +++ b/tracer/src/Datadog.Tracer.Native/iast/dataflow.cpp @@ -78,6 +78,7 @@ static const WSTRING _fixedAssemblyExcludeFilters[] = { WStr("vstest.console"), WStr("testhost.*"), WStr("Oracle.ManagedDataAccess"), + WStr("DelegateDecompiler*"), LastEntry, // Can't have an empty array. This must be the last element }; static const WSTRING _fixedMethodIncludeFilters[] = { @@ -105,6 +106,10 @@ static const WSTRING _fixedMethodExcludeFilters[] = { WStr("MongoDB.*"), LastEntry, // Can't have an empty array. This must be the last element }; +static const WSTRING _fixedMethodAttributeExcludeFilters[] = { + WStr("DelegateDecompiler.ComputedAttribute"), + LastEntry, // Can't have an empty array. This must be the last element +}; ModuleAspects::ModuleAspects(Dataflow* dataflow, ModuleInfo* module) { @@ -203,6 +208,13 @@ HRESULT Dataflow::Init() { _methodExcludeFilters.push_back(_fixedMethodExcludeFilters[x]); } + + // Method attribute filters + for (int x = 0; _fixedMethodAttributeExcludeFilters[x] != LastEntry; x++) + { + _methodAttributeExcludeFilters.push_back(_fixedMethodAttributeExcludeFilters[x]); + } + //}); } catch (std::exception& err) @@ -441,10 +453,19 @@ bool Dataflow::IsAssemblyExcluded(const WSTRING& assemblyName, MatchResult* incl { return IsExcluded(_assemblyIncludeFilters, _assemblyExcludeFilters, assemblyName, includedMatch, excludedMatch); } -bool Dataflow::IsMethodExcluded(const WSTRING& signature, MatchResult* includedMatch, MatchResult* excludedMatch) +bool Dataflow::IsMethodExcluded(const WSTRING& methodSignature, MatchResult* includedMatch, MatchResult* excludedMatch) { - return IsExcluded(_methodIncludeFilters, _methodExcludeFilters, signature, includedMatch, excludedMatch); + return IsExcluded(_methodIncludeFilters, _methodExcludeFilters, methodSignature, includedMatch, excludedMatch); } +bool Dataflow::IsMethodAttributeExcluded(const WSTRING& attributeName, MatchResult* includedMatch, MatchResult* excludedMatch) +{ + return IsExcluded(_methodAttributeIncludeFilters, _methodAttributeExcludeFilters, attributeName, includedMatch, excludedMatch); +} +bool Dataflow::HasMethodAttributeExclusions() +{ + return _methodAttributeExcludeFilters.size() > 0; +} + ICorProfilerInfo* Dataflow::GetCorProfilerInfo() { diff --git a/tracer/src/Datadog.Tracer.Native/iast/dataflow.h b/tracer/src/Datadog.Tracer.Native/iast/dataflow.h index 09d30388e70b..e5c1c2ed0550 100644 --- a/tracer/src/Datadog.Tracer.Native/iast/dataflow.h +++ b/tracer/src/Datadog.Tracer.Native/iast/dataflow.h @@ -63,6 +63,8 @@ namespace iast std::vector _assemblyExcludeFilters; std::vector _methodIncludeFilters; std::vector _methodExcludeFilters; + std::vector _methodAttributeIncludeFilters; + std::vector _methodAttributeExcludeFilters; int _callsiteInstrumentedSources = 0; int _callsiteInstrumentedPropagations = 0; @@ -106,8 +108,11 @@ namespace iast MatchResult* excludedMatch = nullptr); bool IsAssemblyExcluded(const WSTRING& assemblyName, MatchResult* includedMatch = nullptr, MatchResult* excludedMatch = nullptr); - bool IsMethodExcluded(const WSTRING& signature, MatchResult* includedMatch = nullptr, + bool IsMethodExcluded(const WSTRING& methodSignature, MatchResult* includedMatch = nullptr, MatchResult* excludedMatch = nullptr); + bool IsMethodAttributeExcluded(const WSTRING& attributeName, MatchResult* includedMatch = nullptr, + MatchResult* excludedMatch = nullptr); + bool HasMethodAttributeExclusions(); AppDomainInfo* GetAppDomain(AppDomainID id); ModuleInfo* GetModuleInfo(ModuleID moduleId); diff --git a/tracer/src/Datadog.Tracer.Native/iast/method_info.cpp b/tracer/src/Datadog.Tracer.Native/iast/method_info.cpp index cfd70fd0f7c8..639beedd9d4a 100644 --- a/tracer/src/Datadog.Tracer.Native/iast/method_info.cpp +++ b/tracer/src/Datadog.Tracer.Native/iast/method_info.cpp @@ -155,6 +155,11 @@ namespace iast return ELEMENT_TYPE_VOID; } + std::vector MemberRefInfo::GetCustomAttributes() + { + return _module->GetCustomAttributes(_id); + } + //---------------------------- FieldInfo::FieldInfo(ModuleInfo* pModuleInfo, mdFieldDef fieldDef) : @@ -172,6 +177,22 @@ namespace iast } + //---------------------------- + + PropertyInfo::PropertyInfo(ModuleInfo* pModuleInfo, mdProperty propId) : + MemberRefInfo(pModuleInfo, propId) + { + WCHAR methodName[1024]; + ULONG methodNameLength; + DWORD attrs; + + HRESULT hr = pModuleInfo->_metadataImport->GetPropertyProps( + propId, &_typeDef, methodName, 1024, &methodNameLength, &_methodAttributes, &_pSig, &_nSig, nullptr, + nullptr, nullptr, &_setter, &_getter, nullptr, 0, nullptr); + + _name = methodName; + } + //---------------------------- MethodSpec::MethodSpec(ModuleInfo* pModuleInfo, mdMethodSpec methodSpec) : @@ -255,6 +276,18 @@ namespace iast if (_isExcluded < 0) { _isExcluded = _module->_dataflow->IsMethodExcluded(GetFullName()); + if (!_isExcluded && _module->_dataflow->HasMethodAttributeExclusions()) + { + for (auto methodAttribute : GetCustomAttributes()) + { + if (_module->_dataflow->IsMethodAttributeExcluded(methodAttribute)) + { + _isExcluded = true; + break; + } + } + + } } return _isExcluded; } @@ -526,4 +559,30 @@ namespace iast _nMethodIL = 0; DEL_ARR(_pMethodIL); } + + bool MethodInfo::IsPropertyAccessor() + { + auto name = shared::ToString(_name); + return StartsWith(name, "get_") || StartsWith(name, "set_"); + } + + std::vector MethodInfo::GetCustomAttributes() + { + auto res = MemberRefInfo::GetCustomAttributes(); + if (IsPropertyAccessor()) + { + // Retrieve property Attributes + auto properties = _module->GetProperties(_typeDef); + for (auto property : properties) + { + if (property->GetGetterId() != _id && property->GetSetterId() != _id) + { + continue; + } + AddRange(res, _module->GetCustomAttributes(property->GetPropertyId())); + } + } + return res; + } + } diff --git a/tracer/src/Datadog.Tracer.Native/iast/method_info.h b/tracer/src/Datadog.Tracer.Native/iast/method_info.h index 658465986b6c..593df164dc94 100644 --- a/tracer/src/Datadog.Tracer.Native/iast/method_info.h +++ b/tracer/src/Datadog.Tracer.Native/iast/method_info.h @@ -64,6 +64,7 @@ namespace iast virtual SignatureInfo* GetSignature(); ULONG GetParameterCount(); CorElementType GetReturnCorType(); + virtual std::vector GetCustomAttributes(); private: std::atomic _fullNameCounterLock; @@ -95,6 +96,21 @@ namespace iast mdFieldDef GetFieldDef(); }; + class PropertyInfo : public MemberRefInfo + { + friend class ILRewriter; + friend class ModuleInfo; + protected: + mdMethodDef _getter; + mdMethodDef _setter; + public: + PropertyInfo(ModuleInfo* pModuleInfo, mdProperty mdProperty); + + inline mdProperty GetPropertyId() { return _id; } + inline mdMethodDef GetGetterId() { return _getter; } + inline mdMethodDef GetSetterId() { return _setter; } + }; + class MethodInfo : public MemberRefInfo { friend class ILRewriter; @@ -147,5 +163,8 @@ namespace iast void ReJITCompilationFinished(); void DumpIL(std::string message = "", ULONG pnMethodIL = 0, LPCBYTE pMethodIL = nullptr); + + bool IsPropertyAccessor(); + std::vector GetCustomAttributes() override; }; } \ No newline at end of file diff --git a/tracer/src/Datadog.Tracer.Native/iast/module_info.cpp b/tracer/src/Datadog.Tracer.Native/iast/module_info.cpp index 1df1f36e0f45..4addd1359973 100644 --- a/tracer/src/Datadog.Tracer.Native/iast/module_info.cpp +++ b/tracer/src/Datadog.Tracer.Native/iast/module_info.cpp @@ -40,6 +40,7 @@ ModuleInfo::~ModuleInfo() DEL_MAP_VALUES(_specs); DEL_MAP_VALUES(_methods); DEL_MAP_VALUES(_fields); + DEL_MAP_VALUES(_properties); DEL_MAP_VALUES(_signatures); } @@ -159,6 +160,10 @@ MemberRefInfo* ModuleInfo::GetMemberRefInfo(mdMemberRef token) { return GetFieldInfo(token); } + else if (typeFromToken == mdtProperty) + { + return GetPropertyInfo(token); + } else if (typeFromToken == mdtMethodSpec) { return GetMethodSpec(token); @@ -182,6 +187,12 @@ FieldInfo* ModuleInfo::GetFieldInfo(mdFieldDef fieldDef) return Get(_fields, fieldDef, [this, fieldDef]() { return new iast::FieldInfo(this, fieldDef); }); } +PropertyInfo* ModuleInfo::GetPropertyInfo(mdProperty propId) +{ + CSGUARD(_cs); + return Get(_properties, propId, [this, propId]() { return new iast::PropertyInfo(this, propId); }); +} + MethodSpec* ModuleInfo::GetMethodSpec(mdMethodSpec methodSpec) { CSGUARD(_cs); @@ -260,6 +271,34 @@ std::vector ModuleInfo::GetMethods(mdTypeDef typeDef) this->_metadataImport->CloseEnum(hCorEnum); return methods; } + +std::vector ModuleInfo::GetProperties(mdTypeDef typeDef) +{ + HCORENUM hCorEnum = nullptr; + std::vector res; + mdProperty elements[64]; + ULONG elementCount; + + HRESULT hr = this->_metadataImport->EnumProperties(&hCorEnum, typeDef, elements, + sizeof(elements) / sizeof(elements[0]), + &elementCount); + if (SUCCEEDED(hr) && elementCount > 0) + { + res.reserve(elementCount); + for (ULONG i = 0; i < elementCount; i++) + { + auto element = GetPropertyInfo(elements[i]); + if (element) + { + res.push_back(element); + } + } + } + + this->_metadataImport->CloseEnum(hCorEnum); + return res; +} + std::vector ModuleInfo::GetMethods(mdTypeDef typeDef, const WSTRING& name) { HCORENUM hCorEnum = nullptr; @@ -835,4 +874,39 @@ mdToken ModuleInfo::DefineMemberRef(const WSTRING& moduleName, const WSTRING& ty return 0; } } + +std::vector ModuleInfo::GetCustomAttributes(mdToken token) +{ + std::vector res; + + HCORENUM hCorEnum = nullptr; + mdCustomAttribute enumeratedElements[64]; + ULONG enumCount; + + while (_metadataImport->EnumCustomAttributes(&hCorEnum, token, mdTokenNil, enumeratedElements, + sizeof(enumeratedElements) / sizeof(enumeratedElements[0]), + &enumCount) == S_OK) + { + mdToken attributeParentToken = mdTokenNil; + mdToken attributeCtorToken = mdTokenNil; + const void* attribute_data = nullptr; // Pointer to receive attribute data, which is not needed for our purposes + DWORD data_size = 0; + + for (ULONG i = 0; i < enumCount; i++) + { + HRESULT hr = _metadataImport->GetCustomAttributeProps(enumeratedElements[i], &attributeParentToken, + &attributeCtorToken, &attribute_data, &data_size); + if (SUCCEEDED(hr)) + { + auto attrCtor = GetMemberRefInfo(attributeCtorToken); + res.push_back(attrCtor->GetTypeName()); + } + } + } + + _metadataImport->CloseEnum(hCorEnum); + return res; +} + + } // namespace iast diff --git a/tracer/src/Datadog.Tracer.Native/iast/module_info.h b/tracer/src/Datadog.Tracer.Native/iast/module_info.h index 93bd2b9a3ac3..efe7f06d9c96 100644 --- a/tracer/src/Datadog.Tracer.Native/iast/module_info.h +++ b/tracer/src/Datadog.Tracer.Native/iast/module_info.h @@ -17,6 +17,7 @@ namespace iast class MethodSpec; class MethodInfo; class FieldInfo; + class PropertyInfo; class DataflowAspect; class DataflowAspectReference; class AspectFilter; @@ -30,10 +31,12 @@ namespace iast friend class Dataflow; friend class ILRewriter; friend class TypeInfo; + friend class MemberInfo; friend class MemberRefInfo; friend class MethodSpec; friend class MethodInfo; friend class FieldInfo; + friend class PropertyInfo; friend class DataflowAspectClass; friend class DataflowAspect; friend class DataflowAspectReference; @@ -51,6 +54,7 @@ namespace iast std::unordered_map _members; std::unordered_map _methods; std::unordered_map _fields; + std::unordered_map _properties; std::unordered_map _specs; std::unordered_map _signatures; @@ -117,6 +121,7 @@ namespace iast MemberRefInfo* GetMemberRefInfo(mdMemberRef token); MethodInfo* GetMethodInfo(mdMethodDef methodDef); FieldInfo* GetFieldInfo(mdFieldDef fieldDef); + PropertyInfo* GetPropertyInfo(mdProperty propId); MethodSpec* GetMethodSpec(mdMethodSpec methodSpec); SignatureInfo* GetSignature(mdSignature sigToken); WSTRING GetUserString(mdString token); @@ -126,6 +131,8 @@ namespace iast std::vector GetMethods(mdTypeDef typeDef, const WSTRING& methodName, ULONG paramCount); std::vector GetMethods(const WSTRING& typeName, const WSTRING& methodName); + std::vector GetProperties(mdTypeDef typeDef); + MethodInfo* GetMethod(const WSTRING& typeName, const WSTRING& methodName, PCCOR_SIGNATURE pSignature, ULONG nSignature); MethodInfo* GetMethod(mdTypeDef typeDef, const WSTRING& methodName, PCCOR_SIGNATURE pSignature, ULONG nSignature); MethodInfo* GetMethod(const WSTRING& typeName, const WSTRING& methodName, int paramCount = 0); @@ -142,5 +149,7 @@ namespace iast HRESULT FindMemberRefsByName(mdTypeRef typeRef, const WSTRING& memberName, std::vector& members); HRESULT GetAssemblyTypeRef(const WSTRING& assemblyName, const WSTRING& typeName, mdTypeRef* typeRef); + + std::vector GetCustomAttributes(mdToken token); }; } \ No newline at end of file diff --git a/tracer/test/test-applications/integrations/Samples.InstrumentedTests/Samples.InstrumentedTests.csproj b/tracer/test/test-applications/integrations/Samples.InstrumentedTests/Samples.InstrumentedTests.csproj index 1fcc0539f7a9..206c30c33426 100644 --- a/tracer/test/test-applications/integrations/Samples.InstrumentedTests/Samples.InstrumentedTests.csproj +++ b/tracer/test/test-applications/integrations/Samples.InstrumentedTests/Samples.InstrumentedTests.csproj @@ -25,6 +25,7 @@ + diff --git a/tracer/test/test-applications/integrations/Samples.InstrumentedTests/Vulnerabilities/SqlInjection/DabaseHelpers/TestClasses.cs b/tracer/test/test-applications/integrations/Samples.InstrumentedTests/Vulnerabilities/SqlInjection/DabaseHelpers/TestClasses.cs index a7561efa465c..70c569c2e932 100644 --- a/tracer/test/test-applications/integrations/Samples.InstrumentedTests/Vulnerabilities/SqlInjection/DabaseHelpers/TestClasses.cs +++ b/tracer/test/test-applications/integrations/Samples.InstrumentedTests/Vulnerabilities/SqlInjection/DabaseHelpers/TestClasses.cs @@ -5,4 +5,22 @@ public class Book public string Id { get; set; } public string Title { get; set; } public string Author { get; set; } + + [DelegateDecompiler.Computed] + public string FullId + { + get => Id + "-" + Title + "-" + Author; + } + + public string FullTitle + { + [DelegateDecompiler.Computed] + get => Title + "_"; + } + + public string FullAuthor + { + get => "_" + Author; + } + } diff --git a/tracer/test/test-applications/integrations/Samples.InstrumentedTests/Vulnerabilities/SqlInjection/EFTests.cs b/tracer/test/test-applications/integrations/Samples.InstrumentedTests/Vulnerabilities/SqlInjection/EFTests.cs index cb8ac9f371ce..dbd06e9a987e 100644 --- a/tracer/test/test-applications/integrations/Samples.InstrumentedTests/Vulnerabilities/SqlInjection/EFTests.cs +++ b/tracer/test/test-applications/integrations/Samples.InstrumentedTests/Vulnerabilities/SqlInjection/EFTests.cs @@ -1,9 +1,11 @@ #if !NETCOREAPP2_1 +using System; using System.Data; using System.Data.Entity.Core.EntityClient; using System.Data.Entity.Infrastructure; using System.Data.SqlClient; using System.Linq; +using DelegateDecompiler; using FluentAssertions; using Xunit; @@ -51,5 +53,49 @@ protected override EntityCommand GetEntityCommand(string title) var query = new EntityCommand(queryString, conn); return query; } + + [Fact] + public void TestDelegateDecompileLibBug_PropertyAttribute() + { + var all = (db as ApplicationDbContext).Books.ToList(); + all.Count.Should().Be(2); + var book = all[1]; + var txt = book.FullId; + var res = (db as ApplicationDbContext).Books.Decompile().Where(x => x.FullId == txt).ToList(); + res.Count().Should().Be(1); + } + + [Fact] + public void TestDelegateDecompileLibBug_PropertyGetterAttribute() + { + // DecompileDelegate does not support the use of the attribute only on the getter + Assert.Throws(() => + { + var all = (db as ApplicationDbContext).Books.ToList(); + all.Count.Should().Be(2); + var book = all[1]; + var txt = book.FullTitle; + var res = (db as ApplicationDbContext).Books.Decompile().Where(x => x.FullTitle == txt).ToList(); + res.Count().Should().Be(1); + }); + } + + + [Fact] + public void TestDelegateDecompileLibBug_NoAttribute() + { + // We don't support this scenario + Assert.Throws(() => + { + var all = (db as ApplicationDbContext).Books.ToList(); + all.Count.Should().Be(2); + var book = all[1]; + var txt = book.FullAuthor; + var res = (db as ApplicationDbContext).Books.Decompile().Where(x => x.FullAuthor == txt).ToList(); + res.Count().Should().Be(1); + }); + } + + } #endif