MvcCoreLoggerExtensions - Information Leakage Security Vulnerability

[aaronlcope]

Describe the bug

Information leakage found in MVCCoreLoggerExtensions.cs facility.

On line 195 of MvcCoreLoggerExtensions.cs

The action method arguments are logged. MVC applications may contain authentication screens where user sensitive credentials can be obtained from these log files.

To Reproduce

Set up logging in an MVC app, develop a login screen, configure logging at INFO-level and watch the logs spit out user credentials.

Expected behavior

User credentials should never be logged. In fact, any unpredictable data (that is user input) should be left to the implementor to decide if it should be logged. I would think the correct approach is to not log the arguments at all and leave that up to the MVC developer to choose carefully and control that logging on their own.

[blowdart]

Interesting. As Information isn't the default loglevel you wouldn't get this logged by default, but when you do tweak the logging level yes, this is a potential problem if you haven't secured your logs like best practice would dictate.

So I'm not classifying this as a security patch Tuesday type bug, but rather something that will be fixed in the next scheduled releases for 2.x and above. The change will be that rather than the argument name/value pair being logged you'll only see the argument names. Tracing will instead log the full name/value pair.

[blowdart]

@mkArtakMSFT Can you assign this to someone please?

[aaronlcope]

@blowdart - thanks for the eyes on this. much appreciated.

So that I'm clear on deployment, when the fix is in place...I would need to:

1. update local development environment(s) dotnet core sdk to be congruent with the version number the fix is in.
2. update the netcore hosting package on servers, then deploy.

Do I have that correct?

[blowdart]

For future reference, security issues should first go to [email protected], where they can qualify for our bug bounty. Github issues can't qualify.

[mkArtakMSFT]

Thanks @blowdart.
@pranavkm lets get this fixed for 2.1, 2.2 as well as the upcoming releases.

[poke]

Why is the fix for this to completely remove the logging of the arguments? Wouldn’t it make more sense to just reduce the log level on the arguments as the log level documentation would suggest? On Debug or even Trace, this shouldn’t be a problem at all.

In the past, being able to access the action arguments using a more specialized logging level did help a lot when debugging problems. So I would really miss this in the future, and removing this completely will likely end up with people adding new action filters that log out all the stuff again.

I do not really understand OP’s issue where a “login/authentication screen” would show log output though.

[aaronlcope]

@poke - hope this helps with understanding the applied case...

The crux of the issue is argument logging, which happens on every MVC controller action method in aspnetcore, when logging is configured at Information level or below. This means all payloads end up in your log files. Think about when your app would have this controller / method:

class AccountController {

[Post]
Login(string username, string password){ }

...
}

Every time someone logs in to your app, you now have their plain text UN and PW sitting in your log file (unless your logging is configured at Error level or higher).

At first glance, leaving payload logging on at the Debug level seems appropriate as most people don’t typically deploy logging configuration at debug levels.

But even then, MSFT is still putting themselves at risk of data leakage just so developers can find an easier way to debug argument values sent to a controller action method.

What didn’t seem appropriate was the blind logging of every payload captured by any MVC controller action when the log level is set to information. Information level, according to the doc is supposed to help with general flow of the app. Payloads, specifically credentials, don’t supply long term value, they don’t help with assessment of the user moving through the app; worse, they present a rapidly increasing dumpster fire of corporate security risk the more eyes that see them and the longer the information sits at rest.

In my humble opinion, the decision MSFT is making is based on security posture, not ease of use, and I think that’s right.

Also, as I read it, it sounds like they are leaving argument logging on at “Trace” level...which is congruent with how the doc describes this intention of this level, and encourages readers to heed warning as well.

It’s also worth pointing out this isn’t limited to credentials as an only example. This could be:

• personal data
• credit card info
• location data
• anything else you can think of that you wouldn’t want in this log file

Finally, if we really wanted to log all method arguments for debugging, we would probably want control over which ones to log and what data is appropriate for a log file. That can be done pretty easily in an action filter at the discretion of a site developer. At the moment, as a framework, aspnetcore is agnostic about the nature of data when logging method arguments. My point is that stance inadvertently puts everyone at risk without much a developer can do but increase the log level to Error, losing other valuable informational level data.

[poke]

@aaronlcope
Thanks for the details. Actually, consuming form input like that is rather rare. In practice, you would probably end up using a complex type that represents the passed values from the form. This allows you for example to do proper model validation (without making the action signature explode) and it also allows you to use tag helpers within your Razor view.

This solution is for example covered in this Stack Overflow question from 2017.

I personally do think that we should assume that the average developer is careful enough to configure the logging level correctly and that the the log files (if there are any; because by default there aren’t) are protected enough. Not assuming that means that we are crippling all other developers that are protective enough while appreciating the default logging behavior. And if we accept “they can easily add an action filter” as an argument, then we could as well just get rid most logging altogether, going back to classic ASP.NET where all this was a choice developers had to explicitly make (by adding stuff).

And with ideas like #3700 on the horizon, we simply cannot avoid logging something just because someone accidentally exposed some sensitive information.

Also, as I read it, it sounds like they are leaving argument logging on at “Trace” level...which is congruent with how the doc describes this intention of this level, and encourages readers to heed warning as well.

Unfortunately, the PR #9227 for this completely removes the arguments, and does not move it to a different log level. The PR was updated to log the information at Debug level.

[pranavkm]

• 2.1: 836964a
• 2.2: 836964a
• 3.0-preview5: dfae19e