maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Jan!

On Mar 13, Jan Lindström wrote:
> revision-id: 7c21ea9f502ded155c12a0ee3c5ff0602e9d4c9a
> parent(s): 4d0e52189ca945c94146035c2733d85c9c6fd18d
> committer: Jan Lindström
> branch nick: 10.1-innodb
> timestamp: 2015-03-13 14:18:07 +0200
> message:
> 
> MDEV-7772: SIGSEGV on my_aes_encrypt_cbc when -DWITH_SSL=bundled
> 
> Two problems:
> - Read/Write outside of buffer at memcpy() because of incorrect parameters
> . OPENSSL_assert(EVP_CIPHER_CTX_iv_length(&ctx.ctx) == iv_length); // ECB does not use IV, thus incorrect assertion
> 
> Added:
>     mysql-test/include/have_file_key_management_plugin.combinations ( to run with aes_cbc and aes_ecb)
>     mysql-test/include/have_openssl_ctr.combinations ( to run with aes_cbc, aes_ecb and aes_ctr)

The patch is ok, of course.
Just a few suggestions, see below.
Ok to push after that.

> diff --git a/mysql-test/include/have_file_key_management_plugin.combinations b/mysql-test/include/have_file_key_management_plugin.combinations
> new file mode 100644
> index 0000000..6a63b5a
> --- /dev/null
> +++ b/mysql-test/include/have_file_key_management_plugin.combinations
> @@ -0,0 +1,6 @@
> +[aes_cbc]
> +encryption-algorithm=aes_cbc
> +
> +[aes_ecb]
> +encryption-algorithm=aes_ecb
> +

1. Does file_key_management_plugin work with CTR?

2. suggestion: use shorter names for combinations (I'd remove "aes_" prefix)
test names are long enough already. See replication combinations - they are
3-4 letters only (rbr/mix/stmt).

> diff --git a/mysql-test/include/have_openssl_ctr.combinations b/mysql-test/include/have_openssl_ctr.combinations
> new file mode 100644
> index 0000000..2c49c78
> --- /dev/null
> +++ b/mysql-test/include/have_openssl_ctr.combinations
> @@ -0,0 +1,9 @@
> +[aes_cbc]
> +encryption-algorithm=aes_cbc
> +
> +[aes_ecb]
> +encryption-algorithm=aes_ecb
> +
> +[aes_ctr]
> +encryption-algorithm=aes_ctr
> +

This doesn't make a lot of sense now, because example_key_management_plugin
forces CTR:

    my_aes_init_dynamic_encrypt(MY_AES_ALGORITHM_CTR);

so it always uses ctr, no matter what encryption-algorithm is.
which is very confusing, SHOW VARIABLES will show, say, aes_ecb,
but in fact it will be aes_ctr under the hood.

furthermore, it doesn't check for the return value
(so it'll fail later or will misbehave on old openssl versions
that don't support CTR).

This all is a separate bug (in Jira already), but for now I'm just saying
that it's kind of pointless to run example_key_management_plugin
with different encryption-algorithm values.

> diff --git a/mysql-test/suite/innodb/t/innodb_scrub_background.test b/mysql-test/suite/innodb/t/innodb_scrub_background.test
> index 44cb16b..881c124 100644
> --- a/mysql-test/suite/innodb/t/innodb_scrub_background.test
> +++ b/mysql-test/suite/innodb/t/innodb_scrub_background.test
> @@ -1,6 +1,7 @@
>  -- source include/have_innodb.inc
>  -- source include/not_embedded.inc
>  -- source include/have_example_key_management_plugin.inc
> +-- source include/have_openssl_ctr.inc

that's fine, although you could've simply included have_openssl_ctr.inc from
have_example_key_management_plugin.inc :)

>  
>  let $MYSQLD_DATADIR=`select @@datadir`;
>  let ib1_IBD = $MYSQLD_DATADIR/ibdata1;
> diff --git a/storage/innobase/fil/fil0crypt.cc b/storage/innobase/fil/fil0crypt.cc
> index 823c74a..10d49bb 100644
> --- a/storage/innobase/fil/fil0crypt.cc
> +++ b/storage/innobase/fil/fil0crypt.cc
> @@ -734,7 +734,7 @@ fil_space_encrypt(ulint space, ulint offset, lsn_t lsn,
>  	uint32 dstlen;
>  
>  	if (page_compressed) {
> -		srclen = page_size -  FIL_PAGE_DATA;;
> +		srclen = page_size -  FIL_PAGE_DATA;

I'd suggest to move this and all following changes to a separate commit.

>  	}
>  
>  	int rc = (* my_aes_encrypt_dynamic)(src, srclen,

Regards,
Sergei