RemoteCertificateValidationCallback can not detect some incomplete certificate chains #59944
Comments
dotnet-issue-labeler
bot
added
area-System.Security
untriaged
|
Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks Issue DetailsDescriptionIt appears that There does not appear to be a way to detect these issues, which is why this feels like a bug to me. Here is my attempt at overriding the default behavior, but by the time my code it hit, all 3 certificates look like they exist. (https://dotnetfiddle.net/2aybuI) ConfigurationHere is a sample app using .NET 5 (my code base is .NET core 3.1 with the same issue) Here's a .NET Fiddle demonstrating the code above. Regression?Tested on several versions of .NET and this bug has been around for a long time. Other informationcurl requests, for example, fail how I want my requests to fail if the cert chain is not valid. openssl also correctly shows these incomplete-chains as invalid
|
|
Tagging subscribers to this area: @dotnet/ncl, @vcsjones Issue DetailsDescriptionIt appears that There does not appear to be a way to detect these issues, which is why this feels like a bug to me. Here is my attempt at overriding the default behavior, but by the time my code it hit, all 3 certificates look like they exist. (https://dotnetfiddle.net/2aybuI) ConfigurationHere is a sample app using .NET 5 (my code base is .NET core 3.1 with the same issue) Here's a .NET Fiddle demonstrating the code above. Regression?Tested on several versions of .NET and this bug has been around for a long time. Other informationcurl requests, for example, fail how I want my requests to fail if the cert chain is not valid. openssl also correctly shows these incomplete-chains as invalid
|
|
Tangent: Not specifically the problem you're talking about, but if (sslPolicyErrors != SslPolicyErrors.None)
{
return false;
}means that if the system hadn't already filled in the holes you're not going to look at it. |
|
.NET's certificate chains, on Windows, are built via Windows CAPI's CertGetCertificateChain . The CAPI chain engine has a lot of places that it looks for filling in holes:
Of those, only one can be turned off. That means that you can't really use X509Chain alone to determine this scenario. What you really want to know is "did the contents from the handshake look like the chain, except that it was missing the root?". And that means that you'd need to see what was actually put on the wire. It might be true that chain.ChainPolicy.ExtraStore in the callback already matches what was on the handshake (order and content)... but it might also (instead) be the case that SChannel already built the chain once, filled in any missing pieces, and then we already got the normalized chain. I'm not sure. If it's the normalized chain already (and maybe "anyways"), then maybe we have a need for API exposing what was on the wire. |
|
Hey @bartonjs , you're absolutely correct. Do you know of a way to see what was actually put on the wire? Everything I can find in the docs abstracts this away from the developer. I don't really care if the system I'm running on is smart enough to figure out if the request safe, I need to figure out if every system/language/platform is also going come to the same conclusion. (The curl and openssl request are simple examples of this) Yep, this is just the default behavior, and I'm cool with returning early here. if (sslPolicyErrors != SslPolicyErrors.None)
{
return false;
} |
|
The one thing to investigate is if |
|
Yeah it looks like if this is the only way to access the cert info, then this currently isn't possible. |
karelz
added
enhancement
|
Triage: Unclear if it should happen in SslStream or in X509Chain. We should try to solve it in 7.0. |
|
Possible work around:
I'm working on the first one. Just failed with the |
the third work around, which is proved works fine: |
|
@bartonjs is right and the I don't think new chain or rebuild is necessary as that can still get certificates from cache - even with download disabled. maybe I'm missing something but there seems to be no work left. |
|
hey @wfurt, sorry if this is a dumb question, but I just want to clarify. Does "no work left" mean it's already been fixed in the dotnet 7 branch? or it should already be working in dotnet 6? |
|
That depends on "working" means. :) The Note, that main purpose of I'm also don't think the claim "incorrectly validates" is valid. The AIA in certificates exists for a reason so using it does not not seems incorrect. There are already asks to have some more control over the validation so I feel there is no need for another generic issue. |
|
Okay, so it sounds like we could kinda get it work by doing this (code below). But we'd have to rely on comparing Subject and Issuer names rather than the I also noticed the order of certificates is reversed in some environments like dotnetfiddle. I'm assuming that's something dotnetfiddle is doing? and that under normal circumstances the order of the certificates is deterministic? Code also below the outputs. Local Output (works)Dotnetfiddle Output (does not work)Codeusing System.Net.Security;
using System.Net.Sockets;
using System.Security.Cryptography.X509Certificates;
// Valid Chain
Connect("www.google.com");
// Invalid Chain
Connect("incomplete-chain.badssl.com");
static void Connect(string url)
{
try
{
new SslStream(new TcpClient(url, 443).GetStream(), false,
new RemoteCertificateValidationCallback(ServerCertificateValidationCallback))
.AuthenticateAsClient(url);
}
catch { }
}
static bool ServerCertificateValidationCallback(object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors)
{
if (sslPolicyErrors != SslPolicyErrors.None)
{
return false;
}
var certs = chain.ChainPolicy.ExtraStore;
Console.WriteLine("Certificate Subjects:");
foreach (var x in certs)
{
Console.WriteLine(x.Subject);
}
// Add root certificate
var store = GetRootCertificates().Select(x => x.Subject).ToList();
// Assert each parent's subject is the child's issuer
var result = false;
for (var i = certs.Count - 1; i >= 0; i--)
{
if (i > 0)
{
if (certs[i].Issuer != certs[i - 1].Subject)
{
result = false;
break;
}
}
if (store.Contains(certs[i].Issuer))
{
result = true;
break;
}
}
Console.WriteLine($"This chain is {(result ? "" : "not ")}valid");
Console.WriteLine();
return result;
}
static List<X509Certificate2> GetRootCertificates()
{
var certificates = new List<X509Certificate2>();
var stores = new[]
{
new X509Store(StoreName.Root, StoreLocation.LocalMachine),
new X509Store(StoreName.Root, StoreLocation.CurrentUser)
};
foreach (var store in stores)
{
try
{
store.Open(OpenFlags.ReadOnly);
foreach (X509Certificate2 x509 in store.Certificates)
{
certificates.Add(x509);
}
}
finally
{
store.Close();
}
}
return certificates;
} |
|
There was never attempt to order the certificates anyhow. There were some platform differences (like extra leaf cert) I tried to fix in 7.0. Tls 1.3 explicitly states that receiver should make no assumption about the order |
|
BTW here is what I was thinking. It should work on Windows and Linux. on macOS OS gives me all certificates even if there were not sent on wire. I don't think there is anything we can do about it. The static bool ServerCertificateValidationCallback(object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors)
{
if (sslPolicyErrors != SslPolicyErrors.None)
{
return false;
}
foreach (X509ChainElement element in chain.ChainElements)
{
if (certificate.Equals(element.Certificate))
{
Console.WriteLine("Skipping self {0}", element.Certificate.Subject);
continue;
}
if (element.Certificate.Subject == element.Certificate.Issuer)
{
Console.WriteLine("Find root");
break;
}
if (!chain.ChainPolicy.ExtraStore.Contains(element.Certificate))
{
Console.WriteLine("Certificate {0} was not sent on wire", element.Certificate.Subject);
return false;
}
}
Console.WriteLine("Client sent whole chain");
return true;
} |
|
Thanks, that information helps. I'll play around with it some more next week. Oh that's smarter..just compare all them in the chain with the ones sent from extra store... got it. We just won't break when we find the root because it could be first in the list. |
|
So in order to detect all the chain issues, I can't find a way other than more or less what I suggested above. With the modification being accounting for certs in any order. |
|
I'm not sure I fully understand. You still have the original chain to detect issues. I assumed this issue is primarily about ability to know if peer sent the certificate as it should or if they come from caching/AIA/local store. I'm not really sure what we could/should do here. The validation difference with other implementations was also noted in #55322 but I don't think we would change that. We may add know to disable AIA on client but if the complete chain can be constructed, SslStream will use it. |
|
Yeah, that's correct. We just need to be able to detect if a certificate in the chain isn't being sent, but should be sent. Some websites' root certificate is signed by a trusted CA, but that trusted issuer certificate is not actually sent. This appears to be a valid use case and the main reason we can't ONLY check to see if each certificate in the chain is also in the ExtraStore (as was suggested in the code snippet you provided). Some example of sites I found that do this are netflix.com, salesforce.com, icann.org, oracle.com, mysql.com. I understand if you need to close this as functioning as intended. I get that this was never really the intended use case, but if all the other tools (ex: ssl checker, curl, node, etc) see a certificate chain (ex: https://incomplete-chain.badssl.com) as invalid, it would be nice for dotnet to have the same functionality build in. With that being said, the ExtraStore property does provide us with a workaround, so I'm content enough with that. |
|
The trusted root should not be sent. You either have root in your trust or you won't be able to validate anyway. Sending root CA does not really help and it is only waste. It should be valid to send it but the sample code I posted simply ignores root. For TLS 1.2 TLS 1.3 only adds note that order should not mater. As far as the link: It clearly states I agree that adding control for AIA would be useful. That is already tracked by #59979. |
|
Client and server now have option to pass in |
ghost
locked as resolved and limited conversation to collaborators
Description
It appears that
RemoteCertificateValidationCallback(tested withSslStreamandHttpWebRequest) incorrectly validates some certificates with chain issues (ex. https://incomplete-chain.badssl.com). The code I'm attempting to write validates certificates, similar to what this tool is doing https://www.sslshopper.com/ssl-checker.html.There does not appear to be a way to detect these issues, which is why this feels like a bug to me.
Here is my attempt at overriding the default behavior, but by the time my code it hit, all 3 certificates look like they exist. (https://dotnetfiddle.net/2aybuI)
Configuration
Here is a sample app using .NET 5 (my code base is .NET core 3.1 with the same issue)
Here's a .NET Fiddle demonstrating the code above.
https://dotnetfiddle.net/2aybuI
Regression?
Tested on several versions of .NET and this bug has been around for a long time.
Other information
curl requests, for example, fail how I want my requests to fail if the cert chain is not valid.
openssl also correctly shows these incomplete-chains as invalid
The text was updated successfully, but these errors were encountered: