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

Pkcs11 login #3750

Merged
merged 9 commits into from
Apr 16, 2020
Merged

Pkcs11 login #3750

merged 9 commits into from
Apr 16, 2020

Conversation

jenswi-linaro
Copy link
Contributor

No description provided.

jenswi-linaro pushed a commit to jenswi-linaro/optee_client that referenced this pull request Apr 14, 2020
Sync with PKCS11 TA API update from [1] for authentication related
commands.

Link: [1] OP-TEE/optee_os#3750

Signed-off-by: Etienne Carriere <[email protected]>
Signed-off-by: Jens Wiklander <[email protected]>
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

for commit "ta: pkcs11: helper for GPD TEE to PKCS#11 status conversion"

case TEE_ERROR_SHORT_BUFFER:
return PKCS11_CKR_BUFFER_TOO_SMALL;

case TEE_ERROR_MAC_INVALID:
Copy link
Contributor

Choose a reason for hiding this comment

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

for info: TEE_ERROR_SIGNATURE_INVALID from TEE_AsymmetricVerifyDigest() also matches PKCS11_CKR_SIGNATURE_INVALID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll add that here.

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

for commit "ta: pkcs11: helpers for PIN hashing"


res = TEE_AllocateOperation(&oh, TEE_ALG_SHA256, TEE_MODE_DIGEST, 0);
if (res)
return PKCS11_CKR_GENERAL_ERROR;
Copy link
Contributor

Choose a reason for hiding this comment

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

don't you prefer to report at least of out-of-memory status when allocation fails for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll update with tee2pkcs_error().

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

for commit "ta: pkcs11: update PIN fields in struct token_persistent_main"
Reviewed-by: Etienne Carriere <[email protected]>

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

for commit "ta: pkcs11: update PIN fields in struct token_persistent_main"
Reviewed-by: Etienne Carriere <[email protected]>

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

for commit "ta: pkcs11: implement command PKCS11_CMD_INIT_TOKEN":
In commit message: s/implement/implements/


serialargs_init(&ctrlargs, ctrl->memref.buffer, ctrl->memref.size);

rc = serialargs_get(&ctrlargs, &token_id, sizeof(uint32_t));
Copy link
Contributor

Choose a reason for hiding this comment

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

From PKCS11 spec, there are precedence requirement on some (very few) return values.
Invalid slot ID (or token ID, same) is one of them.

You can find below a suggestion to factorize this with a friendly serialargs_get_session() helper function:
etienne-lms@6a1e6d9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PKCS11_CKR_SLOT_ID_INVALID, does not take precedence over any other error code (see 5.1.6 All other Cryptoki function return values).
The precedence rule (as I understand it) only is applicable if there's multiple competing errors. So if unpacking of arguments fail, we return PKCS11_CKR_ARGUMENTS_BAD, there's no other competing error code at this stage.

I'll move and update the pin_size check below though. I guess it makes sense to check the arguments in the order they were supplied to C_InitToken().

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, sorry. I mixed up with the session handle that should take precedence (CKR_SESSION_HANDLE_INVALID).

I think my comment could apply to the commits where session_handle is read from the client arguments.


token = get_token(token_id);
if (!token)
return PKCS11_CKR_SLOT_ID_INVALID;
Copy link
Contributor

Choose a reason for hiding this comment

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

see comment above.

return PKCS11_CKR_PIN_LOCKED;
}

/* Check there's no open session on this token */
Copy link
Contributor

Choose a reason for hiding this comment

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

double space chars.

if (rc)
return rc;

update_persistent_db(token);
Copy link
Contributor

Choose a reason for hiding this comment

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

can't this operation fail?

Commit "ta: pkcs11: helper to update token persistent database" should be placed before this commit in the series.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it can't fail, the TA panics instead since continueing from here could be a security problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

true, sorry. I made this comment before looking at the implementation 👎


if (token->db_main->so_pin_count == 6)
token->db_main->flags |= PKCS11_CKFT_SO_PIN_FINAL_TRY;
if (token->db_main->so_pin_count == 7)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the max number of attempts should be configurable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, I'll fix.

update_persistent_db(token);

label[PKCS11_TOKEN_LABEL_SIZE] = '\0';
IMSG("PKCS11 token %"PRIu32": initialized \"%s\"", token_id, label);
Copy link
Contributor

