Re: [Maria-developers] Review for MDEV-9069

Hi, Georg! On Nov 03, Georg Richter wrote:

Hello Serg,

attached you will find the patch for MDEV-9069.

/Georg

-- Georg Richter, Senior Software Engineer MariaDB Corporation Ab

From bde4b9d0683f115fa55bb6d2e2164739f89d732c Mon Sep 17 00:00:00 2001 From: Georg Richter Date: Thu, 3 Nov 2016 07:44:15 +0100 Subject: [PATCH] Initial implementaion for MDEV-9069: "extend AES_ENCRYPT() and AES_DECRYPT() to support IV and the algorithm"

- Extended function syntax: AES_ENCRYPT(plaintext, key, [[iv, [aad]] using block_encryption_mode); AES_DECRYPT(plaintext, key, [[iv, [aad]] using block_encryption_mode);

This doesn't look right. at least square brackets aren't paired, but also I'd expect [using block_encryption_mode] to be in brackets, and the comma after key should be too.

AES_ crypt functions will return an error, if - length of IV is too small - the number of parameter doesn't match (e.g. if an iv was specified for block cipher mode ECB) AES_crypt functions will return a warning, if - key_size is too small

- Added new session variable block_encryption_mode - Added a new status variable block_encryption_mode_list, which lists available block_encryption modes

This is absolutely not necessary, the list of available values for block_encryption_mode can be retrieved as select enum_value_list from system_variables where variable_name='block_encryption_mode';

- Added new tests for AES_ crypt functions: AES_ECB and AES_CBC vectors from NIST Cryptographic validation program (CAVP) [http://csrc.nist.gov/groups/STM/cavp/]

What license are these tests available under?

- To prevent too many warnings all keys in tests now are hased by sha2() function

What does that mean? That is, no need to explain, I'll see it from the code, but the commit comment is confusing, please reformulate this sentence.

- This patch also includes bug fix for MDEV-11174 (my_aes_crypt crashes in GCM mode)

Note: - AES_GCM and AES_CTR are not well tested - CAVP test vectors couldn't be used for AES_GCM since the auth tag after encryption differs.

why does it differ?

- A key which is longer than the requested key size will be truncated to the appropriate size.

what do you mean, truncated, there is a key derivation function that creates the real key from the user specified long key in Item_aes_crypt::create_key

diff --git a/mysys_ssl/my_crypt.cc b/mysys_ssl/my_crypt.cc index 49bd9af..5485576 100644 --- a/mysys_ssl/my_crypt.cc +++ b/mysys_ssl/my_crypt.cc @@ -155,6 +155,7 @@ class MyCTX_gcm : public MyCTX int real_ivlen= EVP_CIPHER_CTX_iv_length(&ctx); aad= iv + real_ivlen; aadlen= ivlen - real_ivlen; + EVP_CIPHER_CTX_ctrl(&ctx, EVP_CTRL_GCM_SET_IVLEN, 12, NULL);

Hmm, why is that needed?

return res; }

@@ -205,12 +208,56 @@ const EVP_CIPHER *(*ciphers[])(uint)= { aes_ecb, aes_cbc #ifdef HAVE_EncryptAes128Ctr , aes_ctr +#endif #ifdef HAVE_EncryptAes128Gcm , aes_gcm #endif +}; + +const char *my_aes_block_encryption_mode_str[]= { + "aes-128-ecb", + "aes-192-ecb", + "aes-256-ecb", + "aes-128-cbc", + "aes-192-cbc", + "aes-256-cbc" +#ifdef HAVE_EncryptAes128Ctr + , "aes-128-ctr", + "aes-192-ctr", + "aes-256-ctr" #endif +#ifdef HAVE_EncryptAes128Gcm + , "aes-128-gcm", + "aes-192-gcm", + "aes-256-gcm" +#endif + , NULL };

+struct st_my_aes_info my_aes_info[]= { + { my_aes_128_ecb, MY_AES_ECB, 128, 0, 2}, + { my_aes_192_ecb, MY_AES_ECB, 192, 0, 2}, + { my_aes_256_ecb, MY_AES_ECB, 256, 0, 2}, + { my_aes_128_cbc, MY_AES_CBC, 128, MY_AES_BLOCK_SIZE, 3}, + { my_aes_192_cbc, MY_AES_CBC, 192, MY_AES_BLOCK_SIZE, 3}, + { my_aes_256_cbc, MY_AES_CBC, 256, MY_AES_BLOCK_SIZE, 3}, +#ifdef HAVE_EncryptAes128Ctr + { my_aes_128_ctr, MY_AES_CTR, 128, MY_AES_BLOCK_SIZE, 3}, + { my_aes_192_ctr, MY_AES_CTR, 192, MY_AES_BLOCK_SIZE, 3}, + { my_aes_256_ctr, MY_AES_CTR, 256, MY_AES_BLOCK_SIZE, 3}, +#endif +#ifdef HAVE_EncryptAes128Gcm + { my_aes_128_gcm, MY_AES_GCM, 128, 12, 4 }, + { my_aes_192_gcm, MY_AES_GCM, 192, 12, 4 }, + { my_aes_256_gcm, MY_AES_GCM, 256, 12, 4 }, +#endif +}; + +TYPELIB block_encryption_mode_typelib= + { my_aes_block_encryption_end, + "block_encryption_modes", + my_aes_block_encryption_mode_str, NULL}; +

I don't think all that belongs to my_crypt.cc and my_crypt.h. This is part of the SQL interface to aes, it should be in item_strfunc.cc

extern "C" {

int my_aes_crypt_init(void *ctx, enum my_aes_mode mode, int flags, diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc index acd3d74..9a4108c 100644 --- a/sql/item_strfunc.cc +++ b/sql/item_strfunc.cc @@ -359,47 +359,116 @@ void Item_func_sha2::fix_length_and_dec() }

/* Implementation of AES encryption routines */ -void Item_aes_crypt::create_key(String *user_key, uchar *real_key) +void Item_aes_crypt::create_item(String *user_item, uchar *real_item, uint item_size) { - uchar *real_key_end= real_key + AES_KEY_LENGTH / 8; + uchar *real_item_end= real_item + item_size; uchar *ptr; - const char *sptr= user_key->ptr(); - const char *key_end= sptr + user_key->length(); + const char *sptr= user_item->ptr(); + const char *item_end= sptr + user_item->length();

- bzero(real_key, AES_KEY_LENGTH / 8); + bzero(real_item, item_size);

- for (ptr= real_key; sptr < key_end; ptr++, sptr++) + for (ptr= real_item; sptr < item_end; ptr++, sptr++) { - if (ptr == real_key_end) - ptr= real_key; + if (ptr == real_item_end) + return; *ptr ^= (uchar) *sptr; } }

- String *Item_aes_crypt::val_str(String *str) { DBUG_ASSERT(fixed == 1); - StringBuffer<80> user_key_buf; + StringBuffer<80> user_buf; String *sptr= args[0]->val_str(str); - String *user_key= args[1]->val_str(&user_key_buf); - uint32 aes_length; + enum my_aes_mode block_cipher; + + if (block_encryption_mode == (ulong)-1) + block_encryption_mode= current_thd->variables.block_encryption_mode; + + block_cipher= my_aes_info[block_encryption_mode].aes_mode; + + if (arg_count > my_aes_info[block_encryption_mode].param_count) + { + null_value= 1; + my_error(ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT, MYF(0), what ? "AES_ENCRYPT" : "AES_DECRYPT"); + return 0; + }

- if (sptr && user_key) // we need both arguments to be not NULL + if (sptr) { + uchar riv[128]; // MY_AES_BLOCK_SIZE + MAX_AADLEN (=720 bits) + uint32 digest_size; // size of digest in bytes + String *user_iv; + uint iv_size; + + + digest_size= my_aes_get_size(block_cipher, sptr->length()); + iv_size= my_aes_info[block_encryption_mode].iv_size; + + if (iv_size) // check if block encryption mode requires IV + {

I thought you can have an assert here: DBUG_ASSERT(arg_count > 2) because you do ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT earlier.

+ if (arg_count > 2) + { + user_iv= args[2]->val_str(&user_buf); + // throw error if iv is too short + if (!user_iv || user_iv->length() < iv_size) + { + null_value= 1; + my_error(ER_AES_INVALID_IV, MYF(0), iv_size);

This needs to be documented carefully, because it's inconsistent with the behavior for key argument.

+ return 0; + } + create_item(user_iv, riv, iv_size); + } else { + null_value= 1; + my_error(ER_AES_INVALID_IV, MYF(0), iv_size); + return 0; + } + } +#ifdef HAVE_EncryptAes128Gcm + if (arg_count > 3 && + block_cipher == MY_AES_GCM)

why do you check for MY_AES_GCM, isn't arg_count > 3 enough?

+ { + String *user_aad= args[3]->val_str(&user_buf); + if (user_aad) + { + create_item(user_aad, riv + iv_size, user_aad->length()); + iv_size+= user_aad->length(); + } + } +#endif null_value=0; - aes_length=my_aes_get_size(MY_AES_ECB, sptr->length());

- if (!str_value.alloc(aes_length)) // Ensure that memory is free + if (!str_value.alloc(digest_size)) // Ensure that memory is free { - uchar rkey[AES_KEY_LENGTH / 8]; - create_key(user_key, rkey); + String *user_key= args[1]->val_str(&user_buf); + uint32 key_size= my_aes_info[block_encryption_mode].key_size; + uchar rkey[MY_AES_MAX_KEY_LENGTH];

- if (!my_aes_crypt(MY_AES_ECB, what, (uchar*)sptr->ptr(), sptr->length(), - (uchar*)str_value.ptr(), &aes_length, - rkey, AES_KEY_LENGTH / 8, 0, 0)) + if (!user_key || !user_key->length()) { - str_value.length((uint) aes_length); + null_value= 1; + return 0; + }

better to check the key before allocating any memory, just as the old code did.

+ + // if key is too short, throw a warning + if (user_key->length() < (key_size / 8)) + { + push_warning_printf(current_thd, Sql_condition::WARN_LEVEL_WARN, + ER_WARN_AES_KEY_TOO_SHORT, + ER_THD(current_thd, ER_WARN_AES_KEY_TOO_SHORT), + key_size / 8); + } + + create_item(user_key, rkey, key_size / 8); + + if (!my_aes_crypt(block_cipher, + what, (uchar*)sptr->ptr(), sptr->length(), + (uchar*)str_value.ptr(), &digest_size, + rkey, key_size / 8, (arg_count > 2) ? riv : 0, + (arg_count > 2) ? iv_size : 0)) + { + str_value.length((uint) digest_size); return &str_value; } } diff --git a/sql/item_strfunc.h b/sql/item_strfunc.h index 25b63eb..153ca7b 100644 --- a/sql/item_strfunc.h +++ b/sql/item_strfunc.h @@ -184,8 +188,9 @@ class Item_func_aes_encrypt :public Item_aes_crypt class Item_func_aes_decrypt :public Item_aes_crypt { public: - Item_func_aes_decrypt(THD *thd, Item *a, Item *b): - Item_aes_crypt(thd, a, b) {} + Item_func_aes_decrypt(THD *thd, ulong mode, List<Item> &list_item): + Item_aes_crypt(thd, list_item) + { block_encryption_mode= mode; }

you also need to implement check_vcol_func_processor() and check_valid_arguments_processor() to make sure that AES_ENCRYPT/AES_DECRYPT without the USING clause are not used in partitioning expression and in stored generated columns. And add tests for that, please. after I push MDEV-5800 (or now in bb-10.2-vcols branch) see Item_func_week as an example (it also has an optional argument that by default takes a value from a session variable).

void fix_length_and_dec(); const char *func_name() const { return "aes_decrypt"; } Item *get_copy(THD *thd, MEM_ROOT *mem_root) diff --git a/sql/mysqld.cc b/sql/mysqld.cc index 310ccb0..b2f677e 100644 --- a/sql/mysqld.cc +++ b/sql/mysqld.cc @@ -8317,6 +8317,27 @@ static int show_memory_used(THD *thd, SHOW_VAR *var, char *buff, return 0; }

+static int show_block_encrypt_mode_list(THD *thd, SHOW_VAR *var, char *buff, + struct system_status_var *status_var, + enum enum_var_type scope) +{ + uint i; + char *end= buff + SHOW_VAR_FUNC_BUFF_SIZE; + var->type= SHOW_CHAR; + var->value= buff; + + for (i=0; my_aes_block_encryption_mode_str[i] && + buff + strlen(my_aes_block_encryption_mode_str[i]) < end && + i < my_aes_block_encryption_end; i++) + { + buff= strnmov(buff, my_aes_block_encryption_mode_str[i], end-buff-1); + *buff++= ','; + } + if (i) + buff--; + *buff= 0; + return 0; +}

No need to, because block_encryption_mode_list status variable is not needed either.

#ifndef DBUG_OFF static int debug_status_func(THD *thd, SHOW_VAR *var, char *buff, diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt index d42611b..5a4c8bf 100644 --- a/sql/share/errmsg-utf8.txt +++ b/sql/share/errmsg-utf8.txt @@ -7231,4 +7231,16 @@ ER_PARTITION_DEFAULT_ERROR eng "Only one DEFAULT partition allowed" ukr "Припустимо мати тільки один DEFAULT розділ" ER_REFERENCED_TRG_DOES_NOT_EXIST - eng "Referenced trigger '%s' for the given action time and event type does not exist" + eng "Referenced trigger '%s' for the given action time and event type does not exist" +ER_AES_INVALID_IV + eng "The initialization vector supplied is too short. Must be at least %d bytes long" + ger "Der angegebene Initalisierungsvektor ist zu kurz. Die Mindestlänge beträgt %d Bytes" +ER_AES_INVALID_MODE + eng "Unknown block encryption mode '%s'" + ger "Der angegebene Blockcodierungsmodus '%s' exstiert nicht" +ER_AES_INVALID_KEY + eng "Invalid key" + ger "Ungültiger Schlüssel"

This error message seems to be unused.

+ER_WARN_AES_KEY_TOO_SHORT + eng "The provided key is too short. It is more secure to use a key with %d bytes"

s/with %d bytes/of at least %d bytes long/

+ ger "Der angegebene Schlüssel ist zu kurz. Es ist empfehlenswert einen Schlüssel mit %d Bytes zu verwenden" diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index e17a514..12e79d3 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -6670,7 +6673,6 @@ charset: CHAR_SYM SET {} | CHARSET {} ; -

Huh?

charset_name: ident_or_text { @@ -7102,6 +7104,16 @@ opt_component: | '.' ident { $$= $2; } ;

+block_encryption_mode: + ident_or_text + { + if (!($$=find_type($1.str, &block_encryption_mode_typelib, MYF(0)))) + my_yyabort_error((ER_AES_INVALID_MODE, MYF(0), $1.str)); + $$-= 1; + } + | DEFAULT { $$= (ulong)-1;}

why do you want to support USING DEFAULT ?

+ ; + string_list: text_string { Lex->last_field->interval_list.push_back($1, thd->mem_root); } diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc index b6359ff..8b39462 100644 --- a/sql/sys_vars.cc +++ b/sql/sys_vars.cc @@ -554,6 +554,10 @@ static Sys_var_mybool Sys_explicit_defaults_for_timestamp( READ_ONLY GLOBAL_VAR(opt_explicit_defaults_for_timestamp), CMD_LINE(OPT_ARG), DEFAULT(FALSE), NO_MUTEX_GUARD, NOT_IN_BINLOG);

+static Sys_var_enum Sys_block_encryption_mode( + "block_encryption_mode", "mode for AES_ENCRYPT/AES_DECRYPT",

"for AES_ENCRYPT and AES_DECRYPT"

+ SESSION_VAR(block_encryption_mode), CMD_LINE(REQUIRED_ARG), + my_aes_block_encryption_mode_str, DEFAULT(my_aes_128_ecb));

static Sys_var_ulonglong Sys_bulk_insert_buff_size( "bulk_insert_buffer_size", "Size of tree cache used in bulk " diff --git a/mysql-test/r/aes_cbc_crypt.result b/mysql-test/r/aes_cbc_crypt.result new file mode 100644 index 0000000..87ed84b --- /dev/null +++ b/mysql-test/r/aes_cbc_crypt.result @@ -0,0 +1,19300 @@ +# +# Error checking +# +SELECT HEX(AES_ENCRYPT("Die Katze tritt die Treppe krumm", "foo", "1234567890123456" USING "aes-128-cbc"));

I think it's better to use underscores aes_128_cbc, then the mode can be used as an identifier: AES_ENCRYPT("Die Katze", "foo", "1234567890123456" USING aes_128_cbc); (like charset names) otherwise it's not at all intuitive that SELECT AES_ENCRYPT("Die Katze", "foo", "1234567890123456" USING "aes-128-cbc"); works, but SELECT AES_ENCRYPT("Die Katze", "foo", "1234567890123456" USING concat_ws("-", "aes", "128", "cbc")); doesn't. So, either exclude the possibility of this confusion by not allowing a string literal as a mode. Or (actually better) allow any arbitrary non-constant expression as the mode.

+# +# Session variable tests +# +SET session block_encryption_mode="aes-111-cbc"; +ERROR 42000: Variable 'block_encryption_mode' can't be set to the value of 'aes-111-cbc' +SET session block_encryption_mode="aes-128-cbc"; +SELECT LEFT(AES_ENCRYPT(X'53696E676C6520626C6F636B206D7367', X'06A9214036B8A15B512E03D534120006', X'3DAFBA429D9EB430B422DA802C9FAC41'), 16) = X'E353779C1079AEB82708942DBE77181A'; +LEFT(AES_ENCRYPT(X'53696E676C6520626C6F636B206D7367', X'06A9214036B8A15B512E03D534120006', X'3DAFBA429D9EB430B422DA802C9FAC41'), 16) = X'E353779C1079AEB82708942DBE77181A' +1 +SELECT AES_DECRYPT(AES_ENCRYPT(X'53696E676C6520626C6F636B206D7367', X'06A9214036B8A15B512E03D534120006', X'3DAFBA429D9EB430B422DA802C9FAC41'), X'06A9214036B8A15B512E03D534120006', X'3DAFBA429D9EB430B422DA802C9FAC41') = X'53696E676C6520626C6F636B206D7367'; +AES_DECRYPT(AES_ENCRYPT(X'53696E676C6520626C6F636B206D7367', X'06A9214036B8A15B512E03D534120006', X'3DAFBA429D9EB430B422DA802C9FAC41'), X'06A9214036B8A15B512E03D534120006', X'3DAFBA429D9EB430B422DA802C9FAC41') = X'53696E676C6520626C6F636B206D7367'

please add tests for views, default clause, virtual columns, partitioning.

+1 ... I don't think it's necessary to commit megabytes of CAVP tests into the server repository. May be just a few K of them will be enough?

Regards, Sergei Chief Architect MariaDB and security@mariadb.org