MDEV-12112 corruption in encrypted table may be overlooked

After validating the post-encryption checksum on an encrypted page,
Mariabackup should decrypt the page and validate the pre-encryption
checksum as well. This should reduce the probability of accepting
invalid pages as valid ones.

This is a backport and refactoring of a patch that was
originally written by Thirunarayanan Balathandayuthapani
for the 10.2 branch.

extra/mariabackup/fil_cur.cc

@@ -30,6 +30,7 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA
 #include <trx0sys.h>
 
 #include "fil_cur.h"
+#include "fil0crypt.h"
 #include "common.h"
 #include "read_filt.h"
 #include "xtrabackup.h"
@@ -219,7 +220,7 @@ xb_fil_cur_open(
 	posix_fadvise(cursor->file, 0, 0, POSIX_FADV_SEQUENTIAL);
 
 	/* Determine the page size */
-	zip_size = xb_get_zip_size(cursor->file);
+	zip_size = xb_get_zip_size(node);
 	if (zip_size == ULINT_UNDEFINED) {
 		xb_fil_cur_close(cursor);
 		return(XB_FIL_CUR_SKIP);
@@ -282,6 +283,8 @@ xb_fil_cur_read(
 	xb_fil_cur_result_t	ret;
 	ib_int64_t		offset;
 	ib_int64_t		to_read;
+	byte			tmp_frame[UNIV_PAGE_SIZE_MAX];
+	byte			tmp_page[UNIV_PAGE_SIZE_MAX];
 
 	cursor->read_filter->get_next_batch(&cursor->read_filter_ctxt,
 					    &offset, &to_read);
@@ -336,55 +339,63 @@ xb_fil_cur_read(
 		return(XB_FIL_CUR_ERROR);
 	}
 
-	fil_system_enter();
-	fil_space_t *space = fil_space_get_by_id(cursor->space_id);
-	fil_system_exit();
+	fil_space_t *space = fil_space_acquire_for_io(cursor->space_id);
 
 	/* check pages for corruption and re-read if necessary. i.e. in case of
 	partially written pages */
 	for (page = cursor->buf, i = 0; i < npages;
 	     page += cursor->page_size, i++) {
-	ib_int64_t page_no = cursor->buf_page_no + i;
-
-		bool checksum_ok = fil_space_verify_crypt_checksum(page, cursor->zip_size,space, (ulint)page_no);
-
-		if (!checksum_ok &&
-		    buf_page_is_corrupted(true, page, cursor->zip_size,space)) {
+		ulint page_no = cursor->buf_page_no + i;
+
+		if (cursor->space_id == TRX_SYS_SPACE
+		    && page_no >= FSP_EXTENT_SIZE
+		    && page_no < FSP_EXTENT_SIZE * 3) {
+			/* We ignore the doublewrite buffer pages */
+		} else if (fil_space_verify_crypt_checksum(
+				   page, cursor->zip_size,
+				   space, page_no)) {
+			ut_ad(mach_read_from_4(page + FIL_PAGE_SPACE_ID)
+			      == space->id);
+
+			bool decrypted = false;
+
+			memcpy(tmp_page, page, cursor->page_size);
+
+			if (!fil_space_decrypt(space, tmp_frame,
+					       tmp_page, &decrypted)
+			    || buf_page_is_corrupted(true, tmp_page,
+						     cursor->zip_size,
+						     space)) {
+				goto corrupted;
+			}
+		} else if (buf_page_is_corrupted(true, page, cursor->zip_size,
+						 space)) {
+corrupted:
+			retry_count--;
 
-			if (cursor->is_system &&
-			    page_no >= (ib_int64_t)FSP_EXTENT_SIZE &&
-			    page_no < (ib_int64_t) FSP_EXTENT_SIZE * 3) {
-				/* skip doublewrite buffer pages */
-				xb_a(cursor->page_size == UNIV_PAGE_SIZE);
-				msg("[%02u] mariabackup: "
-				    "Page " UINT64PF " is a doublewrite buffer page, "
-				    "skipping.\n", cursor->thread_n, page_no);
-			} else {
-				retry_count--;
-				if (retry_count == 0) {
-					msg("[%02u] mariabackup: "
-					    "Error: failed to read page after "
-					    "10 retries. File %s seems to be "
-					    "corrupted.\n", cursor->thread_n,
-					    cursor->abs_path);
-					ret = XB_FIL_CUR_ERROR;
-					break;
-				}
+			if (retry_count == 0) {
 				msg("[%02u] mariabackup: "
-				    "Database page corruption detected at page "
-				    UINT64PF ", retrying...\n", cursor->thread_n,
-				    page_no);
-
-				os_thread_sleep(100000);
-
-				goto read_retry;
+				    "Error: failed to read page after "
+				    "10 retries. File %s seems to be "
+				    "corrupted.\n", cursor->thread_n,
+				    cursor->abs_path);
+				ret = XB_FIL_CUR_ERROR;
+				break;
 			}
+			msg("[%02u] mariabackup: "
+			    "Database page corruption detected at page "
+			    ULINTPF ", retrying...\n", cursor->thread_n,
+			    page_no);
+
+			os_thread_sleep(100000);
+			goto read_retry;
 		}
 		cursor->buf_read += cursor->page_size;
 		cursor->buf_npages++;
 	}
 
 	posix_fadvise(cursor->file, offset, to_read, POSIX_FADV_DONTNEED);
+	fil_space_release_for_io(space);
 
 	return(ret);
 }

extra/mariabackup/xtrabackup.cc

@@ -2300,7 +2300,7 @@ check_if_skip_table(
 Reads the space flags from a given data file and returns the compressed
 page size, or 0 if the space is not compressed. */
 ulint
-xb_get_zip_size(pfs_os_file_t file)
+xb_get_zip_size(fil_node_t* file)
 {
 	byte	*buf;
 	byte	*page;
@@ -2311,7 +2311,7 @@ xb_get_zip_size(pfs_os_file_t file)
 	buf = static_cast<byte *>(ut_malloc(2 * UNIV_PAGE_SIZE));
 	page = static_cast<byte *>(ut_align(buf, UNIV_PAGE_SIZE));
 
-	success = os_file_read(file, page, 0, UNIV_PAGE_SIZE);
+	success = os_file_read(file->handle, page, 0, UNIV_PAGE_SIZE);
 	if (!success) {
 		goto end;
 	}
@@ -2319,6 +2319,17 @@ xb_get_zip_size(pfs_os_file_t file)
 	space = mach_read_from_4(page + FIL_PAGE_ARCH_LOG_NO_OR_SPACE_ID);
 	zip_size = (space == 0 ) ? 0 :
 		dict_tf_get_zip_size(fsp_header_get_flags(page));
+
+	if (!file->space->crypt_data) {
+		fil_system_enter();
+		if (!file->space->crypt_data) {
+			file->space->crypt_data = fil_space_read_crypt_data(
+				space, page,
+				fsp_header_get_crypt_offset(zip_size));
+		}
+		fil_system_exit();
+	}
+
 end:
 	ut_free(buf);
 

extra/mariabackup/xtrabackup.h

@@ -184,7 +184,7 @@ void xb_data_files_close(void);
 /***********************************************************************
 Reads the space flags from a given data file and returns the compressed
 page size, or 0 if the space is not compressed. */
-ulint xb_get_zip_size(pfs_os_file_t file);
+ulint xb_get_zip_size(fil_node_t* file);
 
 /************************************************************************
 Checks if a table specified as a name in the form "database/name" (InnoDB 5.6)

mysql-test/suite/mariabackup/encrypted_page_corruption.opt

+--innodb-encrypt-log=ON
+--plugin-load-add=$FILE_KEY_MANAGEMENT_SO
+--loose-file-key-management
+--loose-file-key-management-filekey=FILE:$MTR_SUITE_DIR/filekeys-data.key
+--loose-file-key-management-filename=$MTR_SUITE_DIR/filekeys-data.enc
+--loose-file-key-management-encryption-algorithm=aes_cbc

mysql-test/suite/mariabackup/encrypted_page_corruption.result

+call mtr.add_suppression("\\[ERROR\\] InnoDB: The page .* in file .* cannot be decrypted.");
+CREATE TABLE t1(c VARCHAR(128)) ENGINE INNODB, encrypted=yes;
+insert into t1 select repeat('a',100);
+# Corrupt the table
+# xtrabackup backup
+FOUND /Database page corruption detected/ in backup.log
+drop table t1;

mysql-test/suite/mariabackup/encrypted_page_corruption.test

+--source include/have_file_key_management.inc
+
+call mtr.add_suppression("\\[ERROR\\] InnoDB: The page .* in file .* cannot be decrypted.");
+CREATE TABLE t1(c VARCHAR(128)) ENGINE INNODB, encrypted=yes;
+insert into t1 select repeat('a',100);
+
+let $MYSQLD_DATADIR=`select @@datadir`;
+let t1_IBD = $MYSQLD_DATADIR/test/t1.ibd;
+
+--source include/shutdown_mysqld.inc
+
+--echo # Corrupt the table
+
+perl;
+use strict;
+use warnings;
+use Fcntl qw(:DEFAULT :seek);
+
+my $ibd_file = $ENV{'t1_IBD'};
+
+my $chunk;
+my $len;
+
+sysopen IBD_FILE, $ibd_file, O_RDWR || die "Unable to open $ibd_file";
+sysseek IBD_FILE, 16384 * 3, SEEK_CUR;
+$chunk = '\xAA\xAA\xAA\xAA';
+syswrite IBD_FILE, $chunk, 4;
+
+close IBD_FILE;
+EOF
+
+--source include/start_mysqld.inc
+
+echo # xtrabackup backup;
+let $targetdir=$MYSQLTEST_VARDIR/tmp/backup;
+let $backuplog=$MYSQLTEST_VARDIR/tmp/backup.log;
+
+--disable_result_log
+--error 1
+exec $XTRABACKUP --defaults-file=$MYSQLTEST_VARDIR/my.cnf --backup --target-dir=$targetdir > $backuplog;
+--enable_result_log
+
+
+--let SEARCH_PATTERN=Database page corruption detected
+--let SEARCH_FILE=$backuplog
+--source include/search_pattern_in_file.inc
+remove_file $backuplog;
+
+drop table t1;
+rmdir $targetdir;