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

add support for SHA-3 based PRF to PBES2 #16237

Closed
wants to merge 2 commits into from

Conversation

tomato42
Copy link
Contributor

@tomato42 tomato42 commented Aug 5, 2021

As there are no limitations for HMACs used in PBKDF2 inside PBES2,
as more specifically the SHA-3 hashes are drop-in replacements for
SHA-2 hashes, we can easily add support for SHA-3 here.

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

paulidale
paulidale previously approved these changes Aug 5, 2021
@paulidale
Copy link
Contributor

I'd like to see a revamp of the PBE infrastructure. What's there is pretty horrible and a bit of a kludge.

Are there any test vectors for this?

@paulidale paulidale added branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature labels Aug 5, 2021
@paulidale paulidale added this to the Post 3.0.0 milestone Aug 5, 2021
@paulidale
Copy link
Contributor

As safe as this is, I don't think it will make 3.0.

@tomato42
Copy link
Contributor Author

tomato42 commented Aug 5, 2021

I'd like to see a revamp of the PBE infrastructure. What's there is pretty horrible and a bit of a kludge.

Are there any test vectors for this?

I'm not aware of any

@slontis
Copy link
Member

slontis commented Aug 6, 2021

Are there any test vectors for this?

I cant find any test vectors either..
We could at least put in some ref data in test/recipes/30-test_evp_data/ evpkdf_pbkdf2.txt and evppbe_pbkdf2.txt

@slontis
Copy link
Member

slontis commented Aug 6, 2021

I'd like to see a revamp of the PBE infrastructure. What's there is pretty horrible and a bit of a kludge.

Unravelling this is probably not as easy as it sounds. I think @jon-oracle may have tried to do this initially?

@jon-oracle
Copy link
Contributor

I'd like to see a revamp of the PBE infrastructure. What's there is pretty horrible and a bit of a kludge.

Unravelling this is probably not as easy as it sounds. I think @jon-oracle may have tried to do this initially?

Yes I did try but abandoned the attempt as getting it to work alongside the existing API was painful.

It would be useful to deprecate the existing EVP_PBE_alg_add() API (which I'm not sure is even used/tested) so the new infrastructure can start from a clean slate. A big problem is the various *_keyivgen functions taking ASN1_TYPEs for the parameters. Rewriting them to use OSSL_PARAM and/or having some kind of utility to convert ASN.1 AlgorithmParameters to OSSL_PARAM and back might help.

@beldmit
Copy link
Member

beldmit commented Aug 6, 2021

I'd like to have this change in 3.0.0 and some sort of PBE revamp after 3.0.0. @tomato42, did you test the interoperability for these algorithms between OpenSSL and any other implementations?

@t-j-h
Copy link
Member

t-j-h commented Aug 6, 2021

It is a new feature so not 3.0.0 relevant.

@beldmit
Copy link
Member

beldmit commented Aug 6, 2021

Sad but true

@paulidale
Copy link
Contributor

It is also possible to add these from an application via EVP_PBE_alg_add().

@tomato42
Copy link
Contributor Author

tomato42 commented Aug 6, 2021

I'd like to have this change in 3.0.0 and some sort of PBE revamp after 3.0.0. @tomato42, did you test the interoperability for these algorithms between OpenSSL and any other implementations?

No, GnuTLS doesn't support even SHA-512 as a PBKDF2 HMAC, I don't think NSS has SHA-3 implementation at all, and it can't import PKCS#12 files with SHA-3 MAC or KDF

Update: yes, NSS doesn't implement SHA-3 at all: MZBZ#1342546

As there are no limitations for HMACs used in PBKDF2 inside PBES2,
as more specifically the SHA-3 hashes are drop-in replacements for
SHA-2 hashes, we can easily add support for SHA-3 here.
@beldmit beldmit added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member labels May 17, 2022
Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

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

LGTM

@beldmit beldmit removed the approval: otc review pending This pull request needs review by an OTC member label May 17, 2022
@beldmit beldmit requested a review from paulidale May 17, 2022 14:01
@t8m t8m 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 May 17, 2022
@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 May 18, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@beldmit
Copy link
Member

beldmit commented May 18, 2022

Merged, thanks!

@beldmit beldmit closed this May 18, 2022
@tomato42 tomato42 deleted the hmac-sha3-prf-pbe branch May 18, 2022 15:11
openssl-machine pushed a commit that referenced this pull request May 18, 2022
As there are no limitations for HMACs used in PBKDF2 inside PBES2,
as more specifically the SHA-3 hashes are drop-in replacements for
SHA-2 hashes, we can easily add support for SHA-3 here.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #16237)
openssl-machine pushed a commit that referenced this pull request May 18, 2022
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #16237)
t8m pushed a commit to t8m/openssl that referenced this pull request Nov 14, 2022
As there are no limitations for HMACs used in PBKDF2 inside PBES2,
as more specifically the SHA-3 hashes are drop-in replacements for
SHA-2 hashes, we can easily add support for SHA-3 here.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#16237)

(cherry picked from commit c73ba81)
t8m pushed a commit to t8m/openssl that referenced this pull request Nov 14, 2022
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#16237)

(cherry picked from commit 5702392)
openssl-machine pushed a commit that referenced this pull request Nov 21, 2022
As there are no limitations for HMACs used in PBKDF2 inside PBES2,
as more specifically the SHA-3 hashes are drop-in replacements for
SHA-2 hashes, we can easily add support for SHA-3 here.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #16237)

(cherry picked from commit c73ba81)
openssl-machine pushed a commit that referenced this pull request Nov 21, 2022
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #16237)

(cherry picked from commit 5702392)
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 triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants