[Qemu-devel] [PATCH 0/5] Misc fixes and testing of qcow[2] encryption

[Qemu-devel] [PATCH 0/5] Misc fixes and testing of qcow[2] encryption

[Daniel P. Berrange]

I realize that qcow[2] encryption is a feature we have deprecated
and will remove support for running it with the QEMU system
emulators in this cycle. We do still need to make sure it continues
to work for the sake of letting people run qemu-img convert to
retrieve their data though.

Some of the other patches I'm working on which introduce a cypto
cipher API touch this qcow2 code, thus I wanted to be able to test
that it doesn't break anything.

I found that qemu-iotests didn't have any coverage of the qcow2
encryption code. For added fun, I then discovered that qemu-io
doesn't check if an encryption key is required, so ends up
writing plain text to the files instead of cipher, and returning
cipher text for reads, instead of plain text. IOW qemu-io will
corrupt encrypted qcow2 files on write.

This series adds some asserts that will protect against this kind
of mistake, adds support for getting passwords to qemu-io (in the
same manner that qemu-img supports), and finally adds a test case
for reading/writing encrypted qcow2.

Daniel P. Berrange (5):
  qcow2/qcow: protect against uninitialized encryption key
  util: move read_password method out of qemu-img into osdep/oslib
  util: allow \n to terminate password input
  qemu-io: prompt for encryption keys when required
  tests: add test case for encrypted qcow2 read/write

 block/qcow.c               | 10 +++--
 block/qcow2-cluster.c      |  3 +-
 block/qcow2.c              | 18 ++++++---
 include/qemu/osdep.h       |  2 +
 qemu-img.c                 | 93 +---------------------------------------------
 qemu-io.c                  | 21 +++++++++++
 tests/qemu-iotests/131     | 69 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/131.out | 46 +++++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 util/oslib-posix.c         | 67 +++++++++++++++++++++++++++++++++
 util/oslib-win32.c         | 24 ++++++++++++
 11 files changed, 252 insertions(+), 102 deletions(-)
 create mode 100755 tests/qemu-iotests/131
 create mode 100644 tests/qemu-iotests/131.out

-- 
2.1.0

[Qemu-devel] [PATCH 1/5] qcow2/qcow: protect against uninitialized encryption key

[Daniel P. Berrange]

When a qcow[2] file is opened, if the header reports an
encryption method, this is used to set the 'crypt_method_header'
field on the BDRVQcow[2]State struct, and the 'encrypted' flag
in the BDRVState struct.

When doing I/O operations, the 'crypt_method' field on the
BDRVQcow[2]State struct is checked to determine if encryption
needs to be applied.

The crypt_method_header value is copied into crypt_method when
the bdrv_set_key() method is called.

The QEMU code which opens a block device is expected to always
do a check

   if (bdrv_is_encrypted(bs)) {
       bdrv_set_key(bs, ....key...);
   }

If code forgets todo this, then 'crypt_method' is never set
and so when I/O is performed, QEMU writes plain text data
into a sector which is expected to contain cipher text, or
when reading, will return cipher text instead of plain
text.

Change the qcow[2] code to consult bs->encrypted when deciding
whether encryption is required, and assert(s->crypt_method)
to protect against cases where the caller forgets to set the
encryption key.

Also put an assert in the set_key methods to protect against
the case where the caller sets an encryption key on a block
device that does not have encryption

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 block/qcow.c          | 10 +++++++---
 block/qcow2-cluster.c |  3 ++-
 block/qcow2.c         | 18 ++++++++++++------
 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index ab89328..911e59f 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -269,6 +269,7 @@ static int qcow_set_key(BlockDriverState *bs, const char *key)
     for(i = 0;i < len;i++) {
         keybuf[i] = key[i];
     }
+    assert(bs->encrypted);
     s->crypt_method = s->crypt_method_header;
 
     if (AES_set_encrypt_key(keybuf, 128, &s->aes_encrypt_key) != 0)
@@ -411,9 +412,10 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
                 bdrv_truncate(bs->file, cluster_offset + s->cluster_size);
                 /* if encrypted, we must initialize the cluster
                    content which won't be written */
-                if (s->crypt_method &&
+                if (bs->encrypted &&
                     (n_end - n_start) < s->cluster_sectors) {
                     uint64_t start_sect;
+                    assert(s->crypt_method);
                     start_sect = (offset & ~(s->cluster_size - 1)) >> 9;
                     memset(s->cluster_data + 512, 0x00, 512);
                     for(i = 0; i < s->cluster_sectors; i++) {
@@ -590,7 +592,8 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
             if (ret < 0) {
                 break;
             }
-            if (s->crypt_method) {
+            if (bs->encrypted) {
+                assert(s->crypt_method);
                 encrypt_sectors(s, sector_num, buf, buf,
                                 n, 0,
                                 &s->aes_decrypt_key);
@@ -661,7 +664,8 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
             ret = -EIO;
             break;
         }
-        if (s->crypt_method) {
+        if (bs->encrypted) {
+            assert(s->crypt_method);
             if (!cluster_data) {
                 cluster_data = g_malloc0(s->cluster_size);
             }
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index ed2b44d..2d7777d 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -403,7 +403,8 @@ static int coroutine_fn copy_sectors(BlockDriverState *bs,
         goto out;
     }
 
-    if (s->crypt_method) {
+    if (bs->encrypted) {
+        assert(s->crypt_method);
         qcow2_encrypt_sectors(s, start_sect + n_start,
                         iov.iov_base, iov.iov_base, n, 1,
                         &s->aes_encrypt_key);
diff --git a/block/qcow2.c b/block/qcow2.c
index b9a72e3..f7b4cc6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1037,6 +1037,7 @@ static int qcow2_set_key(BlockDriverState *bs, const char *key)
     for(i = 0;i < len;i++) {
         keybuf[i] = key[i];
     }
+    assert(bs->encrypted);
     s->crypt_method = s->crypt_method_header;
 
     if (AES_set_encrypt_key(keybuf, 128, &s->aes_encrypt_key) != 0)
@@ -1224,7 +1225,9 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num,
                 goto fail;
             }
 
-            if (s->crypt_method) {
+            if (bs->encrypted) {
+                assert(s->crypt_method);
+
                 /*
                  * For encrypted images, read everything into a temporary
                  * contiguous buffer on which the AES functions can work.
@@ -1255,7 +1258,8 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num,
             if (ret < 0) {
                 goto fail;
             }
-            if (s->crypt_method) {
+            if (bs->encrypted) {
+                assert(s->crypt_method);
                 qcow2_encrypt_sectors(s, sector_num,  cluster_data,
                     cluster_data, cur_nr_sectors, 0, &s->aes_decrypt_key);
                 qemu_iovec_from_buf(qiov, bytes_done,
@@ -1315,7 +1319,7 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
         trace_qcow2_writev_start_part(qemu_coroutine_self());
         index_in_cluster = sector_num & (s->cluster_sectors - 1);
         cur_nr_sectors = remaining_sectors;
-        if (s->crypt_method &&
+        if (bs->encrypted &&
             cur_nr_sectors >
             QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors - index_in_cluster) {
             cur_nr_sectors =
@@ -1334,7 +1338,8 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
         qemu_iovec_concat(&hd_qiov, qiov, bytes_done,
             cur_nr_sectors * 512);
 
-        if (s->crypt_method) {
+        if (bs->encrypted) {
+            assert(s->crypt_method);
             if (!cluster_data) {
                 cluster_data = qemu_try_blockalign(bs->file,
                                                    QCOW_MAX_CRYPT_CLUSTERS
@@ -1484,7 +1489,8 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
      * that means we don't have to worry about reopening them here.
      */
 
-    if (s->crypt_method) {
+    if (bs->encrypted) {
+        assert(s->crypt_method);
         crypt_method = s->crypt_method;
         memcpy(&aes_encrypt_key, &s->aes_encrypt_key, sizeof(aes_encrypt_key));
         memcpy(&aes_decrypt_key, &s->aes_decrypt_key, sizeof(aes_decrypt_key));
@@ -1513,7 +1519,7 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
         return;
     }
 
-    if (crypt_method) {
+    if (bs->encrypted) {
         s->crypt_method = crypt_method;
         memcpy(&s->aes_encrypt_key, &aes_encrypt_key, sizeof(aes_encrypt_key));
         memcpy(&s->aes_decrypt_key, &aes_decrypt_key, sizeof(aes_decrypt_key));
-- 
2.1.0

Re: [Qemu-devel] [PATCH 1/5] qcow2/qcow: protect against uninitialized encryption key

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1785 bytes --]

On 05/12/2015 10:09 AM, Daniel P. Berrange wrote:
> When a qcow[2] file is opened, if the header reports an
> encryption method, this is used to set the 'crypt_method_header'
> field on the BDRVQcow[2]State struct, and the 'encrypted' flag
> in the BDRVState struct.
> 
> When doing I/O operations, the 'crypt_method' field on the
> BDRVQcow[2]State struct is checked to determine if encryption
> needs to be applied.
> 
> The crypt_method_header value is copied into crypt_method when
> the bdrv_set_key() method is called.
> 
> The QEMU code which opens a block device is expected to always
> do a check
> 
>    if (bdrv_is_encrypted(bs)) {
>        bdrv_set_key(bs, ....key...);
>    }
> 
> If code forgets todo this, then 'crypt_method' is never set

s/todo/to do/

> and so when I/O is performed, QEMU writes plain text data
> into a sector which is expected to contain cipher text, or
> when reading, will return cipher text instead of plain
> text.
> 
> Change the qcow[2] code to consult bs->encrypted when deciding
> whether encryption is required, and assert(s->crypt_method)
> to protect against cases where the caller forgets to set the
> encryption key.
> 
> Also put an assert in the set_key methods to protect against
> the case where the caller sets an encryption key on a block
> device that does not have encryption
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  block/qcow.c          | 10 +++++++---
>  block/qcow2-cluster.c |  3 ++-
>  block/qcow2.c         | 18 ++++++++++++------
>  3 files changed, 21 insertions(+), 10 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 2/5] util: move read_password method out of qemu-img into osdep/oslib

[Daniel P. Berrange]

The qemu-img.c file has a read_password() method impl that is
used to prompt for passwords on the console, with impls for
POSIX and Windows. This will be needed by qemu-io.c too, so
move it into the QEMU osdep/oslib files where it can be shared
without code duplication

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/qemu/osdep.h |  2 ++
 qemu-img.c           | 93 +---------------------------------------------------
 util/oslib-posix.c   | 66 +++++++++++++++++++++++++++++++++++++
 util/oslib-win32.c   | 24 ++++++++++++++
 4 files changed, 93 insertions(+), 92 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index b3300cc..3247364 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -259,4 +259,6 @@ void qemu_set_tty_echo(int fd, bool echo);
 
 void os_mem_prealloc(int fd, char *area, size_t sz);
 
+int qemu_read_password(char *buf, int buf_size);
+
 #endif
diff --git a/qemu-img.c b/qemu-img.c
index 8d30e43..60c820d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -165,97 +165,6 @@ static int GCC_FMT_ATTR(2, 3) qprintf(bool quiet, const char *fmt, ...)
     return ret;
 }
 
-#if defined(WIN32)
-/* XXX: put correct support for win32 */
-static int read_password(char *buf, int buf_size)
-{
-    int c, i;
-
-    printf("Password: ");
-    fflush(stdout);
-    i = 0;
-    for(;;) {
-        c = getchar();
-        if (c < 0) {
-            buf[i] = '\0';
-            return -1;
-        } else if (c == '\n') {
-            break;
-        } else if (i < (buf_size - 1)) {
-            buf[i++] = c;
-        }
-    }
-    buf[i] = '\0';
-    return 0;
-}
-
-#else
-
-#include <termios.h>
-
-static struct termios oldtty;
-
-static void term_exit(void)
-{
-    tcsetattr (0, TCSANOW, &oldtty);
-}
-
-static void term_init(void)
-{
-    struct termios tty;
-
-    tcgetattr (0, &tty);
-    oldtty = tty;
-
-    tty.c_iflag &= ~(IGNBRK|BRKINT|PARMRK|ISTRIP
-                          |INLCR|IGNCR|ICRNL|IXON);
-    tty.c_oflag |= OPOST;
-    tty.c_lflag &= ~(ECHO|ECHONL|ICANON|IEXTEN);
-    tty.c_cflag &= ~(CSIZE|PARENB);
-    tty.c_cflag |= CS8;
-    tty.c_cc[VMIN] = 1;
-    tty.c_cc[VTIME] = 0;
-
-    tcsetattr (0, TCSANOW, &tty);
-
-    atexit(term_exit);
-}
-
-static int read_password(char *buf, int buf_size)
-{
-    uint8_t ch;
-    int i, ret;
-
-    printf("password: ");
-    fflush(stdout);
-    term_init();
-    i = 0;
-    for(;;) {
-        ret = read(0, &ch, 1);
-        if (ret == -1) {
-            if (errno == EAGAIN || errno == EINTR) {
-                continue;
-            } else {
-                break;
-            }
-        } else if (ret == 0) {
-            ret = -1;
-            break;
-        } else {
-            if (ch == '\r') {
-                ret = 0;
-                break;
-            }
-            if (i < (buf_size - 1))
-                buf[i++] = ch;
-        }
-    }
-    term_exit();
-    buf[i] = '\0';
-    printf("\n");
-    return ret;
-}
-#endif
 
 static int print_block_option_help(const char *filename, const char *fmt)
 {
@@ -312,7 +221,7 @@ static BlockBackend *img_open(const char *id, const char *filename,
     bs = blk_bs(blk);
     if (bdrv_is_encrypted(bs) && require_io) {
         qprintf(quiet, "Disk image '%s' is encrypted.\n", filename);
-        if (read_password(password, sizeof(password)) < 0) {
+        if (qemu_read_password(password, sizeof(password)) < 0) {
             error_report("No password given");
             goto fail;
         }
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 37ffd96..1c23fd2 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -50,6 +50,7 @@ extern int daemon(int, int);
 
 #include <termios.h>
 #include <unistd.h>
+#include <termios.h>
 
 #include <glib/gprintf.h>
 
@@ -415,3 +416,68 @@ void os_mem_prealloc(int fd, char *area, size_t memory)
         pthread_sigmask(SIG_SETMASK, &oldset, NULL);
     }
 }
+
+
+static struct termios oldtty;
+
+static void term_exit(void)
+{
+    tcsetattr(0, TCSANOW, &oldtty);
+}
+
+static void term_init(void)
+{
+    struct termios tty;
+
+    tcgetattr(0, &tty);
+    oldtty = tty;
+
+    tty.c_iflag &= ~(IGNBRK|BRKINT|PARMRK|ISTRIP
+                          |INLCR|IGNCR|ICRNL|IXON);
+    tty.c_oflag |= OPOST;
+    tty.c_lflag &= ~(ECHO|ECHONL|ICANON|IEXTEN);
+    tty.c_cflag &= ~(CSIZE|PARENB);
+    tty.c_cflag |= CS8;
+    tty.c_cc[VMIN] = 1;
+    tty.c_cc[VTIME] = 0;
+
+    tcsetattr(0, TCSANOW, &tty);
+
+    atexit(term_exit);
+}
+
+int qemu_read_password(char *buf, int buf_size)
+{
+    uint8_t ch;
+    int i, ret;
+
+    printf("password: ");
+    fflush(stdout);
+    term_init();
+    i = 0;
+    for (;;) {
+        ret = read(0, &ch, 1);
+        if (ret == -1) {
+            if (errno == EAGAIN || errno == EINTR) {
+                continue;
+            } else {
+                break;
+            }
+        } else if (ret == 0) {
+            ret = -1;
+            break;
+        } else {
+            if (ch == '\r') {
+                ret = 0;
+                break;
+            }
+            if (i < (buf_size - 1)) {
+                buf[i++] = ch;
+            }
+        }
+    }
+    term_exit();
+    buf[i] = '\0';
+    printf("\n");
+    return ret;
+}
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 87cfbe0..730a670 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -470,3 +470,27 @@ void os_mem_prealloc(int fd, char *area, size_t memory)
         memset(area + pagesize * i, 0, 1);
     }
 }
+
+
+/* XXX: put correct support for win32 */
+int qemu_read_password(char *buf, int buf_size)
+{
+    int c, i;
+
+    printf("Password: ");
+    fflush(stdout);
+    i = 0;
+    for (;;) {
+        c = getchar();
+        if (c < 0) {
+            buf[i] = '\0';
+            return -1;
+        } else if (c == '\n') {
+            break;
+        } else if (i < (buf_size - 1)) {
+            buf[i++] = c;
+        }
+    }
+    buf[i] = '\0';
+    return 0;
+}
-- 
2.1.0

Re: [Qemu-devel] [PATCH 2/5] util: move read_password method out of qemu-img into osdep/oslib

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1308 bytes --]

On 05/12/2015 10:09 AM, Daniel P. Berrange wrote:
> The qemu-img.c file has a read_password() method impl that is
> used to prompt for passwords on the console, with impls for
> POSIX and Windows. This will be needed by qemu-io.c too, so
> move it into the QEMU osdep/oslib files where it can be shared
> without code duplication
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  include/qemu/osdep.h |  2 ++
>  qemu-img.c           | 93 +---------------------------------------------------
>  util/oslib-posix.c   | 66 +++++++++++++++++++++++++++++++++++++
>  util/oslib-win32.c   | 24 ++++++++++++++
>  4 files changed, 93 insertions(+), 92 deletions(-)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index b3300cc..3247364 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -259,4 +259,6 @@ void qemu_set_tty_echo(int fd, bool echo);
>  
>  void os_mem_prealloc(int fd, char *area, size_t sz);
>  
> +int qemu_read_password(char *buf, int buf_size);

Should we fix it to use size_t buf_size while at it? (or as a followup,
to keep this one limited to code motion)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 3/5] util: allow \n to terminate password input

[Daniel P. Berrange]

The qemu_read_password() method looks for \r to terminate the
reading of the a password. This is what will be seen when
reading the password from a TTY. When scripting though, it is
useful to be able to send the password via a pipe, in which
case we must look for \n to terminate password input.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 util/oslib-posix.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 1c23fd2..3ae4987 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -467,7 +467,8 @@ int qemu_read_password(char *buf, int buf_size)
             ret = -1;
             break;
         } else {
-            if (ch == '\r') {
+            if (ch == '\r' ||
+                ch == '\n') {
                 ret = 0;
                 break;
             }
-- 
2.1.0

Re: [Qemu-devel] [PATCH 3/5] util: allow \n to terminate password input

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]

On 05/12/2015 10:09 AM, Daniel P. Berrange wrote:
> The qemu_read_password() method looks for \r to terminate the
> reading of the a password. This is what will be seen when
> reading the password from a TTY. When scripting though, it is
> useful to be able to send the password via a pipe, in which
> case we must look for \n to terminate password input.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  util/oslib-posix.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 4/5] qemu-io: prompt for encryption keys when required

[Daniel P. Berrange]

The qemu-io tool does not check if the image is encrypted so
historically would silently corrupt the sectors by writing
plain text data into them instead of cipher text. The earlier
commit turns this mistake into a fatal abort, so check for
encryption and prompt for key when required.

This enables us to add unit tests to ensure we don't break
the ability of qemu-img to convert existing encrypted qcow2
files into a non-encrypted format.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qemu-io.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/qemu-io.c b/qemu-io.c
index 8e41080..34ae933 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -52,6 +52,7 @@ static const cmdinfo_t close_cmd = {
 static int openfile(char *name, int flags, QDict *opts)
 {
     Error *local_err = NULL;
+    BlockDriverState *bs;
 
     if (qemuio_blk) {
         fprintf(stderr, "file open already, try 'help close'\n");
@@ -68,7 +69,27 @@ static int openfile(char *name, int flags, QDict *opts)
         return 1;
     }
 
+    bs = blk_bs(qemuio_blk);
+    if (bdrv_is_encrypted(bs)) {
+        char password[256];
+        printf("Disk image '%s' is encrypted.\n", name);
+        if (qemu_read_password(password, sizeof(password)) < 0) {
+            error_report("No password given");
+            goto error;
+        }
+        if (bdrv_set_key(bs, password) < 0) {
+            error_report("invalid password");
+            goto error;
+        }
+    }
+
+
     return 0;
+
+ error:
+    blk_unref(qemuio_blk);
+    qemuio_blk = NULL;
+    return 1;
 }
 
 static void open_help(void)
-- 
2.1.0

Re: [Qemu-devel] [PATCH 4/5] qemu-io: prompt for encryption keys when required

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 999 bytes --]

On 05/12/2015 10:09 AM, Daniel P. Berrange wrote:
> The qemu-io tool does not check if the image is encrypted so
> historically would silently corrupt the sectors by writing
> plain text data into them instead of cipher text. The earlier
> commit turns this mistake into a fatal abort, so check for
> encryption and prompt for key when required.

Doesn't that mean that 'git bisect' gives a crashing qemu-io for 3
patches?  Should this be rearranged so that 1/5 comes after this to
avoid triggering the abort?

> 
> This enables us to add unit tests to ensure we don't break
> the ability of qemu-img to convert existing encrypted qcow2
> files into a non-encrypted format.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qemu-io.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 4/5] qemu-io: prompt for encryption keys when required

[Daniel P. Berrange]

On Tue, May 12, 2015 at 12:32:53PM -0600, Eric Blake wrote:
> On 05/12/2015 10:09 AM, Daniel P. Berrange wrote:
> > The qemu-io tool does not check if the image is encrypted so
> > historically would silently corrupt the sectors by writing
> > plain text data into them instead of cipher text. The earlier
> > commit turns this mistake into a fatal abort, so check for
> > encryption and prompt for key when required.
> 
> Doesn't that mean that 'git bisect' gives a crashing qemu-io for 3
> patches?  Should this be rearranged so that 1/5 comes after this to
> avoid triggering the abort?

I'm ambivalent on that - previously qemu-io was data corrupting
for this scenario, so crashing isn't really that much worse :-)

It is easy enough to reorder these though if that's desired.
The latter patches have no build time dep on the 1st patch, so
its trivial for Kevin to re-order when applying if he thinks it
is worth it.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 4/5] qemu-io: prompt for encryption keys when required

[Markus Armbruster]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Tue, May 12, 2015 at 12:32:53PM -0600, Eric Blake wrote:
>> On 05/12/2015 10:09 AM, Daniel P. Berrange wrote:
>> > The qemu-io tool does not check if the image is encrypted so
>> > historically would silently corrupt the sectors by writing
>> > plain text data into them instead of cipher text. The earlier
>> > commit turns this mistake into a fatal abort, so check for
>> > encryption and prompt for key when required.
>> 
>> Doesn't that mean that 'git bisect' gives a crashing qemu-io for 3
>> patches?  Should this be rearranged so that 1/5 comes after this to
>> avoid triggering the abort?
>
> I'm ambivalent on that - previously qemu-io was data corrupting
> for this scenario, so crashing isn't really that much worse :-)

If it crashes before it can corrupt anything, I'd sell it as an
improvement ;)

[...]

[Qemu-devel] [PATCH 5/5] tests: add test case for encrypted qcow2 read/write

[Daniel P. Berrange]

Add a simple test case for qemu-iotests that covers read/write
with encrypted qcow2 files.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 tests/qemu-iotests/131     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/131.out | 46 +++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 116 insertions(+)
 create mode 100755 tests/qemu-iotests/131
 create mode 100644 tests/qemu-iotests/131.out

diff --git a/tests/qemu-iotests/131 b/tests/qemu-iotests/131
new file mode 100755
index 0000000..f44b0a0
--- /dev/null
+++ b/tests/qemu-iotests/131
@@ -0,0 +1,69 @@
+#!/bin/bash
+#
+# Test encrypted read/write using plain bdrv_read/bdrv_write
+#
+# Copyright (C) 2009 Red Hat, Inc.
+#
+# 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 Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=berrange@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+
+size=128M
+IMGOPTS="encryption=on" _make_test_img $size
+
+echo
+echo "== reading whole image =="
+echo "astrochicken" | $QEMU_IO -c "read 0 $size" "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "== rewriting whole image =="
+echo "astrochicken" | $QEMU_IO -c "write -P 0xa 0 $size" "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "== verify pattern =="
+echo "astrochicken" | $QEMU_IO -c "read -P 0xa 0 $size" "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "== verify pattern failure with wrong password =="
+echo "platypus" | $QEMU_IO -c "read -P 0xa 0 $size" "$TEST_IMG" | _filter_qemu_io
+
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/131.out b/tests/qemu-iotests/131.out
new file mode 100644
index 0000000..4eedb35
--- /dev/null
+++ b/tests/qemu-iotests/131.out
@@ -0,0 +1,46 @@
+QA output created by 131
+qemu-img: Encrypted images are deprecated
+Support for them will be removed in a future release.
+You can use 'qemu-img convert' to convert your image to an unencrypted one.
+qemu-img: Encrypted images are deprecated
+Support for them will be removed in a future release.
+You can use 'qemu-img convert' to convert your image to an unencrypted one.
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on
+
+== reading whole image ==
+Encrypted images are deprecated
+Support for them will be removed in a future release.
+You can use 'qemu-img convert' to convert your image to an unencrypted one.
+Disk image '/home/berrange/src/virt/qemu/tests/qemu-iotests/scratch/t.qcow2' is encrypted.
+password:
+read 134217728/134217728 bytes at offset 0
+128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== rewriting whole image ==
+Encrypted images are deprecated
+Support for them will be removed in a future release.
+You can use 'qemu-img convert' to convert your image to an unencrypted one.
+Disk image '/home/berrange/src/virt/qemu/tests/qemu-iotests/scratch/t.qcow2' is encrypted.
+password:
+wrote 134217728/134217728 bytes at offset 0
+128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verify pattern ==
+Encrypted images are deprecated
+Support for them will be removed in a future release.
+You can use 'qemu-img convert' to convert your image to an unencrypted one.
+Disk image '/home/berrange/src/virt/qemu/tests/qemu-iotests/scratch/t.qcow2' is encrypted.
+password:
+read 134217728/134217728 bytes at offset 0
+128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verify pattern failure with wrong password ==
+Encrypted images are deprecated
+Support for them will be removed in a future release.
+You can use 'qemu-img convert' to convert your image to an unencrypted one.
+Disk image '/home/berrange/src/virt/qemu/tests/qemu-iotests/scratch/t.qcow2' is encrypted.
+password:
+Pattern verification failed at offset 0, 134217728 bytes
+read 134217728/134217728 bytes at offset 0
+128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 6ca3466..34b16cb 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -128,3 +128,4 @@
 128 rw auto quick
 129 rw auto quick
 130 rw auto quick
+131 rw auto quick
-- 
2.1.0

Re: [Qemu-devel] [PATCH 5/5] tests: add test case for encrypted qcow2 read/write

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]

On 05/12/2015 10:09 AM, Daniel P. Berrange wrote:
> Add a simple test case for qemu-iotests that covers read/write
> with encrypted qcow2 files.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  tests/qemu-iotests/131     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/131.out | 46 +++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 116 insertions(+)
>  create mode 100755 tests/qemu-iotests/131
>  create mode 100644 tests/qemu-iotests/131.out
> 
> diff --git a/tests/qemu-iotests/131 b/tests/qemu-iotests/131
> new file mode 100755
> index 0000000..f44b0a0
> --- /dev/null
> +++ b/tests/qemu-iotests/131
> @@ -0,0 +1,69 @@
> +#!/bin/bash
> +#
> +# Test encrypted read/write using plain bdrv_read/bdrv_write
> +#
> +# Copyright (C) 2009 Red Hat, Inc.

Copy-and-paste strikes again; welcome to 2015.  With that fixed,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 5/5] tests: add test case for encrypted qcow2 read/write

[John Snow]

On 05/12/2015 02:35 PM, Eric Blake wrote:
> On 05/12/2015 10:09 AM, Daniel P. Berrange wrote:
>> Add a simple test case for qemu-iotests that covers read/write 
>> with encrypted qcow2 files.
>> 
>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- 
>> tests/qemu-iotests/131     | 69
>> ++++++++++++++++++++++++++++++++++++++++++++++ 
>> tests/qemu-iotests/131.out | 46 +++++++++++++++++++++++++++++++ 
>> tests/qemu-iotests/group   |  1 + 3 files changed, 116
>> insertions(+) create mode 100755 tests/qemu-iotests/131 create
>> mode 100644 tests/qemu-iotests/131.out
>> 
>> diff --git a/tests/qemu-iotests/131 b/tests/qemu-iotests/131 new
>> file mode 100755 index 0000000..f44b0a0 --- /dev/null +++
>> b/tests/qemu-iotests/131 @@ -0,0 +1,69 @@ +#!/bin/bash +# +# Test
>> encrypted read/write using plain bdrv_read/bdrv_write +# +#
>> Copyright (C) 2009 Red Hat, Inc.
> 
> Copy-and-paste strikes again; welcome to 2015.  With that fixed, 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Fam Zheng already has a patch on-list that uses test 131, and I think
his patch was submitted first.

(Unless we want to play the "Who gets merged first?" game.)

--js

Re: [Qemu-devel] [Qemu-block] [PATCH 5/5] tests: add test case for encrypted qcow2 read/write

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 724 bytes --]

On 05/12/2015 01:06 PM, John Snow wrote:
>>> tests/qemu-iotests/131     | 69
>>> ++++++++++++++++++++++++++++++++++++++++++++++ 
>>> tests/qemu-iotests/131.out | 46 +++++++++++++++++++++++++++++++ 

> 
> Fam Zheng already has a patch on-list that uses test 131, and I think
> his patch was submitted first.
> 
> (Unless we want to play the "Who gets merged first?" game.)

That's the sort of conflict that I expect a maintainer can clean up, if
there is no other reason for a respin (although it is not always easy to
coax git into understanding that a patch would be valid if the file is
renamed.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 5/5] tests: add test case for encrypted qcow2 read/write

[John Snow]

On 05/12/2015 03:52 PM, Eric Blake wrote:
> On 05/12/2015 01:06 PM, John Snow wrote:
>>>> tests/qemu-iotests/131     | 69 
>>>> ++++++++++++++++++++++++++++++++++++++++++++++ 
>>>> tests/qemu-iotests/131.out | 46
>>>> +++++++++++++++++++++++++++++++
> 
>> 
>> Fam Zheng already has a patch on-list that uses test 131, and I
>> think his patch was submitted first.
>> 
>> (Unless we want to play the "Who gets merged first?" game.)
> 
> That's the sort of conflict that I expect a maintainer can clean
> up, if there is no other reason for a respin (although it is not
> always easy to coax git into understanding that a patch would be
> valid if the file is renamed.
> 

Sure, whoever fixes it. Just pointing it out.

--js

Re: [Qemu-devel] [PATCH 0/5] Misc fixes and testing of qcow[2] encryption

[Kevin Wolf]

Am 12.05.2015 um 18:09 hat Daniel P. Berrange geschrieben:
> I realize that qcow[2] encryption is a feature we have deprecated
> and will remove support for running it with the QEMU system
> emulators in this cycle. We do still need to make sure it continues
> to work for the sake of letting people run qemu-img convert to
> retrieve their data though.
> 
> Some of the other patches I'm working on which introduce a cypto
> cipher API touch this qcow2 code, thus I wanted to be able to test
> that it doesn't break anything.
> 
> I found that qemu-iotests didn't have any coverage of the qcow2
> encryption code. For added fun, I then discovered that qemu-io
> doesn't check if an encryption key is required, so ends up
> writing plain text to the files instead of cipher, and returning
> cipher text for reads, instead of plain text. IOW qemu-io will
> corrupt encrypted qcow2 files on write.
> 
> This series adds some asserts that will protect against this kind
> of mistake, adds support for getting passwords to qemu-io (in the
> same manner that qemu-img supports), and finally adds a test case
> for reading/writing encrypted qcow2.

Thanks, applied to the block branch.

Kevin