Follow-up to MDEV-12112: corruption in encrypted table may be overlooked

The initial fix only covered a part of Mariabackup. This fix hardens InnoDB and XtraDB in a similar way, in order to reduce the probability of mistaking a corrupted encrypted page for a valid unencrypted one. This is based on work by Thirunarayanan Balathandayuthapani. fil_space_verify_crypt_checksum(): Assert that key_version!=0. Let the callers guarantee that. Now that we have this assertion, we also know that buf_page_is_zeroes() cannot hold. Also, remove all diagnostic output and related parameters, and let the relevant callers emit such messages. Last but not least, validate the post-encryption checksum according to the innodb_checksum_algorithm (only accepting one checksum for the strict variants), and no longer try to validate the page as if it was unencrypted. buf_page_is_zeroes(): Move to the compilation unit of the only callers, and declare static. xb_fil_cur_read(), buf_page_check_corrupt(): Add a condition before calling fil_space_verify_crypt_checksum(). This is a non-functional change. buf_dblwr_process(): Validate the page only as encrypted or unencrypted, but not both.
MariaDB · Dec 17, 2018 · 8c43f96 · 8c43f96
1 parent 517c59c
commit 8c43f96
Show file tree

Hide file tree

Showing 16 changed files with 218 additions and 426 deletions.
diff --git a/extra/innochecksum.cc b/extra/innochecksum.cc
@@ -522,7 +522,16 @@ is_page_corrupted(
 	normal method. */
 	if (is_encrypted && key_version != 0) {
 		is_corrupted = !fil_space_verify_crypt_checksum(buf,
-			page_size.is_compressed() ? page_size.physical() : 0, NULL, cur_page_num);
+			page_size.is_compressed() ? page_size.physical() : 0);
+		if (is_corrupted && log_file) {
+			fprintf(log_file,
+				"Page " ULINTPF ":%llu may be corrupted;"
+				" key_version=" ULINTPF "\n",
+				space_id, cur_page_num,
+				mach_read_from_4(
+					FIL_PAGE_FILE_FLUSH_LSN_OR_KEY_VERSION
+					+ buf));
+		}
 	} else {
 		is_corrupted = true;
 	}