Choose a reason for hiding this comment

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

Label is UTF8 so not sure IMSG() is able to print it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll remove it.

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

for commit "ta: pkcs11: implement command PKCS11_CMD_INIT_PIN':
in commit log: s/implement/implments/, 2 spaces before "Security Officer", missing terminal dot.

return PKCS11_CKR_OK;
}

/* ctrl=[session-handle][pin-size]{pin-arrays], in=unused, out=unused */
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK


serialargs_init(&ctrlargs, ctrl->memref.buffer, ctrl->memref.size);

rc = serialargs_get(&ctrlargs, &session_handle, sizeof(uint32_t));
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: prefer here sizeof(uint32_t) or sizeof(session_handle)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case I prefer sizeof(uint32_t) since it's a bit easier to double check the ABI.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

for commit "ta: pkcs11: implement command PKCS11_CMD_SET_PIN":
in commit log: s/implement/implements/

* [out] memref[0] = 32bit return code, enum pkcs11_rc
*
* This command relates to the PKCS#11 API function
* C_CloseAllSessions().
Copy link
Contributor

Choose a reason for hiding this comment

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

s/C_CloseAllSessions/C_SetPIN.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

return PKCS11_CKR_OK;
}

/* ctrl=[session][old-size]{old-pin][pin-size]{pin], in=unused, out=unused */
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

TEE_Param *ctrl = params;
uint32_t pin_size = 0;
void *old_pin = NULL;
void *pin = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

beautiful trapezium :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks :-)

enum pkcs11_rc rc = PKCS11_CKR_OK;

if (!token->db_main->user_pin_salt ||
!(token->db_main->flags & PKCS11_CKFT_USER_PIN_INITIALIZED))
Copy link
Contributor

Choose a reason for hiding this comment

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

need to check both? info is redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove the flags test.

struct ck_token *token = session->token;
enum pkcs11_rc rc = PKCS11_CKR_OK;

/* Note: intentional return code USER_PIN_NOT_INITIALIZED */
Copy link
Contributor

Choose a reason for hiding this comment

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

C_SetPIN() should not return CKR_USER_PIN_NOT_INITIALIZED. Is it because test below should be happen that this intentionally returns such status? Why not an assertion or a straight panic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll assert.

}

if (token->db_main->flags & (PKCS11_CKFT_SO_PIN_COUNT_LOW |
PKCS11_CKFT_SO_PIN_FINAL_TRY)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer indent to opening parenthesis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

for commit "ta: pkcs11: helper to update token persistent database"


res = open_db_file(token, &db_hdl);
if (res) {
EMSG("Failed to open token persisten db: %#"PRIx32, res);
Copy link
Contributor

Choose a reason for hiding this comment

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

s/persisten/persisent/

ditto below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or persistent even :-)


TEE_CloseObject(db_hdl);

return tee2pkcs_error(res);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

for commit "ta: pkcs11: implement commands PKCS11_CMD_LOGIN/_LOGOUT"

}
}

/* ctrl=[session][user_type][pin-size]{pin], in=unused, out=unused */
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

* This is the point where we could check if another client
* has another user or SO logged in. Currently we don't
* care.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

why not checking that now? We could parse the client list pkcs11_client_list for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spec says:

CKR_USER_TOO_MANY_TYPES: An attempt was made to have more distinct users simultaneously logged into the token than the token and/or library permits. For example, if some application has an open SO session, and another application attempts to log the normal user into a session, the attempt may return this error. It is not required to, however. Only if the simultaneous distinct users cannot be supported does C_Login have to return this value. Note that this error code generalizes to true multi-user tokens.

So we are permitted to skip this check, I don't see any harm in skipping it. The comment should be more clear on this though. I'll update that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok for nit dealing with CKR_USER_TOO_MANY_TYPES.
But maybe another client (client application) has open sessions towards the token. These session should be considered as well as the session owned by the current client.

I think made proto made a shortcut, returning CKR_USER_TOO_MANY_TYPES if other clients were found on same token instead of properly handling their sessions state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please crosscheck with "2.6.8 Example of use of sessions" in "PKCS #11 Cryptographic Token Interface Usage Guide Version 2.40".

To me it seems that clients are quite independent of each other when it comes to logins.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. Discard my comment.

