Skip to content

Add support for TLS1-PRF KDF (TLS 1.2) - #671

Open
ilie-halip-nxp wants to merge 2 commits into
openssl-projects:mainfrom
ilie-halip-nxp:feature/tls12
Open

Add support for TLS1-PRF KDF (TLS 1.2)#671
ilie-halip-nxp wants to merge 2 commits into
openssl-projects:mainfrom
ilie-halip-nxp:feature/tls12

Conversation

@ilie-halip-nxp

Copy link
Copy Markdown
Collaborator

Description

Add an implementation for the TLS1-PRF KDF.

Depending on the token support, it could use either CKM_TLS_KDF or dedicated mechanisms for the derivation steps.

Checklist

  • Code modified for feature
  • Test suite updated with functionality tests
  • Test suite updated with negative tests
  • Documentation updated

Reviewer's checklist:

  • Any issues marked for closing are addressed
  • There is a test suite reasonably covering new functionality or modifications
  • This feature/change has adequate documentation added
  • Code conform to coding style that today cannot yet be enforced via the check style test
  • Commits have short titles and sensible commit messages
  • Coverity Scan has run if needed (code PR) and no new defects were found

Add an implementation for the TLS1-PRF KDF.

When deriving either master secret or key expansion, try to use the
generic CKM_TLS_KDF mechanism. If it is not available or unsuccessful,
fall back to the dedicated mechanisms for each of the derivation steps.

A particual implementation detail is that OpenSSL does not give any
information about the selected ciphersuite. When using
CKM_TLS12_KEY_AND_MAC_DERIVE, we need to provide the sizes of the
mac keys, enc keys and IV, so try to infer these from the key block size
passed by OpenSSL.

Signed-off-by: Ilie Halip <ilie.halip@nxp.com>
Run tests for a variety of digests and output lengths that do all the
TLS1.2 derivation steps.

Signed-off-by: Ilie Halip <ilie.halip@nxp.com>
@ilie-halip-nxp

Copy link
Copy Markdown
Collaborator Author

Seems like tests fail when softokn is too old (pre-3.116 it had an extended master secret vendor mechanism).
Kryoptic also fails for now, I think once latchset/kryoptic#399 is merged it should be good.
tlsfuzzer fails all the time due to the fact that it ends up selecting some other ciphersuites which I haven't accounted for.

Looking into these.

@simo5 simo5 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I did a quick overview, not a deep one, but I left some comments.

One thing I would like to see is to move all of the "fallback" calculations when the mechanism is not available on the token, to a separate "tls1_prf_fallback.c" (or similar name) file.

And perhaps make it a compile-time option.

Additionally I am thinking we'd want to only support the upcoming skey based option in the default compile, I am not sure it makes a lot of sense to use a HW token if they keys are then always extracted back to process memory.

The value of doing TLS-stuff through a HW token is that keys stay in the token and all operations use the SKEY abstraction to access them.

Comment thread src/obj/create.c
{ CKA_TOKEN, &val_token, sizeof(val_token) },
{ CKA_DERIVE, &val_true, sizeof(val_true) },
{ CKA_SENSITIVE, &val_false, sizeof(val_false) },
{ CKA_EXTRACTABLE, &val_true, sizeof(val_true) },

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These two need to be conditional.
In some cases secret keys may be ok to be extractable, but not by default.

Comment thread src/exchange.c
tls1_prfctx->provctx = ctx;

/* mark with mechanism type */
tls1_prfctx->mechtype = CKM_TLS12_MASTER_KEY_DERIVE_DH;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Whether CKM_TLS12_MASTER_KEY_DERIVE_DH or CKM_TLS12_EXTENDED_MASTER_KEY_DERIVE_DH must be selected based on the seed passed in via parameters, amtching "master secret" or "extended master secret" respectively.

Comment thread src/kdf.c
{ CKA_VERIFY, &val_true, sizeof(val_true) },
{ CKA_DERIVE, &val_true, sizeof(val_true) },
{ CKA_SENSITIVE, &val_false, sizeof(val_false) },
{ CKA_EXTRACTABLE, &val_true, sizeof(val_true) }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

when operating in FIPS mode, generally modules will prevent extraction, I think this derivation should be done on modules only using the new skey interfaces (there is support landing upstream for KDFs) and leave keys in the token.

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