make cert/crl/name/attr/revoked/ext/extfactory shareable when frozen

[HoneyryderChuck]

Considering we're discussing the semantics of freezing Certs/CRLs here.

[rhenium]

These classes need more careful review of each OpenSSL function. I haven't reviewed enough, but I can spot a few issues:

• OpenSSL::X509::Certificate#tbs_bytes calls i2d_re_X509_tbs(), which has a side effect of refreshing DER-encoding cache.

• OpenSSL::X509::Certificate#sign modifies itself.

• OpenSSL::X509::Name#to_der seems to be unsafe when the object was previously modified: https://github.com/openssl/openssl/blob/ba6f115ccfbb63fbeb2bc8df3c07918a7a59a186/crypto/x509/x_name.c#L220-L227
  The whole X509_NAME appears to be not written with thread safety in mind.

[HoneyryderChuck]

Added frozen check for the mentioned functions in ossl_x509cert.c.

The whole X509_NAME appears to be not written with thread safety in mind.

Indeed. Zooming out a bit, perhaps we're better off just ensuring "shareable-when-frozen" only for certificates and crls? The whole point of the other objects is to use them to modify certs/crls via the setter methods that are now frozen-checked, so there doesn't seem to be a real usage for them outside of that use-case. LMK what you think.

[rhenium]

Added frozen check for the mentioned functions in ossl_x509cert.c.

I have not reviewed other methods. Please check the manpage of each function and the implementation if necessary, as there are so many edge cases in OpenSSL. Not having "set" or "add" in a function name doesn't necessarily mean it is completely read-only.

Zooming out a bit, perhaps we're better off just ensuring "shareable-when-frozen" only for certificates and crls? The whole point of the other objects is to use them to modify certs/crls via the setter methods that are now frozen-checked, so there doesn't seem to be a real usage for them outside of that use-case. LMK what you think.

I don't agree it's the whole point, except for OpenSSL::X509::ExtensionFactory that is indeed intended for building a certificate. That said, Name, Attribute, Revoked, and Extension are a non-reference-counted object and we can handle separately, after Certificate, etc. are done.

[HoneyryderChuck]

Checked all openssl methods used in ossl_x509cert.c as well as ossl_x509crl.c , and found one method requiring frozen check (since fixed). Everything else seems to be safe (internal pointers, should not be freed by caller). Couldn't find documentation nor implementation for PEM_write_bio_X509 and i2d_X509. Will check other classes some other time.

[HoneyryderChuck]

@rhenium I reviewed the documentation of the remainder of openssl functions used in the scope of these attributes, and while I tweaked/removed some of the previous additions, didn't spot anything particularly dangerous about the assumptions. Implementations may hide some details, but I'm a bit out of my depth navigating openssl code to judge that. But the Name#to_der example you referred to, assuming you mean that it's unsafe due to the modified flag manipulation on encode, seems a bit concerning, as I don't see a way to work around that, and perhaps that limitation extends to other fields? This may mean that it's impossible to consider Ruby Certificate/CRLs shareable when frozen :/ .

[rhenium]

What's really annoying is that OpenSSL's man pages often don't go so much into details. i2d_X509_NAME() is an unsafe function that directly modifies its fields, and it takes a const pointer! There is no mention about that in the man pages. I think it can't be the only instance.

I checked about the subject and issuer name in certificates. OpenSSL::X509::Certificate#subject= and #issuer= use X509_set_*_name() which encodes the name into the canonical (sorted) form and then copy, clearing the modified flag, so OpenSSL::X509::Certificate shouldn't be affected by this, at least in regards to these two fields.

Some part of OpenSSL::X509::Certificate should be shareable, at least. OpenSSL::X509::Certificate#to_der = i2d_X509() is supposed to be thread safe even if it's not explicitly documented so. SSL_CTX can be shared between threads, and that must be implying that i2d_X509() can be called concurrently.

