Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add zlib oneshot compression #19603

Closed
wants to merge 1 commit into from
Closed

Add zlib oneshot compression #19603

wants to merge 1 commit into from

Conversation

tmshort
Copy link
Contributor

@tmshort tmshort commented Nov 4, 2022

Fixes #19520

The existing stream-based ZLIB compression does not close out the stream properly. This is incompatible with RFC1950.
Add a one-shot ZLIB compression method (as used with Brotli and Zstd) to do certificate compression.

This was found by tlsfuzzer.

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

@tmshort tmshort added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug labels Nov 4, 2022
slontis
slontis reviewed Nov 4, 2022
View reviewed changes
ssl/ssl_cert_comp.c Outdated
break;
case TLSEXT_comp_cert_zstd:
method = COMP_zstd_oneshot();
break;
default:
fprintf(stderr, "COMP ERROR LINE=%d\n", __LINE__);
Copy link
Member

Choose a reason for hiding this comment

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

LINE is not used in many places in OpenSSL. I can see there is a OPENSSL_LINE in macros.h.

Copy link
Member

Choose a reason for hiding this comment

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

@tmshort fprintf to stderr is not a good idea for error reporting in libssl. Either use trace api if this is rather tracing thing or ERR_raise_data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was my debugging that I missed! It should not be there.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... at least some of these calls actually make sense to be converted into ERR_raise_data().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is fair, the default condition here, and some of the error cases could lead to ERR_raise_data().

slontis
slontis reviewed Nov 4, 2022
View reviewed changes
crypto/comp/c_zlib.c
if (compress(out, &out_size, in, ilen) != Z_OK)
return -1;

if (out_size > OSSL_SSIZE_MAX)
Copy link
Member

Choose a reason for hiding this comment

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

Does this imply a memory trash if it got here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check is here to make sure that the cast on the following line is safe.

mattcaswell
mattcaswell requested changes Nov 4, 2022
View reviewed changes
crypto/comp/c_zlib.c Outdated Show resolved Hide resolved
crypto/comp/c_zlib.c Outdated Show resolved Hide resolved
crypto/comp/c_zlib.c Outdated Show resolved Hide resolved
crypto/comp/c_zlib.c Outdated Show resolved Hide resolved
t8m
t8m approved these changes Nov 4, 2022
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

Approved if CI passes.

Hopefully the added bound checks won't trigger Coverity. If so we can fix that later though.

@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Nov 4, 2022
@mattcaswell mattcaswell 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 4, 2022
@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but this PR has failing CI tests. Once the tests pass it will get moved to 'approval: ready to merge' automatically, alternatively please review and set the label manually.

@tmshort
Copy link
Contributor Author

tmshort commented Nov 6, 2022

One of the tests is still pending after 24+ ?

@tmshort
Copy link
Contributor Author

tmshort commented Nov 7, 2022

As far as I can tell, looking at buildbot, there have never been any completed buildbot/master:unix-macos11-m1 builds? @levitte ?

@t8m t8m added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Nov 7, 2022
@t8m
Copy link
Member

t8m commented Nov 7, 2022

Merged to master branch. Thank you for your contribution.

@t8m t8m closed this Nov 7, 2022
openssl-machine pushed a commit that referenced this pull request Nov 7, 2022
@tmshort @t8m
Add zlib oneshot compression
3840271 
Fixes #19520

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19603)
@tmshort tmshort deleted the fix-zlib branch November 7, 2022 13:39
@tmshort
Copy link
Contributor Author

tmshort commented Nov 8, 2022

Hopefully the added bound checks won't trigger Coverity. If so we can fix that later though.

Appears to have. The failure can only occur if sizeof(size_t) > sizeof(unsigned long), which corresponds to an LLP64 system (Windows 64-bit systems with VC++ or MinGW). I can either create a Coverity exception, or do a #if with sizeof().
@t8m any preference?

@t8m
Copy link
Member

t8m commented Nov 8, 2022

I've dismissed the Coverity entries. IMO not worth fixing at all but if you want to submit a PR that silences Coverity in one way or another I'd approve it.

beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@tmshort @beldmit
Add zlib oneshot compression
c7b7392 
Fixes openssl#19520

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#19603)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slontis slontis slontis left review comments

@t8m t8m t8m approved these changes

@mattcaswell mattcaswell mattcaswell approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch 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.

zlib-compressed certificates aren't interoperable
5 participants
@tmshort @openssl-machine @t8m @mattcaswell @slontis