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 <[email protected]> > 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 [email protected] _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
