Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-19582

Out-of-bounds memory accesses by WolfSSL

Details

    Description

      MDEV-18531 replaced YaSSL with WolfSSL for the cmake -DWITH_SSL=bundled. Unfortunately, it is introducing some memory overflows. It looks like it could be worked around by allocating extra 16 bytes at the end of each buffer.

      With the patch below, the crypt variants of the tests innodb.innodb-index-online and innodb.innodb-table-online will not crash, but they will fail, because some calls to deallocate_large() will use a wrong length.

      Adding the 16 bytes to the allocations looks very wrong, because the allocate_large() could add a lot of extra padding. I hope that this can be fixed in WolfSSL instead of requiring changes to InnoDB.

      diff --git a/storage/innobase/log/log0crypt.cc b/storage/innobase/log/log0crypt.cc
      index d3843221141..beead7eaeb6 100644
      --- a/storage/innobase/log/log0crypt.cc
      +++ b/storage/innobase/log/log0crypt.cc
      @@ -49,8 +49,14 @@ struct crypt_info_t {
       	uint		key_version;   /*!< mysqld key version */
       	/** random string for encrypting the key */
       	aes_block_t	crypt_msg;
      +#if 1 /* Workaround for WolfSSL */
      +	aes_block_t	crypt_msg_wolfssl_pad;
      +#endif
       	/** the secret key */
       	aes_block_t	crypt_key;
      +#if 1 /* Workaround for WolfSSL */
      +	aes_block_t	crypt_key_wolfssl_pad;
      +#endif
       	/** a random string for the per-block initialization vector */
       	union {
       		uint32_t	word;
      diff --git a/storage/innobase/row/row0log.cc b/storage/innobase/row/row0log.cc
      index 8b5cdb8f9b3..fa847b1e12f 100644
      --- a/storage/innobase/row/row0log.cc
      +++ b/storage/innobase/row/row0log.cc
      @@ -214,11 +214,13 @@ struct row_log_t {
       	byte*		crypt_tail; /*!< writer context;
       				temporary buffer used in encryption,
       				decryption or NULL*/
      +	size_t		crypt_tail_size;
       	row_log_buf_t	head;	/*!< reader context; protected by MDL only;
       				modifiable by row_log_apply_ops() */
       	byte*		crypt_head; /*!< reader context;
       				temporary buffer used in encryption,
       				decryption or NULL */
      +	size_t		crypt_head_size;
       	const char*	path;	/*!< where to create temporary file during
       				log operation */
       	/** the number of core fields in the clustered index of the
      @@ -293,7 +295,11 @@ row_log_block_allocate(
       		);
       
       		log_buf.block = ut_allocator<byte>(mem_key_row_log_buf)
      -			.allocate_large(srv_sort_buf_size, &log_buf.block_pfx);
      +			.allocate_large(
      +#if 1 /* Workaround for WolfSSL */
      +				MY_AES_BLOCK_SIZE +
      +#endif
      +				srv_sort_buf_size, &log_buf.block_pfx);
       
       		if (log_buf.block == NULL) {
       			DBUG_RETURN(false);
      @@ -313,7 +319,11 @@ row_log_block_free(
       	DBUG_ENTER("row_log_block_free");
       	if (log_buf.block != NULL) {
       		ut_allocator<byte>(mem_key_row_log_buf).deallocate_large(
      -			log_buf.block, &log_buf.block_pfx, log_buf.size);
      +			log_buf.block, &log_buf.block_pfx,
      +#if 1 /* Workaround for WolfSSL */
      +			MY_AES_BLOCK_SIZE +
      +#endif
      +			log_buf.size);
       		log_buf.block = NULL;
       	}
       	DBUG_VOID_RETURN;
      @@ -3205,6 +3215,7 @@ row_log_allocate(
       	log->tail.total = 0;
       	log->tail.block = log->head.block = NULL;
       	log->crypt_tail = log->crypt_head = NULL;
      +	log->crypt_tail_size = log->crypt_head_size = 0;
       	log->head.blocks = log->head.bytes = 0;
       	log->head.total = 0;
       	log->path = path;
      @@ -3231,9 +3242,15 @@ row_log_allocate(
       	index->online_log = log;
       
       	if (log_tmp_is_encrypted()) {
      -		ulint size = srv_sort_buf_size;
      -		log->crypt_head = static_cast<byte *>(os_mem_alloc_large(&size));
      -		log->crypt_tail = static_cast<byte *>(os_mem_alloc_large(&size));
      +		log->crypt_head_size = log->crypt_tail_size =
      +#if 1 /* Work around WolfSSL */
      +			MY_AES_BLOCK_SIZE +
      +#endif
      +			srv_sort_buf_size;
      +		log->crypt_head = static_cast<byte*>(
      +			os_mem_alloc_large(&log->crypt_head_size));
      +		log->crypt_tail = static_cast<byte*>(
      +			os_mem_alloc_large(&log->crypt_tail_size));
       
       		if (!log->crypt_head || !log->crypt_tail) {
       			row_log_free(log);
      @@ -3265,11 +3282,11 @@ row_log_free(
       	row_merge_file_destroy_low(log->fd);
       
       	if (log->crypt_head) {
      -		os_mem_free_large(log->crypt_head, srv_sort_buf_size);
      +		os_mem_free_large(log->crypt_head, log->crypt_head_size);
       	}
       
       	if (log->crypt_tail) {
      -		os_mem_free_large(log->crypt_tail, srv_sort_buf_size);
      +		os_mem_free_large(log->crypt_tail, log->crypt_tail_size);
       	}
       
       	mutex_free(&log->mutex);
      diff --git a/storage/innobase/row/row0merge.cc b/storage/innobase/row/row0merge.cc
      index 087f4e89125..9a61ffee385 100644
      --- a/storage/innobase/row/row0merge.cc
      +++ b/storage/innobase/row/row0merge.cc
      @@ -4628,8 +4628,11 @@ row_merge_build_indexes(
       
       	if (log_tmp_is_encrypted()) {
       		crypt_block = static_cast<row_merge_block_t*>(
      -			alloc.allocate_large(block_size,
      -					     &crypt_pfx));
      +			alloc.allocate_large(
      +#if 1 /* Workaround for WolfSSL */
      +				MY_AES_BLOCK_SIZE +
      +#endif
      +				block_size, &crypt_pfx));
       
       		if (crypt_block == NULL) {
       			DBUG_RETURN(DB_OUT_OF_MEMORY);
      @@ -4998,7 +5001,11 @@ row_merge_build_indexes(
       	alloc.deallocate_large(block, &block_pfx, block_size);
       
       	if (crypt_block) {
      -		alloc.deallocate_large(crypt_block, &crypt_pfx, block_size);
      +		alloc.deallocate_large(
      +#if 1 /* Workaround for WolfSSL */
      +			MY_AES_BLOCK_SIZE +
      +#endif
      +			crypt_block, &crypt_pfx, block_size);
       	}
       
       	DICT_TF2_FLAG_UNSET(new_table, DICT_TF2_FTS_ADD_DOC_ID);
      

      Side note: Do we really need so big buffers in row_log_t, doubling the memory usage? We could read or write data in multiple system calls, using a smaller encryption buffer. (It looks like we must use separate buffers for head and tail, though.)

      Attachments

        Issue Links

          Activity

            Implemented a workaround.
            Fixing this in WolfSSL is something beyond me. I of course should report a bug.
            Another idea of a fix is to implement a "WolfSSL-native" my_aes_crypt, which does not go through the openssl compatibility layer. MySQL seems to have done this way, alas, we would still need EVP_CipherUpdate for the binlog encryption. But, this would be for encrypting, which is not affected (only decrypt is affected by the bug).

            wlad Vladislav Vaintroub added a comment - Implemented a workaround. Fixing this in WolfSSL is something beyond me. I of course should report a bug. Another idea of a fix is to implement a "WolfSSL-native" my_aes_crypt, which does not go through the openssl compatibility layer. MySQL seems to have done this way, alas, we would still need EVP_CipherUpdate for the binlog encryption. But, this would be for encrypting, which is not affected (only decrypt is affected by the bug).

            The code still crashes for me when using clang-7:

            10.4 f465ec8c45d1ff2371d9ccdf7144173b2797bca5

            innodb.innodb-table-online '16k,crypt,innodb' w1 [ fail ]
                    Test ended at 2019-05-27 08:47:43
             
            CURRENT_TEST: innodb.innodb-table-online
            mysqltest: At line 317: query 'reap' failed: 2013: Lost connection to MySQL server during query
            ==33823==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7fc110e5b000 at pc 0x55f069759be6 bp 0x7fc111028240 sp 0x7fc1110279f0
            READ of size 16 at 0x7fc110e5b000 thread T28
                #0 0x55f069759be5 in __asan_memcpy (/dev/shm/10.4/sql/mysqld+0x109bbe5)
                #1 0x55f06b715946 in wolfSSL_EVP_CipherUpdate /mariadb/10.4/extra/wolfssl/wolfssl/wolfcrypt/src/evp.c:361:17
                #2 0x55f06b6ea9ee in MyCTX::update(unsigned char const*, unsigned int, unsigned char*, unsigned int*) /mariadb/10.4/mysys_ssl/my_crypt.cc:63:9
                #3 0x55f06b6ea9ee in MyCTX_nopad::update(unsigned char const*, unsigned int, unsigned char*, unsigned int*) /mariadb/10.4/mysys_ssl/my_crypt.cc:121
                #4 0x55f06aae785e in encryption_crypt(unsigned char const*, unsigned int, unsigned char*, unsigned int*, unsigned char const*, unsigned int, unsigned char const*, unsigned int, int, unsigned int, unsigned int) /mariadb/10.4/include/mysql/service_encryption.h:119:9
                #5 0x55f06aae9577 in log_tmp_block_encrypt(unsigned char const*, unsigned long, unsigned char*, unsigned long, unsigned long, bool) /mariadb/10.4/storage/innobase/log/log0crypt.cc:444:11
                #6 0x55f06ac7e72b in log_tmp_block_decrypt(unsigned char const*, unsigned long, unsigned char*, unsigned long, unsigned long) /mariadb/10.4/storage/innobase/include/log0crypt.h:129:9
                #7 0x55f06ac7e72b in row_log_table_apply_ops(que_thr_t*, row_merge_dup_t*, ut_stage_alter_t*) /mariadb/10.4/storage/innobase/row/row0log.cc:2885
            

            If the problem is solely related to decryption witht he NOPAD attribute, a better workaround for the temporary files written and read by row0log.cc and row0merge.cc might be to remove that attribute.

            marko Marko Mäkelä added a comment - The code still crashes for me when using clang-7 : 10.4 f465ec8c45d1ff2371d9ccdf7144173b2797bca5 innodb.innodb-table-online '16k,crypt,innodb' w1 [ fail ] Test ended at 2019-05-27 08:47:43   CURRENT_TEST: innodb.innodb-table-online mysqltest: At line 317: query 'reap' failed: 2013: Lost connection to MySQL server during query … ==33823==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7fc110e5b000 at pc 0x55f069759be6 bp 0x7fc111028240 sp 0x7fc1110279f0 READ of size 16 at 0x7fc110e5b000 thread T28 #0 0x55f069759be5 in __asan_memcpy (/dev/shm/10.4/sql/mysqld+0x109bbe5) #1 0x55f06b715946 in wolfSSL_EVP_CipherUpdate /mariadb/10.4/extra/wolfssl/wolfssl/wolfcrypt/src/evp.c:361:17 #2 0x55f06b6ea9ee in MyCTX::update(unsigned char const*, unsigned int, unsigned char*, unsigned int*) /mariadb/10.4/mysys_ssl/my_crypt.cc:63:9 #3 0x55f06b6ea9ee in MyCTX_nopad::update(unsigned char const*, unsigned int, unsigned char*, unsigned int*) /mariadb/10.4/mysys_ssl/my_crypt.cc:121 #4 0x55f06aae785e in encryption_crypt(unsigned char const*, unsigned int, unsigned char*, unsigned int*, unsigned char const*, unsigned int, unsigned char const*, unsigned int, int, unsigned int, unsigned int) /mariadb/10.4/include/mysql/service_encryption.h:119:9 #5 0x55f06aae9577 in log_tmp_block_encrypt(unsigned char const*, unsigned long, unsigned char*, unsigned long, unsigned long, bool) /mariadb/10.4/storage/innobase/log/log0crypt.cc:444:11 #6 0x55f06ac7e72b in log_tmp_block_decrypt(unsigned char const*, unsigned long, unsigned char*, unsigned long, unsigned long) /mariadb/10.4/storage/innobase/include/log0crypt.h:129:9 #7 0x55f06ac7e72b in row_log_table_apply_ops(que_thr_t*, row_merge_dup_t*, ut_stage_alter_t*) /mariadb/10.4/storage/innobase/row/row0log.cc:2885 If the problem is solely related to decryption witht he NOPAD attribute, a better workaround for the temporary files written and read by row0log.cc and row0merge.cc might be to remove that attribute.

            For me, removing NOPAD did not remove the buffer overflows. I applied the relevant parts of my patch in Description, and ensured that -DHAVE_WOLFSSL is actually being passed to InnoDB compilation.

            marko Marko Mäkelä added a comment - For me, removing NOPAD did not remove the buffer overflows. I applied the relevant parts of my patch in Description, and ensured that -DHAVE_WOLFSSL is actually being passed to InnoDB compilation.
            wlad Vladislav Vaintroub added a comment - WolfSSL bug report https://github.com/wolfSSL/wolfssl/issues/2264
            danblack Daniel Black added a comment -

            Now that upstream is fixed, time to update submodule and revert e32212c63c96 and 88b7926ff845?

            danblack Daniel Black added a comment - Now that upstream is fixed, time to update submodule and revert e32212c63c96 and 88b7926ff845?

            People

              marko Marko Mäkelä
              marko Marko Mäkelä
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Git Integration

                  Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.