Skip to content

Fix CertificateCompressionAlgorithm to be read as 2-octet-wide#19600

Closed
t184256 wants to merge 1 commit into
openssl:masterfrom
t184256:2octet-CertificateCompressionAlgorithm
Closed

Fix CertificateCompressionAlgorithm to be read as 2-octet-wide#19600
t184256 wants to merge 1 commit into
openssl:masterfrom
t184256:2octet-CertificateCompressionAlgorithm

Conversation

@t184256

@t184256 t184256 commented Nov 3, 2022

Copy link
Copy Markdown
Contributor

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
tmshort approved these changes Nov 3, 2022
View reviewed changes

@tmshort tmshort left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tmshort tmshort added branch: master Applies to master branch approval: otc review pending triaged: bug The issue/pr is/fixes a bug labels Nov 3, 2022
@beldmit

beldmit commented Nov 3, 2022

Copy link
Copy Markdown
Member

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

@tmshort

tmshort commented Nov 3, 2022

Copy link
Copy Markdown
Contributor

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

tmshort commented Nov 3, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@t184256

t184256 commented Nov 3, 2022

Copy link
Copy Markdown
Contributor Author

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

t8m
t8m approved these changes Nov 7, 2022
View reviewed changes

@t8m t8m left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with CLA: trivial

@t8m t8m added approval: done This pull request has the required number of approvals cla: trivial One of the commits is marked as 'CLA: trivial'. Review for non-trivial change in such commit. and removed approval: otc review pending labels Nov 7, 2022
@t8m

t8m commented Nov 7, 2022

Copy link
Copy Markdown
Member

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

@t8m t8m added approval: review pending This pull request needs review by a committer and removed approval: done This pull request has the required number of approvals labels Nov 7, 2022
@tmshort

tmshort commented Nov 7, 2022

Copy link
Copy Markdown
Contributor

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

I'm good with CLA:trivial

@t8m t8m added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Nov 7, 2022
@beldmit

beldmit commented Nov 7, 2022

Copy link
Copy Markdown
Member

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

@tmshort

tmshort commented Nov 7, 2022

Copy link
Copy Markdown
Contributor

@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

beldmit commented Nov 7, 2022

Copy link
Copy Markdown
Member

@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 commented Nov 7, 2022

Copy link
Copy Markdown
Contributor

@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

openssl-machine commented Nov 8, 2022

Copy link
Copy Markdown
Collaborator

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

t8m commented Nov 8, 2022

Copy link
Copy Markdown
Member

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

@t8m t8m closed this Nov 8, 2022
openssl-machine pushed a commit that referenced this pull request Nov 8, 2022
@t184256 @t8m
Fix CertificateCompressionAlgorithm to be read as 2-octet-wide
ce74e3f 
CLA: trivial

Reviewed-by: Todd Short <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #19600)
@beldmit beldmit mentioned this pull request Nov 8, 2022
Open
@tmshort tmshort mentioned this pull request Nov 11, 2022
Closed
2 tasks
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@t184256 @beldmit
Fix CertificateCompressionAlgorithm to be read as 2-octet-wide
c16c764 
CLA: trivial

Reviewed-by: Todd Short <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#19600)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@t8m t8m t8m approved these changes
@tmshort tmshort tmshort approved these changes

Assignees

No one assigned

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch cla: trivial One of the commits is marked as 'CLA: trivial'. Review for non-trivial change in such commit. triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@t184256 @beldmit @tmshort @t8m @openssl-machine