Skip to content

Make mcrypt input handling stricter #610

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

Make mcrypt input handling stricter #610

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
27d75b8
Abort on invalid IV size
nikic Mar 1, 2014
5480b64
Abort on missing IV if the enc_mode requires it
nikic Mar 1, 2014
88d0053
Abort on invalid key size
nikic Mar 1, 2014
9539d1c
Clean up do_crypt code
nikic Mar 1, 2014
c7f4ffe
Print supported key sizes in error message
nikic Mar 2, 2014
b2925ed
Call mcrypt_module_close on error
nikic Mar 2, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
182 changes: 115 additions & 67 deletions ext/mcrypt/mcrypt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
#include "php_globals.h"
#include "ext/standard/info.h"
#include "ext/standard/php_rand.h"
#include "ext/standard/php_smart_str.h"
#include "php_mcrypt_filter.h"

static int le_mcrypt;
Expand Down Expand Up @@ -658,7 +659,7 @@ PHP_FUNCTION(mcrypt_generic)
char *data;
int data_len;
php_mcrypt *pm;
unsigned char* data_s;
char* data_s;
int block_size, data_size;

if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "rs", &mcryptind, &data, &data_len) == FAILURE) {
Expand Down Expand Up @@ -1165,14 +1166,95 @@ PHP_FUNCTION(mcrypt_get_cipher_name)
}
/* }}} */

static void php_mcrypt_do_crypt(char* cipher, const char *key, int key_len, const char *data, int data_len, char *mode, const char *iv, int iv_len, int argc, int dencrypt, zval* return_value TSRMLS_DC) /* {{{ */
static char *php_mcrypt_get_key_size_str(
int max_key_size, const int *key_sizes, int key_size_count) /* {{{ */
{
if (key_size_count == 0) {
char *str;
spprintf(&str, 0, "Only keys of size 1 to %d supported", max_key_size);
return str;
} else if (key_size_count == 1) {
char *str;
spprintf(&str, 0, "Only keys of size %d supported", key_sizes[0]);
return str;
} else {
int i;
smart_str str = {0};
smart_str_appends(&str, "Only keys of sizes ");

for (i = 0; i < key_size_count; ++i) {
if (i == key_size_count - 1) {
smart_str_appends(&str, " or ");
} else if (i != 0) {
smart_str_appends(&str, ", ");
}

smart_str_append_long(&str, key_sizes[i]);
}

smart_str_appends(&str, " supported");
smart_str_0(&str);
return str.c;
}
}
/* }}} */

static zend_bool php_mcrypt_is_valid_key_size(
Copy link
Member

Choose a reason for hiding this comment

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

determine (extra r)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's in a deleted piece of code ;)

int key_size, int max_key_size, int *key_sizes, int key_size_count) /* {{{ */
{
int i;

if (key_size <= 0 || key_size > max_key_size) {
return 0;
}

if (key_size_count == 0) {
/* All key sizes are valid */
return 1;
}

for (i = 0; i < key_size_count; i++) {
if (key_sizes[i] == key_size) {
return 1;
}
}

return 0;
}
/* }}} */

static int php_mcrypt_ensure_valid_key_size(MCRYPT td, int key_size TSRMLS_DC) /* {{{ */
{
int key_size_count;
int max_key_size = mcrypt_enc_get_key_size(td);
int *key_sizes = mcrypt_enc_get_supported_key_sizes(td, &key_size_count);

zend_bool is_valid_key_size = php_mcrypt_is_valid_key_size(
key_size, max_key_size, key_sizes, key_size_count
);
if (!is_valid_key_size) {
char *key_size_str = php_mcrypt_get_key_size_str(
max_key_size, key_sizes, key_size_count
);
php_error_docref(NULL TSRMLS_CC, E_WARNING,
"Key of size %d not supported by this algorithm. %s", key_size, key_size_str
);
efree(key_size_str);
}

if (key_sizes) {
mcrypt_free(key_sizes);
}

return is_valid_key_size ? SUCCESS : FAILURE;
}
/* }}} */

