[tls] Fix incorrect use of certificate verification callbacks

[davidben]

As documented in [0], there are two certificate verification callbacks in the OpenSSL/BoringSSL TLS API. The one taken as a parameter to SSL_CTX_set_verify is the "verify callback". It is called multiple times during a single certificate verification is used to suppress errors and otherwise be notified about various events during verification.

Such a callback is not appropriate for accepting all certificates (you waste time processing things that will be thrown away), nor for post-verification inspection of the result (it will run multiple times). This is, however, what gRPC does with it.

Rather, gRPC should have used SSL_CTX_set_cert_verify_callback, which swaps out the verification process entirely. That is called exactly once per handshake and allows you to skip the verification, or verify and then inspect the results afterwards. Fix gRPC to heed the documentation.

In addition, this PR fixes a lifetime bug in gRPC's handling of the root certificate. RootCertExtractCallback stashes the root certificate without retaining it anywhere, but the X509_STORE_CTX will shortly be destroyed. There is no immediate guarantee the X509 object lasts as long as the SSL object. It most likely does because the object is often cached in the X509_STORE, which lives on the SSL_CTX, but this is at best, non-obvious. Instead, gRPC should have made
g_ssl_ex_verified_root_cert_index own a refcount to the X509 object by registering a free function and calling X509_up_ref when saving the value.

[0] https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#SSL_CTX_set_verify

[gtcooke94]

Thanks for putting this together

I have two questions:

1. Regarding which verification callback is set

Rather, gRPC should have used SSL_CTX_set_cert_verify_callback, which swaps out the verification process entirely.

I need to spend some more time with the code on this, but my immediate worry is that we are inherently depending on base verification behaviors in BoringSSL, then these callbacks are designed to be additional rather than replacement. And if we were to now replace that behavior, would we be breaking a significant assumption?

2. On RootCertExtractCallback

RootCertExtractCallback stashes the root certificate without retaining it anywhere, but the X509_STORE_CTX will shortly be destroyed. There is no immediate guarantee the X509 object lasts as long as the SSL object. It most likely does because the object is often cached in the X509_STORE, which lives on the SSL_CTX, but this is at best, non-obvious.

From looking at the API I thought this would be guaranteed, but I defer to you. I made the assumption because we get the SSL object from the X509_STORE_CTX with Open/BoringSSL APIs, not our own storing and caching
https://www.openssl.org/docs/man3.0/man3/SSL_get_ex_data_X509_STORE_CTX_idx.html

This ends up being used in the following chain

1. Get the stored value if it exists

   grpc/src/core/tsi/ssl_transport_security.cc

   Lines 1290 to 1291 in 853e8c6

   	X509* verified_root_cert = static_cast<X509*>(
   	SSL_get_ex_data(impl->ssl, g_ssl_ex_verified_root_cert_index));

2. Parse subject name from the X509 cert, eventually copy it into a string buffer**

   grpc/src/core/tsi/ssl_transport_security.cc

   Lines 325 to 354 in 853e8c6

   	static tsi_result peer_property_from_x509_subject(X509* cert,
   	tsi_peer_property* property,
   	bool is_verified_root_cert) {
   	X509_NAME* subject_name = X509_get_subject_name(cert);
   	if (subject_name == nullptr) {
   	gpr_log(GPR_INFO, "Could not get subject name from certificate.");
   	return TSI_NOT_FOUND;
   	}
   	BIO* bio = BIO_new(BIO_s_mem());
   	X509_NAME_print_ex(bio, subject_name, 0, XN_FLAG_RFC2253);
   	char* contents;
   	long len = BIO_get_mem_data(bio, &contents);
   	if (len < 0) {
   	gpr_log(GPR_ERROR, "Could not get subject entry from certificate.");
   	BIO_free(bio);
   	return TSI_INTERNAL_ERROR;
   	}
   	tsi_result result;
   	if (!is_verified_root_cert) {
   	result = tsi_construct_string_peer_property(
   	TSI_X509_SUBJECT_PEER_PROPERTY, contents, static_cast<size_t>(len),
   	property);
   	} else {
   	result = tsi_construct_string_peer_property(
   	TSI_X509_VERIFIED_ROOT_CERT_SUBECT_PEER_PROPERTY, contents,
   	static_cast<size_t>(len), property);
   	}
   	BIO_free(bio);
   	return result;
   	}

So long as the lifetimes are ok here, this specific piece should be ok?

** - From on our other discussion this likely needs to be adjusted to use a stable string API as well because at one point it uses X509_name_print_ex

[davidben]

