From 7a591c8edfe32cbd0b9b286ffe657d11c2f674e7 Mon Sep 17 00:00:00 2001 From: DotNet Bot Date: Sun, 10 Mar 2024 05:36:08 +0000 Subject: [PATCH 1/5] [internal/release/7.0] Update dependencies from dnceng/internal/dotnet-winforms --- NuGet.config | 12 ++++++------ eng/Version.Details.xml | 8 ++++---- eng/Versions.props | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/NuGet.config b/NuGet.config index aebd551f241..582a51bb118 100644 --- a/NuGet.config +++ b/NuGet.config @@ -6,9 +6,9 @@ - - - + + + @@ -24,10 +24,10 @@ + + + - - - diff --git a/eng/Version.Details.xml b/eng/Version.Details.xml index c7c186767a3..17399049b67 100644 --- a/eng/Version.Details.xml +++ b/eng/Version.Details.xml @@ -1,13 +1,13 @@ - + https://dev.azure.com/dnceng/internal/_git/dotnet-winforms - a21b74e7f5e96ccb8d95dcde605669feb30f6c5b + e712a599db6509c8a7c7df3722a29cb147bb49b8 - + https://dev.azure.com/dnceng/internal/_git/dotnet-winforms - a21b74e7f5e96ccb8d95dcde605669feb30f6c5b + e712a599db6509c8a7c7df3722a29cb147bb49b8 https://dev.azure.com/dnceng/internal/_git/dotnet-runtime diff --git a/eng/Versions.props b/eng/Versions.props index 3ea2074b825..fa9035cad5a 100644 --- a/eng/Versions.props +++ b/eng/Versions.props @@ -22,7 +22,7 @@ - 7.0.17-servicing.24116.2 + 7.0.18-servicing.24158.8 From ade7048e38c24818b992bd38447948c4b45de58b Mon Sep 17 00:00:00 2001 From: dipeshmsft Date: Mon, 11 Mar 2024 16:44:30 +0530 Subject: [PATCH 2/5] Fixes PenIMC UAF MSRC .NET 7 --- .../src/PenImc/dll/ComLockableWrapper.hpp | 5 ++ .../src/PenImc/dll/PimcManager.cpp | 88 +++++++++++++++++++ 2 files changed, 93 insertions(+) diff --git a/src/Microsoft.DotNet.Wpf/src/PenImc/dll/ComLockableWrapper.hpp b/src/Microsoft.DotNet.Wpf/src/PenImc/dll/ComLockableWrapper.hpp index b16aae55806..0d3f01befc0 100644 --- a/src/Microsoft.DotNet.Wpf/src/PenImc/dll/ComLockableWrapper.hpp +++ b/src/Microsoft.DotNet.Wpf/src/PenImc/dll/ComLockableWrapper.hpp @@ -40,6 +40,11 @@ namespace ComUtils // The apartment is verified during this call. HRESULT Unlock(); + // Unlocking a wrapper permanently nulls out the server object pointer, so a + // wrapper contains a non-null server object pointer if and only if it is + // bound to a server object which has never been unlocked. + bool HasNotBeenUnlocked() { return (m_serverObject != nullptr); } + private: IUnknown *m_serverObject; diff --git a/src/Microsoft.DotNet.Wpf/src/PenImc/dll/PimcManager.cpp b/src/Microsoft.DotNet.Wpf/src/PenImc/dll/PimcManager.cpp index 0c8b10a8af4..40aba9f1a24 100644 --- a/src/Microsoft.DotNet.Wpf/src/PenImc/dll/PimcManager.cpp +++ b/src/Microsoft.DotNet.Wpf/src/PenImc/dll/PimcManager.cpp @@ -396,6 +396,94 @@ void CPimcManager::TerminateHookThread(__inout CHookThreadItem * pThread) void CPimcManager::FinalRelease() { m_wispManagerLock.RevokeIfValid(); + + // + // The CComObject destructor is the only function which calls into this + // FinalRelease code. + // + // In all successful usage of CPimcManager: 1) Managed WPF code uses CoCreateInstance + // to acquire an IPimcManager2 interface to a brand-new CPimcManager instance (created by + // the ATL CComCreator::CreateInstance machinery), meaning FinalConstruct by-definition + // completes successfully, meaning "m_managerLock" is therefore guaranteed to be locked; + // 2) Managed WPF code then runs through its full end-to-end usage of the CPimcManager + // object (generally managed by the code in PenThreadWorker.cs); 3) When/if the managed WPF + // code determines that the CPimcManager object is no longer needed, it sends a + // RELEASE_MANAGER_EXT message (see UnsafeNativeMethods.ReleaseManagerExternalLock()) which + // unlocks "m_managerLock"; 4) Now that it is unlocked, the CComObject object + // can be destroyed when/if its refcount drops to zero, and this FinalRelease function will + // run at that time. + // + // So in all successful usage cases, it is guaranteed that "m_managerLock" is already + // unlocked when this code runs (because if it was still locked, the lock itself would have + // prevented the refcount from reaching zero, and would have prevented this function from + // ever running). + // + // That said, in unsuccessful usage cases, the ATL CComCreator::CreateInstance machinery + // can fail, meaning it will destroy the brand-new CPimcManager instance before returning + // an error back to the CreateInstance caller. Destroying the brand-new instance triggers + // the CComObject destructor and therefore calls into this function during + // the CComCreator::CreateInstance operation itself. + // + // The final step in CComCreator::CreateInstance is a QI which queries the newly-created + // object for whatever interface has been requested by the caller. This operation is the + // main way that CComCreator::CreateInstance can fail. For example, this QI is + // guaranteed to fail whenever the CoCreateInstance caller targets the CPimcManager CLSID + // but passes in a "random" IID that has nothing to do with IPimcManager2 or anything else + // that CPimcManager implements. + // + // (In CPimcManager construction, outside of pathological cases (e.g., where a small heap + // allocation in OS code fails due to out-of-memory), there are no other known ways that + // the CComCreator::CreateInstance sequence can fail; so the QI failure is the only + // failure mode that is known to be of general interest.) + // + // The QI failure can only occur after the preceding FinalConstruct call has completed + // successfully (since any FinalConstruct failure would have caused + // CComCreator::CreateInstance to abort without ever trying the QI); since + // CPimcManager::FinalConstruct always locks the "m_managerLock", this implies that the + // "m_managerLock" is guaranteed to be locked when this code runs (which is exactly + // opposite to what happens in all successful usage cases as discussed above). + // + // In this case, it is crucial to unlock "m_managerLock" before allowing this CPimcManager + // object to be destroyed. Without the unlock, this CPimcManager object would be destroyed + // while the associated CStdIdentity in the OS code still holds a reference to it; during + // any future apartment unload, the OS code would release this reference, and the release + // would be a use-after-free at that point. + // + // Note that the crucial unlock causes overactive ATL debug asserts to fire if a chk build + // of this DLL is used; specifically: + // + // - The code in the CComObject destructor always stomps the refcount to + // 0xc0000001 (i.e., "-(LONG_MAX/2)"), meaning this CPimcManager object's refcount is + // always 0xc0000001 when this code runs; unlocking "m_managerLock" will cause the refcount + // to drop by one (because, as discussed above, the crucial operation which prevents + // use-after-free problems will release the associated CStdIdentity's reference to this + // CPimcManager object, and in this way releases the reference that was added when + // "managerLock" was locked during FinalConstruct); as a result, unlocking "m_managerLock" + // will move this CPimcManager object's refcount through a "0xc0000001 -> 0xc0000000" + // transition. + // + // - Both of the CComObjectRootEx::InternalRelease specializations contain debug asserts + // which will fire whenever the refcount drops below 0xc0000001, so this transition always + // triggers a debug assert when using a chk build of this DLL. + // + // - That said, all evidence strongly suggests that this is just an overactive assert in + // the ATL code (probably just indicating that it is rare for FinalConstruct to add + // "self-references" like it does for CPimcManager (since these self-references generally + // prevent the server object from being destroyed unless a manual action like the + // RELEASE_MANAGER_EXT message is taken later on), meaning it is rare to have a situation + // where FinalRelease needs to release self-references that were acquired in + // FinalConstruct, meaning this is a rare enough case that the ATL authors either didn't + // test it or didn't think it was common enough to warrant adjusting the assert). + // + // Since this change is being made in servicing, attempt to change behavior as little as + // possible in the "successful usage" cases where "m_managerLock" is already unlocked, + // while still ensuring that FinalRelease will always run the crucial unlock in all + // "unsuccessful usage" cases. + // + if (m_managerLock.HasNotBeenUnlocked()) + { + m_managerLock.Unlock(); + } } ///////////////////////////////////////////////////////////////////////////// From fce8e9b01fbbadc0e7d6d6872e3fb3b78241cc81 Mon Sep 17 00:00:00 2001 From: DotNet-Bot Date: Tue, 12 Mar 2024 03:24:00 +0000 Subject: [PATCH 3/5] Update dependencies from https://dev.azure.com/dnceng/internal/_git/dotnet-winforms build 20240311.11 Microsoft.Dotnet.WinForms.ProjectTemplates , Microsoft.Private.Winforms From Version 7.0.18-servicing.24158.8 -> To Version 7.0.18-servicing.24161.11 --- eng/Version.Details.xml | 8 ++++---- eng/Versions.props | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/eng/Version.Details.xml b/eng/Version.Details.xml index 17399049b67..cd408b3060d 100644 --- a/eng/Version.Details.xml +++ b/eng/Version.Details.xml @@ -1,13 +1,13 @@ - + https://dev.azure.com/dnceng/internal/_git/dotnet-winforms - e712a599db6509c8a7c7df3722a29cb147bb49b8 + c6e14bc32bbbb07c3dd91efd245e696214599d01 - + https://dev.azure.com/dnceng/internal/_git/dotnet-winforms - e712a599db6509c8a7c7df3722a29cb147bb49b8 + c6e14bc32bbbb07c3dd91efd245e696214599d01 https://dev.azure.com/dnceng/internal/_git/dotnet-runtime diff --git a/eng/Versions.props b/eng/Versions.props index fa9035cad5a..61b1a4a0939 100644 --- a/eng/Versions.props +++ b/eng/Versions.props @@ -22,7 +22,7 @@ - 7.0.18-servicing.24158.8 + 7.0.18-servicing.24161.11 From f5177f87007b54bc227724f7ef60a1035e95a16e Mon Sep 17 00:00:00 2001 From: Sean Reeser Date: Mon, 18 Mar 2024 11:48:40 -0700 Subject: [PATCH 4/5] Local dependencies updated based on build with BAR id 217431 (20240314.4 from https://dev.azure.com/dnceng/internal/_git/dotnet-winforms@refs/heads/internal/release/7.0) --- NuGet.config | 10 ++-------- eng/Version.Details.xml | 20 ++++++++++---------- eng/Versions.props | 8 ++++---- 3 files changed, 16 insertions(+), 22 deletions(-) diff --git a/NuGet.config b/NuGet.config index 582a51bb118..468990e032c 100644 --- a/NuGet.config +++ b/NuGet.config @@ -5,10 +5,7 @@ - - - - + @@ -24,10 +21,7 @@ - - - - + diff --git a/eng/Version.Details.xml b/eng/Version.Details.xml index cd408b3060d..1f906f266db 100644 --- a/eng/Version.Details.xml +++ b/eng/Version.Details.xml @@ -1,13 +1,13 @@ - + https://dev.azure.com/dnceng/internal/_git/dotnet-winforms - c6e14bc32bbbb07c3dd91efd245e696214599d01 + 825dfb818b455c03681f7215ce60ca304b3456d5 - + https://dev.azure.com/dnceng/internal/_git/dotnet-winforms - c6e14bc32bbbb07c3dd91efd245e696214599d01 + 825dfb818b455c03681f7215ce60ca304b3456d5 https://dev.azure.com/dnceng/internal/_git/dotnet-runtime @@ -61,17 +61,17 @@ https://dev.azure.com/dnceng/internal/_git/dotnet-runtime d099f075e45d2aa6007a22b71b45a08758559f80 - + https://dev.azure.com/dnceng/internal/_git/dotnet-runtime - dff486f2d78d3f932d0f9bfa38043f85e358fb8c + e963835596dce0e4258709f53167661b91728a62 - + https://dev.azure.com/dnceng/internal/_git/dotnet-runtime - dff486f2d78d3f932d0f9bfa38043f85e358fb8c + e963835596dce0e4258709f53167661b91728a62 - + https://dev.azure.com/dnceng/internal/_git/dotnet-runtime - dff486f2d78d3f932d0f9bfa38043f85e358fb8c + e963835596dce0e4258709f53167661b91728a62 diff --git a/eng/Versions.props b/eng/Versions.props index 61b1a4a0939..beb9bbd3730 100644 --- a/eng/Versions.props +++ b/eng/Versions.props @@ -22,13 +22,13 @@ - 7.0.18-servicing.24161.11 + 7.0.18-servicing.24164.4 - 7.0.17-servicing.24115.8 - 7.0.17 - 7.0.17 + 7.0.18-servicing.24163.20 + 7.0.18 + 7.0.18 7.0.4 7.0.0 7.0.0 From 867af66a2f5e3a4ca303fe0c63ab289eacc38f8e Mon Sep 17 00:00:00 2001 From: DotNet-Bot Date: Wed, 20 Mar 2024 04:34:35 +0000 Subject: [PATCH 5/5] Update dependencies from https://dev.azure.com/dnceng/internal/_git/dotnet-winforms build Microsoft.Dotnet.WinForms.ProjectTemplates , Microsoft.Private.Winforms From Version 7.0.18-servicing.24164.4 -> To Version 7.0.18-servicing.24169.17 Dependency coherency updates Microsoft.NETCore.App.Ref,Microsoft.NETCore.App.Runtime.win-x64,VS.Redist.Common.NetCore.SharedFramework.x64.7.0 From Version 7.0.18 -> To Version 7.0.18 (parent: Microsoft.Private.Winforms --- NuGet.config | 4 ++-- eng/Version.Details.xml | 16 ++++++++-------- eng/Versions.props | 4 ++-- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/NuGet.config b/NuGet.config index 468990e032c..f9a5688a957 100644 --- a/NuGet.config +++ b/NuGet.config @@ -5,7 +5,7 @@ - + @@ -21,7 +21,7 @@ - + diff --git a/eng/Version.Details.xml b/eng/Version.Details.xml index 1f906f266db..6c8c5446cf2 100644 --- a/eng/Version.Details.xml +++ b/eng/Version.Details.xml @@ -1,13 +1,13 @@ - + https://dev.azure.com/dnceng/internal/_git/dotnet-winforms - 825dfb818b455c03681f7215ce60ca304b3456d5 + 1a5db9acafb5677490e44ad00d91cfff0dc11ddf - + https://dev.azure.com/dnceng/internal/_git/dotnet-winforms - 825dfb818b455c03681f7215ce60ca304b3456d5 + 1a5db9acafb5677490e44ad00d91cfff0dc11ddf https://dev.azure.com/dnceng/internal/_git/dotnet-runtime @@ -63,15 +63,15 @@ https://dev.azure.com/dnceng/internal/_git/dotnet-runtime - e963835596dce0e4258709f53167661b91728a62 + 544c7e6eb3d5525c6f85341f51217d81d7c8ed80 https://dev.azure.com/dnceng/internal/_git/dotnet-runtime - e963835596dce0e4258709f53167661b91728a62 + 544c7e6eb3d5525c6f85341f51217d81d7c8ed80 - + https://dev.azure.com/dnceng/internal/_git/dotnet-runtime - e963835596dce0e4258709f53167661b91728a62 + 544c7e6eb3d5525c6f85341f51217d81d7c8ed80 diff --git a/eng/Versions.props b/eng/Versions.props index beb9bbd3730..419c2f6d566 100644 --- a/eng/Versions.props +++ b/eng/Versions.props @@ -22,11 +22,11 @@ - 7.0.18-servicing.24164.4 + 7.0.18-servicing.24169.17 - 7.0.18-servicing.24163.20 + 7.0.18-servicing.24169.14 7.0.18 7.0.18 7.0.4