Fix a race condition between certain Has properties and their collection property #843

Merged

mrvoorhe merged 1 commit from master-fix-race-between-has-property into master

2022-09-29 16:11:45 -05:00

mrvoorhe commented

2022-03-21 09:57:28 -05:00

(Migrated from github.com)

We found a rare race condition between MethodDefinition.HasOverrides and MethodDefinition.Overrides.

What can happen is

Thread 1 get's past the null check in MethodDefinition.HasOverrides and then is suspended.
Thread 2, calls MethodDefinition.Overrides and executes at least as far as the metadata.RemoveOverrideMapping (method) call in AssemblyReader.ReadOverrides
Thread 1 resumes on return HasImage && Module.Read (this, (method, reader) => reader.HasOverrides (method)); It now proceeds to AssemblyReader.HasOverrides. No overrides are found and false is returned due to the overrides for that method having been removed from MetadataSystem

To recap, the two notable behaviors are triggering this are

a) The following check in MethodDefinition.HasOverrides happens outside of the module lock.

if (overrides != null)
    return overrides.Count > 0;

b) The call to metadata.RemoveOverrideMapping in AssemblyReader.ReadOverrides means that AssemblyReader.ReadOverrides and AssemblyReader.HasOverrides cannot be called again after the first call to AssemblyReader.ReadOverrides

I did not attempt to reproduce this vulnerability for every pair of properties that follows this pattern. However, I think it's safe to assume any pair of properties that follows this same pattern are vulnerable.

Using ReadingMode.Deferred also appears to be a required prerequisite to encounter this problem.

We had two thoughts on how to fix this

Repeat the collection null check after obtaining the module lock in Module.Read during MethodDefinition.HasOverrides
Remove the behavior of AssemblyReader removing data from the MetadataSystem.

I decided to go with Fix 2 because it was easy to find all of problematic property pairings by searching MetadataSystem.cs for Remove. I also feel that this behavior of modifying the metadata system is asking for problems and probably not worth the freed memory is provides.

If you'd prefer Fix 1 instead. Or both Fix 1 & Fix 2, or some other fix, let me know and I can change around the PR.

We found a rare race condition between `MethodDefinition.HasOverrides` and `MethodDefinition.Overrides`. What can happen is 1) Thread 1 get's past the null check in `MethodDefinition.HasOverrides` and then is suspended. 2) Thread 2, calls `MethodDefinition.Overrides` and executes at least as far as the `metadata.RemoveOverrideMapping (method)` call in `AssemblyReader.ReadOverrides` 3) Thread 1 resumes on `return HasImage && Module.Read (this, (method, reader) => reader.HasOverrides (method));` It now proceeds to AssemblyReader.HasOverrides. No overrides are found and false is returned due to the overrides for that method having been removed from `MetadataSystem` To recap, the two notable behaviors are triggering this are a) The following check in `MethodDefinition.HasOverrides` happens outside of the module lock. ``` if (overrides != null) return overrides.Count > 0; ``` b) The call to `metadata.RemoveOverrideMapping` in `AssemblyReader.ReadOverrides` means that `AssemblyReader.ReadOverrides` and `AssemblyReader.HasOverrides` cannot be called again after the first call to `AssemblyReader.ReadOverrides` I did not attempt to reproduce this vulnerability for every pair of properties that follows this pattern. However, I think it's safe to assume any pair of properties that follows this same pattern are vulnerable. Using `ReadingMode.Deferred` also appears to be a required prerequisite to encounter this problem. We had two thoughts on how to fix this 1) Repeat the collection null check after obtaining the module lock in `Module.Read` during `MethodDefinition.HasOverrides` 2) Remove the behavior of `AssemblyReader` removing data from the `MetadataSystem`. I decided to go with Fix 2 because it was easy to find all of problematic property pairings by searching `MetadataSystem.cs` for `Remove`. I also feel that this behavior of modifying the metadata system is asking for problems and probably not worth the freed memory is provides. If you'd prefer Fix 1 instead. Or both Fix 1 & Fix 2, or some other fix, let me know and I can change around the PR.

mrvoorhe commented

2022-04-13 07:08:43 -05:00

(Migrated from github.com)