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

Code review of changes in page encryption

    Details

      Description

      We received the list of comments and change requests and we have addressed them in the recent pull request. Please take a look at our changes we have made.

      Notes:

      • It was mentioned, that platform dependent functions, e.g. OpenSSL, are not necessarily available on a target platform:
        requirement for OpenSSL was removed. yaSSL library is alternatively used. e.g. in Windows.
        Preprocessor instructions were used to make it compilable for different platforms. This was tested with Linux and Windows.
        OpenSSL command line tools still can be used to generate encrypted key files
        (a description can be found in xtradb/enc/EncKeys.cc)
      • It was asked, why the source code for key file (EncKeys.cc), etc. is located in the xtradb folder:
        xtradb is currently the supported storage engine. The files were not moved.
      • Since some questions occurred concerning the encryption, a description of the process follows:
        Pages are encrypted with AES/CBC/NoPadding algorithm.
        Supported are AES keys of 128, 192, or 256 bits.

      "No padding" is used to ensure, that the encrypted page does not exceed the page size.
      If "no padding" is used, the input for encryption must be of size (multiple * AES blocksize). AES blocksize is usually 16 (bytes).

      Everything in the page is encrypted except for the 38 byte FIL header.
      Since the length of the payload is not a multiple of the AES blocksize,
      and to ensure that every byte of the payload is encrypted, two encryption operations are done.
      Each time with a block of adequate size as input.
      1st block contains everything from beginning of payload bytes except for the remainder.
      2nd block is of size 64 and contains the remainder and the last (64 - sizeof(remainder)) bytes of the encrypted 1st block.

      Each encrypted page receives a new page type for PAGE_ENCRYPTION.
      The original page type (2 bytes) is stored in the Checksum header of the page (position FIL_PAGE_SPACE_OR_CHKSUM).
      Additionally the encryption key identifier is stored in the Checksum Header. This uses 1 byte.
      Checksum verification for encrypted pages is disabled. This checksum should be restored after decryption.

      To be able to verify decryption in a later stage, a 1-byte checksum at position 4 of the FIL_PAGE_SPACE_OR_CHKSUM header is stored.

      We oriented at the page compression feature, which also disables the checksum check.

      Please note, that pages with type FIL_PAGE_TYPE_FSP_HDR or FIL_PAGE_TYPE_XDES are not encrypted.
      Since these should not contain security relevant information, it should be Ok.
      It is necessary, so that the server can read tablespace flags on startup and detect the correct page size.

      As mentioned, not the key is stored inside the page, but a key identifier.
      Therefore no easy brute force attack should be possible.
      This identifier links to the real key (and initialization vector - iv), which are provided by the key file.

      At server startup, encryption keys must be available.
      If not, encrypted tables can not be accessed.
      It is currently not supported to edit the key file during runtime.
      Besides there is currently no support for the case, if the real AES key/iv change after a table was encrypted. That means, the key identifier stays the same.
      It would be possible to change the encryption key with
      an alter table statement. Example
      alter table t page_encryption=1 page_encryption_key=3;

      • It was asked, whether it was tested with all row formats
        It was tested with row formats compact, redundant, compressed and dynamic,
        using tablespace files up to a size of multiple gigabytes.
      • Source code was re-formatted on request
      • Superfluous things were removed

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              jplindst Jan Lindström added a comment -

              I run to some problems with Windows compiler, with InnoDB I get following kind of errors:

              171> Creating library C:/buildbot/winx64-packages/build/mariadb-10.1.2/storage/innobase/RelWithDebInfo/ha_innodb.lib and object C:/buildbot/winx64-packages/build/mariadb-10.1.2/storage/innobase/RelWithDebInfo/ha_innodb.exp

              171>EncKeys.obj : error LNK2001: unresolved external symbol pcre_free

              171>EncKeys.obj : error LNK2019: unresolved external symbol pcre_exec referenced in function "private: bool __cdecl EncKeys::isComment(char const *)" (?isComment@EncKeys@@AEAA_NPEBD@Z)

              171>EncKeys.obj : error LNK2019: unresolved external symbol pcre_compile referenced in function "private: bool __cdecl EncKeys::isComment(char const *)" (?isComment@EncKeys@@AEAA_NPEBD@Z)

              171>C:\buildbot\winx64-packages\build\mariadb-10.1.2\storage\innobase\RelWithDebInfo\ha_innodb.dll : fatal error LNK1120: 3 unresolved externals

              You should be able to get this branch with:

              (1) git clone git@github.com:MariaDB/server.git 10.1-eperi
              (2) cd 10.1-eperi
              (c) git checkout bb-10.1-eperi

              R: Jan

              Show
              jplindst Jan Lindström added a comment - I run to some problems with Windows compiler, with InnoDB I get following kind of errors: 171> Creating library C:/buildbot/winx64-packages/build/mariadb-10.1.2/storage/innobase/RelWithDebInfo/ha_innodb.lib and object C:/buildbot/winx64-packages/build/mariadb-10.1.2/storage/innobase/RelWithDebInfo/ha_innodb.exp 171>EncKeys.obj : error LNK2001: unresolved external symbol pcre_free 171>EncKeys.obj : error LNK2019: unresolved external symbol pcre_exec referenced in function "private: bool __cdecl EncKeys::isComment(char const *)" (?isComment@EncKeys@@AEAA_NPEBD@Z) 171>EncKeys.obj : error LNK2019: unresolved external symbol pcre_compile referenced in function "private: bool __cdecl EncKeys::isComment(char const *)" (?isComment@EncKeys@@AEAA_NPEBD@Z) 171>C:\buildbot\winx64-packages\build\mariadb-10.1.2\storage\innobase\RelWithDebInfo\ha_innodb.dll : fatal error LNK1120: 3 unresolved externals You should be able to get this branch with: (1) git clone git@github.com:MariaDB/server.git 10.1-eperi (2) cd 10.1-eperi (c) git checkout bb-10.1-eperi R: Jan
              Hide
              jplindst Jan Lindström added a comment -

              Ok solved by:

              diff --git a/storage/innobase/CMakeLists.txt b/storage/innobase/CMakeLists.txt
              index 2a4d161..6898144 100644
              --- a/storage/innobase/CMakeLists.txt
              +++ b/storage/innobase/CMakeLists.txt
              @@ -473,5 +473,5 @@ ENDIF()
               MYSQL_ADD_PLUGIN(innobase ${INNOBASE_SOURCES} STORAGE_ENGINE
                 MODULE_ONLY
                 MODULE_OUTPUT_NAME ha_innodb
              -  LINK_LIBRARIES ${ZLIB_LIBRARY} ${LINKER_SCRIPT})
              +  LINK_LIBRARIES ${ZLIB_LIBRARY} ${LINKER_SCRIPT} pcre pcreposix)
              
              Show
              jplindst Jan Lindström added a comment - Ok solved by: diff --git a/storage/innobase/CMakeLists.txt b/storage/innobase/CMakeLists.txt index 2a4d161..6898144 100644 --- a/storage/innobase/CMakeLists.txt +++ b/storage/innobase/CMakeLists.txt @@ -473,5 +473,5 @@ ENDIF() MYSQL_ADD_PLUGIN(innobase ${INNOBASE_SOURCES} STORAGE_ENGINE MODULE_ONLY MODULE_OUTPUT_NAME ha_innodb - LINK_LIBRARIES ${ZLIB_LIBRARY} ${LINKER_SCRIPT}) + LINK_LIBRARIES ${ZLIB_LIBRARY} ${LINKER_SCRIPT} pcre pcreposix)
              Hide
              eperi eperi added a comment -

              Ok.. We need pcre.
              I can not remember having changed anything for xtradb apart from inserting

              #ifdef _WIN_
              #define PCRE_STATIC 1
              #endif

              before including pcre.h
              to make it usable with windows.
              Looks like it is statically linked for xtradb.

              Show
              eperi eperi added a comment - Ok.. We need pcre. I can not remember having changed anything for xtradb apart from inserting #ifdef _ WIN _ #define PCRE_STATIC 1 #endif before including pcre.h to make it usable with windows. Looks like it is statically linked for xtradb.
              Hide
              jplindst Jan Lindström added a comment -

              About the encryption method is CTR and GCM more secure than CBC ? Can we use CTR/GCM ?

              R: Jan

              Show
              jplindst Jan Lindström added a comment - About the encryption method is CTR and GCM more secure than CBC ? Can we use CTR/GCM ? R: Jan
              Hide
              eperi eperi added a comment -

              I'm not aware about theoretical security of block cipher modes ctr, cgm, cbc...
              Assuming that encryption algorithm AES is secure and that should be the case according to current knowledge, it should be secure to use any of these.
              CBC is more commonly used.
              With the yaSSL library provided with mariadb I think there's no support for other mode than CBC (or ECB, which is regarded as less safe).
              Since this library is used, It can not be changed easily.

              Show
              eperi eperi added a comment - I'm not aware about theoretical security of block cipher modes ctr, cgm, cbc... Assuming that encryption algorithm AES is secure and that should be the case according to current knowledge, it should be secure to use any of these. CBC is more commonly used. With the yaSSL library provided with mariadb I think there's no support for other mode than CBC (or ECB, which is regarded as less safe). Since this library is used, It can not be changed easily.

                People

                • Assignee:
                  serg Sergei Golubchik
                  Reporter:
                  eperi eperi
                • Votes:
                  0 Vote for this issue
                  Watchers:
                  5 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved: