ComodoSecurity / openedr

Open EDR public repository
Other
2.19k stars 434 forks source link

Apply a Security Descriptor to the Driver's Device Object #1

Closed matterpreter closed 3 years ago

matterpreter commented 3 years ago

Hey there,

Congrats on the release! This is a really exciting project.

During a quick review, I found that you all are using IoCreateDevice() to create the driver's device object in ioctl.cpp. This function does not allow access to the device object to be restricted, so it defaults to being accessible by all users. With access to the device object through the associated symlink, any* user could start sending IOCTLs to the driver.

More information: CWE-782

The protection you all have in place to check whether the process sending the IRP is in the list of those authorized to communicate with the driver mitigates a good amount of the risk as even Local Administrators can't directly talk to the driver if their process isn't explicitly allowed.

Subverting this check would involve injecting code into one of the "allowed" processes, which typically would require administrator rights. However, if there were an external misconfiguration, such as if low-privileged users are granted SeDebugPrivilege/SeTakeOwnershipPrivilege (which would never happen in an enterprise, right? 😉), they would gain the ability to inject into the process(es) and interact with the driver. Its not immediately clear how this list is initially populated, but it looks like access is controlled via PID, which has been proven to be able to be bypassed as well. This may be a separate thing to look at though.

Additionally, isCurProcessTrusted() is not implemented in CMD_ERDDRV_IOCTL_STOP, which wold allow any user to stop monitoring on the host regardless of what process they are in.

This PR migrates to using IoCreateDeviceSecure() and a security descriptor which only allows LocalSystem and Administrators access to the device object.

Note: I was unable to fully validate this due to some issues compiling with the Netfilter and MadCodeHook dependencies, but this is a relatively simple change.

ComodoMelih commented 3 years ago

The architectural question is : What do we want from EDR? Do we want a) code that protects b) code that provides telemetry (which in turn is used to help detect etc)

Architecturally they are two different things for me (my subjective view). Just like a Door that protects a home vs burglar alarm. In my view the protection should be provided by EPP (End Point Protection) unit, while EDR should be providing the telemetry. So I would expect the protection to run on an endpoint to help avoid the issues you are raising. But like I said, I am coming from the perspective of two different layers (protection and telemetry).

matterpreter commented 3 years ago

I'm not sure I'm tracking as those two things don't need to be mutually exclusive. By applying a restrictive security descriptor to the device object, you are protecting the integrity/availability of the telemetry source (the driver). Using a secondary component, such as an EPP unit, to try to protect it in the same way would introduce significant complexity and likely not be as effective as using IoCreateDeviceSecure().

The primary question is should any user of the system be able to stop the driver from running or modify its configuration?

ComodoMelih commented 3 years ago

I'm not sure I'm tracking as those two things don't need to be mutually exclusive. By applying a restrictive security descriptor to the device object, you are protecting the integrity/availability of the telemetry source (the driver). Using a secondary component, such as an EPP unit, to try to protect it in the same way would introduce significant complexity and likely not be as effective as using IoCreateDeviceSecure().

The primary question is should any user of the system be able to stop the driver from running or modify its configuration?

we both agree protection should be provided... where we have a disagreement who should provide that protection...

You say: add that protection code into the EDR... I say: EPP code should provide the protection...

Here is the dilemma I see: If we say: EDR should provide the protection, then unless we put all EPP capability into EDR code, then what criteria do we follow as to where to stop.

ComodoMelih commented 3 years ago

btw: congrats @matterpreter for being the first to post contribution!

ozercomodo commented 3 years ago

thanks for the first pull request , we are going to accept it but we just realized that we need to organize our branches add stable/current/release branches and also publish our flow.

matterpreter commented 3 years ago

I say: EPP code should provide the protection...

What would an implementation of using an EPP unit to protect a device object look like?

ComodoMelih commented 3 years ago

What would an implementation of using an EPP unit to protect a device object look like?

the way I would do it is by virtualizing the APIs for any unknown executable that hits the endpoint. (https://techtalk.comodo.com/2020/08/17/comodos-patented-kernel-api-virtualization-under-the-hood/ https://techtalk.comodo.com/2020/10/10/applying-attack-surface-reduction-on-top-of-attack-surface-reduction-asr2/ )