static void php_mcrypt_do_crypt(char* cipher, const char *key, int key_len, const char *data, int data_len, char *mode, const char *iv, int iv_len, int dencrypt, zval* return_value TSRMLS_DC) /* {{{ */
{
char *cipher_dir_string;
char *module_dir_string;
int block_size, max_key_length, use_key_length, i, count, iv_size;
unsigned long int data_size;
int *key_length_sizes;
char *key_s = NULL, *iv_s;
char *data_s;
MCRYPT td;

Expand All @@ -1183,92 +1265,58 @@ static void php_mcrypt_do_crypt(char* cipher, const char *key, int key_len, cons
php_error_docref(NULL TSRMLS_CC, E_WARNING, MCRYPT_OPEN_MODULE_FAILED);
RETURN_FALSE;
}

/* Checking for key-length */
max_key_length = mcrypt_enc_get_key_size(td);
if (key_len > max_key_length) {
php_error_docref(NULL TSRMLS_CC, E_WARNING, "Size of key is too large for this algorithm");
}
key_length_sizes = mcrypt_enc_get_supported_key_sizes(td, &count);
if (count == 0 && key_length_sizes == NULL) { /* all lengths 1 - k_l_s = OK */
use_key_length = key_len;
key_s = emalloc(use_key_length);
memset(key_s, 0, use_key_length);
memcpy(key_s, key, use_key_length);
} else if (count == 1) { /* only m_k_l = OK */
key_s = emalloc(key_length_sizes[0]);
memset(key_s, 0, key_length_sizes[0]);
memcpy(key_s, key, MIN(key_len, key_length_sizes[0]));
use_key_length = key_length_sizes[0];
} else { /* dertermine smallest supported key > length of requested key */
use_key_length = max_key_length; /* start with max key length */
for (i = 0; i < count; i++) {
if (key_length_sizes[i] >= key_len &&
key_length_sizes[i] < use_key_length)
{
use_key_length = key_length_sizes[i];
}
}
key_s = emalloc(use_key_length);
memset(key_s, 0, use_key_length);
memcpy(key_s, key, MIN(key_len, use_key_length));
if (php_mcrypt_ensure_valid_key_size(td, key_len TSRMLS_CC) == FAILURE) {
mcrypt_module_close(td);
RETURN_FALSE;
}
mcrypt_free (key_length_sizes);

/* Check IV */
iv_s = NULL;
iv_size = mcrypt_enc_get_iv_size (td);

/* IV is required */
if (mcrypt_enc_mode_has_iv(td) == 1) {
if (argc == 5) {
if (iv_size != iv_len) {
php_error_docref(NULL TSRMLS_CC, E_WARNING, MCRYPT_IV_WRONG_SIZE);
} else {
iv_s = emalloc(iv_size + 1);
memcpy(iv_s, iv, iv_size);
}
} else if (argc == 4) {
php_error_docref(NULL TSRMLS_CC, E_WARNING, "Attempt to use an empty IV, which is NOT recommend");
iv_s = emalloc(iv_size + 1);
memset(iv_s, 0, iv_size + 1);
if (!iv) {
php_error_docref(NULL TSRMLS_CC, E_WARNING, "Encryption mode requires an initialization vector");
mcrypt_module_close(td);
RETURN_FALSE;
}

if (iv_len != mcrypt_enc_get_iv_size(td)) {
php_error_docref(NULL TSRMLS_CC, E_WARNING, MCRYPT_IV_WRONG_SIZE);
mcrypt_module_close(td);
RETURN_FALSE;
}
}

/* Check blocksize */
if (mcrypt_enc_is_block_mode(td) == 1) { /* It's a block algorithm */
block_size = mcrypt_enc_get_block_size(td);
int block_size = mcrypt_enc_get_block_size(td);
data_size = (((data_len - 1) / block_size) + 1) * block_size;
data_s = emalloc(data_size);
data_s = emalloc(data_size + 1);
memset(data_s, 0, data_size);
memcpy(data_s, data, data_len);
} else { /* It's not a block algorithm */
data_size = data_len;
data_s = emalloc(data_size);
memset(data_s, 0, data_size);
data_s = emalloc(data_size + 1);
memcpy(data_s, data, data_len);
}

if (mcrypt_generic_init(td, key_s, use_key_length, iv_s) < 0) {
if (mcrypt_generic_init(td, (void *) key, key_len, (void *) iv) < 0) {
php_error_docref(NULL TSRMLS_CC, E_RECOVERABLE_ERROR, "Mcrypt initialisation failed");
mcrypt_module_close(td);
RETURN_FALSE;
}

if (dencrypt == MCRYPT_ENCRYPT) {
mcrypt_generic(td, data_s, data_size);
} else {
mdecrypt_generic(td, data_s, data_size);
}

RETVAL_STRINGL(data_s, data_size, 1);
data_s[data_size] = 0;
RETVAL_STRINGL(data_s, data_size, 0);

/* freeing vars */
mcrypt_generic_end(td);
if (key_s != NULL) {
efree (key_s);
}
if (iv_s != NULL) {
efree (iv_s);
}
efree (data_s);
}
/* }}} */

Expand All @@ -1284,7 +1332,7 @@ PHP_FUNCTION(mcrypt_encrypt)

convert_to_string_ex(mode);

php_mcrypt_do_crypt(cipher, key, key_len, data, data_len, Z_STRVAL_PP(mode), iv, iv_len, ZEND_NUM_ARGS(), MCRYPT_ENCRYPT, return_value TSRMLS_CC);
php_mcrypt_do_crypt(cipher, key, key_len, data, data_len, Z_STRVAL_PP(mode), iv, iv_len, MCRYPT_ENCRYPT, return_value TSRMLS_CC);
}
/* }}} */

Expand All @@ -1300,7 +1348,7 @@ PHP_FUNCTION(mcrypt_decrypt)

convert_to_string_ex(mode);

php_mcrypt_do_crypt(cipher, key, key_len, data, data_len, Z_STRVAL_PP(mode), iv, iv_len, ZEND_NUM_ARGS(), MCRYPT_DECRYPT, return_value TSRMLS_CC);
php_mcrypt_do_crypt(cipher, key, key_len, data, data_len, Z_STRVAL_PP(mode), iv, iv_len, MCRYPT_DECRYPT, return_value TSRMLS_CC);
}
/* }}} */

Expand All @@ -1316,7 +1364,7 @@ PHP_FUNCTION(mcrypt_ecb)

convert_to_long_ex(mode);

php_mcrypt_do_crypt(cipher, key, key_len, data, data_len, "ecb", iv, iv_len, ZEND_NUM_ARGS(), Z_LVAL_PP(mode), return_value TSRMLS_CC);
php_mcrypt_do_crypt(cipher, key, key_len, data, data_len, "ecb", iv, iv_len, Z_LVAL_PP(mode), return_value TSRMLS_CC);
}
/* }}} */

