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

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: