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.

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

Make CipherCtx::cipher_update more flexible #1733

Merged
merged 2 commits into from
Nov 26, 2022

Conversation

wiktor-k
Copy link
Contributor

This change relaxes constraints on the output buffer size when it can be safely determined how many bytes will be put in the output buffer.

For supported cryptographic backends (OpenSSL >= 1.1) the cipher's num parameter will be consulted for the number of bytes in the block cache. For unsupported backends the behavior will not change (the code will assume full block in the cache).

For callers that do the check themselves and want to use other backends (e.g. BoringSSL or LibreSSL) and unsafe unchecked_cipher_update function is added.

Additionally a CipherCtx::minimal_output_size function is added for letting the callers know how big should the output buffer be for the next cipher_update call.

Fixes #1729.

See: https://mta.openssl.org/pipermail/openssl-users/2022-November/015623.html

openssl/src/cipher_ctx.rs Outdated Show resolved Hide resolved
@sfackler
Copy link
Owner

Looks good to me other than the method naming!

This change relaxes constraints on the output buffer size when it can be
safely determined how many bytes will be put in the output buffer.

For supported cryptographic backends (OpenSSL >= 1.1) the cipher's `num`
parameter will be consulted for the number of bytes in the block cache.
For unsupported backends the behavior will not change (the code will
assume full block in the cache).

For callers that do the check themselves and want to use other backends
(e.g. BoringSSL or LibreSSL) and unsafe `cipher_update_unchecked`
function is added.

Additionally a `CipherCtx::minimal_output_size` function is added for
letting the callers know how big should the output buffer be for the
next `cipher_update` call.

Fixes sfackler#1729.

See: https://mta.openssl.org/pipermail/openssl-users/2022-November/015623.html
@wiktor-k
Copy link
Contributor Author

Rebased on top of master and fixed the naming issue.

Thanks for your reviews! I really appreciate it 🙏

🤔 It seems I missed a release with this PR... if you don't mind I'll file a couple of small documentation + example PRs.

@sfackler sfackler merged commit 4edda63 into sfackler:master Nov 26, 2022
@sfackler
Copy link
Owner

PRs are always welcome!

@sfackler
Copy link
Owner

I'm happy to cut a new release as well if you'd like.

@wiktor-k
Copy link
Contributor Author

I'm happy to cut a new release as well if you'd like.

Thank you! I'll be back with one more documentation PR and then I'll kindly ask you for a new release (it will also look better on docs.rs). Thanks in advance, I'll be back :)

@wiktor-k wiktor-k deleted the relax-cipher-update branch November 28, 2022 09:15
wiktor-k added a commit to wiktor-k/rust-openssl that referenced this pull request Dec 20, 2022
Sadly the condition used to relax output buffer checks that depended
on the `num` parameter does not really hold so this change effectively
reverts PR sfackler#1733.

As clarified on the OpenSSL mailing list [0] and during integration tests
the `num` parameter does not reflect the internal buffer cache size thus
one needs to pessimistically assume that each call to `cipher_update`
will need sufficient size to contain one additional block. Streaming
ciphers are not affected by this revert.

[0]: https://mta.openssl.org/pipermail/openssl-users/2022-December/015727.html
wiktor-k added a commit to wiktor-k/rust-openssl that referenced this pull request Dec 20, 2022
Sadly the condition used to relax output buffer checks that depended
on the `num` parameter does not really hold so this change effectively
reverts PR sfackler#1733 without breaking the API.

As clarified on the OpenSSL mailing list [0] and during integration tests
the `num` parameter does not reflect the internal buffer cache size thus
one needs to pessimistically assume that each call to `cipher_update`
will need sufficient size to contain one additional block. Streaming
ciphers are not affected by this revert.

[0]: https://mta.openssl.org/pipermail/openssl-users/2022-December/015727.html
@wiktor-k wiktor-k mentioned this pull request Dec 20, 2022
wiktor-k added a commit to wiktor-k/rust-openssl that referenced this pull request Dec 20, 2022
Sadly the condition used to relax output buffer checks that depended
on the `num` parameter does not really hold so this change effectively
reverts PR sfackler#1733.

As clarified on the OpenSSL mailing list [0] and during integration tests
the `num` parameter does not reflect the internal buffer cache size thus
one needs to pessimistically assume that each call to `cipher_update`
will need sufficient size to contain one additional block. Streaming
ciphers are not affected by this revert.

[0]: https://mta.openssl.org/pipermail/openssl-users/2022-December/015727.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CipherCtx::cipher_update is too pessimistic
2 participants