Skip to content

Commit

Permalink
fix(auth): Fix a potential buffer overflow in iperf3 client.
Browse files Browse the repository at this point in the history
This condition was only possible when configuration authentication
via the libiperf API.

While here, also fix a few other sundry issues:

* Remove arbitrary length limits on username and password.

* Improved error handling.

* Updated error messages for readability.

* Fixed minor typo in some identifiers.

Fixes #996.
  • Loading branch information
bmah888 committed May 20, 2020
1 parent 79630e8 commit 06e3f08
Show file tree
Hide file tree
Showing 5 changed files with 80 additions and 32 deletions.
31 changes: 18 additions & 13 deletions src/iperf_api.c
Original file line number Diff line number Diff line change
Expand Up @@ -1294,12 +1294,6 @@ iperf_parse_arguments(struct iperf_test *test, int argc, char **argv)
else if (iperf_getpass(&client_password, &s, stdin) < 0){
return -1;
}

if (strlen(client_username) > 20 || strlen(client_password) > 20){
i_errno = IESETCLIENTAUTH;
return -1;
}

if (test_load_pubkey_from_file(client_rsa_public_key) < 0){
i_errno = IESETCLIENTAUTH;
return -1;
Expand Down Expand Up @@ -1589,15 +1583,18 @@ int test_is_authorized(struct iperf_test *test){
if (test->settings->authtoken){
char *username = NULL, *password = NULL;
time_t ts;
decode_auth_setting(test->debug, test->settings->authtoken, test->server_rsa_private_key, &username, &password, &ts);
int rc = decode_auth_setting(test->debug, test->settings->authtoken, test->server_rsa_private_key, &username, &password, &ts);
if (rc) {
return -1;
}
int ret = check_authentication(username, password, ts, test->server_authorized_users);
if (ret == 0){
iperf_printf(test, report_authetication_successed, username, ts);
iperf_printf(test, report_authentication_succeeded, username, ts);
free(username);
free(password);
return 0;
} else {
iperf_printf(test, report_authetication_failed, username, ts);
iperf_printf(test, report_authentication_failed, username, ts);
free(username);
free(password);
return -1;
Expand Down Expand Up @@ -1760,10 +1757,18 @@ send_parameters(struct iperf_test *test)
if (test->repeating_payload)
cJSON_AddNumberToObject(j, "repeating_payload", test->repeating_payload);
#if defined(HAVE_SSL)
if (test->settings->client_username && test->settings->client_password && test->settings->client_rsa_pubkey){
encode_auth_setting(test->settings->client_username, test->settings->client_password, test->settings->client_rsa_pubkey, &test->settings->authtoken);
cJSON_AddStringToObject(j, "authtoken", test->settings->authtoken);
}
/* Send authentication parameters */
if (test->settings->client_username && test->settings->client_password && test->settings->client_rsa_pubkey){
int rc = encode_auth_setting(test->settings->client_username, test->settings->client_password, test->settings->client_rsa_pubkey, &test->settings->authtoken);

if (rc) {
cJSON_Delete(j);
i_errno = IESENDPARAMS;
return -1;
}

cJSON_AddStringToObject(j, "authtoken", test->settings->authtoken);
}
#endif // HAVE_SSL
cJSON_AddStringToObject(j, "client_version", IPERF_VERSION);

Expand Down
63 changes: 53 additions & 10 deletions src/iperf_auth.c
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* iperf, Copyright (c) 2014-2018, The Regents of the University of
* iperf, Copyright (c) 2014-2020, The Regents of the University of
* California, through Lawrence Berkeley National Laboratory (subject
* to receipt of any required approvals from the U.S. Dept. of
* Energy). All rights reserved.
Expand Down Expand Up @@ -43,6 +43,9 @@
#include <openssl/pem.h>
#include <openssl/sha.h>
#include <openssl/buffer.h>
#include <openssl/err.h>

const char *auth_text_format = "user: %s\npwd: %s\nts: %ld";

void sha256(const char *string, char outputBuffer[65])
{
Expand Down Expand Up @@ -238,6 +241,11 @@ int encrypt_rsa_message(const char *plaintext, EVP_PKEY *public_key, unsigned ch
OPENSSL_free(rsa_buffer);
BIO_free(bioBuff);

if (encryptedtext_len < 0) {
/* We probably shoudln't be printing stuff like this */
fprintf(stderr, "%s\n", ERR_error_string(ERR_get_error(), NULL));
}

return encryptedtext_len;
}

Expand All @@ -260,17 +268,35 @@ int decrypt_rsa_message(const unsigned char *encryptedtext, const int encryptedt
OPENSSL_free(rsa_buffer);
BIO_free(bioBuff);

if (plaintext_len < 0) {
/* We probably shoudln't be printing stuff like this */
fprintf(stderr, "%s\n", ERR_error_string(ERR_get_error(), NULL));
}

return plaintext_len;
}

int encode_auth_setting(const char *username, const char *password, EVP_PKEY *public_key, char **authtoken){
time_t t = time(NULL);
time_t utc_seconds = mktime(localtime(&t));
char text[150];
sprintf (text, "user: %s\npwd: %s\nts: %ld", username, password, utc_seconds);

/*
* Compute a pessimistic/conservative estimate of storage required.
* It's OK to allocate too much storage but too little is bad.
*/
const int text_len = strlen(auth_text_format) + strlen(username) + strlen(password) + 32;
char *text = (char *) calloc(text_len, sizeof(char));

This comment has been minimized.

Copy link
@bmah888

bmah888 May 22, 2020

Author Contributor

Thanks, good catch! Will fix.

if (text == NULL) {
return -1;
}
snprintf(text, text_len, auth_text_format, username, password, utc_seconds);

unsigned char *encrypted = NULL;
int encrypted_len;
encrypted_len = encrypt_rsa_message(text, public_key, &encrypted);
if (encrypted_len < 0) {
return -1;
}
Base64Encode(encrypted, encrypted_len, authtoken);
OPENSSL_free(encrypted);

Expand All @@ -285,19 +311,36 @@ int decode_auth_setting(int enable_debug, char *authtoken, EVP_PKEY *private_key
unsigned char *plaintext = NULL;
int plaintext_len;
plaintext_len = decrypt_rsa_message(encrypted_b64, encrypted_len_b64, private_key, &plaintext);
plaintext[plaintext_len] = '\0';
free(encrypted_b64);
if (plaintext_len < 0) {
return -1;
}
plaintext[plaintext_len] = '\0';

char *s_username, *s_password;
s_username = (char *) calloc(plaintext_len, sizeof(char));
if (s_username == NULL) {
return -1;
}
s_password = (char *) calloc(plaintext_len, sizeof(char));
if (s_password == NULL) {
free(s_username);
return -1;
}

int rc = sscanf((char *) plaintext, auth_text_format, s_username, s_password, ts);
if (rc != 3) {
free(s_password);
free(s_username);
return -1;
}

char s_username[20], s_password[20];
sscanf ((char *)plaintext,"user: %s\npwd: %s\nts: %ld", s_username, s_password, ts);
if (enable_debug) {
printf("Auth Token Content:\n%s\n", plaintext);
printf("Auth Token Credentials:\n--> %s %s\n", s_username, s_password);
}
*username = (char *) calloc(21, sizeof(char));
*password = (char *) calloc(21, sizeof(char));
strncpy(*username, s_username, 20);
strncpy(*password, s_password, 20);
*username = s_username;
*password = s_password;
OPENSSL_free(plaintext);
return (0);
}
Expand Down
6 changes: 3 additions & 3 deletions src/iperf_error.c
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* iperf, Copyright (c) 2014-2019, The Regents of the University of
* iperf, Copyright (c) 2014-2020, The Regents of the University of
* California, through Lawrence Berkeley National Laboratory (subject
* to receipt of any required approvals from the U.S. Dept. of
* Energy). All rights reserved.
Expand Down Expand Up @@ -134,10 +134,10 @@ iperf_strerror(int int_errno)
snprintf(errstr, len, "bad TOS value (must be between 0 and 255 inclusive)");
break;
case IESETCLIENTAUTH:
snprintf(errstr, len, "you must specify username (max 20 chars), password (max 20 chars) and a path to a valid public rsa client to be used");
snprintf(errstr, len, "you must specify a username, password, and path to a valid RSA public key");
break;
case IESETSERVERAUTH:
snprintf(errstr, len, "you must specify path to a valid private rsa server to be used and a user credential file");
snprintf(errstr, len, "you must specify a path to a valid RSA private key and a user credential file");
break;
case IEBADFORMAT:
snprintf(errstr, len, "bad format specifier (valid formats are in the set [kmgtKMGT])");
Expand Down
6 changes: 3 additions & 3 deletions src/iperf_locale.c
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*---------------------------------------------------------------
* iperf, Copyright (c) 2014-2018, The Regents of the University of
* iperf, Copyright (c) 2014-2020, The Regents of the University of
* California, through Lawrence Berkeley National Laboratory (subject
* to receipt of any required approvals from the U.S. Dept. of
* Energy). All rights reserved.
Expand Down Expand Up @@ -272,10 +272,10 @@ const char report_time[] =
const char report_connecting[] =
"Connecting to host %s, port %d\n";

const char report_authetication_successed[] =
const char report_authentication_succeeded[] =
"Authentication successed for user '%s' ts %ld\n";

const char report_authetication_failed[] =
const char report_authentication_failed[] =
"Authentication failed for user '%s' ts %ld\n";

const char report_reverse[] =
Expand Down
6 changes: 3 additions & 3 deletions src/iperf_locale.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* iperf, Copyright (c) 2014-2018, The Regents of the University of
* iperf, Copyright (c) 2014-2020, The Regents of the University of
* California, through Lawrence Berkeley National Laboratory (subject
* to receipt of any required approvals from the U.S. Dept. of
* Energy). All rights reserved.
Expand Down Expand Up @@ -54,8 +54,8 @@ extern const char report_reverse[] ;
extern const char report_accepted[] ;
extern const char report_cookie[] ;
extern const char report_connected[] ;
extern const char report_authetication_successed[] ;
extern const char report_authetication_failed[] ;
extern const char report_authentication_succeeded[] ;
extern const char report_authentication_failed[] ;
extern const char report_window[] ;
extern const char report_autotune[] ;
extern const char report_omit_done[] ;
Expand Down

0 comments on commit 06e3f08

Please sign in to comment.