Fix CertificateCompressionAlgorithm to be read as 2-octet-wide

[t184256]

RFC8879
defines CertificateCompressionAlgorithm as a two-octet-wide enum:

       enum {
           zlib(1),
           brotli(2),
           zstd(3),
           (65535)
       } CertificateCompressionAlgorithm;

OpenSSL is reading them as one-octet-wide values though,
so that yet-unallocated values like 770 (0x302) or 16385 (0x4001)
are considered valid algorithms and result in compression negotiated and used.

@tmshort, please also double-check the width in other places,
I only gave it a cursory look.

Checklist

• documentation is added or updated
• tests are added or updated

[tmshort]

The committed code is working because the two-byte values have 0 as the leading byte, which is ignored, so it ends up being a no-op. But that does mean that only single-byte values can be used.

[beldmit]

As after this change the tests still pass, it means we are undertested here :(

[tmshort]

As after this change the tests still pass, it means we are undertested here :(

It's the nature of the allowed data. Even if there were unsupported algorithms, what would happen is that they would be ignored, unless one of the bytes matched a known but unsupported algorithm by the sender.

The only thing I can think of is some sort of fuzzing test where the client only sends algorithm (0x101 * a supported algorithm), (e.g. 0x101, 0x202 or 0x303, depending on config), which then gets interpreted as a supported algorithm. The test would fail if a compressed certificate is sent by the server. This would, of course, break once any of those values become valid. This would likely need to fall under the GREASE category of testing, and I haven't figured that out yet (since there almost no GREASE testing in OpenSSL that I can find).

[tmshort]

@t184256, I'm assuming this is something that would be tested via an updated tlsfuzzer?

[t184256]

Yes, this has been found with a test I'm planning to add to tlsfuzzer.

[t8m]

I agree with CLA: trivial

[t8m]

@tmshort please confirm you're OK with CLA: trivial.

[tmshort]

@tmshort please confirm you're OK with CLA: trivial.

I'm good with CLA:trivial

[beldmit]

@openssl/otc do we need to raise a separate issue because of the lack of tests here?..

[tmshort]

@beldmit, this was discovered during the development of the tlsfuzzer support for encryption. So, once that's been updated, it will be available as part of the tlsfuzzer CI.

[beldmit]

@tmshort let me disagree.

TLSFuzzer is external project and the level of its integration to OpenSSL is not enough (I don't have spare brain to extend the tests for more than 6 months and no signs I will get them soon). This issue should be tested locally.

[tmshort]

@tmshort let me disagree.

TLSFuzzer is external project and the level of its integration to OpenSSL is not enough (I don't have spare brain to extend the tests for more than 6 months and no signs I will get them soon). This issue should be tested locally.

I understand. During the initial review there was a request to do GREASE-like permutations of the extensions, but there's no generic mechanism for GREASE right now (AFAIK, if I'm wrong please let me know, I'm not sure the TLSProxy can do it). I do intend to introduce some form of GREASE testing of the extensions, possibly writing it in a generic way to allow others to also GREASE some tests. Since I wrote the Certificate Compression code; it is top of mind for me.

[openssl-machine]

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

[t8m]

Merged to master branch. @beldmit if you think there should be more testing of the certificate compression, feel free to open an issue.