return rc;
}

/* ctrl=[session], in=unused, out=unused */
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comment

@jenswi-linaro
Copy link
Contributor Author

Update

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

Following commit "ta: pkcs11: update PIN fields in struct token_persistent_main", could you add a commit to remove init_pin_keys(), open_pin_file() and get_pin_file_name() local functions from persistent_token.c?


update_persistent_db(token);

label[PKCS11_TOKEN_LABEL_SIZE] = '\0';
Copy link
Contributor

Choose a reason for hiding this comment

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

can remove this, and:

-	char label[PKCS11_TOKEN_LABEL_SIZE + 1] = { 0 };
+	char label[PKCS11_TOKEN_LABEL_SIZE] = { 0 };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@etienne-lms
Copy link
Contributor

@wamserma, @Emantor, @ricardosalveti could you have a look at this P-R and related OP-TEE/optee_client#205 and OP-TEE/optee_test#418?

@jenswi-linaro
Copy link
Contributor Author

I'm going to squash the commits and rebase on master to make it easier to review.

@jenswi-linaro
Copy link
Contributor Author

Updated and rebased on master

Copy link
Contributor

@Emantor Emantor left a comment

Choose a reason for hiding this comment

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

Acked-by: Rouven Czerwinski <[email protected]>

if (rc)
return rc;

IMSG("PKCS11 session %"PRIu32": set PIN", session_handle);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add an equivalent IMSG for the SO case above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add one.

jenswi-linaro pushed a commit to jenswi-linaro/optee_client that referenced this pull request Apr 16, 2020
Sync with PKCS11 TA API update from [1] for authentication related
commands.

Link: [1] OP-TEE/optee_os#3750

Reviewed-by: Rouven Czerwinski <[email protected]
Signed-off-by: Etienne Carriere <[email protected]>
Signed-off-by: Jens Wiklander <[email protected]>
jenswi-linaro pushed a commit to jenswi-linaro/optee_client that referenced this pull request Apr 16, 2020
Sync with PKCS11 TA API update from [1] for authentication related
commands.

Link: [1] OP-TEE/optee_os#3750

Reviewed-by: Rouven Czerwinski <[email protected]>
Signed-off-by: Etienne Carriere <[email protected]>
Signed-off-by: Jens Wiklander <[email protected]>
Introduce helper function pkcs2tee_error() for the several TEE Core
Internal APIs called for which return value needs to be reported to
caller in PKCS#11 return code format.

The function returns PKCS11_CKR_GENERAL_ERROR for TEE_Result values that
do not strictly match a PKCS#11 return code.

Acked-by: Rouven Czerwinski <[email protected]>
Co-developed-by: Etienne Carriere <[email protected]>
Signed-off-by: Etienne Carriere <[email protected]>
Signed-off-by: Jens Wiklander <[email protected]>
Adds helpers to hash PIN and to verify the hash of a PIN. The PIN is
hashed together with user type and a generated salt. A used salt never
takes the value 0 so that can be used to tell if a PIN is set.

Acked-by: Rouven Czerwinski <[email protected]>
Signed-off-by: Jens Wiklander <[email protected]>
Replaces the fields use to keep track of encrypted PINs with fields to
keep track of hashed PINs instead.

Acked-by: Rouven Czerwinski <[email protected]>
Reviewed-by: Etienne Carriere <[email protected]>
Signed-off-by: Jens Wiklander <[email protected]>
update_persistent_db() updates the persistent database or panics on
failure.

Acked-by: Rouven Czerwinski <[email protected]>
Co-developed-by: Etienne Carriere <[email protected]>
Signed-off-by: Etienne Carriere <[email protected]>
Signed-off-by: Jens Wiklander <[email protected]>
PKCS11_CMD_INIT_TOKEN implements C_InitToken() client API function that
is in charge of initializing the Security Officer login PIN if not
already done and destroy objects that can be. As objects are not yet
supported in the TA, this later feature is not implemented.

Acked-by: Rouven Czerwinski <[email protected]>
Co-developed-by: Etienne Carriere <[email protected]>
Signed-off-by: Etienne Carriere <[email protected]>
Signed-off-by: Jens Wiklander <[email protected]>
PKCS11_CMD_INIT_PIN implements C_InitPIN() client API function that is in
charge of initializing the normal user login PIN.  Security Officer must
be logged to current session in order to call this function

