Skip to content
This repository has been archived by the owner on Dec 13, 2022. It is now read-only.

XSS in the RequestLoggerMiddleware.cs #3279

Closed
JameelNabbo opened this issue May 21, 2019 · 7 comments
Closed

XSS in the RequestLoggerMiddleware.cs #3279

JameelNabbo opened this issue May 21, 2019 · 7 comments
Labels

Comments

@JameelNabbo
Copy link

Hi,
https://github.com/IdentityServer/IdentityServer4/blob/master/src/IdentityServer4/host/Extensions/RequestLoggerMiddleware.cs

in the LogForErrorContext method the PARAM httpContext is not filtred, and can be injected with XSS payload onerror="alert(String.fromCharCode(88,83,83)) in which can be triggred from the log as well.
static ILogger LogForErrorContext(HttpContext httpContext)
{
var request = httpContext.Request;

        var result = Log
            .ForContext("RequestHeaders", request.Headers.ToDictionary(h => h.Key, h => h.Value.ToString()), destructureObjects: true)
            .ForContext("RequestHost", request.Host)
            .ForContext("RequestProtocol", request.Protocol);

        if (request.HasFormContentType)
            result = result.ForContext("RequestForm", request.Form.ToDictionary(v => v.Key, v => v.Value.ToString()));

        return result;
    }
@leastprivilege
Copy link
Member

XSS is very context specific. What you describe above would e.g. not do any harm in a console output (and we cannot know how you plan to output your log files).

IOW - you protect yourself against XSS with output encoding, not input sanitization.

As a side note - this logger is only part of our test host. Not IdentityServer.

@leastprivilege
Copy link
Member

btw - are you the author of this

https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-12250

@scottbrady91
Copy link
Member

Quick heads up, there are guidelines in place for reporting security issues: https://github.com/IdentityServer/IdentityServer4/blob/master/SECURITY.MD

@leastprivilege
Copy link
Member

@JameelNabbo I contacted CVE to take the entry down, as it is obviously not a security problem. If you are the author please do the same.

For the "security researcher" I have advice for the future

  • do better research
  • learn about "responsible disclosure"

@pseudoramble
Copy link

This came up as an item for the team I'm on to check into since it shows up on CVE lists still. I wanted to share a bit more detailed explanation about some reasons why I believe this is not an issue for IdenttiyServer4 consumers.

(To be clear, I'm not a security researcher. But if the earlier points didn't clarify it enough, I think these points are more than sufficient to alleviate concerns for teams using IdentityServer).

  • RequestLoggerMiddleware is marked internal to the EntityFramework portion of IdentityServer. That means another assembly can't reference to it unless it's part of the same assembly. A consumer cannot build this middleware into their own setup.
  • RequestLoggerMiddleware is not set up behind the scenes in an app consuming IdentityServer. If you search for references to the middleware it only appears in the Startup.cs classes in the EntityFramework portion of IdentityServer. That's the code that runs the "test host" explained earlier in this thread. So only code used to test out the library makes use of this.
  • Even if you were able to put a source code change into IdentityServer to make it publicly accessible, it will not be setup unless it's configured in a consumer's Startup.cs file while setting up other middleware.
  • Even if you were able to change IdentityServer to make the middleware consumable and then inject it into a consumers application without them knowing about it and then submit JS to the instance of it, an application must consume these logs and interpret it as JS. At that point, the application reading those logs should take that input and be sanitizing them to be sure since at that point it's an input to that new application.

Hope this helps somebody else out.

@leastprivilege
Copy link
Member

The real problem is that @JameelNabbo doesn’t know what he is talking about, ignored all best practices wrt to responsible disclosure and opened a CVE instead. Now he doesn’t even have the decency to correct this mistake. Case closed.

@lock
Copy link

lock bot commented Jan 10, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants