Use RegDeleteTree in RegistryKey.DeleteSubKeyTree

[huoyaoyuan]

Fixes #82586

[ghost]

Tagging subscribers to this area: @dotnet/area-microsoft-win32
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes #82586

Author:	huoyaoyuan
Assignees:	-
Labels:	area-Microsoft.Win32, community-contribution
Milestone:	-

[carlossanlop]

cc @stephentoub @danmoseley FYI

[danmoseley]

I'm guessing this was simply because the API only appeared in Vista and this was written before then?

[danmoseley]

Doc says:

A handle to an open registry key. The key must have been opened with the following access rights: DELETE, KEY_ENUMERATE_SUB_KEYS, and KEY_QUERY_VALUE. For more information, see Registry Key Security and Access Rights.

but for RegDeleteKeyEx it does not require this

A handle to an open registry key. The access rights of this key do not affect the delete operation. For more information about access rights, see Registry Key Security and Access Rights.

Is there any way this change could cause something to fail that worked before?

[huoyaoyuan]

I don't think it can cause deletion to fail. Should we add a manual access check to restore existing behavior?

[danmoseley]

you mean, you've verified that all codepaths that lead here provide a handle that has DELETE, KEY_ENUMERATE_SUB_KEYS, and KEY_QUERY_VALUE?

[huoyaoyuan]

This is a public API. I mean that we may perform additional access check to match the document.
I've checked Windows source code and RegDeleteKeyEx uses the same enumeration process. I'm not sure where the access check happens though.

[ericstj]

Maybe you can try to answer this question through testing. Is there a case where you can specify different values of RegistryRights when opening the key and see if you can create a situation where it would pass before but fails now?

I tend to agree with you that the old case should have needed these permissions - should be pretty easy to test to prove that.

[huoyaoyuan]

Perhaps we need a matrix for behaviors of the old and new way. The old way should also cause partial deletion in some cases.

[huoyaoyuan]

Notes after reading more about registry:

• The RegistryKeyPermissionCheck or isWritable check only happens at managed side. It can be easily replicated.
• Registry checks the access of current user, not desired access of the handle.

So special casing "" should be enough. No matrix needed actually.

[huoyaoyuan]

The following code fails to delete with P/Invoke, but successes with DeleteSubKeyTree on .NET 7:

I can't reproduce the failure now. Maybe I missed something.

Edit: RegDeleteTree returns ACCESS_DENIED when lpSubKey is "" and the key contains values.

[danmoseley]

I think the key thing here is to have tests that pass on both the old and new ways, if we don't already, to prove there's no visible change in behavior. There are no doubt snippets of code elsewhere in the tests that you can grab to set ACL's on keys, etc.

[huoyaoyuan]

The failures should be the documented case of RegDeleteTree:

If the key has values, it must be opened with KEY_SET_VALUE or this function will fail with ERROR_ACCESS_DENIED.

I think covering the case should be enough. ACL handlings shouldn't be changed.

[ViktorHofer]

@huoyaoyuan can you please add tests that protects the new behavior and assert that scenarios that should fail, do fail (above conversation)?

[huoyaoyuan]

Sure, I'll also decide the matrix to test when I'm available.

[huoyaoyuan]

I think it's ready now. @danmoseley should we bring this into 8.0? How about the risk?

[danmoseley]

That's up to the area owner but I see no particular reason to port it.

[stephentoub]

If it gets merged into main before main forks for .NET 9 (which has not yet happened), then it'll be in .NET 8. If it misses that cut-off, it'd be in .NET 9.

[ViktorHofer]

Thank you a lot for the contribution. As you restored the previous behavior around key self-deletion and you added tests for it, I think the change is ready. Merging.