Patrick Williams | 7363086 | 2024-03-01 14:30:19 -0600 | [diff] [blame] | 1 | From d75c42ff2847b090d5b1f11c49067cd41fcc2734 Mon Sep 17 00:00:00 2001 |
| 2 | From: Loic Poulain <loic.poulain@linaro.org> |
| 3 | Date: Tue, 31 Oct 2023 11:07:00 +0100 |
| 4 | Subject: [PATCH] ta: pkcs11: Improve PIN counter handling robustness |
| 5 | |
| 6 | Make sure PIN check attempt is saved persistently before continuing with |
| 7 | the actual PIN verification, improving counter and flags coherency in |
| 8 | case of subsequent failure with persistent saving. |
| 9 | |
| 10 | Signed-off-by: Loic Poulain <loic.poulain@linaro.org> |
| 11 | Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> |
| 12 | Acked-by: Jerome Forissier <jerome.forissier@linaro.org> |
| 13 | Upstream-Status: Backport [https://github.com/OP-TEE/optee_os/pull/6445/commits/0a74733d9437d94a5b4b2db6c40c5755cabc5393] |
| 14 | --- |
| 15 | ta/pkcs11/src/pkcs11_token.c | 126 +++++++++++++++++------------------ |
| 16 | 1 file changed, 62 insertions(+), 64 deletions(-) |
| 17 | |
| 18 | diff --git a/ta/pkcs11/src/pkcs11_token.c b/ta/pkcs11/src/pkcs11_token.c |
| 19 | index ab0fc291e..c5271e449 100644 |
| 20 | --- a/ta/pkcs11/src/pkcs11_token.c |
| 21 | +++ b/ta/pkcs11/src/pkcs11_token.c |
| 22 | @@ -1132,117 +1132,115 @@ static enum pkcs11_rc check_so_pin(struct pkcs11_session *session, |
| 23 | uint8_t *pin, size_t pin_size) |
| 24 | { |
| 25 | struct ck_token *token = session->token; |
| 26 | + struct token_persistent_main *db = token->db_main; |
| 27 | enum pkcs11_rc rc = PKCS11_CKR_OK; |
| 28 | |
| 29 | - assert(token->db_main->flags & PKCS11_CKFT_TOKEN_INITIALIZED); |
| 30 | + assert(db->flags & PKCS11_CKFT_TOKEN_INITIALIZED); |
| 31 | |
| 32 | if (IS_ENABLED(CFG_PKCS11_TA_AUTH_TEE_IDENTITY) && |
| 33 | - token->db_main->flags & PKCS11_CKFT_PROTECTED_AUTHENTICATION_PATH) |
| 34 | + db->flags & PKCS11_CKFT_PROTECTED_AUTHENTICATION_PATH) |
| 35 | return verify_identity_auth(token, PKCS11_CKU_SO); |
| 36 | |
| 37 | - if (token->db_main->flags & PKCS11_CKFT_SO_PIN_LOCKED) |
| 38 | + if (db->flags & PKCS11_CKFT_SO_PIN_LOCKED) |
| 39 | return PKCS11_CKR_PIN_LOCKED; |
| 40 | |
| 41 | - rc = verify_pin(PKCS11_CKU_SO, pin, pin_size, |
| 42 | - token->db_main->so_pin_salt, |
| 43 | - token->db_main->so_pin_hash); |
| 44 | - if (rc) { |
| 45 | - unsigned int pin_count = 0; |
| 46 | + /* |
| 47 | + * Preset the counter and flags conservatively in the database so that |
| 48 | + * the tentative is saved whatever happens next. |
| 49 | + */ |
| 50 | + db->flags |= PKCS11_CKFT_SO_PIN_COUNT_LOW; |
| 51 | + db->so_pin_count++; |
| 52 | |
| 53 | - if (rc != PKCS11_CKR_PIN_INCORRECT) |
| 54 | - return rc; |
| 55 | + if (db->so_pin_count == PKCS11_TOKEN_SO_PIN_COUNT_MAX - 1) |
| 56 | + db->flags |= PKCS11_CKFT_SO_PIN_FINAL_TRY; |
| 57 | + else if (db->so_pin_count == PKCS11_TOKEN_SO_PIN_COUNT_MAX) |
| 58 | + db->flags |= PKCS11_CKFT_SO_PIN_LOCKED; |
| 59 | |
| 60 | - token->db_main->flags |= PKCS11_CKFT_SO_PIN_COUNT_LOW; |
| 61 | - token->db_main->so_pin_count++; |
| 62 | - |
| 63 | - pin_count = token->db_main->so_pin_count; |
| 64 | - if (pin_count == PKCS11_TOKEN_SO_PIN_COUNT_MAX - 1) |
| 65 | - token->db_main->flags |= PKCS11_CKFT_SO_PIN_FINAL_TRY; |
| 66 | - if (pin_count == PKCS11_TOKEN_SO_PIN_COUNT_MAX) |
| 67 | - token->db_main->flags |= PKCS11_CKFT_SO_PIN_LOCKED; |
| 68 | - |
| 69 | - update_persistent_db(token); |
| 70 | + update_persistent_db(token); |
| 71 | |
| 72 | - if (token->db_main->flags & PKCS11_CKFT_SO_PIN_LOCKED) |
| 73 | + rc = verify_pin(PKCS11_CKU_SO, pin, pin_size, |
| 74 | + db->so_pin_salt, |
| 75 | + db->so_pin_hash); |
| 76 | + if (rc == PKCS11_CKR_PIN_INCORRECT) { |
| 77 | + if (db->flags & PKCS11_CKFT_SO_PIN_LOCKED) |
| 78 | return PKCS11_CKR_PIN_LOCKED; |
| 79 | |
| 80 | return PKCS11_CKR_PIN_INCORRECT; |
| 81 | } |
| 82 | |
| 83 | - if (token->db_main->so_pin_count) { |
| 84 | - token->db_main->so_pin_count = 0; |
| 85 | + if (rc) |
| 86 | + db->so_pin_count--; |
| 87 | + else |
| 88 | + db->so_pin_count = 0; |
| 89 | |
| 90 | - update_persistent_db(token); |
| 91 | + db->flags &= ~PKCS11_CKFT_SO_PIN_LOCKED; |
| 92 | + if (db->so_pin_count < PKCS11_TOKEN_SO_PIN_COUNT_MAX - 1) { |
| 93 | + db->flags &= ~PKCS11_CKFT_SO_PIN_FINAL_TRY; |
| 94 | + if (!db->so_pin_count) |
| 95 | + db->flags &= ~PKCS11_CKFT_SO_PIN_COUNT_LOW; |
| 96 | } |
| 97 | |
| 98 | - if (token->db_main->flags & (PKCS11_CKFT_SO_PIN_COUNT_LOW | |
| 99 | - PKCS11_CKFT_SO_PIN_FINAL_TRY)) { |
| 100 | - token->db_main->flags &= ~(PKCS11_CKFT_SO_PIN_COUNT_LOW | |
| 101 | - PKCS11_CKFT_SO_PIN_FINAL_TRY); |
| 102 | - |
| 103 | - update_persistent_db(token); |
| 104 | - } |
| 105 | + update_persistent_db(token); |
| 106 | |
| 107 | - return PKCS11_CKR_OK; |
| 108 | + return rc; |
| 109 | } |
| 110 | |
| 111 | static enum pkcs11_rc check_user_pin(struct pkcs11_session *session, |
| 112 | uint8_t *pin, size_t pin_size) |
| 113 | { |
| 114 | struct ck_token *token = session->token; |
| 115 | + struct token_persistent_main *db = token->db_main; |
| 116 | enum pkcs11_rc rc = PKCS11_CKR_OK; |
| 117 | |
| 118 | if (IS_ENABLED(CFG_PKCS11_TA_AUTH_TEE_IDENTITY) && |
| 119 | - token->db_main->flags & PKCS11_CKFT_PROTECTED_AUTHENTICATION_PATH) |
| 120 | + db->flags & PKCS11_CKFT_PROTECTED_AUTHENTICATION_PATH) |
| 121 | return verify_identity_auth(token, PKCS11_CKU_USER); |
| 122 | |
| 123 | - if (!token->db_main->user_pin_salt) |
| 124 | + if (!db->user_pin_salt) |
| 125 | return PKCS11_CKR_USER_PIN_NOT_INITIALIZED; |
| 126 | |
| 127 | - if (token->db_main->flags & PKCS11_CKFT_USER_PIN_LOCKED) |
| 128 | + if (db->flags & PKCS11_CKFT_USER_PIN_LOCKED) |
| 129 | return PKCS11_CKR_PIN_LOCKED; |
| 130 | |
| 131 | - rc = verify_pin(PKCS11_CKU_USER, pin, pin_size, |
| 132 | - token->db_main->user_pin_salt, |
| 133 | - token->db_main->user_pin_hash); |
| 134 | - if (rc) { |
| 135 | - unsigned int pin_count = 0; |
| 136 | - |
| 137 | - if (rc != PKCS11_CKR_PIN_INCORRECT) |
| 138 | - return rc; |
| 139 | - |
| 140 | - token->db_main->flags |= PKCS11_CKFT_USER_PIN_COUNT_LOW; |
| 141 | - token->db_main->user_pin_count++; |
| 142 | + /* |
| 143 | + * Preset the counter and flags conservatively in the database so that |
| 144 | + * the tentative is saved whatever happens next. |
| 145 | + */ |
| 146 | + db->flags |= PKCS11_CKFT_USER_PIN_COUNT_LOW; |
| 147 | + db->user_pin_count++; |
| 148 | |
| 149 | - pin_count = token->db_main->user_pin_count; |
| 150 | - if (pin_count == PKCS11_TOKEN_USER_PIN_COUNT_MAX - 1) |
| 151 | - token->db_main->flags |= PKCS11_CKFT_USER_PIN_FINAL_TRY; |
| 152 | - if (pin_count == PKCS11_TOKEN_USER_PIN_COUNT_MAX) |
| 153 | - token->db_main->flags |= PKCS11_CKFT_USER_PIN_LOCKED; |
| 154 | + if (db->user_pin_count == PKCS11_TOKEN_USER_PIN_COUNT_MAX - 1) |
| 155 | + db->flags |= PKCS11_CKFT_USER_PIN_FINAL_TRY; |
| 156 | + else if (db->user_pin_count == PKCS11_TOKEN_USER_PIN_COUNT_MAX) |
| 157 | + db->flags |= PKCS11_CKFT_USER_PIN_LOCKED; |
| 158 | |
| 159 | - update_persistent_db(token); |
| 160 | + update_persistent_db(token); |
| 161 | |
| 162 | - if (token->db_main->flags & PKCS11_CKFT_USER_PIN_LOCKED) |
| 163 | + rc = verify_pin(PKCS11_CKU_USER, pin, pin_size, |
| 164 | + db->user_pin_salt, |
| 165 | + db->user_pin_hash); |
| 166 | + if (rc == PKCS11_CKR_PIN_INCORRECT) { |
| 167 | + if (db->flags & PKCS11_CKFT_USER_PIN_LOCKED) |
| 168 | return PKCS11_CKR_PIN_LOCKED; |
| 169 | |
| 170 | return PKCS11_CKR_PIN_INCORRECT; |
| 171 | } |
| 172 | |
| 173 | - if (token->db_main->user_pin_count) { |
| 174 | - token->db_main->user_pin_count = 0; |
| 175 | + if (rc) |
| 176 | + db->user_pin_count--; |
| 177 | + else |
| 178 | + db->user_pin_count = 0; |
| 179 | |
| 180 | - update_persistent_db(token); |
| 181 | + db->flags &= ~PKCS11_CKFT_USER_PIN_LOCKED; |
| 182 | + if (db->user_pin_count < PKCS11_TOKEN_USER_PIN_COUNT_MAX - 1) { |
| 183 | + db->flags &= ~PKCS11_CKFT_USER_PIN_FINAL_TRY; |
| 184 | + if (!db->user_pin_count) |
| 185 | + db->flags &= ~PKCS11_CKFT_USER_PIN_COUNT_LOW; |
| 186 | } |
| 187 | |
| 188 | - if (token->db_main->flags & (PKCS11_CKFT_USER_PIN_COUNT_LOW | |
| 189 | - PKCS11_CKFT_USER_PIN_FINAL_TRY)) { |
| 190 | - token->db_main->flags &= ~(PKCS11_CKFT_USER_PIN_COUNT_LOW | |
| 191 | - PKCS11_CKFT_USER_PIN_FINAL_TRY); |
| 192 | - |
| 193 | - update_persistent_db(token); |
| 194 | - } |
| 195 | + update_persistent_db(token); |
| 196 | |
| 197 | - return PKCS11_CKR_OK; |
| 198 | + return rc; |
| 199 | } |
| 200 | |
| 201 | enum pkcs11_rc entry_ck_set_pin(struct pkcs11_client *client, |
| 202 | -- |
| 203 | 2.25.1 |
| 204 | |
| 205 | |