Acked-by: Rouven Czerwinski <[email protected]>
Co-developed-by: Etienne Carriere <[email protected]>
Signed-off-by: Etienne Carriere <[email protected]>
Signed-off-by: Jens Wiklander <[email protected]>
PKCS11_CMD_SET_PIN implements C_SetPIN() client API function that is in
charge of modifying a login PIN.

Acked-by: Rouven Czerwinski <[email protected]>
Co-developed-by: Etienne Carriere <[email protected]>
Signed-off-by: Etienne Carriere <[email protected]>
Signed-off-by: Jens Wiklander <[email protected]>
Implements login/logout support.

Acked-by: Rouven Czerwinski <[email protected]>
Co-developed-by: Etienne Carriere <[email protected]>
Signed-off-by: Etienne Carriere <[email protected]>
Signed-off-by: Jens Wiklander <[email protected]>
PINs are hashed with a salt instead of being encrypted with a secret
key. So remove the now unused management of these secret keys.

Acked-by: Rouven Czerwinski <[email protected]>
Signed-off-by: Jens Wiklander <[email protected]>
@jenswi-linaro
Copy link
Contributor Author

Squashed and tag applied

@jforissier jforissier merged commit aa442cd into OP-TEE:master Apr 16, 2020
jforissier pushed a commit to OP-TEE/optee_client that referenced this pull request Apr 16, 2020
Sync with PKCS11 TA API update from [1] for authentication related
commands.

Link: [1] OP-TEE/optee_os#3750

Reviewed-by: Rouven Czerwinski <[email protected]>
Signed-off-by: Etienne Carriere <[email protected]>
Signed-off-by: Jens Wiklander <[email protected]>
@jenswi-linaro jenswi-linaro deleted the pkcs11_login branch April 16, 2020 14:13
Copy link
Contributor

@wamserma wamserma left a comment

Choose a reason for hiding this comment

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

Oh, you merged before I finished reviewing..