Please note that OpenSSL::X509::Certificate and CRL depend on OpenSSL::PKey::PKey being shareable, which is not covered by this PR. I think pkeys can be assumed to be immutable in OpenSSL >= 3.0, but some work is needed for LibreSSL and older versions of OpenSSL.

[HoneyryderChuck]

Looking at your last comment, I believe we could go with the following compromise:

• Name, Attribute, and Revoked objects can't be shareable when frozen, due to to_der unsafe functions. I couldn't follow which internal functions * i2d_X509_ATTRIBUTE and i2d_X509_REVOKED call, but I suspect that safety won't be ensured by design.
• Certificate and CRL objects can be shareable when frozen, as long as their to_der implementation is safe. Setters do not set ivars, and instead call openssl APIs, so as long as they aren't callable when frozen, that should be good enough.
• agree this is dependent on making pkeys immutable. I remember you mentioning that pkeys were safe except for openssl versions lower than 3 using deprecated API for setters, but I didn't find reference to setters. I saw the PR you closed, but not sure exactly whether what it protecting itself against.

WDYT?

[HoneyryderChuck]

removed RUBY_TYPED_FROZEN_SHAREABLE flag from Name, Attribute, and Revoked.

[rhenium]

Looking at your last comment, I believe we could go with the following compromise:

• Name, Attribute, and Revoked objects can't be shareable when frozen, due to to_der unsafe functions. I couldn't follow which internal functions * i2d_X509_ATTRIBUTE and i2d_X509_REVOKED call, but I suspect that safety won't be ensured by design.

Implementations for X509_* objects should be in crypto/x509 in the OpenSSL tree. I don't know about other objects. Others may be safe.

However, as I mentioned earlier, making them shareable isn't a prerequisite for making Certificate or CRL shareable. We can do it at a later time if it's needed.

• Certificate and CRL objects can be shareable when frozen, as long as their to_der implementation is safe. Setters do not set ivars, and instead call openssl APIs, so as long as they aren't callable when frozen, that should be good enough.

The requirement is that it's not possible to trigger any C undefined behavior from Ruby. Unsafe getters may exist too (e.g., Certificate#tbs_bytes, which I only happened to know because I reviewed the PR to add the method) and we must also ensure they are not callable.

Allowing objects to become shareable when its #to_der method is safe seems like a good boundary.

• agree this is dependent on making pkeys immutable. I remember you mentioning that pkeys were safe except for openssl versions lower than 3 using deprecated API for setters, but I didn't find reference to setters. I saw the PR you closed, but not sure exactly whether what it protecting itself against.

OpenSSL::PKey::PKey doesn't provide access to any destructive OpenSSL functions when compiled with OpenSSL 3.0 or later. I think all current methods are thread safe, but I'm not sure. Please don't assume as there are a lot of surprises in the OpenSSL API. I will review it again, and I want you to double check too.

#817 was an attempt to solve the problem I mentioned in #803 (comment), which turned out to be unfixable with OpenSSL <= 1.1.1 or LibreSSL. Since it didn't work, we'll depend on OpenSSL::PKey::PKey being immutable.

[rhenium]

Also,

• rb_check_frozen() in OpenSSL::X509::CRL#next_update= is missing.
• OpenSSL::X509::ExtensionFactory doesn't look safe.

[HoneyryderChuck]

Allowing objects to become shareable when its #to_der method is safe seems like a good boundary.

For certificates, this function seems to be in use, which seems safe.
For CRLs, i2d_X509_CRL_bio calls ASN1_item_i2d_bio, which also seems face.

As you mentioned, i2d_re_X509_tbs isn't safe, already marked as uncallable for frozen certificates 👍

rb_check_frozen() in OpenSSL::X509::CRL#next_update= is missing.

fixed, good catch.

OpenSSL::X509::ExtensionFactory doesn't look safe.

Indeed, overlooked marking the setters. fixed that as well, let me know if it's good.