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

TLS AEAD ciphers: more bytes for key_block than needed #13035

Closed
wants to merge 1 commit into from

Conversation

maximmasiutin
Copy link
Contributor

@maximmasiutin maximmasiutin commented Sep 29, 2020

TLS AEAD ciphers: more bytes for key_block than needed.

This pull request fixes the issue "More bytes are generated for key_block than needed for AEAD ciphers". In doing the fix, I have reused existing code that was already calculating IV length within the key block. See the discussion at #12007 for more details. It was the idea of @jwalch to use the existing code. I have taken existing code using the so-called "extract function" refactoring pattern. After that, @richsalz has proposed to optimize the code that by removing "k", and just replacing the assignments with return statements, then removing the "else".

This pull request is basically about the two things:

(1) Enables tracing of the key block length to show the bug by adding 'BIO_printf(trc_out, "key block length: %ld\n", num);'
(2) Fixes the bug by creating a function 'tls_iv_length_within_key_block' from existing code and calling it from that old place and from a second (new) place where IV length was taking regardless its context within the key_block.

I have also changed the order of addendums to correspond to one given in the RFC. The old order was somewhat counter-intuitive.

Old:

num = EVP_CIPHER_key_length(c) + mac_secret_size + EVP_CIPHER_iv_length(c);

New:

num = mac_secret_size + EVP_CIPHER_key_length(c) + tls_iv_length_within_key_block(c);

Here is the order in the RFC:

client_write_MAC_key[SecurityParameters.mac_key_length]
server_write_MAC_key[SecurityParameters.mac_key_length]
client_write_key[SecurityParameters.enc_key_length]
server_write_key[SecurityParameters.enc_key_length]
client_write_IV[SecurityParameters.fixed_iv_length]
server_write_IV[SecurityParameters.fixed_iv_length]

So, MAC_key comes first, then come cipher key and then the IV. That's why I put "mac_secret_size" first.

@mattcaswell - please review.

Fixes #12007

@maximmasiutin maximmasiutin changed the title Fixes #12007 Fixes the bug "More bytes are generated for Key Material than needed for AEAD cipher" Sep 29, 2020
@t8m
Copy link
Member

t8m commented Sep 29, 2020

Could you please amend the commit message to be more descriptive and put the Fixes line in the commit message body not on the first line? Use git commit --amend and then git push --force to do that.

@t8m
Copy link
Member

t8m commented Sep 29, 2020

Unfortunately the formatting is less than perfect. There are missing line separators. For example the Fixes #12007 should be alone on a separate line and without the parentheses. Also the first line should be as concise as possible - something like: TLS AEAD ciphers: do not generate more bytes for key_block than needed

@t8m
Copy link
Member

t8m commented Sep 29, 2020

Also the message body does not need to include the history of the changes and it usually is not written in first person, but it also does not hurt so that is up to you.

@maximmasiutin maximmasiutin changed the title Fixes the bug "More bytes are generated for Key Material than needed for AEAD cipher" TLS AEAD ciphers: more bytes for key_block than needed Sep 29, 2020
Fixes openssl#12007
The key_block length was not written to trace, thus it was not obvious that extra key_bytes were generated for TLS AEAD. The problem was that EVP_CIPHER_iv_length was called even for AEAD ciphers to figure out how many bytes from the key_block were needed for the IV. The correct way was to take cipher mode (GCM, CCM, etc) into consideration rather than simply callin the general function EVP_CIPHER_iv_length. The new function tls_iv_length_within_key_block takes this into consideration. Besides that, the order of addendums was counter-intuitive MAC length was second, but it have to be first to correspond the order given in the RFC.
@maximmasiutin
Copy link
Contributor Author

@t8m I have updated both the pull request description at the github and also the git commit description. It is now multiline. Can you please confirm that it is now OK?

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.

LGTM if @mattcaswell also approves.

@t8m t8m added the branch: master Merge to master branch label Sep 30, 2020
@mattcaswell mattcaswell added the approval: done This pull request has the required number of approvals label Sep 30, 2020
@openssl-machine openssl-machine 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 Oct 1, 2020
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Oct 2, 2020
Fixes #12007
The key_block length was not written to trace, thus it was not obvious
that extra key_bytes were generated for TLS AEAD.

The problem was that EVP_CIPHER_iv_length was called even for AEAD ciphers
to figure out how many bytes from the key_block were needed for the IV.
The correct way was to take cipher mode (GCM, CCM, etc) into
consideration rather than simply callin the general function
EVP_CIPHER_iv_length.

The new function tls_iv_length_within_key_block takes this into
consideration.

Besides that, the order of addendums was counter-intuitive MAC length
was second, but it have to be first to correspond the order given in the RFC.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #13035)
@t8m
Copy link
Member

t8m commented Oct 2, 2020

Merged to master. Thank you for the PR.

@t8m t8m closed this Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More bytes are generated for Key Material than needed for AEAD ciphers
4 participants