Schannel read certs simplify

[Vasiliy-Kkk]

In schannel_verify.c, CryptQueryObject is used to get input certificates. Before calling CryptQueryObject, the code parses the data and expects that the data will be in Base64. My changes are:

1. Use the CERT_QUERY_FORMAT_BASE64_ENCODED flag in CryptQueryObject, because the data is always expected to be in Base64.
2. Do not check actual_content_type, because if the content type is not a certificate in Base64, CryptQueryObject will return FALSE.
3. Free only with CertFreeCertificateContext.

[vszakats]

CI test failures seem related to the changes made:

FAIL 311: 'HTTPS wrong subjectAltName but right CN' HTTPS, HTTP GET, PEM certificate
FAIL 312: 'HTTPS GET to localhost and null-prefixed CN cert' HTTPS, HTTP GET, PEM certificate
FAIL 678: 'HTTPS GET using CURLOPT_CAINFO_BLOB' HTTPS, HTTP GET, PEM certificate
FAIL 2033: 'simple HTTPS GET with DER public key pinning (Schannel variant)' HTTPS, HTTP GET, PEM certificate
FAIL 2070: 'Ignore certificate revocation "best effort" strategy' HTTPS, HTTP GET, PEM certificate
FAIL 2079: 'simple HTTPS GET with PEM public key pinning (Schannel variant)' HTTPS, HTTP GET, PEM certificate
FAIL 2087: 'simple HTTPS GET with base64-sha256 public key pinning (Schannel variant)' HTTPS, HTTP GET, PEM certificate

[Vasiliy-Kkk]

Yes, I'm currently dealing with errors in the tests. curl returns CURLE_SSL_CACERT_BADFILE, but CURLE_PEER_FAILED_VERIFICATION is expected.

[Vasiliy-Kkk]

If I use CERT_QUERY_FORMAT_FLAG_ALL instead of CERT_QUERY_FORMAT_BASE64_ENCODED, the tests pass. But I don't understand why. Can someone explain it to me? The failed tests used a Base64 certificate (test-ca.crt). I’m running the tests on Linux, and I can see that the certificate is Base64-encoded (and the code before CryptQueryObject expects Base64 headers and footers). However, the call to CryptQueryObject fails. In the logs, I can see the CRYPT_E_NO_MATCH error.

[Vasiliy-Kkk]

Okay, I really can't understand why only CERT_QUERY_FORMAT_BASE64_ENCODED doesn't work. I wrote tests for CryptQueryObject, and it works correctly with CERT_QUERY_FORMAT_BASE64_ENCODED for PEM certificates from OpenSSL after parsing in that code.

But I suggest leaving points 2–3 as they are (these changes are already in the PR). Could you rerun the tests, please? I think they were canceled not because of my changes.

UPD: I used the wrong flag: CERT_QUERY_FORMAT_BASE64_ENCODED instead of CERT_QUERY_FORMAT_FLAG_BASE64_ENCODED. I’ll fix it. I think everything should work.

Maybe, later, would you like to support certificates in DER format as well?

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

[Vasiliy-Kkk]

In documentation: "This API is deprecated.". Not function.

[vszakats]

I agree with the LLM here, the pre-existing form was more readable without knowing the surrounding line, e.g. in a grep hit. Perhaps keep the original?

[Vasiliy-Kkk]

Maybe delete this comment because the Windows documentation says, first of all, that CryptoAPI 1.0 is deprecated and recommends using CNG instead. However, this function belongs to CryptoAPI 2.0, and CNG has no equivalent for it.

For example, the functions CryptDecodeObject, CryptStringToBinary, and CertCreateCertificateContext are not deprecated. The code could be rewritten equivalently using these functions. However, it seems to me that CryptQueryObject handles this task well. Using CNG, as recommended by Microsoft in the documentation, it does not seem possible to do the same thing.

StackOverflow:

In general, CryptoAPI functions that deal with ASN.1 data structures (certs, CSRs, CRLs and the like) are not deprecated and have no counterpart in CNG API. Maybe this one was marked as deprecated by mistake.

Microsoft Questions:

For what its worth take a look at the sample code in what-is-new-alternative-for-cryptqueryobject-because-it-is-deprecated

But the link to the example is not available.

[vszakats]

Agree, but I still see no reason to alter or delete this comment:
The information/form is valid, even if we don't readily want
to replace this particular function, and such change also seems
unrelated to the purpose of the PR.

[Vasiliy-Kkk]

Okay, fixed it in commit c1772d9.

[vszakats]

UPD: I used the wrong flag: CERT_QUERY_FORMAT_BASE64_ENCODED instead of CERT_QUERY_FORMAT_FLAG_BASE64_ENCODED. I’ll fix it. I think everything should work.

Maybe, later, would you like to support certificates in DER format as well?

curl's documentation says PEM format is required, and there wasn't any request
that I'm aware of to extend it to DER. Other backends also would need to support
this and then updated accordingly, to avoid introducing a TLS-specific feature.

TL;DR IMO DER support is not desired in this case.

[jay]

curl's documentation says PEM format is required

hm. CURLOPT_CAINFO technically does not specify the format for the certificates, yet this PR changes the certificate loading procedure to only allow base64 style certificates. Though the tool option --cacert (which maps to CURLOPT_CAINFO) does specify PEM only. So I wonder if this will break anything for anyone.

[Vasiliy-Kkk]

So I wonder if this will break anything for anyone.

It shouldn't break because the code requires a Base64 header.

curl/lib/vtls/schannel_verify.c

Lines 131 to 137 in 3d721a1

	const char *begin_cert_ptr = c_memmem(current_ca_file_ptr,
	ca_buffer_limit -
	current_ca_file_ptr - 1,
	BEGIN_CERT,
	begin_cert_len);
	if(!begin_cert_ptr || !ISNEWLINE(begin_cert_ptr[begin_cert_len])) {
	more_certs = 0;

[jay]

begin_cert_ptr = c_memmem(current_ca_file_ptr,

Ok I took a closer look and you're right. I see the certificate data is extracted from in between "-----BEGIN CERTIFICATE-----" and "-----END CERTIFICATE-----" and then passed to CryptQueryObject.

According to that doc setting dwExpectedFormatTypeFlags with the flag CERT_QUERY_FORMAT_FLAG_BASE64_ENCODED indicates "the expected format of the returned type" which to me seems somewhat unclear because it doesn't say expected format of the input content. In other words I wonder if it is possible for the input content to have a different format and the function to be successful.

Regardless, if the expected content type is CERT_QUERY_CONTENT_FLAG_CERT, then I don't see a problem with removing the checks to see what the actual content type is since if the function is successful it should have the expected content type.

[Vasiliy-Kkk]

According to that doc setting dwExpectedFormatTypeFlags with the flag CERT_QUERY_FORMAT_FLAG_BASE64_ENCODED indicates "the expected format of the returned type" which to me seems somewhat unclear because it doesn't say expected format of the input content. In other words I wonder if it is possible for the input content to have a different format and the function to be successful.

I understand your concerns about the dwExpectedFormatTypeFlags parameter. Indeed, the documentation uses a strange wording. But this flag means that the input data needs to be interpreted exactly as Base64.

For example, the description of the CERT_QUERY_FORMAT_FLAG_BASE64_ENCODED flag "The content should be returned in Base64 encoded format" is also strange. The function does not return content in Base64 format. If it is a certificate, a pointer to CERT_CONTEXT will be returned. It cannot contain data in Base64.

I am sure that this flag indicates exactly the type of input data. I can send you tests that will confirm my claim.

If you still want to leave using the CERT_QUERY_FORMAT_FLAG_ALL flag, then I can do this. If I understand correctly, then the function shouldn't have much more work.

Or I can make a more reliable change by enlarging the code a bit. Replace CryptQueryObject with CryptStringToBinary+CertCreateCertificateContext. It will be more explicit.

Did I understand correctly that you agree with the rest of the changes?

[jay]

It's fine no more changes are needed. I think the MS documentation is unclear so I submitted MicrosoftDocs/sdk-api#2221.

[jay]

Thanks