Skip to content

Conversation

@bernd-edlinger
Copy link
Member

No description provided.

@bernd-edlinger bernd-edlinger added branch: master Applies to master branch 1.1.0 labels Feb 21, 2018
@kroeckx
Copy link
Member

kroeckx commented Feb 21, 2018

This looks like something that could use test cases.

@bernd-edlinger
Copy link
Member Author

bernd-edlinger commented Feb 22, 2018

This is difficult to test because the failing code path is only entered when the input length
is larger than MAXBITCHUNK / EVP_MAXBLOCKSIZE.

for AES / camellia we have:
# define MAXBITCHUNK ((size_t)1<<(sizeof(size_t)*8-4))
for most other ciphers we have:
# define EVP_MAXCHUNK ((size_t)1<<(sizeof(long)*8-2))
If that constant is lowered to say 256 bytes you can see 20-test_enc and
20-test_enc_more.t failing without this patch.

@bernd-edlinger
Copy link
Member Author

However, I think EVP_MAXCHUNK is too high if shifted up by 3 bits it will overflow.....

@bernd-edlinger
Copy link
Member Author

Disregard the last comment, the chunk size is shifted down by 3 bits.
so the input length needs to be larger than EVP_MAXBLOCKSIZE/8
to cause a problem.

@bernd-edlinger
Copy link
Member Author

So on a 32-bit target encrypting more than 2^27 bytes = 128 MB is bound to fail.

((cbits == 1) \
&& !EVP_CIPHER_CTX_test_flags(ctx, EVP_CIPH_FLAG_LENGTH_BITS) \
? inl*8 : inl), \
? chunk*8 : chunk), \
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm... Originally it was chunk...

@dot-asm
Copy link
Contributor

dot-asm commented Feb 23, 2018

2^28, right? It's unreasonable to mount such test. So I'd say it's ready as is.

[Just in case, since it was mentioned, in case one wonders that is. What's EVP_MAXCHUNK and why does it look the way it does. It's there to reconcile the fact that EVP accepts len as size_t, while some of lower level subroutines as [signed] long :-(]

levitte pushed a commit that referenced this pull request Feb 23, 2018
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #5426)
levitte pushed a commit that referenced this pull request Feb 23, 2018
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #5426)

(cherry picked from commit 604e591)
@bernd-edlinger
Copy link
Member Author

Merged to master and 1.1.0 so far.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch: master Applies to master branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants