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

PBKDF2 #101

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

PBKDF2 #101

wants to merge 5 commits into from

Conversation

mamckee
Copy link
Collaborator

@mamckee mamckee commented Dec 3, 2024

This PR adds PBKDF2 and associated tests to the SymCrypt provider.

if (ctx == NULL)
return;

SymCryptWipeKnownSize(&ctx->expandedKey, sizeof(SYMCRYPT_SRTPKDF_EXPANDED_KEY));
Copy link
Contributor

Choose a reason for hiding this comment

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

SYMCRYPT_SRTPKDF_EXPANDED_KEY

SYMCRYPT_PBKDF2_EXPANDED_KEY?

#endif

// Constants defined SP800-132 that should be checked
// unless the OSSL_KDF_PARAM_PKCS5 paremeter gets set
Copy link
Contributor

Choose a reason for hiding this comment

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

paremeter

NP: parameter

{
ctx->libctx = provctx->libctx;
ctx->pMac = SymCryptHmacSha1Algorithm;
ctx->iterationCount = PKCS5_DEFAULT_ITER;
Copy link
Contributor

Choose a reason for hiding this comment

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

PKCS5_DEFAULT_ITER

I think this constant should probably be defined locally in this file too

ctx->libctx = provctx->libctx;
ctx->pMac = SymCryptHmacSha1Algorithm;
ctx->iterationCount = PKCS5_DEFAULT_ITER;
ctx->checkMinSizes = TRUE;
Copy link
Contributor

Choose a reason for hiding this comment

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

TRUE

I think if we want backwards compatibility then the default should be permissive (i.e. FALSE)?

else
{
copyCtx->initialized = FALSE;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a little weird to me; when we set password / mac in p_scossl_pbkdf2_set_ctx_params, we explicitly do not eagerly initialize expandedKey, but wait until the next call to p_scossl_pbkdf2_derive.

This logic seems to eagerly expand the key in the destination context even if it may not yet be expanded in the source context; which just seems overcomplex vs. always setting copyCtx->initialized = FALSE?

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.

2 participants