Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add_cert_to_certinfo should reverse the order of the certificates for SChannel #4518

Closed
RoguePointer80 opened this issue Oct 22, 2019 · 2 comments

Comments

@RoguePointer80
Copy link

RoguePointer80 commented Oct 22, 2019

I did this

Context : we have a unit test that is using libCurl built with OpenSSL. We recently changed the SSL backend to WinSSL (SChannel), and one of our unit tests broke. The following is what the unit test is essentially doing, and what is wrong.

curl_certinfo* pCertinfo = nullptr;
curl_easy_getinfo(m_pSession, CURLINFO_CERTINFO, &pCertinfo);
...
if (pCertinfo!= nullptr && pCertinfo->num_of_certs > 0) {
   std::string subject = ExtractSubjectFromCert(pCertInfo->certinfo[0]);
   UNIT_TEST_ASSERT(expectedSubject, subject);
}

Notice here that the method only looks at index 0 in the array, that is the first certificate.
It worked well with OpenSSL for many years.
However it appears that with SChannel, the order of the certificates in the chain is reversed.
With OpenSSL, the peer is the first index, while with SChannel it is the last.
OpenSSL is following an RFC.
Ref: https://curl.haxx.se/mail/lib-2018-10/0099.html
While SChannel "Finds a certificate context that contains the end certificate supplied by the server."
Ref: https://docs.microsoft.com/en-us/windows/win32/api/sspi/nf-sspi-querycontextattributesexa

I expected the following

I expected that the order of the certificates would be the same when changing the backend. I suggest using the OpenSSL order, i.e. the peer certificate first as it is detailed in an RFC and this order has been in the code base for longer than SChannel.

Suggestion

File : lib/vtls/schannel.c
the method add_cert_to_certinfo should be modified. We could add to Adder_args the total number of certificates, and inside add_cert_to_certinfo instead of passing (args->idx)++, then pass total_certs -1 - (args->idx)++. Ok I admit this last one is hard to read, but the idea is "total - currentIndex".

curl/libcurl version

7.65.1

[curl -V output]

operating system

Windows 10, version 1903.

@bagder
Copy link
Member

bagder commented Oct 22, 2019

Thanks! Would you be able to provide your proposed fix as a pull request?

@RoguePointer80
Copy link
Author

Sure, I'll do that.

@bagder bagder closed this as completed in 8986df8 Oct 24, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

2 participants