Expand All @@ -1332,7 +1380,7 @@ PHP_FUNCTION(mcrypt_cbc)

convert_to_long_ex(mode);

php_mcrypt_do_crypt(cipher, key, key_len, data, data_len, "cbc", iv, iv_len, ZEND_NUM_ARGS(), Z_LVAL_PP(mode), return_value TSRMLS_CC);
php_mcrypt_do_crypt(cipher, key, key_len, data, data_len, "cbc", iv, iv_len, Z_LVAL_PP(mode), return_value TSRMLS_CC);
}
/* }}} */

Expand All @@ -1348,7 +1396,7 @@ PHP_FUNCTION(mcrypt_cfb)

convert_to_long_ex(mode);

php_mcrypt_do_crypt(cipher, key, key_len, data, data_len, "cfb", iv, iv_len, ZEND_NUM_ARGS(), Z_LVAL_PP(mode), return_value TSRMLS_CC);
php_mcrypt_do_crypt(cipher, key, key_len, data, data_len, "cfb", iv, iv_len, Z_LVAL_PP(mode), return_value TSRMLS_CC);
}
/* }}} */

Expand All @@ -1364,7 +1412,7 @@ PHP_FUNCTION(mcrypt_ofb)

convert_to_long_ex(mode);

php_mcrypt_do_crypt(cipher, key, key_len, data, data_len, "ofb", iv, iv_len, ZEND_NUM_ARGS(), Z_LVAL_PP(mode), return_value TSRMLS_CC);
php_mcrypt_do_crypt(cipher, key, key_len, data, data_len, "ofb", iv, iv_len, Z_LVAL_PP(mode), return_value TSRMLS_CC);
}
/* }}} */

