Clean up PKCS5_PBKDF2_HMAC.
This was a mess. HMAC_CTX_copy_ex would avoid having to cleanup and init
the HMAC_CTX repeatedly, but even that is unnecessary. hctx_tpl was just
to reuse the key. Instead, HMAC_CTX already can be reset with the same
key. (Alas, with a slightly odd API, but so it goes.) Do that, and use
goto err to cleanup the error-handling.
Thanks to upstream's b98530d6e09f4cb34c791b8840e936c1fc1467cf for
drawing attention to this. (Though we've diverged significantly from
upstream with all the heap-allocated bits, so I didn't use the change
itself.)
While I'm here, tidy up some variable names and cite the newer RFC.
Change-Id: Ic1259f46b7c5a14dc341b8cee385be5508ac4daf
Reviewed-on: https://boringssl-review.googlesource.com/14605
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/crypto/evp/pbkdf.c b/crypto/evp/pbkdf.c
index 1792cdc..daebb2d 100644
--- a/crypto/evp/pbkdf.c
+++ b/crypto/evp/pbkdf.c
@@ -65,83 +65,76 @@
int PKCS5_PBKDF2_HMAC(const char *password, size_t password_len,
const uint8_t *salt, size_t salt_len, unsigned iterations,
const EVP_MD *digest, size_t key_len, uint8_t *out_key) {
- uint8_t digest_tmp[EVP_MAX_MD_SIZE], *p, itmp[4];
- size_t cplen, mdlen, tkeylen, k;
- unsigned j;
+ /* See RFC 8018, section 5.2. */
+ int ret = 0;
+ size_t md_len = EVP_MD_size(digest);
uint32_t i = 1;
- HMAC_CTX hctx_tpl, hctx;
+ HMAC_CTX hctx;
+ HMAC_CTX_init(&hctx);
- mdlen = EVP_MD_size(digest);
- HMAC_CTX_init(&hctx_tpl);
- p = out_key;
- tkeylen = key_len;
- if (!HMAC_Init_ex(&hctx_tpl, password, password_len, digest, NULL)) {
- HMAC_CTX_cleanup(&hctx_tpl);
- return 0;
+ if (!HMAC_Init_ex(&hctx, password, password_len, digest, NULL)) {
+ goto err;
}
- while (tkeylen) {
- if (tkeylen > mdlen) {
- cplen = mdlen;
- } else {
- cplen = tkeylen;
+
+ while (key_len > 0) {
+ size_t todo = md_len;
+ if (todo > key_len) {
+ todo = key_len;
}
- /* We are unlikely to ever use more than 256 blocks (5120 bits!)
- * but just in case... */
- itmp[0] = (uint8_t)((i >> 24) & 0xff);
- itmp[1] = (uint8_t)((i >> 16) & 0xff);
- itmp[2] = (uint8_t)((i >> 8) & 0xff);
- itmp[3] = (uint8_t)(i & 0xff);
- if (!HMAC_CTX_copy(&hctx, &hctx_tpl)) {
- HMAC_CTX_cleanup(&hctx_tpl);
- return 0;
- }
- if (!HMAC_Update(&hctx, salt, salt_len) ||
- !HMAC_Update(&hctx, itmp, 4) ||
+
+ uint8_t i_buf[4];
+ i_buf[0] = (uint8_t)((i >> 24) & 0xff);
+ i_buf[1] = (uint8_t)((i >> 16) & 0xff);
+ i_buf[2] = (uint8_t)((i >> 8) & 0xff);
+ i_buf[3] = (uint8_t)(i & 0xff);
+
+ /* Compute U_1. */
+ uint8_t digest_tmp[EVP_MAX_MD_SIZE];
+ if (!HMAC_Init_ex(&hctx, NULL, 0, NULL, NULL) ||
+ !HMAC_Update(&hctx, salt, salt_len) ||
+ !HMAC_Update(&hctx, i_buf, 4) ||
!HMAC_Final(&hctx, digest_tmp, NULL)) {
- HMAC_CTX_cleanup(&hctx_tpl);
- HMAC_CTX_cleanup(&hctx);
- return 0;
+ goto err;
}
- HMAC_CTX_cleanup(&hctx);
- OPENSSL_memcpy(p, digest_tmp, cplen);
- for (j = 1; j < iterations; j++) {
- if (!HMAC_CTX_copy(&hctx, &hctx_tpl)) {
- HMAC_CTX_cleanup(&hctx_tpl);
- return 0;
- }
- if (!HMAC_Update(&hctx, digest_tmp, mdlen) ||
+
+ OPENSSL_memcpy(out_key, digest_tmp, todo);
+ for (unsigned j = 1; j < iterations; j++) {
+ /* Compute the remaining U_* values and XOR. */
+ if (!HMAC_Init_ex(&hctx, NULL, 0, NULL, NULL) ||
+ !HMAC_Update(&hctx, digest_tmp, md_len) ||
!HMAC_Final(&hctx, digest_tmp, NULL)) {
- HMAC_CTX_cleanup(&hctx_tpl);
- HMAC_CTX_cleanup(&hctx);
- return 0;
+ goto err;
}
- HMAC_CTX_cleanup(&hctx);
- for (k = 0; k < cplen; k++) {
- p[k] ^= digest_tmp[k];
+ for (size_t k = 0; k < todo; k++) {
+ out_key[k] ^= digest_tmp[k];
}
}
- tkeylen -= cplen;
+
+ key_len -= todo;
+ out_key += todo;
i++;
- p += cplen;
}
- HMAC_CTX_cleanup(&hctx_tpl);
- // RFC 2898 describes iterations (c) as being a "positive integer", so a
- // value of 0 is an error.
- //
- // Unfortunatley not all consumers of PKCS5_PBKDF2_HMAC() check their return
- // value, expecting it to succeed and unconditonally using |out_key|.
- // As a precaution for such callsites in external code, the old behavior
- // of iterations < 1 being treated as iterations == 1 is preserved, but
- // additionally an error result is returned.
- //
- // TODO(eroman): Figure out how to remove this compatibility hack, or change
- // the default to something more sensible like 2048.
+ /* RFC 8018 describes iterations (c) as being a "positive integer", so a
+ * value of 0 is an error.
+ *
+ * Unfortunately not all consumers of PKCS5_PBKDF2_HMAC() check their return
+ * value, expecting it to succeed and unconditionally using |out_key|. As a
+ * precaution for such callsites in external code, the old behavior of
+ * iterations < 1 being treated as iterations == 1 is preserved, but
+ * additionally an error result is returned.
+ *
+ * TODO(eroman): Figure out how to remove this compatibility hack, or change
+ * the default to something more sensible like 2048. */
if (iterations == 0) {
- return 0;
+ goto err;
}
- return 1;
+ ret = 1;
+
+err:
+ HMAC_CTX_cleanup(&hctx);
+ return ret;
}
int PKCS5_PBKDF2_HMAC_SHA1(const char *password, size_t password_len,
