dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.24k stars 4.73k forks source link

HttpListener sends malformed WWW-Authenticate mutual auth header #24807

Closed mattpwhite closed 4 years ago

mattpwhite commented 6 years ago

Hope this is the right place to file this, not clear to me if .NET Framework issues are in scope for this repo.

In the RFC for SPNEGO in HTTP, section 5 shows the expected exchange. In the final 200 response from the server, the server sets a WWW-Authenticate header to Negoitate base64(gssapi-data). HttpListener sets this header to only base64(gssapi-data). For other Windows .NET clients, this seems to be accepted, but things that have a stricter interpretation of the spec, such as the requests-kerberos Python library, it doesn't fly.

I'm pretty deeply confused about this because generally speaking, my understanding is that mutual auth and Negotiate in general is more or less http.sys's responsibility. With stuff hosted in IIS, I see the header being set with the appropriate Negotiate prefix. I threw together a very simple .NET Core app using HttpSysListener, and it also sets the header correctly. I even hacked some C sample code for the HTTP Server API to enable Negotiate and send 401s as necessary, and it also sets the header correctly. The only thing that doesn't is HttpListener. I have not had much luck digging through the source to figure out where this might be going wrong. In my actual app, I even tried re-writing the header myself in some Owin middleware, but the actual response on the wire was still wrong.

To repro...

Hello world


Sample code:

```csharp
using System;
using System.Net;
using System.Text;

namespace HttpListenerTest
{
    class Program
    {
        static void Main()
        {
            try
            {
                var responseString = "Hello world";
                var buffer = Encoding.UTF8.GetBytes(responseString);
                var listener = new HttpListener {AuthenticationSchemes = AuthenticationSchemes.Negotiate};
                var prefix = "http://+:9002/test/";
                listener.Prefixes.Add(prefix);
                listener.Start();
                Console.WriteLine($"Listening on {prefix}...");

                while (true)
                {
                    var context = listener.GetContext();
                    var request = context.Request;
                    Console.WriteLine($"Hit from {request.RemoteEndPoint.Address}...");
                    var response = context.Response;
                    response.ContentLength64 = buffer.Length;
                    var output = response.OutputStream;
                    output.Write(buffer, 0, buffer.Length);
                    output.Close();
                }
            }
            catch (Exception e)
            {
                Console.WriteLine(e);
            }
        }
    }
}
davidsh commented 6 years ago

Hope this is the right place to file this, not clear to me if .NET Framework issues are in scope for this repo.

Does this repro in .NET Core?

.NET Framework issues are not tracked here. Please open up .NET Framework issues here:

https://developercommunity.visualstudio.com/spaces/61/index.html

mattpwhite commented 6 years ago

Just threw together the repro below running on .NET Core. The response is still malformed with HttpListener. It is correct with HttpSysListener, there's a switch to control which one is used. My understanding of .NET versions leaves something to be desired, but I think this is current:

get-package

Id                                  Versions                                 ProjectName                                                                                                                                                  
--                                  --------                                 -----------                                                                                                                                                  
Microsoft.AspNetCore.All            {2.0.5}                                  HttpRepro                                                                                                                                                    
Microsoft.NETCore.App               {2.0}                                    HttpRepro  

I took the default ASP.NET Core blank template in VS, targeting .NET Core 2.0, deleted Startup.cs and pasted the below into Program.cs:

using System;
using System.Net;
using System.Text;
using Microsoft.AspNetCore;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.DependencyInjection;

namespace HttpRepro
{
    public class Program
    {
        public const string Prefix = "http://+:9002/test/";
        public const string ResponseString = "Hello world";
        public static readonly byte[] Buffer = Encoding.UTF8.GetBytes(ResponseString);

        static void Main(string[] args)
        {
            try
            {
                if (args.Length == 0 || !"httpsyslistener".Equals(args[0], StringComparison.OrdinalIgnoreCase))
                    UseHttpListener();
                else
                    UseHttpSysListener();
            }
            catch (Exception e)
            {
                Console.WriteLine(e);
            }
        }

        static void UseHttpListener()
        {
            var listener = new HttpListener { AuthenticationSchemes = AuthenticationSchemes.Negotiate };
            listener.Prefixes.Add(Prefix);
            listener.Start();
            Console.WriteLine($"Listening on {Prefix} with HttpListener...");

            while (true)
            {
                var context = listener.GetContext();
                var request = context.Request;
                Console.WriteLine($"Hit from {request.RemoteEndPoint.Address}...");
                var response = context.Response;
                response.ContentLength64 = Buffer.Length;
                var output = response.OutputStream;
                output.Write(Buffer, 0, Buffer.Length);
                output.Close();
            }
        }

        public static void UseHttpSysListener()
        {
            Console.WriteLine($"Listening on {Prefix} with HttpSysListener...");
            WebHost.CreateDefaultBuilder()
                .UseStartup<Startup>()
                .UseHttpSys(options =>
                {
                    options.Authentication.Schemes = Microsoft.AspNetCore.Server.HttpSys.AuthenticationSchemes.Negotiate;
                    options.Authentication.AllowAnonymous = false;
                    options.UrlPrefixes.Add(Prefix);
                })
                .Build().Run();
        }
    }

    public class Startup
    {
        public void ConfigureServices(IServiceCollection services)
        {
        }

        public void Configure(IApplicationBuilder app, IHostingEnvironment env)
        {
            app.Run(async context =>
            {
                await context.Response.WriteAsync(Program.ResponseString);
            });
        }
    }
}
davidsh commented 6 years ago

Just threw together the repro below running on .NET Core. The response is still malformed with HttpListener. It is correct with HttpSysListener, there's a switch to control which one is used. My understanding of .NET versions leaves something to be desired, but I think this is current:

HttpListener is the .NET Core class similar to .NET Framework's HttpLIstener.

"HttpSysLIstener" is a new class handled in ASP.NET Core that uses http.sys underneath similar to HttpListener, but has some additional features.

mattpwhite commented 6 years ago

HttpListener is the .NET Core class similar to .NET Framework's HttpLIstener.

Yeah, that's what I gathered. They both behave the same way (incorrectly) wrt this issue.

"HttpSysLIstener" is a new class handled in ASP.NET Core that uses http.sys underneath similar to HttpListener, but has some additional features.

Yeah, it does look like a nicer implementation from what I can see (even putting aside that it does mutual auth correctly). I'd love to be able to just drop HttpListener for HttpSysListener here, but that seems like it would completely blow up the size of my app. I get the push to containerized services, but my app is not that, it's basically a tiny agent that I have to put a lot of places that are definitely not container hosts. Fitting in a 2MB msi with a dozen files, loading mostly ngen-ed stuff out of the GAC that's already mapped in a dozen other processes (so low memory footprint) and first class support for running as a Windows Service (I know that is being worked on in Core) are valuable things I get with the full framework. The story for that isn't there on Core yet.

Anyway, I'll go cross-post at the link you provided. I do think this is a bug in Core, and it would be nice if you fixed it, but if I ever get to Core, I'd be using HttpSysListener anyway.

davidsh commented 6 years ago

cc: @dotnet/ncl

karelz commented 6 years ago

Isn't HttpListener backed by http.sys on Windows core and on .NET Framework. If they both fail, then it is likely bug in http.sys, or how we set it up. There is also managed implementation of HttpListener (used on Linux). I wonder if you hit same problem there (it may lack some authentication support).

mattpwhite commented 6 years ago

Isn't HttpListener backed by http.sys on Windows core and on .NET Framework. If they both fail, then it is likely bug in http.sys, or how we set it up.

It is backed by http.sys, but so (obviously) is HttpSysListener in Core, and it doesn't have the problem. Nor does the awful thing I put together in C based on some sample in the Win7 SDK that is basically the bare minimum use of the HTTP Server API to use http.sys and enable Negotiate. My guess is it's "or how we set it up".

You can ask http.sys to give the usermode server the mutual auth data rather than have http.sys insert the header on the way out. I looked for some place HttpLister might be doing this, but could not find it. See ReceiveMutualAuth here - https://msdn.microsoft.com/en-us/library/windows/desktop/aa364638(v=vs.85).aspx

karelz commented 6 years ago

@mattpwhite given your knowledge of http.sys and the fact you have a setup handy, would you be willing to debug .NET Core source code and help us find the root cause? If we know the root cause and/or if there is PR, it would make it easier to get a fix in ...

mattpwhite commented 6 years ago

your knowledge of http.sys

That may be overstating things, but I'll give it a shot.

mattpwhite commented 6 years ago

OK, think I found it.

HttpListener is explicitly writing the WWW-Authenticate headers in responses to clients. It appears based on my testing that http.sys will tag mutual auth data onto an outgoing response if no WWW-Authenticate header is set by user mode, but if one is, it will leave it as is. That's all fine, provided HttpListener puts the right data in the header, but it does not. It calls into SSPI and takes the output buffer from AcceptSecurityContext(), base64s it, and puts that base64 into the header value, without the authentication scheme prefix. The base64 token is correct, the SSPI call is the right one, the issue is in formatting the (not HTTP specific) binary token for use with HTTP.

The flow is:

  1. Grab the Authorization header base64, stripping the scheme prefix
  2. Decode the base64 into bytes
  3. Call the GetOutgoingBlob helper method in NTAuthentication.Common with the binary token
  4. GetOutgoingBlob passes the binary to the native AcceptSecurityContext function and gets a binary response along with an error code indicating whether we are done or have more rounds to do. If we're done, and Kerberos is the underlying mechanism, then this binary is the mutual auth token. NTLM doesn't support mutual auth, so presumably it's null in that case.
  5. GetOutgoingBlob returns the binary response.
  6. The binary token we got back is base64-ed.
  7. The base64-ed token is attached to the HttpListenerContext and stored in a _mutualAuthentication field.
  8. Later on, the MutualAuthentication property that exposes that field is checked, and if non-null, a WWW-Authenticate header with its value is added.

Someone in steps 6-8 needs to prefix the value that is ultimately going to be written to the header. Or maybe steps 6-8 shouldn't exist at all, and http.sys should be relied upon to put the header in. That's simpler, but if you ever wanted this to support Kerberos on non-Windows platforms, having the managed code make the platform specific native calls probably makes more sense (there are GSSAPI equivalents of all of the Windows APIs being used here, gss_accept_sec_context would replace AcceptSecurityContext and so on).

mattpwhite commented 6 years ago

FWIW, the .NET Framework version of this bug is essentially identical.

  1. https://github.com/Microsoft/referencesource/blob/4fe4349175f4c5091d972a7e56ea12012f1e7170/System/net/System/Net/HttpListener.cs#L1670
  2. https://github.com/Microsoft/referencesource/blob/4fe4349175f4c5091d972a7e56ea12012f1e7170/System/net/System/Net/HttpListener.cs#L1776 and https://github.com/Microsoft/referencesource/blob/4fe4349175f4c5091d972a7e56ea12012f1e7170/System/net/System/Net/HttpListenerContext.cs#L44
  3. https://github.com/Microsoft/referencesource/blob/4fe4349175f4c5091d972a7e56ea12012f1e7170/System/net/System/Net/HttpListenerResponse.cs#L702
karelz commented 6 years ago

@caesar1995 can you please take a look?

caesar-chen commented 6 years ago

Thank you for the detailed analysis! You are correct on how we construct the WWW-Authenticate header. However, the Negotiate prefix is not required by RFC.

From RFC2616:

WWW-Authenticate  = "WWW-Authenticate" ":" 1#challenge

From RFC7235 (which obsoletes 2616):

The "WWW-Authenticate" header field indicates the authentication scheme(s) and parameters applicable to the target resource. WWW-Authenticate = 1#challenge

Adding the not required Negotiate prefix to WWW-Authenticate header may break existing apps, so we don't want to change the behavior this time.

mattpwhite commented 6 years ago

From RFC7235 (which obsoletes 2616):

That isn't the relevant RFC, RFC 4559 is. That RFC, which was incidentally written by Microsoft, about HTTP/Negotiate, which was invented by Microsoft, is fairly clear about it.

Adding the not required Negotiate prefix to WWW-Authenticate header may break existing apps

This is unlikely. Such apps would only ever have worked with HttpListener since nothing else does this. Not IIS, not http.sys, not the relevant Kerberos authentication modules for apache or nginx.

mattpwhite commented 6 years ago

Here's another reference for you that the Negotiate scheme prefix is in fact required. This predates (appears to be written in 2002) the RFC because the RFC basically just codifies what IE and IIS did in Windows 2000 and was later adopted by everyone else.

RFC 7235 does not mention SPNEGO, GSSAPI or Kerberos; it's unrelated. Well it's HTTP, obviously it is related, but not related to how this header works in this context.

caesar-chen commented 6 years ago

Hey Matt, we have discussed the issue offline, and agree that it's a bug which we would take a fix, if it doesn't break existing .NET client (.NET Framework 4.7.1, .NET Core 2.0)

Would you be interested in creating a PR?

mattpwhite commented 6 years ago

Thanks, really happy to hear that. I wouldn't mind creating a PR. I'd need a day or two to get some one to OK me signing a CLA, but wouldn't anticipate that being a problem.

I hadn't tried building CoreFX before, but I managed to follow https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/dogfooding.md and think I more or less got it working. Is that what I should be looking at or something else?

And I know there has been some work on improving the formatting APIs, is there something other than System.Convert.ToBase64String() I should be using? Going from the byte[] to "Negotiate base64", seems like it would require allocating the base64 string and then another string with the Negotiate prefix and the base64.

Lastly, would you need tests? The external dependencies seem like they would make that tricky here.

caesar-chen commented 6 years ago

Here are additional information:

  1. Steps to build corefx on Windows.
  2. How to build and run your changes, and run existing tests.
  3. How to add new tests.

would you need tests?

Yes please. On .NET Core, you can use the HttpClient class and writing FunctionalTest for the new behavior. For .NET Framework, a manual testing and posting result could be good evidence.

is there something other than System.Convert.ToBase64String() I should be using?

I'll let other people comment on this. We definitely need to avoid unnecessary memory allocations.

mattpwhite commented 6 years ago

Yes please. On .NET Core, you can use the HttpClient class and writing FunctionalTest for the new behavior.

What I meant was that's not sufficient to test this. You need a functional Kerberos infrastructure, which means:

This is no more than you need to test something using Kerberos without mutual authentication (HTTP or otherwise), so perhaps this has already come up and these requirements are addressed to support testing something else (NegotiateStream?). If it hasn't, then maybe the best I can do is to provide a sample app that would verify behavior in an environment where the above criteria is met?

karelz commented 6 years ago

I would suggest to start with a fix prototype and manual verification that it does not break .NET Framework or .NET Core clients (simple app running against the fixed HttpListener). Then we can think if there is a test we could/should add for the scenario.

Starting with System.Convert.ToBase64String() sounds reasonable to me - we will see in code review if it is something we should replace with something else.

mattpwhite commented 6 years ago

I have a patch that I think should do the trick, managed to figure out how to get a sample app to use a private corefx and verified that it doesn't break existing .NET clients, and now supports the Python clients that caused me to log this issue. I need approval from my employer before I send this over and accept what the agreement though, hopefully soon.

One distressing thing I discovered in the course of testing this is that HttpWebRequest seems like it might be lying when it says it implements mutual authentication. The HttpWebResponse will tell you that you're mutually authenticated, regardless of what the server puts in the 200 WWW-Authenticate, so long as Kerberos is used for client authentication. If NTLM is used it will say no, and if you set AuthenticationLevel to MutualAuthRequired, it will throw if NTLM is used, but that's it. I modified my little C http.sys server to set the header to literally "garbage" and it's totally happy with that. The server proves its identity to the client by setting WWW-Authenticate to a value that proves that it was able to decrypt the session key that the client sent it. Without that, mutual authentication hasn't happened.

HttpClient (in core or the framework) doesn't seem to make any promises about mutual auth one way or the other, so it's excusable there.

FWIW, the HttpWebRequest issue is not really a concern for me, I just thought I should mention it. My reason for pursuing any of this was that I wanted clients that do implement the spec to be able to talk to my service. Actually relying on Kerberos (only) for mutual auth in the context of HTTP is a dicey thing to do in general, it's vastly easier (not to mention the privacy and integrity) to say that server auth happens with TLS (so it happens before you start talking).

mattpwhite commented 6 years ago

I need approval from my employer before I send this over and accept what the agreement though, hopefully soon.

Taking longer than expected to get the OK from our legal team, should have word next week.