Expand Down
13 changes: 7 additions & 6 deletions ext/mcrypt/tests/bug46010.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,13 @@ Bug #46010 (warnings incorrectly generated for iv in ecb mode)
--FILE--
<?php

var_dump(bin2hex(mcrypt_encrypt(MCRYPT_TRIPLEDES, "key", "data", MCRYPT_MODE_ECB)));
var_dump(bin2hex(mcrypt_encrypt(MCRYPT_TRIPLEDES, "key", "data", MCRYPT_MODE_ECB, "a")));
var_dump(bin2hex(mcrypt_encrypt(MCRYPT_TRIPLEDES, "key", "data", MCRYPT_MODE_ECB, "12345678")));
$key = "012345678901234567890123";
var_dump(bin2hex(mcrypt_encrypt(MCRYPT_TRIPLEDES, $key, "data", MCRYPT_MODE_ECB)));
var_dump(bin2hex(mcrypt_encrypt(MCRYPT_TRIPLEDES, $key, "data", MCRYPT_MODE_ECB, "a")));
var_dump(bin2hex(mcrypt_encrypt(MCRYPT_TRIPLEDES, $key, "data", MCRYPT_MODE_ECB, "12345678")));

?>
--EXPECTF--
string(16) "372eeb4a524b8d31"
string(16) "372eeb4a524b8d31"
string(16) "372eeb4a524b8d31"
string(16) "f7a2ce11d4002294"
string(16) "f7a2ce11d4002294"
string(16) "f7a2ce11d4002294"
8 changes: 5 additions & 3 deletions ext/mcrypt/tests/mcrypt_cbc.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ mcrypt_cbc
<?php if (!extension_loaded("mcrypt")) print "skip"; ?>
--FILE--
<?php
$key = "FooBar";
$key = "0123456789012345";
$secret = "PHP Testfest 2008";
$cipher = MCRYPT_RIJNDAEL_128;

Expand All @@ -15,8 +15,9 @@ $enc_data = mcrypt_cbc($cipher, $key, $secret, MCRYPT_ENCRYPT, $iv);
echo trim(mcrypt_cbc($cipher, $key, $enc_data, MCRYPT_DECRYPT, $iv)) . "\n";

// a warning must be issued if we don't use a IV on a AES cipher, that usually requires an IV
mcrypt_cbc($cipher, $key, $enc_data, MCRYPT_DECRYPT);
var_dump(mcrypt_cbc($cipher, $key, $enc_data, MCRYPT_DECRYPT));

?>
--EXPECTF--

Deprecated: Function mcrypt_cbc() is deprecated in %s on line %d
Expand All @@ -26,4 +27,5 @@ PHP Testfest 2008

Deprecated: Function mcrypt_cbc() is deprecated in %s on line %d

Warning: mcrypt_cbc(): Attempt to use an empty IV, which is NOT recommend in %s on line %d
Warning: mcrypt_cbc(): Encryption mode requires an initialization vector in %s on line %d
bool(false)
Loading