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

Added CERTIFICATE_VERIFY_MAX_LENGTH constant #20486

Closed
wants to merge 2 commits into from
Closed

Added CERTIFICATE_VERIFY_MAX_LENGTH constant #20486

wants to merge 2 commits into from

Conversation

VeronikaNguyen
Copy link
Contributor

@VeronikaNguyen VeronikaNguyen commented Mar 10, 2023 •
edited

  • Set CERTIFICATE_VERIFY_MAX_LENGTH to 65539 = 2^16 - 1 + 2 + 2
  • Changed SSL3_RT_MAX_PLAIN_LENGTH to CERTIFICATE_VERIFY_MAX_LENGTH in case TLS_ST_CR_CERT_VRFY in the function ossl_statem_client_max_message_size of statem_clnt.c and in the function ossl_statem_server_max_message_size of statem_srvr.c
  • Addresses #20478

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Mar 10, 2023
baentsch
baentsch suggested changes Mar 11, 2023
View reviewed changes
Copy link
Contributor

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

@VeronikaNguyen You are aware that you(r employer) needs to provide the CLA before this can move forward? Second comment: For testing these changes, may it be sensible to extend the artificial "signature algorithm" in the test tls-provider to a(n even more artificial) length that actually triggers/necessitates these constant changes? It might be as simple as setting a suitably large number here:

openssl/test/tls-provider.c

Line 2747 in 9313694

*siglen = max_sig_len;
and here

openssl/test/tls-provider.c

Line 2766 in 9313694

*siglen = xor_sig_len;
(maybe dependent on the changed constants?). That way we can be sure it's not just oqsprovider that tests these limits.

@VeronikaNguyen
Copy link
Contributor Author

Sorry for the late answer. Thanks @baentsch for the review. Yes, I submitted a CLA on Friday. I will look into your testing proposal and update the PR this week.

@t8m t8m 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: feature The issue/pr requests/adds a feature tests: exempted The PR is exempt from requirements for testing labels Mar 13, 2023
@mattcaswell
Copy link
Member

We don't accept merge commits. Can you remove the last merge commit? Thanks!

@mattcaswell
Copy link
Member

Aside from that - this PR looks good to me.

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Mar 17, 2023
@paulidale paulidale removed the approval: otc review pending This pull request needs review by an OTC member label Mar 19, 2023
@paulidale paulidale closed this Mar 19, 2023
@paulidale paulidale reopened this Mar 19, 2023
@paulidale
Copy link
Contributor

It looks like the author on the commits is not the same as on your submitted CLA. Could you change the author via git commit --amend --author="" please.

@github-actions github-actions bot removed the severity: fips change The pull request changes FIPS provider sources label Mar 19, 2023
@VeronikaNguyen
Added CERTIFICATE_VERIFY_MAX_LENGTH constant
a915b0c 
- Set `CERTIFICATE_VERIFY_MAX_LENGTH` to `65538 = 2^16+2`
- Changed `SSL3_RT_MAX_PLAIN_LENGTH` to `CERTIFICATE_VERIFY_MAX_LENGTH`  in `statem_srvr.c` and `statem_clnt.c`
@VeronikaNguyen
Modified CERTIFICATE_VERIFY_MAX_LENGTH
8106ea2 
- to 65539 (2 bytes for the algorithm identifier + 2 bytes of signature length + 65535 bytes of signature)
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Mar 20, 2023
@paulidale paulidale 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 Mar 20, 2023
@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 Mar 21, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m
Copy link
Member

t8m commented Mar 21, 2023

Squashed the commits and merged to master branch. Thank you for your contribution.

@t8m t8m closed this Mar 21, 2023
openssl-machine pushed a commit that referenced this pull request Mar 21, 2023
@VeronikaNguyen @t8m
Added CERTIFICATE_VERIFY_MAX_LENGTH constant
c6d14bf 
- Set `CERTIFICATE_VERIFY_MAX_LENGTH` to 65539
  (2 bytes for the algorithm identifier + 2 bytes of signature length
   + 65535 bytes of signature)
- Changed `SSL3_RT_MAX_PLAIN_LENGTH` to `CERTIFICATE_VERIFY_MAX_LENGTH`
  in `statem_srvr.c` and `statem_clnt.c`

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #20486)
@baentsch baentsch mentioned this pull request Mar 22, 2023
Merged
@ambrisko ambrisko mentioned this pull request Jul 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baentsch baentsch baentsch requested changes

@t8m t8m t8m approved these changes

@mattcaswell mattcaswell mattcaswell approved these changes

@paulidale paulidale paulidale 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 tests: exempted The PR is exempt from requirements for testing triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@VeronikaNguyen @mattcaswell @paulidale @openssl-machine @t8m @baentsch