Does aspnetcore have a galaxy-sized security hole with controllers or am I missing something?

Question

Checking a colleague's library I saw two ways he described of working with his new StatusController. Either override a GetStatus method to return more specific data or simple let the default implementation work. Okay, so I do the override as I'm not yet sure what he means about the latter and it works. Another colleague asks if I've looked at the latter and I reply

"well no, 'cos it won't work if I just reference the project/nuget and not tell the framework about it in some way"

"well try it"

"ok, but I'll be staggered if it works".

I am now staggered.

I then went on to try this by an indirect reference and it still worked. I'm deliberately not showing any code because it is simply the boiler plate controller code from a Visual Studio MVC web app or web API and you can literally knock this out in minutes. But let me give you an example.

Let's say I'm writing an application for www.electioncomission.gov.us and I want to do some special string parsing on a particular file format and there's a nuget package that can help me, let's call that VotingMachineFormatParser and unbeknown to me it uses a package called Tviker (Russian for Tweaker), don't know why but they found it useful. Tviker has inside it a controller class called GosudarstvennoyeVmeshatelStvoController which does something - it means State Interference.

If you now go to www.electioncomission.gov.us/gosudarstvennoyevmeshatelstvo that something code is now running, doing whatever.

I thought you were supposed to use the concept of Application Parts to pull a controller in from another assembly. Please, what am I missing? Surely this can't be the hole I think it is?

Edit: I've not included any code as that's the point, you don't need any code. I forgot to say that this is behaviour that is "new" to .Net Core 3.1+ It did NOT work that way previously. The only way to pull in a controller from another assembly was via ApplicationParts.

It's easy to test this without doing more than renaming two boiler-plate classes.

Let VStudio create a boiler-plate WebAPI in .NetCore 5 and select the OpenAPI check box. Do this in two separate folders but for one of them rename the WeatherForecastController to ToldYouController and rename (with refactoring) the WeatherForecast class to say WeatherForecast1.

Reference the project with ToldYouController in the first project, run and you'll see both controllers in the Swagger and you did literally NOTHING more than reference to make that happen! You can try this by having an indirect reference and it'll do the same.

Answer 1

I lodged this issue with Microsoft's Security Centre and had the following brief email discussion; TLDR; It IS (as previously commented by @pfx) "by design" but they admit I have a point (i.e. There is a problem)!

On Tue, 5 Jan 2021 at 00:08, Microsoft Security Response Center <secure@microsoft.commailto:secure@microsoft.com> wrote: Hello William,

Thank you again for your report. Our team has completed their review, and we have determined that this is the expected behavior of .NET Core when scanning or loading assemblies. Per the .NET team: "Auto-scanning assemblies is a MVC feature, and we ship framework features such as Identity.UI and other Razor Class Libraries that rely on it.

If a harmless nuget package happens to reference another package that contains a bad actor controller and an application makes use of the great options available in the harmless package, they unbeknown have then incorporated the bad actor controller into their application.

We have a small doc section that talks configuring the assemblies that MVC can look for resources in - Share controllers, views, Razor Pages and more with Application Parts in ASP.NET Core | Microsoft Docs https://learn.microsoft.com/en-us/aspnet/core/mvc/advanced/app-parts?view%3Daspnetcore-5.0%23prevent-loading-resources&data=04%7c01%7csecure%40microsoft.com%7cf99261c9a0684480189008d8b161250b%7c72f988bf86f141af91ab2d7cd011db47%7c1%7c0%7c637454378085369085%7cUnknown%7cTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7c1000&sdata=BPxm68OOP6M7mzZUS9qMtYQnhvWabq1CMpSP7sfWLc4%3D&reserved=0.

Outside of that, the recommendation would be to clearly understand the implications of referencing a NuGet package, or any other external dependency in your application. MVC discovering additional controllers may not be the only problem when a harmful package is referenced by your application."

Thank you again for giving us the opportunity to review your findings. We will be closing this case, but I encourage to send any future findings to us at ...

Regards, Daniel MSRC

From: William Hopkins Received: Tue Jan 05 2021 02:03:28 GMT-0800 (Pacific Standard Time) To: ; Microsoft Security Response Center; Microsoft Security Response Center Subject: Re: MSRC Case 62669 CRM:0279001377

Thanks for the response Daniel,

I must say it's a disappointing stance. The behaviour introduced in .Net Core 3 flipped the inclusion of unbeknown controllers with no real fanfare as to the consequences. Yes I get that it makes life easier and "cooler" for some of the new framework features, but even for those, the "search" & "inclusion" could be flipped ON through initialisation calls. Having a very short and unpublicised piece of documentation which even then seems to need to require knowing specifically WHICH assemblies to exclude is really getting security back to front.

Yes, I do understand that including a NuGet package can also introduce other risks, but the biggest problem for a bad actor is to get the invocation to occur. This behaviour - even if by (very poor) design - makes that trivial. Coupled with the fact that many of my colleagues do not see the problem until I actually show them with the same example I gave to you guys, means I believe this is a ticking time bomb. The amount of extra effort now required in order to be able to use any non-significant-mainstream package means going forward I have to advise my clients very strongly against using the NuGet system. I fear that this could lead to the eventual crumbling of any confidence in NuGet packages.

And all for the sake of making it insecure by default, rather than insecure by action.

Regards,

Will Hopkins

Microsoft Security Response Center

AttachmentsTue, 5 Jan, 22:02 (16 hours ago)

to Microsoft, me Hi Will,

Thanks for providing some feedback on this. I shared it with the team who agree that you have some fair points, and because we've closed this case as a design issue, they have no problems with this becoming a public discussion. If you'd like, you can post your concerns on the GitHub repo, and members of the team would gladly continue to discuss your concerns there. Thanks again for your report in the interest of improving the security of our products!

Regards, Daniel MSRC

Answer 2

.NET Core 3.x has automatic application part registration for assemblies that reference MVC.

From Andrew Locks blog:

Note that in ASP.NET Core 3.x, when you compile an assembly that references ASP.NET Core, an assembly attribute is added to the output, [ApplicationPart]. ASP.NET Core 3.x apps look for this attribute on referenced assemblies and registers them as application parts automatically.

About ApplicationPartAttribute on the ASP.NET Core documentation:

Specifies an assembly to be added as an ApplicationPart.

In the ordinary case, MVC will generate ApplicationPartAttribute instances on the entry assembly for each dependency that references MVC. Each of these assemblies is treated as an ApplicationPart.

---

Since GosudarstvennoyeVmeshatelStvoController is a Controller, its assembly will have a reference to MVC and therefore be registered as an application part.