schannel: verify hostname independent of verify cert

[jay]

Prior to this change when CURLOPT_SSL_VERIFYPEER (verifypeer) was off and CURLOPT_SSL_VERIFYHOST (verifyhost) was on we did not verify the hostname in schannel code.

This fixes KNOWN_BUG 2.8 "Schannel disable CURLOPT_SSL_VERIFYPEER and verify hostname". We discussed a fix several years ago in #3285 but it went stale.

Assisted-by: Daniel Stenberg

Bug: https://curl.haxx.se/mail/lib-2018-10/0113.html
Reported-by: Martin Galvan

Ref: https://github.com/curl/curl/blob/curl-7_86_0/docs/KNOWN_BUGS#L304-L308
Ref: #3285

Fixes #3284
Closes #xxxx

---

This PR looks like a lot of change but it's really not. Most of the change is I moved two functions out of the HAS_MANUAL_VERIFY_API guard and then made one of them public (Curl_verify_host) so that hostnames could be verified independently of the server certificate.

[bagder]

👍

[bagder]

I had totally forgot about those old discussions. Thanks for this.

[jay]

Original mingw CI failed so I've added some fixes, also moved the functions back to their original location opting instead to change the position of the HAS_MANUAL_VERIFY_API guards so that it is easier to see the changes in the diff.

[bagder]

You probably want a /* !checksrc! disable TYPEDEFSTRUCT 4 */ before those typedefs.

[jay]

Done. What's odd is that there are other typedef structs in the header, possibly an issue with checksrc parsing?

I will take a look again at this tomorrow so if you don't want to wait for the KNOWN_BUGS PR you could omit this bug.

[bagder]

I've removed it from #10043

[bagder]

@jay what's the state of this PR now?

[jay]

This was held up by #9706. I can't find anything in the doc to support his claim that SECPKG_ATTR_REMOTE_CERT_CONTEXT is not the server certificate although it does not say explicitly that it is the server cert: "Finds a certificate context that contains the end certificate supplied by the server." That's good enough for me but I was hoping to look into #9706 and see what happens in Windows 11 that causes him to think it's actually a chain (if I understand right... refer to comments in issue) I just haven't had the time yet.

[jay]

I've reviewed #9706 (comment) and I do not see anything in the MS documentation that indicates the entire certificate store (the certificate chain) associated with the server cert context (ie the server certificate) is searched by functions that take the server cert as a parameter. I tested in Windows 11 and our code that uses SECPKG_ATTR_REMOTE_CERT_CONTEXT works the same as it does in earlier versions (which makes sense or we would have heard about this in reports). And finally I checked the ReactOS source code and I don't see where CertGetNameStringW (and its cert_find_alt_name_entry) enumerates the entire chain on the basis of one certificate. Even if it did that would be out of line with MS doc.

Based on above I see no reason to delay this any further.

/cc @nmoinvaz

[nmoinvaz]

There appear to be two places in curl code where SECPKG_ATTR_REMOTE_CERT_CONTEXT is used.

• In one place traverse_cert_store is called directly after, which calls CertEnumCertificatesInStore. This appears to use the CERT_CONTEXT->hCertStore parameter.
• In the other place, it uses CERT_CONTEXT->cbCertEncoded. If the CERT_CONTEXT->cbCertEncoded is always the end-entity (leaf) certificate, then it would make sense why functions like CertGetNameString work on it.

In my previous comment, I assumed that you were using the hCertStore parameter.

Regarding #9706, it might be possible to instead infer the ordering of the chain in hCertStore by using cbCertEncoded as a guide. If it comes first, then no need to reorder, otherwise we need to reorder. I will have to do some research on that.