I'm trying to use Jan-Piet Mens' pbkdf2 code from his mosquitto-auth-plug in a multithreaded app.
It all works fine when I run the code in a single thread, but I start getting Invalid reads (using valgrind) for many of the internal free()s in the pbkdf2_check() function.
I'm not using any global shared resources in my code and have setup openSSL locking callbacks using this libcurl example, but I still keep getting Invalid reads, mostly with the free()s in pbkdf2_check().
Here's my code:
pbkdf2.c (Based off Jan-Piet Mens' pbkdf2.c code)
/*
* Copyright (c) 2013 Jan-Piet Mens <jpmens()gmail.com>
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are met:
*
* 1. Redistributions of source code must retain the above copyright notice,
* this list of conditions and the following disclaimer.
* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in the
* documentation and/or other materials provided with the distribution.
* 3. Neither the name of mosquitto nor the names of its
* contributors may be used to endorse or promote products derived from
* this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
* ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
* LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
* CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
* SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
* INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
* CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
* POSSIBILITY OF SUCH DAMAGE.
*/
#include "opensslthreadlock.h"
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <openssl/evp.h>
#include "base64.h"
#define SEPARATOR "$"
#define TRUE (1)
#define FALSE (0)
#define NUMT 5
/*
* Split PBKDF2$... string into their components. The caller must free()
* the strings.
*/
static int detoken(char *pbkstr, char **sha, int *iter, char **salt, char **key)
{
char *p, *s, *save;
int rc = 1;
save = s = strdup(pbkstr);
if ((p = strsep(&s, SEPARATOR)) == NULL)
goto out;
if (strcmp(p, "PBKDF2") != 0)
goto out;
if ((p = strsep(&s, SEPARATOR)) == NULL)
goto out;
*sha = strdup(p);
if ((p = strsep(&s, SEPARATOR)) == NULL)
goto out;
*iter = atoi(p);
if ((p = strsep(&s, SEPARATOR)) == NULL)
goto out;
*salt = strdup(p);
if ((p = strsep(&s, SEPARATOR)) == NULL)
goto out;
*key = strdup(p);
rc = 0;
out:
free(save);
return rc;
}
int pbkdf2_check(char *password, char *hash)
{
static char *sha, *salt, *h_pw;
int iterations, saltlen, blen;
char *b64, *keybuf;
unsigned char *out;
int match = FALSE;
const EVP_MD *evpmd;
int keylen, rc;
if (detoken(hash, &sha, &iterations, &salt, &h_pw) != 0)
return match;
/* Determine key length by decoding base64 */
if ((keybuf = malloc(strlen(h_pw) + 1)) == NULL) {
fprintf(stderr, "Out of memory\n");
return FALSE;
}
keylen = base64_decode(h_pw, keybuf);
if (keylen < 1) {
free(keybuf);
return (FALSE);
}
free(keybuf);
if ((out = malloc(keylen)) == NULL) {
fprintf(stderr, "Cannot allocate out; out of memory\n");
return (FALSE);
}
#ifdef PWDEBUG
fprintf(stderr, "sha =[%s]\n", sha);
fprintf(stderr, "iterations =%d\n", iterations);
fprintf(stderr, "salt =[%s]\n", salt);
fprintf(stderr, "h_pw =[%s]\n", h_pw);
fprintf(stderr, "kenlen =[%d]\n", keylen);
#endif
saltlen = strlen((char *)salt);
evpmd = EVP_sha256();
if (strcmp(sha, "sha1") == 0) {
evpmd = EVP_sha1();
} else if (strcmp(sha, "sha512") == 0) {
evpmd = EVP_sha512();
}
rc = PKCS5_PBKDF2_HMAC(password, strlen(password),
(unsigned char *)salt, saltlen,
iterations,
evpmd, keylen, out);
if (rc != 1) {
goto out;
}
blen = base64_encode(out, keylen, &b64);
if (blen > 0) {
int i, diff = 0, hlen = strlen(h_pw);
#ifdef PWDEBUG
fprintf(stderr, "HMAC b64 =[%s]\n", b64);
#endif
/* "manual" strcmp() to ensure constant time */
for (i = 0; (i < blen) && (i < hlen); i++) {
diff |= h_pw[i] ^ b64[i];
}
match = diff == 0;
if (hlen != blen)
match = 0;
free(b64);
}
out:
free(sha);
free(salt);
free(h_pw);
free(out);
return match;
}
void *test_pbkdf2(void *argt) {
int i;
for (i = 0; i < 3; ++i) {
char password[] = "password";
char pbkstr[] = "PBKDF2$sha1$98$XaIs9vQgmLujKHZG4/B3dNTbeP2PyaVKySTirZznBrE=$2DX/HZDTojVbfgAIdozBi6CihjWP1+akYnh/h9uQfIVl6pLoAiwJe1ey2WW2BnT+";
int match;
printf("Checking password [%s] for %s\n", password, pbkstr);
match = pbkdf2_check(password, pbkstr);
printf("match == %d\n", match);
}
pthread_exit();
}
int main(int argc, char **argv)
{
pthread_t tid[NUMT];
int i;
int error;
(void)argc; /* we don't use any arguments in this example */
(void)argv;
thread_setup();
for(i=0; i< NUMT; i++) {
error = pthread_create(&tid[i],
NULL, /* default attributes please */
test_pbkdf2,
NULL);
if(0 != error)
fprintf(stderr, "Couldn't run thread number %d, errno %d\n", i, error);
}
/* now wait for all threads to terminate */
for(i=0; i< NUMT; i++) {
error = pthread_join(tid[i], NULL);
fprintf(stderr, "Thread %d terminated\n", i);
}
thread_cleanup();
return 0;
}
opensslthreadlock.h:
/***************************************************************************
* _ _ ____ _
* Project ___| | | | _ \| |
* / __| | | | |_) | |
* | (__| |_| | _ <| |___
* \___|\___/|_| \_\_____|
*
* Copyright (C) 1998 - 2016, Daniel Stenberg, <[email protected]>, et al.
*
* This software is licensed as described in the file COPYING, which
* you should have received as part of this distribution. The terms
* are also available at https://curl.haxx.se/docs/copyright.html.
*
* You may opt to use, copy, modify, merge, publish, distribute and/or sell
* copies of the Software, and permit persons to whom the Software is
* furnished to do so, under the terms of the COPYING file.
*
* This software is distributed on an "AS IS" basis, WITHOUT WARRANTY OF ANY
* KIND, either express or implied.
*
***************************************************************************/
/* <DESC>
* one way to set the necessary OpenSSL locking callbacks if you want to do
* multi-threaded transfers with HTTPS/FTPS with libcurl built to use OpenSSL.
* </DESC>
*/
/*
* This is not a complete stand-alone example.
*
* Author: Jeremy Brown
*/
int thread_setup(void);
int thread_cleanup(void);
The implementation is here (opensslthreadlock.c). Here's the code for base64.c/h.
I'm using this to build and test:
gcc -o pbkdf2test pbkdf2.c opensslthreadlock.c base64.c -lcrypto -lpthread
valgrind --tool=memcheck --leak-check=full --track-origins=yes -v ./pbkdf2test
It all works fine when I run the code in a single thread, but I start getting Invalid reads (using valgrind) for many of the internal free()s in the pbkdf2_check() function.
That appears to be because of:
static char *sha, *salt, *h_pw;
That will always suffer a race condition as pbkdf2_check is written.
Also, once these run:
out:
free(sha);
free(salt);
free(h_pw);
free(out);
The dangling pointers are reused due to the static storage class.
You should probably remove the static storage class for sha, salt and h_pw, and initialize everything to NULL or 0. Use C99 (-std=c99) and you can:
char *sha=NULL, *salt=NULL, *h_pw=NULL;
int iterations=0, saltlen=0, blen=0;
char *b64=NULL, *keybuf=NULL;
unsigned char *out=NULL;
int match = FALSE;
const EVP_MD *evpmd=NULL;
int keylen=0, rc=-1;
Then, the optimizer will remove the initializers/writes that are not needed.