I need to spend some more time with the code on this, but my immediate worry is that we are inherently depending on base verification behaviors in BoringSSL, then these callbacks are designed to be additional rather than replacement. And if we were to now replace that behavior, would we be breaking a significant assumption?

I'm not following. The callback you are currently using is the one that will break as BoringSSL evolves, because it exposes a bunch of implementation details about the order in which we do checks, and the granularity at which they're reported. The one this PR switches you to is the well-defined one, called exactly once per verification.

Can you elaborate on this? For context, I'm one of the maintainers of BoringSSL. You all are decidedly doing the wrong thing right now.

From looking at the API I thought this would be guaranteed, but I defer to you. I made the assumption because we get the SSL object from the X509_STORE_CTX with Open/BoringSSL APIs, not our own storing and caching

The SSL object outlives the X509_STORE_CTX, not the other way around. That is the problem. Your callback is passed an X509_STORE_CTX. You use that to get your SSL. That is all fine but also irrelevant to the issue here.

For the duration of the callback, anything you get off the X509_STORE_CTX is, of course, alive. However that's only true for the duration of the callback. After verification is done, the X509_STORE_CTX is destroyed. (X509_STORE_CTX != X509_STORE.)

You all are saving an X509 that you get from X509_STORE_CTX, onto the SSL. There is no guarantee that the lifetimes match up. Now, as it happens, the root X509 will come out of the X509_STORE, which persistently caches things it looks up in memory, and the X509_STORE does live as long as the SSL. So it's probably fine. However this is subtle enough, and lifetime bugs serious enough, that you all should be bumping the refcount. It also has nothing to do with SSL_get_ex_data_X509_STORE_CTX_idx.

[davidben]

From on our other discussion this likely needs to be adjusted to use a stable string API as well because at one point it uses X509_name_print_ex

The RFC 2253 output is at least a well-defined format. So, depending on what you're trying to do, it can potentially be okay. Your CRL bits were using something truly ad-hoc. (Though no text format would work for what you were doing with CRLs.)

[gtcooke94]

I'm not following. The callback you are currently using is the one that will break as BoringSSL evolves, because it exposes a bunch of implementation details about the order in which we do checks, and the granularity at which they're reported. The one this PR switches you to is the well-defined one, called exactly once per verification.

Can you elaborate on this? For context, I'm one of the maintainers of BoringSSL. You all are decidedly doing the wrong thing right now.

Like I said, I defer to you on this because you certainly know BoringSSL better than me. However, I do want to understand the changes to this codebase we're making.
Based on my current understanding of the functions, this would represent a significant behavior change - making these callbacks replace the existing verification vs. currently being additional checks after the built-in verification? I'm basing this off the documentation here under WARNINGS - "Do not mix the verification callback described in this function with the verify_callback function called during the verification process. The latter is set using the SSL_CTX_set_verify(3) family of functions. Providing a complete verification procedure including certificate purpose settings etc is a complex task. The built-in procedure is quite powerful and in most cases it should be sufficient to modify its behaviour using the verify_callback function."

And in RootCertExtractCallback you added a line to do verification (X509_verify_cert), and the doc for that reads "Applications rarely call this function directly but it is used by OpenSSL internally for certificate validation, in both the S/MIME and SSL/TLS code."

So based on my current understanding, this PR would be moving to the full replacement and calling a function that is meant to be used internally? But I don't think we want a full replacement, we still want the built-in procedure that "is quite powerful and in most cases it should be sufficient to modify its behaviour using the verify_callback function."

In short, I'm having trouble reconciling you're suggestion with the documentation suggesting that using the verify_callback with SSL_CTX_set_verify being generally sufficient since fully replacing with SSL_CTX_set_cert_verify_callback is complex? By replacing it then calling X509_verify_cert are we exactly still using the internal procedure, but not having the callback called many times?

The SSL object outlives the X509_STORE_CTX, not the other way around. That is the problem.......

Thanks for the detailed explanation

[davidben]

Ah. Yeah, we and OpenSSL do not always have the same views on which of their APIs are problematic. 😄 While we try to be mostly compatible, we expect projects like gRPC to reference BoringSSL's documentation and not just OpenSSL's. (The SSL-level APIs have been document for a while now. I expect to be done with the X509-level APIs this year.)

But really this is just a question of understanding what the two callbacks are doing. The documentation is consistent, but it sounds like you are reading too much into "rarely" and "in most cases".