*
* [in] memref[0] = [
* 32bit slot ID,
* 32bit PIN length,
Copy link
Contributor

Choose a reason for hiding this comment

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

align description of arguments with those of remaining pin functions, e.g. PIN length -> PIN byte size

* 32bit session handle,
* 32bit old PIN byte size,
* 32bit new PIN byte size,
* byte array: PIN data,
Copy link
Contributor

Choose a reason for hiding this comment

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

old PIN data

PKCS11_CMD_SET_PIN = 12,

/*
* PKCS11_CMD_LOGIN - Initialize user PIN
Copy link
Contributor

Choose a reason for hiding this comment

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

-> PKCS11_CMD_LOGIN - Log into token


if (res == TEE_SUCCESS)
DMSG("PIN key found");
res = TEE_AllocateOperation(&oh, TEE_ALG_SHA256, TEE_MODE_DIGEST, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

SHA256 might not be the best choice here. Ideally it would be Argon2, but to do with what we have we could at least use PBKDF2 unless this was specifically excluded from OP-TEE in conf.mk.
This is a second line of defence in case RPMB-based retry-counters and/or secure storage are not available.

Copy link
Contributor

@Emantor Emantor Apr 16, 2020

Choose a reason for hiding this comment

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

Ah nice, I checked this to against the TEE specification, but did conclude that the specifications best hash function for this was SHA256 (or SHA512). As OP-TEE defines PBKDF2, we should definitely switch to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, with sensible parameters for iteration count. What to do if PBKDF2 is deactivated from the config? Also deactivate PKCS#11 support or fall back to SHA256 (with more than one iteration)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can depend on PBKDF2 being available.

res = TEE_AllocateTransientObject(TEE_TYPE_AES, 128, &hdl);
if (res)
TEE_Panic(0);
enum pkcs11_rc hash_pin(enum pkcs11_user_type user, const uint8_t *pin,
Copy link
Contributor

Choose a reason for hiding this comment

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

I got a bit confused by the naming of the function. With hash_pin one would expect to get the same hash for a given set of inputs (and this function being used in the verification step), but this function also generates a fresh salt.
Maybe rename to init_pin_hash.

TEE_Panic(0);
TEE_GenerateRandom(&s, sizeof(s));
if (!s)
s++;
Copy link
Contributor

Choose a reason for hiding this comment

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

This creates a slight bias towards a salt of value 1 and looks a bit like a weird tacked-on fix. Is there a specific reason for excluding the value 0? If so, why not

uint32_t s = 0;
while (s == 0) { TEE_GenerateRandom(&s, sizeof(s)); }

for better readability.
If one also needs to cover the case of a broken GenerateRandom that always returns 0, add a constant in the loop body.

Copy link
Contributor

Choose a reason for hiding this comment

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

A salt of 0 indicates that there is no salt set AFAIR from the rest of the code. I think the bias would be small, but agree that your implementation for this would be cleaner. Do you want to create a PR against current master with your fixes?

Copy link
Contributor

Choose a reason for hiding this comment

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

From reading the rest of the code a salt of 0 indicates the user PIN has not yet been initialised.


if (res)
TEE_Panic(res);
if (buf_compare_ct(tmp_hash, hash, TEE_MAX_HASH_SIZE))
Copy link
Contributor

Choose a reason for hiding this comment

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


TEE_CloseObject(key_hdl);
return rc;
Copy link
Contributor

Choose a reason for hiding this comment

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

Execution flow arrives here only with rc being 0. But imho explicitly setting the OK in the else path of the hash-comparison would be clearer.

@@ -39,6 +40,8 @@ struct pkcs11_client;
#define PKCS11_MAX_USERS 2
#define PKCS11_TOKEN_PIN_SIZE_MAX 128
#define PKCS11_TOKEN_PIN_SIZE_MIN 10
#define PKCS11_TOKEN_SO_PIN_COUNT_MAX 7
#define PKCS11_TOKEN_USER_PIN_COUNT_MAX 7
Copy link
Contributor

Choose a reason for hiding this comment

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

tabs/spacing

@@ -47,22 +50,22 @@ struct pkcs11_client;
* @label - pkcs11 formatted token label, set by client
* @flags - pkcs11 token flags
* @so_pin_count - counter on security officer login failure
* @so_pin_size - byte size of the provisioned SO PIN
* @so_pin - stores the SO PIN
* @so_pin_salt - stores salt in hash of SO PIN, 0 if not set
Copy link
Contributor

Choose a reason for hiding this comment

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

stores salt for hash... (in is ambiguous: could also mean the salt is prepended to the hash instead of being stored in its own field)

Copy link
Contributor

@wamserma wamserma left a comment

Choose a reason for hiding this comment

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

Part 2 of my review.

struct ck_token *token = session->token;
enum pkcs11_rc rc = PKCS11_CKR_OK;

assert(token->db_main->flags & PKCS11_CKFT_TOKEN_INITIALIZED);
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand the specification, we could also return CKR_OPERATION_NOT_INITIALIZED here instead of using the assert statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This "can't happen", that flag is always checked before calling this function.

Copy link
Contributor

Choose a reason for hiding this comment

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

So it is either dead code and can be removed or the abort is desired and a TEE_Panic would be the proper way to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All asserts are supposed to be dead code.


TAILQ_FOREACH(sess, &client->session_list, link) {
if (sess->token != session->token)
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

This does assume that we eventually find the right one. Otherwise simply the last session in the list is used.
Maybe we should add

if (sess->token != session->token) return;

here to avoid manipulating the state of a wrong session (and possibly elevate privileges)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're not just looking for one session, we're looking for all sessions for the client on this token.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad. Got the scope of the FOREACH wrong while reading.

TAILQ_FOREACH(sess, &client->session_list, link) {
if (sess->token != session->token)
continue;

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ingore.

TAILQ_FOREACH(sess, &client->session_list, link) {
if (sess->token != session->token)
continue;

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ignore.

@jforissier
Copy link
Contributor

Oh, you merged before I finished reviewing..

😮 sorry about that. You're right, I should leave a bit more time for reviewers...

But it should not prevent us from addressing your comments, let's address them in a new PR if you don't mind, @jenswi-linaro?

@jenswi-linaro
Copy link
Contributor Author

@wamserma, feel free to raise PR to fix what you think need to be fixed.

@wamserma
Copy link
Contributor

@jforissier It's ok. There were no super-critical comments and I don't want to slow down anything.
@jenswi-linaro A few of the points need discussion before a new PR can solve the remainders.

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.

5 participants