diff --git a/extra/mariabackup/fil_cur.cc b/extra/mariabackup/fil_cur.cc
@@ -351,9 +351,14 @@ xb_fil_cur_read(
 		    && 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)) {
+		} else if (mach_read_from_4(
+				   page
+				   + FIL_PAGE_FILE_FLUSH_LSN_OR_KEY_VERSION)
+			   && space->crypt_data
+			   && space->crypt_data->type
+			   != CRYPT_SCHEME_UNENCRYPTED
+			   && fil_space_verify_crypt_checksum(
+				   page, cursor->zip_size)) {
 			ut_ad(mach_read_from_4(page + FIL_PAGE_SPACE_ID)
 			      == space->id);
 

diff --git a/mysql-test/suite/encryption/r/innodb-force-corrupt.result b/mysql-test/suite/encryption/r/innodb-force-corrupt.result
@@ -1,5 +1,4 @@
-call mtr.add_suppression("InnoDB: The page \\[page id: space=[1-9][0-9]*, page number=[1-9][0-9]*\\] in file '.*test.t[123]\\.ibd' cannot be decrypted\\.");
-call mtr.add_suppression("InnoDB: Database page corruption on disk or a failed file read of tablespace test/t[0-9]+ page \[page id: space=[0-9]+, page number=[0-9]+\]. You may have to recover from a backup.");
+call mtr.add_suppression("InnoDB: Encrypted page \\d+:[36] in file .*test.t[123]\\.ibd looks corrupted; key_version=3221342974");
 SET GLOBAL innodb_file_format = `Barracuda`;
 SET GLOBAL innodb_file_per_table = ON;
 set global innodb_compression_algorithm = 1;

diff --git a/mysql-test/suite/encryption/t/innodb-force-corrupt.test b/mysql-test/suite/encryption/t/innodb-force-corrupt.test
@@ -7,8 +7,7 @@
 # Don't test under embedded
 -- source include/not_embedded.inc
 
-call mtr.add_suppression("InnoDB: The page \\[page id: space=[1-9][0-9]*, page number=[1-9][0-9]*\\] in file '.*test.t[123]\\.ibd' cannot be decrypted\\.");
-call mtr.add_suppression("InnoDB: Database page corruption on disk or a failed file read of tablespace test/t[0-9]+ page \[page id: space=[0-9]+, page number=[0-9]+\]. You may have to recover from a backup.");
+call mtr.add_suppression("InnoDB: Encrypted page \\d+:[36] in file .*test.t[123]\\.ibd looks corrupted; key_version=3221342974");
 
 --disable_warnings
 SET GLOBAL innodb_file_format = `Barracuda`;
@@ -53,17 +52,17 @@ perl;
 open(FILE, "+<", "$ENV{MYSQLD_DATADIR}/test/t1.ibd") or die "open";
 binmode FILE;
 seek(FILE, $ENV{'INNODB_PAGE_SIZE'} * 3 + 26, SEEK_SET) or die "seek";
-print FILE pack("H*", "c00lcafedeadb017");
+print FILE pack("H*", "c001cafedeadb017");
 close FILE or die "close";
 open(FILE, "+<", "$ENV{MYSQLD_DATADIR}/test/t2.ibd") or die "open";
 binmode FILE;
 seek(FILE, $ENV{'INNODB_PAGE_SIZE'} * 3 + 26, SEEK_SET) or die "seek";
-print FILE pack("H*", "c00lcafedeadb017");
+print FILE pack("H*", "c001cafedeadb017");
 close FILE or die "close";
 open(FILE, "+<", "$ENV{MYSQLD_DATADIR}/test/t3.ibd") or die "open";
 binmode FILE;
 seek(FILE, $ENV{'INNODB_PAGE_SIZE'} * 3 + 26, SEEK_SET) or die "seek";
-print FILE pack("H*", "c00lcafedeadb017");
+print FILE pack("H*", "c001cafedeadb017");
 close FILE or die "close";
 EOF
 

diff --git a/storage/innobase/buf/buf0buf.cc b/storage/innobase/buf/buf0buf.cc
@@ -451,8 +451,15 @@ static bool buf_page_decrypt_after_read(buf_page_t* bpage, fil_space_t* space)
 		/* Verify encryption checksum before we even try to
 		decrypt. */
 		if (!fil_space_verify_crypt_checksum(
-			    dst_frame, buf_page_get_zip_size(bpage), NULL,
-			    bpage->offset)) {
+			    dst_frame, buf_page_get_zip_size(bpage))) {
+			ib_logf(IB_LOG_LEVEL_ERROR,
+				"Encrypted page %u:%u in file %s"
+				" looks corrupted; key_version=" ULINTPF,
+				bpage->space, bpage->offset,
+				space->chain.start->name,
+				mach_read_from_4(
+					FIL_PAGE_FILE_FLUSH_LSN_OR_KEY_VERSION
+					+ dst_frame));
 decrypt_failed:
 			/* Mark page encrypted in case it should be. */
 			if (space->crypt_data->type
@@ -667,24 +674,6 @@ buf_block_alloc(
 #endif /* !UNIV_HOTBACKUP */
 #endif /* !UNIV_INNOCHECKSUM */
 
-/** Check if a page is all zeroes.
-@param[in]	read_buf	database page
-@param[in]	zip_size	ROW_FORMAT=COMPRESSED page size, or 0
-@return	whether the page is all zeroes */
-UNIV_INTERN
-bool
-buf_page_is_zeroes(const byte* read_buf, ulint zip_size)
-{
-	const ulint page_size = zip_size ? zip_size : UNIV_PAGE_SIZE;
-
-	for (ulint i = 0; i < page_size; i++) {
-		if (read_buf[i] != 0) {
-			return(false);
-		}
-	}
-	return(true);
-}
-
 /** Checks if the page is in crc32 checksum format.
 @param[in]	read_buf	database page
 @param[in]	checksum_field1	new checksum field
@@ -4752,31 +4741,28 @@ or decrypt/decompress just failed.
 @retval	DB_DECRYPTION_FAILED	if page post encryption checksum matches but
 after decryption normal page checksum does not match.
 @retval	DB_TABLESPACE_DELETED	if accessed tablespace is not found */
-static
-dberr_t
-buf_page_check_corrupt(buf_page_t* bpage, fil_space_t* space)
+static dberr_t buf_page_check_corrupt(buf_page_t* bpage, fil_space_t* space)
 {
 	ut_ad(space->n_pending_ios > 0);
 
 	ulint zip_size = buf_page_get_zip_size(bpage);
 	byte* dst_frame = (zip_size) ? bpage->zip.data :
 		((buf_block_t*) bpage)->frame;
-	bool still_encrypted = false;
 	dberr_t err = DB_SUCCESS;
 	bool corrupted = false;
-	fil_space_crypt_t* crypt_data = space->crypt_data;
 
 	/* In buf_decrypt_after_read we have either decrypted the page if
 	page post encryption checksum matches and used key_id is found
 	from the encryption plugin. If checksum did not match page was
 	not decrypted and it could be either encrypted and corrupted
 	or corrupted or good page. If we decrypted, there page could
 	still be corrupted if used key does not match. */
-	still_encrypted = (crypt_data &&
-		crypt_data->type != CRYPT_SCHEME_UNENCRYPTED &&
-		!bpage->encrypted &&
-		fil_space_verify_crypt_checksum(dst_frame, zip_size,
-			space, bpage->offset));
+	const bool still_encrypted = mach_read_from_4(
+		dst_frame + FIL_PAGE_FILE_FLUSH_LSN_OR_KEY_VERSION)
+		&& space->crypt_data
+		&& space->crypt_data->type != CRYPT_SCHEME_UNENCRYPTED
+		&& !bpage->encrypted
+		&& fil_space_verify_crypt_checksum(dst_frame, zip_size);
 
 	if (!still_encrypted) {
 		/* If traditional checksums match, we assume that page is

diff --git a/storage/innobase/buf/buf0dblwr.cc b/storage/innobase/buf/buf0dblwr.cc
@@ -1,7 +1,7 @@
 /*****************************************************************************
 
 Copyright (c) 1995, 2017, Oracle and/or its affiliates. All Rights Reserved.
-Copyright (c) 2013, 2017, MariaDB Corporation.
+Copyright (c) 2013, 2018, MariaDB Corporation.
 
 This program is free software; you can redistribute it and/or modify it under
 the terms of the GNU General Public License as published by the Free Software
@@ -362,6 +362,22 @@ buf_dblwr_create()
 	goto start_again;
 }
 
+/** Check if a page is all zeroes.
+@param[in]	read_buf	database page
+@param[in]	zip_size	ROW_FORMAT=COMPRESSED page size, or 0
+@return	whether the page is all zeroes */
+static bool buf_page_is_zeroes(const byte* read_buf, ulint zip_size)
+{
+	const ulint page_size = zip_size ? zip_size : UNIV_PAGE_SIZE;
+
+	for (ulint i = 0; i < page_size; i++) {
+		if (read_buf[i] != 0) {
+			return false;
+		}
+	}
+	return true;
+}
+
 /****************************************************************//**
 At a database startup initializes the doublewrite buffer memory structure if
 we already have a doublewrite buffer created in the data files. If we are
@@ -556,6 +572,9 @@ buf_dblwr_process()
 
 		const bool is_all_zero = buf_page_is_zeroes(
 			read_buf, zip_size);
+		const bool expect_encrypted = space()->crypt_data
+			&& space()->crypt_data->type
+			!= CRYPT_SCHEME_UNENCRYPTED;
 
 		if (is_all_zero) {
 			/* We will check if the copy in the
@@ -570,10 +589,13 @@ buf_dblwr_process()
 				goto bad;
 			}
 
-			if (fil_space_verify_crypt_checksum(
-				    read_buf, zip_size, NULL, page_no)
-			    || !buf_page_is_corrupted(
-				    true, read_buf, zip_size, space())) {
+			if (expect_encrypted && mach_read_from_4(
+				    read_buf
+				    + FIL_PAGE_FILE_FLUSH_LSN_OR_KEY_VERSION)
+			    ? fil_space_verify_crypt_checksum(read_buf,
+							      zip_size)
+			    : !buf_page_is_corrupted(true, read_buf,
+						     zip_size, space())) {
 				/* The page is good; there is no need
 				to consult the doublewrite buffer. */
 				continue;
@@ -592,9 +614,11 @@ buf_dblwr_process()
 		if (!decomp || (decomp != srv_page_size && zip_size)) {
 			goto bad_doublewrite;
 		}
-		if (!fil_space_verify_crypt_checksum(page, zip_size, NULL,
-						     page_no)
-		    && buf_page_is_corrupted(true, page, zip_size, space)) {
+
+		if (expect_encrypted && mach_read_from_4(
+			    page + FIL_PAGE_FILE_FLUSH_LSN_OR_KEY_VERSION)
+		    ? !fil_space_verify_crypt_checksum(page, zip_size)
+		    : buf_page_is_corrupted(true, page, zip_size, space())) {
 			if (!is_all_zero) {
 bad_doublewrite:
 				ib_logf(IB_LOG_LEVEL_WARN,