I'll try to elaborate a bit. If you imagine this were a C++ class with virtual methods, there's two hooks here:

  // Verifies the certificate using the parameters in `ctx`, and saves the result into
  // `ctx`. The default implementation uses the built-in verifier. Overwriting this is
  // `SSL_CTX_set_cert_verify_callback`.
  virtual bool VerifyCertificate(X509_STORE_CTX *ctx);

  // Called multiple times, and at points in the process of the built-in verifier.
  // Inspect `ctx` to see the state of things. The return value may be used to
  // suppress or introduce errors. This is the verify callback.
  virtual bool OnVerifyEvent(bool ok, X509_STORE_CTX *ctx);

Now, given those two hooks, hopefully some things are clear:

1. OnVerifyEvent gets called many, many times. It is not a good place to run post-verification checks, to tweak configuration pre-verification, or to skip certificate verification altogether
2. Certificate verification is complex. The order and granularity of OnVerifyEvent cannot possibly be reliable across fixes and improvements to the library.
3. If you override out VerifyCertificate, that reimplementation needs to provide a full certificate verification function...
4. ...but if the base class's implementation is available (and it is), you can always just call that.

Something that may not be clear at a glance, but becomes abundantly clear once you've actually worked on a certificate verifier, is that making every arbitrary error in certificate verification suppressible is unbelievely fragile and unpredictable. E.g. if I suppress X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY, what happens to the signature check? Does that implicitly suppress that too? What some operations that implicitly check other things (e.g. inability to parse our supported extensions in the process of looking up candidate issuers)? If I suppress an error there, does that break later code which relies on some states being unreachable? It's really just a terrible customization point.

Now, what are gRPC's use cases here:

1. Accept all certs (i.e. don't verify anything!)
2. Run cert verification but save the root cert afterwards

OnVerifyEvent doesn't make sense for either of those. For the first use case, you simply want

bool VerifyCertificate(X509_STORE_CTX *ctx) override { return true; }

For the second, you want:

bool VerifyCertificate(X509_STORE_CTX *ctx) override {
  if (!BaseClass::VerifyCertificate(ctx)) {
    return false;
  }
  SaveRootCertificate(ctx);
  return true;
}

If you need to tweak the config, you might also imagine doing this. (Note there is no way to do this with OnVerifyEvent at all.)

bool VerifyCertificate(X509_STORE_CTX *ctx) override {
  AddSomeGrpcSpecificConfigLikeCRLs(ctx);
  return BaseClass::VerifyCertificate(ctx);
}

That is what this PR is doing. Now for the particular snippets of OpenSSL documentation that you cited...

Do not mix the verification callback described in this function with the verify_callback function called during the verification process. The latter is set using the SSL_CTX_set_verify(3) family of functions.

This is saying you shouldn't confuse the two. Indeed they are different callbacks and you should not mix them up.

Providing a complete verification procedure including certificate purpose settings etc is a complex task. The built-in procedure is quite powerful and in most cases it should be sufficient to modify its behaviour using the verify_callback function.

Honestly, this warning is badly written. Yes, providing a complete verification procedure is complex. But also OpenSSL provides you the inputs to the built-in validator and you can call the "base class" implementation by simply calling X509_verify_cert. (The only thing this callback does under the hood is swap out the X509_verify_cert call. If you implement the callback with X509_verify_cert, it is a no-op.)

Beyond that, the verify callback is pretty finicky, so I don't think using it a good idea. But, sure, if your customization looks like error suppression, you could use that.

Applications rarely call this function directly but it is used by OpenSSL internally for certificate validation, in both the S/MIME and SSL/TLS code.

This is also not very well-written. This is saying that, if you use the S/MIME and TLS APIs, they will run certificate validation for you as part of S/MIME or TLS. But if you are in a position where you need to run certificate validation itself, X509_verify_cert is the API.

[gtcooke94]

Thanks for the detailed explanations! These changes make a lot of sense now.

It looks like the CI is hitting a few small things - some strict build errors related to unused args, and compiler errors when building with OpenSSL1.0.2 since X509_up_ref was introduced in 1.1.1

[davidben]

Ah fun. Will fix that up tomorrow. (I'd forgotten X509_up_ref is that recent. Guess everyone was manually reaching into the refcount in the 1.0.x days.)

BTW, another fun thing about the verify callback: you might think that the calls with ok = 1 happen at the end, after we've decided that the certificate is okay. But, nope! They're actually sandwiched in the middle of verification, immediately after the signature check for each cert, but before other checks like name constraints or policy validation. So you might be called with ok = 1 and then learn later that actually verification failed. Truly bizarre API. :-)

(I think it used to be sort of at the end, but then they moved some checks afterwards because they're pretty expensive so you want them done after the signature check.)

[davidben]

The CI output seems to be ACLed, so I can't see the error messages. I've pushed a speculative fix. If there are still issues, can you post the errors here?