Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CA5393 warning results from allowing less secure dll locations #733

Open
AArnott opened this issue Oct 15, 2022 · 3 comments
Open

CA5393 warning results from allowing less secure dll locations #733

AArnott opened this issue Oct 15, 2022 · 3 comments
Labels
bug Something isn't working

Comments

@AArnott
Copy link
Member

AArnott commented Oct 15, 2022

I had to disable CA5393 to avoid warnings in the generated CsWin32 sources. Since that rule is built into the SDK, I wonder if it would be reasonable for CsWin32 to suppress that rule automatically on relevant methods?

Originally posted by @alexrp in #617 (comment)

@AArnott
Copy link
Member Author

AArnott commented Oct 15, 2022

I'm reading up on that warning. I'm confused by the fact that it considers ApplicationDirectory to be an unsafe location, but it considers SafeDirectories to be safe, though SafeDirectories includes the application directory. Go figure.
I'm trying to work out what to do here. I feel that if this is a security vulnerability, we shouldn't suppress the warning. But perhaps we shouldn't use the insecure setting by default either. If we use the safe settings by default and allow folks to override that setting to a less secure setting, then they can also decide what to do about the warning that results.

@AArnott AArnott added the bug Something isn't working label Oct 15, 2022
@alexrp
Copy link

alexrp commented Oct 15, 2022

There is some relevant discussion over here: dotnet/roslyn-analyzers#2855

So the rule was disabled by default because the attribute has no effect on Unix systems anyway. (I use AnalysisLevel=latest-all in my projects, causing it to be enabled.)

Besides the fact that there are legitimate use cases for shimming/replacing DLLs used by an application, I have to say that this whole mechanism seems rather... futile? If an attacker has enough access that they can write arbitrary files to an application's directory, you're probably already majorly screwed, and the question of whether you load DLLs from there is pretty low on the list of immediate concerns...

Anyhow, using AnalysisLevel=latest-all is a fairly common thing, and I would imagine that I'm not the only person using the DbgHelp APIs via CsWin32. So, in the interest of providing an error-free default experience, this:

But perhaps we shouldn't use the insecure setting by default either. If we use the safe settings by default and allow folks to override that setting to a less secure setting, then they can also decide what to do about the warning that results.

seems like a reasonable solution to me.

@AArnott
Copy link
Member Author

AArnott commented Oct 15, 2022

If an attacker has enough access that they can write arbitrary files to an application's directory, you're probably already majorly screwed

It's not as hard as you might think. The more common use case I heard is your Downloads directory. You download a few files, and accumulate a bunch of stuff in your downloads directory. Eventually you run an .exe in there that otherwise would be safe, except that you previously downloaded a .dll (or a .zip and unzipped it) that is there to spoof a Windows dll. Now the harmless .exe loads the virus .dll and invades your system, even though you only ran the trustworthy .exe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants