* [Qemu-devel] [PATCH v3 0/5] Add framework for passing secrets to QEMU
@ 2015-11-27 16:30 Daniel P. Berrange
2015-11-27 16:30 ` [Qemu-devel] [PATCH v3 1/5] util: add base64 decoding function Daniel P. Berrange
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Daniel P. Berrange @ 2015-11-27 16:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Markus Armbruster
This small series contains just the first two patches for
adding the secrets object to QEMU previously shown here:
https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04748.html
The QCryptoSecret object provides a QOM object that allows
passing secrets (passwords, encryption keys, etc) to QEMU
in a secure manner, via an external file, or on the CLI
or monitor with encryption. It also uses this to allow
use of encrypted x509 keys with the TLS handling code.
Changed in v3:
- s/adhoc/ad hoc/ (Eric)
- Fix mistake in unit test sample data (Eric)
- Permit inclusion of newline in base64 and test it (Eric)
- Switch to g_file_get_contents() instead of hand written
equivalent (Eric)
- Add missing conversion to qbase64_decode (Eric)
Changed in v2:
- Fix version tag in QAPI schema to be 2.6 (Eric)
- Changed "echo -n" to "printf" (Eric)
- Misc typo fixes (Eric)
- Added a genmeric qbase64_decode() wrapper around
g_base64_decode() that does error checking (Markus)
- Convert callers of g_base64_decode() to qbase64_decode()
to get error checking (Markus)
Daniel P. Berrange (5):
util: add base64 decoding function
qemu-char: convert to use error checked base64 decode
qga: convert to use error checked base64 decode
crypto: add QCryptoSecret object class for password/key handling
crypto: add support for loading encrypted x509 keys
crypto/Makefile.objs | 1 +
crypto/secret.c | 524 ++++++++++++++++++++++++++++++++++++++++++
crypto/tlscredsx509.c | 48 ++++
include/crypto/secret.h | 148 ++++++++++++
include/crypto/tlscredsx509.h | 1 +
include/qemu/base64.h | 56 +++++
qapi-schema.json | 2 -
qapi/crypto.json | 14 ++
qemu-char.c | 8 +-
qemu-options.hx | 85 ++++++-
qga/commands-posix.c | 11 +-
qga/commands-win32.c | 11 +-
qga/commands.c | 13 +-
qmp-commands.hx | 2 -
tests/.gitignore | 2 +
tests/Makefile | 5 +
tests/test-base64.c | 98 ++++++++
tests/test-crypto-secret.c | 446 +++++++++++++++++++++++++++++++++++
util/Makefile.objs | 1 +
util/base64.c | 60 +++++
20 files changed, 1525 insertions(+), 11 deletions(-)
create mode 100644 crypto/secret.c
create mode 100644 include/crypto/secret.h
create mode 100644 include/qemu/base64.h
create mode 100644 tests/test-base64.c
create mode 100644 tests/test-crypto-secret.c
create mode 100644 util/base64.c
--
2.5.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v3 1/5] util: add base64 decoding function
2015-11-27 16:30 [Qemu-devel] [PATCH v3 0/5] Add framework for passing secrets to QEMU Daniel P. Berrange
@ 2015-11-27 16:30 ` Daniel P. Berrange
2015-12-08 16:18 ` Eric Blake
2015-11-27 16:30 ` [Qemu-devel] [PATCH v3 2/5] qemu-char: convert to use error checked base64 decode Daniel P. Berrange
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Daniel P. Berrange @ 2015-11-27 16:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Markus Armbruster
The standard glib provided g_base64_decode doesn't provide any
kind of sensible error checking on its input. Add a QEMU custom
wrapper qbase64_decode which can be used with untrustworthy
input that can contain invalid base64 characters, embedded
NUL characters, or not be NUL terminated at all.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
include/qemu/base64.h | 56 +++++++++++++++++++++++++++++
tests/.gitignore | 1 +
tests/Makefile | 3 ++
tests/test-base64.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++
util/Makefile.objs | 1 +
util/base64.c | 60 +++++++++++++++++++++++++++++++
6 files changed, 219 insertions(+)
create mode 100644 include/qemu/base64.h
create mode 100644 tests/test-base64.c
create mode 100644 util/base64.c
diff --git a/include/qemu/base64.h b/include/qemu/base64.h
new file mode 100644
index 0000000..ea9e70f
--- /dev/null
+++ b/include/qemu/base64.h
@@ -0,0 +1,56 @@
+/*
+ * QEMU base64 helpers
+ *
+ * Copyright (c) 2015 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#ifndef QEMU_BASE64_H__
+#define QEMU_BASE64_H__
+
+#include "qemu-common.h"
+
+
+/**
+ * qbase64_decode:
+ * @input: the (possibly) base64 encoded text
+ * @in_len: length of @input or -1 if NUL terminated
+ * @out_len: filled with length of decoded data
+ * @errp: pointer to uninitialized error object
+ *
+ * Attempt to decode the (possibly) base64 encoded
+ * text provided in @input. If the @input text may
+ * contain embedded NUL characters, or may not be
+ * NUL terminated, then @in_len must be set to the
+ * known size of the @input buffer.
+ *
+ * Note that embedded NULs, or lack of a NUL terminator
+ * are considered invalid base64 data and errors
+ * will be reported to this effect.
+ *
+ * If decoding is successful, the decoded data will
+ * be returned and @out_len set to indicate the
+ * number of bytes in the decoded data.
+ *
+ * Returns: the decoded data or NULL
+ */
+uint8_t *qbase64_decode(const char *input,
+ size_t in_len,
+ size_t *out_len,
+ Error **errp);
+
+
+#endif /* QEMU_BUFFER_H__ */
diff --git a/tests/.gitignore b/tests/.gitignore
index 1e55722..00a7fed 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -8,6 +8,7 @@ check-qom-interface
check-qom-proplist
rcutorture
test-aio
+test-base64
test-bitops
test-blockjob-txn
test-coroutine
diff --git a/tests/Makefile b/tests/Makefile
index 0ef00a1..7d9dcc7 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -84,6 +84,7 @@ check-unit-$(CONFIG_GNUTLS) += tests/test-crypto-tlscredsx509$(EXESUF)
check-unit-$(CONFIG_GNUTLS) += tests/test-crypto-tlssession$(EXESUF)
check-unit-$(CONFIG_LINUX) += tests/test-qga$(EXESUF)
check-unit-y += tests/test-timed-average$(EXESUF)
+check-unit-y += tests/test-base64$(EXESUF)
check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
@@ -416,6 +417,8 @@ tests/test-vmstate$(EXESUF): tests/test-vmstate.o \
$(test-qom-obj-y)
tests/test-timed-average$(EXESUF): tests/test-timed-average.o qemu-timer.o \
$(test-util-obj-y)
+tests/test-base64$(EXESUF): tests/test-base64.o \
+ libqemuutil.a libqemustub.a
tests/test-qapi-types.c tests/test-qapi-types.h :\
$(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
diff --git a/tests/test-base64.c b/tests/test-base64.c
new file mode 100644
index 0000000..fdf0150
--- /dev/null
+++ b/tests/test-base64.c
@@ -0,0 +1,98 @@
+/*
+ * QEMU base64 heper test
+ *
+ * Copyright (c) 2015 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include <glib.h>
+
+#include "qemu/base64.h"
+
+
+static void test_base64_good(void)
+{
+ const char *input =
+ "QmVjYXVzZSB3ZSBmb2N1c2VkIG9uIHRoZSBzbmFrZSwgd2UgbW\n"
+ "lzc2VkIHRoZSBzY29ycGlvbi4=";
+ const char *expect = "Because we focused on the snake, "
+ "we missed the scorpion.";
+
+ size_t len;
+ uint8_t *actual = qbase64_decode(input,
+ -1,
+ &len,
+ &error_abort);
+
+ g_assert(actual != NULL);
+ g_assert_cmpint(len, ==, strlen(expect));
+ g_assert_cmpstr((char *)actual, ==, expect);
+ g_free(actual);
+}
+
+
+static void test_base64_bad(const char *input,
+ size_t input_len)
+{
+ size_t len;
+ Error *err = NULL;
+ uint8_t *actual = qbase64_decode(input,
+ input_len,
+ &len,
+ &err);
+
+ g_assert(err != NULL);
+ g_assert(actual == NULL);
+ g_assert_cmpint(len, ==, 0);
+ error_free(err);
+}
+
+
+static void test_base64_embedded_nul(void)
+{
+ const char input[] = "There's no such\0thing as a free lunch.";
+
+ test_base64_bad(input, G_N_ELEMENTS(input) - 1);
+}
+
+
+static void test_base64_not_nul_terminated(void)
+{
+ char input[] = "There's no such thing as a free lunch.";
+ input[G_N_ELEMENTS(input) - 1] = '!';
+
+ test_base64_bad(input, G_N_ELEMENTS(input) - 1);
+}
+
+
+static void test_base64_invalid_chars(void)
+{
+ const char *input = "There's no such thing as a free lunch.";
+
+ test_base64_bad(input, strlen(input));
+}
+
+
+int main(int argc, char **argv)
+{
+ g_test_init(&argc, &argv, NULL);
+ g_test_add_func("/util/base64/good", test_base64_good);
+ g_test_add_func("/util/base64/embedded-nul", test_base64_embedded_nul);
+ g_test_add_func("/util/base64/not-nul-terminated",
+ test_base64_not_nul_terminated);
+ g_test_add_func("/util/base64/invalid-chars", test_base64_invalid_chars);
+ return g_test_run();
+}
diff --git a/util/Makefile.objs b/util/Makefile.objs
index 89dd80e..8620a80 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -30,3 +30,4 @@ util-obj-y += qemu-coroutine-sleep.o
util-obj-y += coroutine-$(CONFIG_COROUTINE_BACKEND).o
util-obj-y += buffer.o
util-obj-y += timed-average.o
+util-obj-y += base64.o
diff --git a/util/base64.c b/util/base64.c
new file mode 100644
index 0000000..f82caa7
--- /dev/null
+++ b/util/base64.c
@@ -0,0 +1,60 @@
+/*
+ * QEMU base64 helpers
+ *
+ * Copyright (c) 2015 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include <config-host.h>
+
+#include "qemu/base64.h"
+
+static const char *base64_valid_chars =
+ "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/=\n";
+
+uint8_t *qbase64_decode(const char *input,
+ size_t in_len,
+ size_t *out_len,
+ Error **errp)
+{
+ *out_len = 0;
+
+ if (in_len != -1) {
+ /* Lack of NUL terminator is an error */
+ if (input[in_len] != '\0') {
+ error_setg(errp, "Base64 data is not NUL terminated");
+ return NULL;
+ }
+ /* Check there's no NULs embedded since we expect
+ * this to be valid base64 data */
+ if (memchr(input, '\0', in_len) != NULL) {
+ error_setg(errp, "Base64 data contains embedded NUL characters");
+ return NULL;
+ }
+
+ /* Now we know its a valid nul terminated string
+ * strspn is safe to use... */
+ } else {
+ in_len = strlen(input);
+ }
+
+ if (strspn(input, base64_valid_chars) != in_len) {
+ error_setg(errp, "Base64 data contains invalid characters");
+ return NULL;
+ }
+
+ return g_base64_decode(input, out_len);
+}
--
2.5.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v3 2/5] qemu-char: convert to use error checked base64 decode
2015-11-27 16:30 [Qemu-devel] [PATCH v3 0/5] Add framework for passing secrets to QEMU Daniel P. Berrange
2015-11-27 16:30 ` [Qemu-devel] [PATCH v3 1/5] util: add base64 decoding function Daniel P. Berrange
@ 2015-11-27 16:30 ` Daniel P. Berrange
2015-11-27 16:30 ` [Qemu-devel] [PATCH v3 3/5] qga: " Daniel P. Berrange
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Daniel P. Berrange @ 2015-11-27 16:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Markus Armbruster
Switch from using g_base64_decode over to qbase64_decode
in order to get error checking of the base64 input data.
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
qapi-schema.json | 2 --
qemu-char.c | 8 +++++++-
qmp-commands.hx | 2 --
3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/qapi-schema.json b/qapi-schema.json
index 8b1a423..7d92c04 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -295,8 +295,6 @@
# @format: #optional data encoding (default 'utf8').
# - base64: data must be base64 encoded text. Its binary
# decoding gets written.
-# Bug: invalid base64 is currently not rejected.
-# Whitespace *is* invalid.
# - utf8: data's UTF-8 encoding is written
# - data itself is always Unicode regardless of format, like
# any other string.
diff --git a/qemu-char.c b/qemu-char.c
index 5448b0f..ac01d73 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -32,6 +32,7 @@
#include "qapi/qmp-input-visitor.h"
#include "qapi/qmp-output-visitor.h"
#include "qapi-visit.h"
+#include "qemu/base64.h"
#include <unistd.h>
#include <fcntl.h>
@@ -3259,7 +3260,12 @@ void qmp_ringbuf_write(const char *device, const char *data,
}
if (has_format && (format == DATA_FORMAT_BASE64)) {
- write_data = g_base64_decode(data, &write_count);
+ write_data = qbase64_decode(data, -1,
+ &write_count,
+ errp);
+ if (!write_data) {
+ return;
+ }
} else {
write_data = (uint8_t *)data;
write_count = strlen(data);
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 9d8b42f..1b47cdd 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -512,8 +512,6 @@ Arguments:
- "data": data to write (json-string)
- "format": data format (json-string, optional)
- Possible values: "utf8" (default), "base64"
- Bug: invalid base64 is currently not rejected.
- Whitespace *is* invalid.
Example:
--
2.5.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v3 3/5] qga: convert to use error checked base64 decode
2015-11-27 16:30 [Qemu-devel] [PATCH v3 0/5] Add framework for passing secrets to QEMU Daniel P. Berrange
2015-11-27 16:30 ` [Qemu-devel] [PATCH v3 1/5] util: add base64 decoding function Daniel P. Berrange
2015-11-27 16:30 ` [Qemu-devel] [PATCH v3 2/5] qemu-char: convert to use error checked base64 decode Daniel P. Berrange
@ 2015-11-27 16:30 ` Daniel P. Berrange
2015-11-27 16:30 ` [Qemu-devel] [PATCH v3 4/5] crypto: add QCryptoSecret object class for password/key handling Daniel P. Berrange
2015-11-27 16:30 ` [Qemu-devel] [PATCH v3 5/5] crypto: add support for loading encrypted x509 keys Daniel P. Berrange
4 siblings, 0 replies; 11+ messages in thread
From: Daniel P. Berrange @ 2015-11-27 16:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Markus Armbruster
Switch from using g_base64_decode over to qbase64_decode
in order to get error checking of the base64 input data.
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
qga/commands-posix.c | 11 +++++++++--
qga/commands-win32.c | 11 +++++++++--
qga/commands.c | 13 ++++++++++++-
3 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index c2ff970..8fe708f 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -29,6 +29,7 @@
#include "qemu/queue.h"
#include "qemu/host-utils.h"
#include "qemu/sockets.h"
+#include "qemu/base64.h"
#ifndef CONFIG_HAS_ENVIRON
#ifdef __APPLE__
@@ -525,7 +526,10 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
gfh->state = RW_STATE_NEW;
}
- buf = g_base64_decode(buf_b64, &buf_len);
+ buf = qbase64_decode(buf_b64, -1, &buf_len, errp);
+ if (!buf) {
+ return NULL;
+ }
if (!has_count) {
count = buf_len;
@@ -1963,7 +1967,10 @@ void qmp_guest_set_user_password(const char *username,
char *chpasswddata = NULL;
size_t chpasswdlen;
- rawpasswddata = (char *)g_base64_decode(password, &rawpasswdlen);
+ rawpasswddata = (char *)qbase64_decode(password, -1, &rawpasswdlen, errp);
+ if (!rawpasswddata) {
+ return;
+ }
rawpasswddata = g_renew(char, rawpasswddata, rawpasswdlen + 1);
rawpasswddata[rawpasswdlen] = '\0';
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 0654fe4..61ffbdf 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -34,6 +34,7 @@
#include "qapi/qmp/qerror.h"
#include "qemu/queue.h"
#include "qemu/host-utils.h"
+#include "qemu/base64.h"
#ifndef SHTDN_REASON_FLAG_PLANNED
#define SHTDN_REASON_FLAG_PLANNED 0x80000000
@@ -357,7 +358,10 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
return NULL;
}
fh = gfh->fh;
- buf = g_base64_decode(buf_b64, &buf_len);
+ buf = qbase64_decode(buf_b64, -1, &buf_len, errp);
+ if (!buf) {
+ return NULL;
+ }
if (!has_count) {
count = buf_len;
@@ -1294,7 +1298,10 @@ void qmp_guest_set_user_password(const char *username,
return;
}
- rawpasswddata = (char *)g_base64_decode(password, &rawpasswdlen);
+ rawpasswddata = (char *)qbase64_decode(password, -1, &rawpasswdlen, errp);
+ if (!rawpasswddata) {
+ return;
+ }
rawpasswddata = g_renew(char, rawpasswddata, rawpasswdlen + 1);
rawpasswddata[rawpasswdlen] = '\0';
diff --git a/qga/commands.c b/qga/commands.c
index bb73e7d..58568d8 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -14,6 +14,7 @@
#include "qga/guest-agent-core.h"
#include "qga-qmp-commands.h"
#include "qapi/qmp/qerror.h"
+#include "qemu/base64.h"
/* Maximum captured guest-exec out_data/err_data - 16MB */
#define GUEST_EXEC_MAX_OUTPUT (16*1024*1024)
@@ -393,10 +394,19 @@ GuestExec *qmp_guest_exec(const char *path,
GIOChannel *in_ch, *out_ch, *err_ch;
GSpawnFlags flags;
bool has_output = (has_capture_output && capture_output);
+ uint8_t *input = NULL;
+ size_t ninput = 0;
arglist.value = (char *)path;
arglist.next = has_arg ? arg : NULL;
+ if (has_input_data) {
+ input = qbase64_decode(input_data, -1, &ninput, err);
+ if (!input) {
+ return NULL;
+ }
+ }
+
argv = guest_exec_get_args(&arglist, true);
envp = has_env ? guest_exec_get_args(env, false) : NULL;
@@ -425,7 +435,8 @@ GuestExec *qmp_guest_exec(const char *path,
g_child_watch_add(pid, guest_exec_child_watch, gei);
if (has_input_data) {
- gei->in.data = g_base64_decode(input_data, &gei->in.size);
+ gei->in.data = input;
+ gei->in.size = ninput;
#ifdef G_OS_WIN32
in_ch = g_io_channel_win32_new_fd(in_fd);
#else
--
2.5.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v3 4/5] crypto: add QCryptoSecret object class for password/key handling
2015-11-27 16:30 [Qemu-devel] [PATCH v3 0/5] Add framework for passing secrets to QEMU Daniel P. Berrange
` (2 preceding siblings ...)
2015-11-27 16:30 ` [Qemu-devel] [PATCH v3 3/5] qga: " Daniel P. Berrange
@ 2015-11-27 16:30 ` Daniel P. Berrange
2015-12-08 16:49 ` Eric Blake
2015-11-27 16:30 ` [Qemu-devel] [PATCH v3 5/5] crypto: add support for loading encrypted x509 keys Daniel P. Berrange
4 siblings, 1 reply; 11+ messages in thread
From: Daniel P. Berrange @ 2015-11-27 16:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Markus Armbruster
Introduce a new QCryptoSecret object class which will be used
for providing passwords and keys to other objects which need
sensitive credentials.
The new object can provide secret values directly as properties,
or indirectly via a file. The latter includes support for file
descriptor passing syntax on UNIX platforms. Ordinarily passing
secret values directly as properties is insecure, since they
are visible in process listings, or in log files showing the
CLI args / QMP commands. It is possible to use AES-256-CBC to
encrypt the secret values though, in which case all that is
visible is the ciphertext. For ad hoc developer testing though,
it is fine to provide the secrets directly without encryption
so this is not explicitly forbidden.
The anticipated scenario is that libvirtd will create a random
master key per QEMU instance (eg /var/run/libvirt/qemu/$VMNAME.key)
and will use that key to encrypt all passwords it provides to
QEMU via '-object secret,....'. This avoids the need for libvirt
(or other mgmt apps) to worry about file descriptor passing.
It also makes life easier for people who are scripting the
management of QEMU, for whom FD passing is significantly more
complex.
Providing data inline (insecure, only for ad hoc dev testing)
$QEMU -object secret,id=sec0,data=letmein
Providing data indirectly in raw format
printf "letmein" > mypasswd.txt
$QEMU -object secret,id=sec0,file=mypasswd.txt
Providing data indirectly in base64 format
$QEMU -object secret,id=sec0,file=mykey.b64,format=base64
Providing data with encryption
$QEMU -object secret,id=master0,file=mykey.b64,format=base64 \
-object secret,id=sec0,data=[base64 ciphertext],\
keyid=master0,iv=[base64 IV],format=base64
Note that 'format' here refers to the format of the ciphertext
data. The decrypted data must always be in raw byte format.
More examples are shown in the updated docs.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
crypto/Makefile.objs | 1 +
crypto/secret.c | 524 +++++++++++++++++++++++++++++++++++++++++++++
include/crypto/secret.h | 148 +++++++++++++
qapi/crypto.json | 14 ++
qemu-options.hx | 77 +++++++
tests/.gitignore | 1 +
tests/Makefile | 2 +
tests/test-crypto-secret.c | 446 ++++++++++++++++++++++++++++++++++++++
8 files changed, 1213 insertions(+)
create mode 100644 crypto/secret.c
create mode 100644 include/crypto/secret.h
create mode 100644 tests/test-crypto-secret.c
diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs
index b2a0e0b..a3135f1 100644
--- a/crypto/Makefile.objs
+++ b/crypto/Makefile.objs
@@ -7,6 +7,7 @@ crypto-obj-y += tlscreds.o
crypto-obj-y += tlscredsanon.o
crypto-obj-y += tlscredsx509.o
crypto-obj-y += tlssession.o
+crypto-obj-y += secret.o
# Let the userspace emulators avoid linking gnutls/etc
crypto-aes-obj-y = aes.o
diff --git a/crypto/secret.c b/crypto/secret.c
new file mode 100644
index 0000000..db6e2d5
--- /dev/null
+++ b/crypto/secret.c
@@ -0,0 +1,524 @@
+/*
+ * QEMU crypto secret support
+ *
+ * Copyright (c) 2015 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "crypto/secret.h"
+#include "crypto/cipher.h"
+#include "qom/object_interfaces.h"
+#include "qemu/base64.h"
+#include "trace.h"
+
+
+static void
+qcrypto_secret_load_data(QCryptoSecret *secret,
+ uint8_t **output,
+ size_t *outputlen,
+ Error **errp)
+{
+ char *data = NULL;
+ size_t length = 0;
+ GError *gerr = NULL;
+
+ *output = NULL;
+ *outputlen = 0;
+
+ if (secret->file) {
+ if (secret->data) {
+ error_setg(errp,
+ "'file' and 'data' are mutually exclusive");
+ return;
+ }
+ if (!g_file_get_contents(secret->file, &data, &length, &gerr)) {
+ error_setg(errp,
+ "Unable to read %s: %s",
+ secret->file, gerr->message);
+ g_error_free(gerr);
+ return;
+ }
+ if (length) {
+ /* Even though data is raw 8-bit, so may contain
+ * arbitrary NULs, ensure it is explicitly NUL
+ * terminated */
+ *output = g_renew(uint8_t, data, length + 1);
+ (*output)[length] = '\0';
+ *outputlen = length;
+ } else {
+ error_setg(errp, "Secret file %s is empty",
+ secret->file);
+ g_free(data);
+ }
+ } else if (secret->data) {
+ *outputlen = strlen(secret->data);
+ *output = g_new(uint8_t, *outputlen + 1);
+ memcpy(*output, secret->data, *outputlen + 1);
+ } else {
+ error_setg(errp, "Either 'file' or 'data' must be provided");
+ }
+}
+
+
+static void qcrypto_secret_decrypt(QCryptoSecret *secret,
+ const uint8_t *input,
+ size_t inputlen,
+ uint8_t **output,
+ size_t *outputlen,
+ Error **errp)
+{
+ uint8_t *key = NULL, *ciphertext = NULL, *iv = NULL;
+ size_t keylen, ciphertextlen, ivlen;
+ QCryptoCipher *aes = NULL;
+ uint8_t *plaintext = NULL;
+
+ *output = NULL;
+ *outputlen = 0;
+
+ if (qcrypto_secret_lookup(secret->keyid,
+ &key, &keylen,
+ errp) < 0) {
+ goto cleanup;
+ }
+
+ if (keylen != 32) {
+ error_setg(errp, "Key should be 32 bytes in length");
+ goto cleanup;
+ }
+
+ if (!secret->iv) {
+ error_setg(errp, "IV is required to decrypt secret");
+ goto cleanup;
+ }
+
+ iv = qbase64_decode(secret->iv, -1, &ivlen, errp);
+ if (!iv) {
+ goto cleanup;
+ }
+ if (ivlen != 16) {
+ error_setg(errp, "IV should be 16 bytes in length not %zu",
+ ivlen);
+ goto cleanup;
+ }
+
+ aes = qcrypto_cipher_new(QCRYPTO_CIPHER_ALG_AES_256,
+ QCRYPTO_CIPHER_MODE_CBC,
+ key, keylen,
+ errp);
+ if (!aes) {
+ goto cleanup;
+ }
+
+ if (qcrypto_cipher_setiv(aes, iv, ivlen, errp) < 0) {
+ goto cleanup;
+ }
+
+ if (secret->format == QCRYPTO_SECRET_FORMAT_BASE64) {
+ ciphertext = qbase64_decode((const gchar*)input,
+ inputlen,
+ &ciphertextlen,
+ errp);
+ if (!ciphertext) {
+ goto cleanup;
+ }
+ plaintext = g_new0(uint8_t, ciphertextlen + 1);
+ } else {
+ ciphertextlen = inputlen;
+ plaintext = g_new0(uint8_t, inputlen + 1);
+ }
+ if (qcrypto_cipher_decrypt(aes,
+ ciphertext ? ciphertext : input,
+ plaintext,
+ ciphertextlen,
+ errp) < 0) {
+ plaintext = NULL;
+ goto cleanup;
+ }
+
+ if (plaintext[ciphertextlen - 1] > 16 ||
+ plaintext[ciphertextlen - 1] > ciphertextlen) {
+ error_setg(errp, "Incorrect number of padding bytes (%d) "
+ "found on decrypted data",
+ (int)plaintext[ciphertextlen - 1]);
+ g_free(plaintext);
+ plaintext = NULL;
+ goto cleanup;
+ }
+
+ /* Even though plaintext may contain arbitrary NUL
+ * ensure it is explicitly NUL terminated.
+ */
+ ciphertextlen -= plaintext[ciphertextlen - 1];
+ plaintext[ciphertextlen] = '\0';
+
+ *output = plaintext;
+ *outputlen = ciphertextlen;
+
+ cleanup:
+ g_free(ciphertext);
+ g_free(iv);
+ g_free(key);
+ qcrypto_cipher_free(aes);
+}
+
+
+static void qcrypto_secret_decode(const uint8_t *input,
+ size_t inputlen,
+ uint8_t **output,
+ size_t *outputlen,
+ Error **errp)
+{
+ *output = qbase64_decode((const gchar*)input,
+ inputlen,
+ outputlen,
+ errp);
+}
+
+
+static void
+qcrypto_secret_prop_set_loaded(Object *obj,
+ bool value,
+ Error **errp)
+{
+ QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+
+ if (value) {
+ Error *local_err = NULL;
+ uint8_t *input = NULL;
+ size_t inputlen = 0;
+ uint8_t *output = NULL;
+ size_t outputlen = 0;
+
+ qcrypto_secret_load_data(secret, &input, &inputlen, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ if (secret->keyid) {
+ qcrypto_secret_decrypt(secret, input, inputlen,
+ &output, &outputlen, &local_err);
+ g_free(input);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+ input = output;
+ inputlen = outputlen;
+ } else {
+ if (secret->format != QCRYPTO_SECRET_FORMAT_RAW) {
+ qcrypto_secret_decode(input, inputlen,
+ &output, &outputlen, &local_err);
+ g_free(input);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+ input = output;
+ inputlen = outputlen;
+ }
+ }
+
+ secret->rawdata = input;
+ secret->rawlen = inputlen;
+ } else {
+ g_free(secret->rawdata);
+ secret->rawlen = 0;
+ }
+}
+
+
+static bool
+qcrypto_secret_prop_get_loaded(Object *obj,
+ Error **errp G_GNUC_UNUSED)
+{
+ QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+ return secret->data != NULL;
+}
+
+
+static void
+qcrypto_secret_prop_set_format(Object *obj,
+ int value,
+ Error **errp G_GNUC_UNUSED)
+{
+ QCryptoSecret *creds = QCRYPTO_SECRET(obj);
+
+ creds->format = value;
+}
+
+
+static int
+qcrypto_secret_prop_get_format(Object *obj,
+ Error **errp G_GNUC_UNUSED)
+{
+ QCryptoSecret *creds = QCRYPTO_SECRET(obj);
+
+ return creds->format;
+}
+
+
+static void
+qcrypto_secret_prop_set_data(Object *obj,
+ const char *value,
+ Error **errp)
+{
+ QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+
+ g_free(secret->data);
+ secret->data = g_strdup(value);
+}
+
+
+static char *
+qcrypto_secret_prop_get_data(Object *obj,
+ Error **errp)
+{
+ QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+ return g_strdup(secret->data);
+}
+
+
+static void
+qcrypto_secret_prop_set_file(Object *obj,
+ const char *value,
+ Error **errp)
+{
+ QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+
+ g_free(secret->file);
+ secret->file = g_strdup(value);
+}
+
+
+static char *
+qcrypto_secret_prop_get_file(Object *obj,
+ Error **errp)
+{
+ QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+ return g_strdup(secret->file);
+}
+
+
+static void
+qcrypto_secret_prop_set_iv(Object *obj,
+ const char *value,
+ Error **errp)
+{
+ QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+
+ g_free(secret->iv);
+ secret->iv = g_strdup(value);
+}
+
+
+static char *
+qcrypto_secret_prop_get_iv(Object *obj,
+ Error **errp)
+{
+ QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+ return g_strdup(secret->iv);
+}
+
+
+static void
+qcrypto_secret_prop_set_keyid(Object *obj,
+ const char *value,
+ Error **errp)
+{
+ QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+
+ g_free(secret->keyid);
+ secret->keyid = g_strdup(value);
+}
+
+
+static char *
+qcrypto_secret_prop_get_keyid(Object *obj,
+ Error **errp)
+{
+ QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+ return g_strdup(secret->keyid);
+}
+
+
+static void
+qcrypto_secret_complete(UserCreatable *uc, Error **errp)
+{
+ object_property_set_bool(OBJECT(uc), true, "loaded", errp);
+}
+
+
+static void
+qcrypto_secret_init(Object *obj)
+{
+ object_property_add_bool(obj, "loaded",
+ qcrypto_secret_prop_get_loaded,
+ qcrypto_secret_prop_set_loaded,
+ NULL);
+ object_property_add_enum(obj, "format",
+ "QCryptoSecretFormat",
+ QCryptoSecretFormat_lookup,
+ qcrypto_secret_prop_get_format,
+ qcrypto_secret_prop_set_format,
+ NULL);
+ object_property_add_str(obj, "data",
+ qcrypto_secret_prop_get_data,
+ qcrypto_secret_prop_set_data,
+ NULL);
+ object_property_add_str(obj, "file",
+ qcrypto_secret_prop_get_file,
+ qcrypto_secret_prop_set_file,
+ NULL);
+ object_property_add_str(obj, "keyid",
+ qcrypto_secret_prop_get_keyid,
+ qcrypto_secret_prop_set_keyid,
+ NULL);
+ object_property_add_str(obj, "iv",
+ qcrypto_secret_prop_get_iv,
+ qcrypto_secret_prop_set_iv,
+ NULL);
+}
+
+
+static void
+qcrypto_secret_finalize(Object *obj)
+{
+ QCryptoSecret *secret = QCRYPTO_SECRET(obj);
+
+ g_free(secret->iv);
+ g_free(secret->file);
+ g_free(secret->keyid);
+ g_free(secret->rawdata);
+ g_free(secret->data);
+}
+
+static void
+qcrypto_secret_class_init(ObjectClass *oc, void *data)
+{
+ UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
+
+ ucc->complete = qcrypto_secret_complete;
+}
+
+
+int qcrypto_secret_lookup(const char *secretid,
+ uint8_t **data,
+ size_t *datalen,
+ Error **errp)
+{
+ Object *obj;
+ QCryptoSecret *secret;
+
+ obj = object_resolve_path_component(
+ object_get_objects_root(), secretid);
+ if (!obj) {
+ error_setg(errp, "No secret with id '%s'", secretid);
+ return -1;
+ }
+
+ secret = (QCryptoSecret *)
+ object_dynamic_cast(obj,
+ TYPE_QCRYPTO_SECRET);
+ if (!secret) {
+ error_setg(errp, "Object with id '%s' is not a secret",
+ secretid);
+ return -1;
+ }
+
+ if (!secret->rawdata) {
+ error_setg(errp, "Secret with id '%s' has no data",
+ secretid);
+ return -1;
+ }
+
+ *data = g_new0(uint8, secret->rawlen + 1);
+ memcpy(*data, secret->rawdata, secret->rawlen);
+ (*data)[secret->rawlen] = '\0';
+ *datalen = secret->rawlen;
+
+ return 0;
+}
+
+
+char *qcrypto_secret_lookup_as_utf8(const char *secretid,
+ Error **errp)
+{
+ uint8_t *data;
+ size_t datalen;
+
+ if (qcrypto_secret_lookup(secretid,
+ &data,
+ &datalen,
+ errp) < 0) {
+ return NULL;
+ }
+
+ if (!g_utf8_validate((const gchar*)data, datalen, NULL)) {
+ error_setg(errp,
+ "Data from secret %s is not valid UTF-8",
+ secretid);
+ g_free(data);
+ return NULL;
+ }
+
+ return (char *)data;
+}
+
+
+char *qcrypto_secret_lookup_as_base64(const char *secretid,
+ Error **errp)
+{
+ uint8_t *data;
+ size_t datalen;
+ char *ret;
+
+ if (qcrypto_secret_lookup(secretid,
+ &data,
+ &datalen,
+ errp) < 0) {
+ return NULL;
+ }
+
+ ret = g_base64_encode(data, datalen);
+ g_free(data);
+ return ret;
+}
+
+
+static const TypeInfo qcrypto_secret_info = {
+ .parent = TYPE_OBJECT,
+ .name = TYPE_QCRYPTO_SECRET,
+ .instance_size = sizeof(QCryptoSecret),
+ .instance_init = qcrypto_secret_init,
+ .instance_finalize = qcrypto_secret_finalize,
+ .class_size = sizeof(QCryptoSecretClass),
+ .class_init = qcrypto_secret_class_init,
+ .interfaces = (InterfaceInfo[]) {
+ { TYPE_USER_CREATABLE },
+ { }
+ }
+};
+
+
+static void
+qcrypto_secret_register_types(void)
+{
+ type_register_static(&qcrypto_secret_info);
+}
+
+
+type_init(qcrypto_secret_register_types);
diff --git a/include/crypto/secret.h b/include/crypto/secret.h
new file mode 100644
index 0000000..913519a
--- /dev/null
+++ b/include/crypto/secret.h
@@ -0,0 +1,148 @@
+/*
+ * QEMU crypto secret support
+ *
+ * Copyright (c) 2015 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#ifndef QCRYPTO_SECRET_H__
+#define QCRYPTO_SECRET_H__
+
+#include "qemu-common.h"
+#include "qapi/error.h"
+#include "qom/object.h"
+
+#define TYPE_QCRYPTO_SECRET "secret"
+#define QCRYPTO_SECRET(obj) \
+ OBJECT_CHECK(QCryptoSecret, (obj), TYPE_QCRYPTO_SECRET)
+
+typedef struct QCryptoSecret QCryptoSecret;
+typedef struct QCryptoSecretClass QCryptoSecretClass;
+
+/**
+ * QCryptoSecret:
+ *
+ * The QCryptoSecret object provides storage of secrets,
+ * which may be user passwords, encryption keys or any
+ * other kind of sensitive data that is represented as
+ * a sequence of bytes.
+ *
+ * The sensitive data associated with the secret can
+ * be provided directly via the 'data' property, or
+ * indirectly via the 'file' property. In the latter
+ * case there is support for file descriptor passing
+ * via the usual /dev/fdset/NN syntax that QEMU uses.
+ *
+ * The data for a secret can be provided in two formats,
+ * either as a UTF-8 string (the default), or as base64
+ * encoded 8-bit binary data. The latter is appropriate
+ * for raw encryption keys, while the former is appropriate
+ * for user entered passwords.
+ *
+ * The data may be optionally encrypted with AES-256-CBC,
+ * and the decryption key provided by another
+ * QCryptoSecret instance identified by the 'keyid'
+ * property. When passing sensitive data directly
+ * via the 'data' property it is strongly recommended
+ * to use the AES encryption facility to prevent the
+ * sensitive data being exposed in the process listing
+ * or system log files.
+ *
+ * Providing data directly, insecurely (suitable for
+ * ad hoc developer testing only)
+ *
+ * $QEMU -object secret,id=sec0,data=letmein
+ *
+ * Providing data indirectly:
+ *
+ * # printf "letmein" > password.txt
+ * # $QEMU \
+ * -object secret,id=sec0,file=password.txt
+ *
+ * Using a master encryption key with data.
+ *
+ * The master key needs to be created as 32 secure
+ * random bytes (optionally base64 encoded)
+ *
+ * # openssl rand -base64 32 > key.b64
+ * # KEY=$(base64 -d key.b64 | hexdump -v -e '/1 "%02X"')
+ *
+ * Each secret to be encrypted needs to have a random
+ * initialization vector generated. These do not need
+ * to be kept secret
+ *
+ * # openssl rand -base64 16 > iv.b64
+ * # IV=$(base64 -d iv.b64 | hexdump -v -e '/1 "%02X"')
+ *
+ * A secret to be defined can now be encrypted
+ *
+ * # SECRET=$(printf "letmein" |
+ * openssl enc -aes-256-cbc -a -K $KEY -iv $IV)
+ *
+ * When launching QEMU, create a master secret pointing
+ * to key.b64 and specify that to be used to decrypt
+ * the user password
+ *
+ * # $QEMU \
+ * -object secret,id=secmaster0,format=base64,file=key.b64 \
+ * -object secret,id=sec0,keyid=secmaster0,format=base64,\
+ * data=$SECRET,iv=$(<iv.b64)
+ *
+ * When encrypting, the data can still be provided via an
+ * external file, in which case it is possible to use either
+ * raw binary data, or base64 encoded. This example uses
+ * raw format
+ *
+ * # printf "letmein" |
+ * openssl enc -aes-256-cbc -K $KEY -iv $IV -o pw.aes
+ * # $QEMU \
+ * -object secret,id=secmaster0,format=base64,file=key.b64 \
+ * -object secret,id=sec0,keyid=secmaster0,\
+ * file=pw.aes,iv=$(<iv.b64)
+ *
+ * Note that the ciphertext can be in either raw or base64
+ * format, as indicated by the 'format' parameter, but the
+ * plaintext resulting from decryption is expected to always
+ * be in raw format.
+ */
+
+struct QCryptoSecret {
+ Object parent_obj;
+ uint8_t *rawdata;
+ size_t rawlen;
+ QCryptoSecretFormat format;
+ char *data;
+ char *file;
+ char *keyid;
+ char *iv;
+};
+
+
+struct QCryptoSecretClass {
+ ObjectClass parent_class;
+};
+
+
+extern int qcrypto_secret_lookup(const char *secretid,
+ uint8_t **data,
+ size_t *datalen,
+ Error **errp);
+extern char *qcrypto_secret_lookup_as_utf8(const char *secretid,
+ Error **errp);
+extern char *qcrypto_secret_lookup_as_base64(const char *secretid,
+ Error **errp);
+
+#endif /* QCRYPTO_SECRET_H__ */
diff --git a/qapi/crypto.json b/qapi/crypto.json
index b058b14..4012659 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -19,3 +19,17 @@
{ 'enum': 'QCryptoTLSCredsEndpoint',
'prefix': 'QCRYPTO_TLS_CREDS_ENDPOINT',
'data': ['client', 'server']}
+
+
+##
+# QCryptoSecretFormat:
+#
+# The data format that the secret is provided in
+#
+# @raw: raw bytes. When encoded in JSON only valid UTF-8 sequences can be used
+# @base64: arbitrary base64 encoded binary data
+# Since: 2.6
+##
+{ 'enum': 'QCryptoSecretFormat',
+ 'prefix': 'QCRYPTO_SECRET_FORMAT',
+ 'data': ['raw', 'base64']}
diff --git a/qemu-options.hx b/qemu-options.hx
index 0eea4ee..5aa22fc 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3677,6 +3677,83 @@ Dump the network traffic on netdev @var{dev} to the file specified by
The file format is libpcap, so it can be analyzed with tools such as tcpdump
or Wireshark.
+@item -object secret,id=@var{id},data=@var{string},format=@var{raw|base64}[,keyid=@var{secretid},iv=@var{string}]
+@item -object secret,id=@var{id},file=@var{filename},format=@var{raw|base64}[,keyid=@var{secretid},iv=@var{string}]
+
+Defines a secret to store a password, encryption key, or some other sensitive
+data. The sensitive data can either be passed directly via the @var{data}
+parameter, or indirectly via the @var{file} parameter. Using the @var{data}
+parameter is insecure unless the sensitive data is encrypted.
+
+The sensitive data can be provided in raw format (the default), or base64.
+When encoded as JSON, the raw format only supports valid UTF-8 characters,
+so base64 is recommended for sending binary data. QEMU will convert from
+which ever format is provided to the format it needs internally. eg, an
+RBD password can be provided in raw format, even though it will be base64
+encoded when passed onto the RBD sever.
+
+For added protection, it is possible to encrypt the data associated with
+a secret using the AES-256-CBC cipher. Use of encryption is indicated
+by providing the @var{keyid} and @var{iv} parameters. The @var{keyid}
+parameter provides the ID of a previously defined secret that contains
+the AES-256 decryption key. This key should be 32-bytes long and be
+base64 encoded. The @var{iv} parameter provides the random initialization
+vector used for encryption of this particular secret and should be a
+base64 encrypted string of the 32-byte IV.
+
+The simplest (insecure) usage is to provide the secret inline
+
+@example
+
+ # $QEMU -object secret,id=sec0,data=letmein,format=raw
+
+@end example
+
+The simplest secure usage is to provide the secret via a file
+
+ # echo -n "letmein" > mypasswd.txt
+ # $QEMU -object secret,id=sec0,file=mypasswd.txt,format=raw
+
+For greater security, AES-256-CBC should be used. To illustrate usage,
+consider the openssl command line tool which can encrypt the data. Note
+that when encrypting, the plaintext must be padded to the cipher block
+size (32 bytes) using the standard PKCS#5/6 compatible padding algorithm.
+
+First a master key needs to be created in base64 encoding:
+
+@example
+ # openssl rand -base64 32 > key.b64
+ # KEY=$(base64 -d key.b64 | hexdump -v -e '/1 "%02X"')
+@end example
+
+Each secret to be encrypted needs to have a random initialization vector
+generated. These do not need to be kept secret
+
+@example
+ # openssl rand -base64 16 > iv.b64
+ # IV=$(base64 -d iv.b64 | hexdump -v -e '/1 "%02X"')
+@end example
+
+The secret to be defined can now be encrypted, in this case we're
+telling openssl to base64 encode the result, but it could be left
+as raw bytes if desired.
+
+@example
+ # SECRET=$(echo -n "letmein" |
+ openssl enc -aes-256-cbc -a -K $KEY -iv $IV)
+@end example
+
+When launching QEMU, create a master secret pointing to @code{key.b64}
+and specify that to be used to decrypt the user password. Pass the
+contents of @code{iv.b64} to the second secret
+
+@example
+ # $QEMU \
+ -object secret,id=secmaster0,format=base64,file=key.b64 \
+ -object secret,id=sec0,keyid=secmaster0,format=base64,\
+ data=$SECRET,iv=$(<iv.b64)
+@end example
+
@end table
ETEXI
diff --git a/tests/.gitignore b/tests/.gitignore
index 00a7fed..dbe7c06 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -14,6 +14,7 @@ test-blockjob-txn
test-coroutine
test-crypto-cipher
test-crypto-hash
+test-crypto-secret
test-crypto-tlscredsx509
test-crypto-tlscredsx509-work/
test-crypto-tlscredsx509-certs/
diff --git a/tests/Makefile b/tests/Makefile
index 7d9dcc7..42d84f4 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -80,6 +80,7 @@ check-unit-y += tests/test-write-threshold$(EXESUF)
gcov-files-test-write-threshold-y = block/write-threshold.c
check-unit-$(CONFIG_GNUTLS_HASH) += tests/test-crypto-hash$(EXESUF)
check-unit-y += tests/test-crypto-cipher$(EXESUF)
+check-unit-y += tests/test-crypto-secret$(EXESUF)
check-unit-$(CONFIG_GNUTLS) += tests/test-crypto-tlscredsx509$(EXESUF)
check-unit-$(CONFIG_GNUTLS) += tests/test-crypto-tlssession$(EXESUF)
check-unit-$(CONFIG_LINUX) += tests/test-qga$(EXESUF)
@@ -460,6 +461,7 @@ tests/test-mul64$(EXESUF): tests/test-mul64.o $(test-util-obj-y)
tests/test-bitops$(EXESUF): tests/test-bitops.o $(test-util-obj-y)
tests/test-crypto-hash$(EXESUF): tests/test-crypto-hash.o $(test-crypto-obj-y)
tests/test-crypto-cipher$(EXESUF): tests/test-crypto-cipher.o $(test-crypto-obj-y)
+tests/test-crypto-secret$(EXESUF): tests/test-crypto-secret.o $(test-crypto-obj-y)
tests/crypto-tls-x509-helpers.o-cflags := $(TASN1_CFLAGS)
tests/crypto-tls-x509-helpers.o-libs := $(TASN1_LIBS)
diff --git a/tests/test-crypto-secret.c b/tests/test-crypto-secret.c
new file mode 100644
index 0000000..6743608
--- /dev/null
+++ b/tests/test-crypto-secret.c
@@ -0,0 +1,446 @@
+/*
+ * QEMU Crypto secret handling
+ *
+ * Copyright (c) 2015 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include <glib.h>
+
+#include "crypto/init.h"
+#include "crypto/secret.h"
+
+static void test_secret_direct(void)
+{
+ Object *sec = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "sec0",
+ &error_abort,
+ "data", "123456",
+ NULL);
+
+ char *pw = qcrypto_secret_lookup_as_utf8("sec0",
+ &error_abort);
+
+ g_assert_cmpstr(pw, ==, "123456");
+
+ object_unparent(sec);
+ g_free(pw);
+}
+
+
+static void test_secret_indirect_good(void)
+{
+ Object *sec;
+ char *fname = NULL;
+ int fd = g_file_open_tmp("secretXXXXXX",
+ &fname,
+ NULL);
+
+ g_assert(fd >= 0);
+ g_assert_nonnull(fname);
+
+ g_assert(write(fd, "123456", 6) == 6);
+
+ sec = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "sec0",
+ &error_abort,
+ "file", fname,
+ NULL);
+
+ char *pw = qcrypto_secret_lookup_as_utf8("sec0",
+ &error_abort);
+
+ g_assert_cmpstr(pw, ==, "123456");
+
+ object_unparent(sec);
+ g_free(pw);
+ close(fd);
+ g_free(fname);
+}
+
+
+static void test_secret_indirect_badfile(void)
+{
+ Object *sec = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "sec0",
+ NULL,
+ "file", "does-not-exist",
+ NULL);
+
+ g_assert(sec == NULL);
+}
+
+
+static void test_secret_indirect_emptyfile(void)
+{
+ Object *sec;
+ char *fname = NULL;
+ int fd = g_file_open_tmp("secretXXXXXX",
+ &fname,
+ NULL);
+
+ g_assert(fd >= 0);
+ g_assert_nonnull(fname);
+
+ sec = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "sec0",
+ NULL,
+ "file", fname,
+ NULL);
+
+ close(fd);
+ g_free(fname);
+ g_assert(sec == NULL);
+}
+
+
+static void test_secret_noconv_base64_good(void)
+{
+ Object *sec = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "sec0",
+ &error_abort,
+ "data", "MTIzNDU2",
+ "format", "base64",
+ NULL);
+
+ char *pw = qcrypto_secret_lookup_as_base64("sec0",
+ &error_abort);
+
+ g_assert_cmpstr(pw, ==, "MTIzNDU2");
+
+ object_unparent(sec);
+ g_free(pw);
+}
+
+
+static void test_secret_noconv_base64_bad(void)
+{
+ Object *sec = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "sec0",
+ NULL,
+ "data", "MTI$NDU2",
+ "format", "base64",
+ NULL);
+
+ g_assert(sec == NULL);
+}
+
+
+static void test_secret_noconv_utf8(void)
+{
+ Object *sec = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "sec0",
+ &error_abort,
+ "data", "123456",
+ "format", "raw",
+ NULL);
+
+ char *pw = qcrypto_secret_lookup_as_utf8("sec0",
+ &error_abort);
+
+ g_assert_cmpstr(pw, ==, "123456");
+
+ object_unparent(sec);
+ g_free(pw);
+}
+
+
+static void test_secret_conv_base64_utf8valid(void)
+{
+ Object *sec = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "sec0",
+ &error_abort,
+ "data", "MTIzNDU2",
+ "format", "base64",
+ NULL);
+
+ char *pw = qcrypto_secret_lookup_as_utf8("sec0",
+ &error_abort);
+
+ g_assert_cmpstr(pw, ==, "123456");
+
+ object_unparent(sec);
+ g_free(pw);
+}
+
+
+static void test_secret_conv_base64_utf8invalid(void)
+{
+ Object *sec = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "sec0",
+ &error_abort,
+ "data", "f0VMRgIBAQAAAA==",
+ "format", "base64",
+ NULL);
+
+ char *pw = qcrypto_secret_lookup_as_utf8("sec0",
+ NULL);
+ g_assert(pw == NULL);
+
+ object_unparent(sec);
+}
+
+
+static void test_secret_conv_utf8_base64(void)
+{
+ Object *sec = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "sec0",
+ &error_abort,
+ "data", "123456",
+ NULL);
+
+ char *pw = qcrypto_secret_lookup_as_base64("sec0",
+ &error_abort);
+
+ g_assert_cmpstr(pw, ==, "MTIzNDU2");
+
+ object_unparent(sec);
+ g_free(pw);
+}
+
+
+static void test_secret_crypt_raw(void)
+{
+ Object *master = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "master",
+ &error_abort,
+ "data", "9miloPQCzGy+TL6aonfzVcptibCmCIhKzrnlfwiWivk=",
+ "format", "base64",
+ NULL);
+ Object *sec = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "sec0",
+ &error_abort,
+ "data",
+ "\xCC\xBF\xF7\x09\x46\x19\x0B\x52\x2A\x3A\xB4\x6B\xCD\x7A\xB0\xB0",
+ "format", "raw",
+ "keyid", "master",
+ "iv", "0I7Gw/TKuA+Old2W2apQ3g==",
+ NULL);
+
+ char *pw = qcrypto_secret_lookup_as_utf8("sec0",
+ &error_abort);
+
+ g_assert_cmpstr(pw, ==, "123456");
+
+ object_unparent(sec);
+ object_unparent(master);
+ g_free(pw);
+}
+
+
+static void test_secret_crypt_base64(void)
+{
+ Object *master = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "master",
+ &error_abort,
+ "data", "9miloPQCzGy+TL6aonfzVcptibCmCIhKzrnlfwiWivk=",
+ "format", "base64",
+ NULL);
+ Object *sec = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "sec0",
+ &error_abort,
+ "data", "zL/3CUYZC1IqOrRrzXqwsA==",
+ "format", "base64",
+ "keyid", "master",
+ "iv", "0I7Gw/TKuA+Old2W2apQ3g==",
+ NULL);
+
+ char *pw = qcrypto_secret_lookup_as_utf8("sec0",
+ &error_abort);
+
+ g_assert_cmpstr(pw, ==, "123456");
+
+ object_unparent(sec);
+ object_unparent(master);
+ g_free(pw);
+}
+
+
+static void test_secret_crypt_short_key(void)
+{
+ Object *master = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "master",
+ &error_abort,
+ "data", "9miloPQCzGy+TL6aonfzVc",
+ "format", "base64",
+ NULL);
+ Object *sec = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "sec0",
+ NULL,
+ "data", "zL/3CUYZC1IqOrRrzXqwsA==",
+ "format", "raw",
+ "keyid", "master",
+ "iv", "0I7Gw/TKuA+Old2W2apQ3g==",
+ NULL);
+
+ g_assert(sec == NULL);
+ object_unparent(master);
+}
+
+
+static void test_secret_crypt_short_iv(void)
+{
+ Object *master = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "master",
+ &error_abort,
+ "data", "9miloPQCzGy+TL6aonfzVcptibCmCIhKzrnlfwiWivk=",
+ "format", "base64",
+ NULL);
+ Object *sec = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "sec0",
+ NULL,
+ "data", "zL/3CUYZC1IqOrRrzXqwsA==",
+ "format", "raw",
+ "keyid", "master",
+ "iv", "0I7Gw/TKuA+Old2W2a",
+ NULL);
+
+ g_assert(sec == NULL);
+ object_unparent(master);
+}
+
+
+static void test_secret_crypt_missing_iv(void)
+{
+ Object *master = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "master",
+ &error_abort,
+ "data", "9miloPQCzGy+TL6aonfzVcptibCmCIhKzrnlfwiWivk=",
+ "format", "base64",
+ NULL);
+ Object *sec = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "sec0",
+ NULL,
+ "data", "zL/3CUYZC1IqOrRrzXqwsA==",
+ "format", "raw",
+ "keyid", "master",
+ NULL);
+
+ g_assert(sec == NULL);
+ object_unparent(master);
+}
+
+
+static void test_secret_crypt_bad_iv(void)
+{
+ Object *master = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "master",
+ &error_abort,
+ "data", "9miloPQCzGy+TL6aonfzVcptibCmCIhKzrnlfwiWivk=",
+ "format", "base64",
+ NULL);
+ Object *sec = object_new_with_props(
+ TYPE_QCRYPTO_SECRET,
+ object_get_objects_root(),
+ "sec0",
+ NULL,
+ "data", "zL/3CUYZC1IqOrRrzXqwsA==",
+ "format", "raw",
+ "keyid", "master",
+ "iv", "0I7Gw/TK$$uA+Old2W2a",
+ NULL);
+
+ g_assert(sec == NULL);
+ object_unparent(master);
+}
+
+
+int main(int argc, char **argv)
+{
+ module_call_init(MODULE_INIT_QOM);
+ g_test_init(&argc, &argv, NULL);
+
+ g_assert(qcrypto_init(NULL) == 0);
+
+ g_test_add_func("/crypto/secret/direct",
+ test_secret_direct);
+ g_test_add_func("/crypto/secret/indirect/good",
+ test_secret_indirect_good);
+ g_test_add_func("/crypto/secret/indirect/badfile",
+ test_secret_indirect_badfile);
+ g_test_add_func("/crypto/secret/indirect/emptyfile",
+ test_secret_indirect_emptyfile);
+
+ g_test_add_func("/crypto/secret/noconv/base64/good",
+ test_secret_noconv_base64_good);
+ g_test_add_func("/crypto/secret/noconv/base64/bad",
+ test_secret_noconv_base64_bad);
+ g_test_add_func("/crypto/secret/noconv/utf8",
+ test_secret_noconv_utf8);
+ g_test_add_func("/crypto/secret/conv/base64/utf8valid",
+ test_secret_conv_base64_utf8valid);
+ g_test_add_func("/crypto/secret/conv/base64/utf8invalid",
+ test_secret_conv_base64_utf8invalid);
+ g_test_add_func("/crypto/secret/conv/utf8/base64",
+ test_secret_conv_utf8_base64);
+
+ g_test_add_func("/crypto/secret/crypt/raw",
+ test_secret_crypt_raw);
+ g_test_add_func("/crypto/secret/crypt/base64",
+ test_secret_crypt_base64);
+ g_test_add_func("/crypto/secret/crypt/shortkey",
+ test_secret_crypt_short_key);
+ g_test_add_func("/crypto/secret/crypt/shortiv",
+ test_secret_crypt_short_iv);
+ g_test_add_func("/crypto/secret/crypt/missingiv",
+ test_secret_crypt_missing_iv);
+ g_test_add_func("/crypto/secret/crypt/badiv",
+ test_secret_crypt_bad_iv);
+
+ return g_test_run();
+}
--
2.5.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v3 5/5] crypto: add support for loading encrypted x509 keys
2015-11-27 16:30 [Qemu-devel] [PATCH v3 0/5] Add framework for passing secrets to QEMU Daniel P. Berrange
` (3 preceding siblings ...)
2015-11-27 16:30 ` [Qemu-devel] [PATCH v3 4/5] crypto: add QCryptoSecret object class for password/key handling Daniel P. Berrange
@ 2015-11-27 16:30 ` Daniel P. Berrange
4 siblings, 0 replies; 11+ messages in thread
From: Daniel P. Berrange @ 2015-11-27 16:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Markus Armbruster
Make use of the QCryptoSecret object to support loading of
encrypted x509 keys. The optional 'passwordid' parameter
to the tls-creds-x509 object type, provides the ID of a
secret object instance that holds the decryption password
for the PEM file.
# printf "123456" > mypasswd.txt
# $QEMU \
-object secret,id=sec0,filename=mypasswd.txt \
-object tls-creds-x509,passwordid=sec0,id=creds0,\
dir=/home/berrange/.pki/qemu,endpoint=server \
-vnc :1,tls-creds=creds0
This requires QEMU to be linked to GNUTLS >= 3.1.11. If
GNUTLS is too old an error will be reported if an attempt
is made to pass a decryption password.
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
crypto/tlscredsx509.c | 48 +++++++++++++++++++++++++++++++++++++++++++
include/crypto/tlscredsx509.h | 1 +
qemu-options.hx | 8 +++++++-
3 files changed, 56 insertions(+), 1 deletion(-)
diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index d080deb..c06edbb 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -20,6 +20,7 @@
#include "crypto/tlscredsx509.h"
#include "crypto/tlscredspriv.h"
+#include "crypto/secret.h"
#include "qom/object_interfaces.h"
#include "trace.h"
@@ -606,9 +607,30 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
}
if (cert != NULL && key != NULL) {
+#if GNUTLS_VERSION_NUMBER >= 0x030111
+ char *password = NULL;
+ if (creds->passwordid) {
+ password = qcrypto_secret_lookup_as_utf8(creds->passwordid,
+ errp);
+ if (!password) {
+ goto cleanup;
+ }
+ }
+ ret = gnutls_certificate_set_x509_key_file2(creds->data,
+ cert, key,
+ GNUTLS_X509_FMT_PEM,
+ password,
+ 0);
+ g_free(password);
+#else /* GNUTLS_VERSION_NUMBER < 0x030111 */
+ if (creds->passwordid) {
+ error_setg(errp, "PKCS8 decryption requires GNUTLS >= 3.1.11");
+ goto cleanup;
+ }
ret = gnutls_certificate_set_x509_key_file(creds->data,
cert, key,
GNUTLS_X509_FMT_PEM);
+#endif /* GNUTLS_VERSION_NUMBER < 0x030111 */
if (ret < 0) {
error_setg(errp, "Cannot load certificate '%s' & key '%s': %s",
cert, key, gnutls_strerror(ret));
@@ -736,6 +758,27 @@ qcrypto_tls_creds_x509_prop_set_sanity(Object *obj,
}
+static void
+qcrypto_tls_creds_x509_prop_set_passwordid(Object *obj,
+ const char *value,
+ Error **errp G_GNUC_UNUSED)
+{
+ QCryptoTLSCredsX509 *creds = QCRYPTO_TLS_CREDS_X509(obj);
+
+ creds->passwordid = g_strdup(value);
+}
+
+
+static char *
+qcrypto_tls_creds_x509_prop_get_passwordid(Object *obj,
+ Error **errp G_GNUC_UNUSED)
+{
+ QCryptoTLSCredsX509 *creds = QCRYPTO_TLS_CREDS_X509(obj);
+
+ return g_strdup(creds->passwordid);
+}
+
+
static bool
qcrypto_tls_creds_x509_prop_get_sanity(Object *obj,
Error **errp G_GNUC_UNUSED)
@@ -768,6 +811,10 @@ qcrypto_tls_creds_x509_init(Object *obj)
qcrypto_tls_creds_x509_prop_get_sanity,
qcrypto_tls_creds_x509_prop_set_sanity,
NULL);
+ object_property_add_str(obj, "passwordid",
+ qcrypto_tls_creds_x509_prop_get_passwordid,
+ qcrypto_tls_creds_x509_prop_set_passwordid,
+ NULL);
}
@@ -776,6 +823,7 @@ qcrypto_tls_creds_x509_finalize(Object *obj)
{
QCryptoTLSCredsX509 *creds = QCRYPTO_TLS_CREDS_X509(obj);
+ g_free(creds->passwordid);
qcrypto_tls_creds_x509_unload(creds);
}
diff --git a/include/crypto/tlscredsx509.h b/include/crypto/tlscredsx509.h
index b9785fd..25796d7 100644
--- a/include/crypto/tlscredsx509.h
+++ b/include/crypto/tlscredsx509.h
@@ -101,6 +101,7 @@ struct QCryptoTLSCredsX509 {
gnutls_certificate_credentials_t data;
#endif
bool sanityCheck;
+ char *passwordid;
};
diff --git a/qemu-options.hx b/qemu-options.hx
index 5aa22fc..0a8c640 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3626,7 +3626,7 @@ expensive operation that consumes random pool entropy, so it is
recommended that a persistent set of parameters be generated
upfront and saved.
-@item -object tls-creds-x509,id=@var{id},endpoint=@var{endpoint},dir=@var{/path/to/cred/dir},verify-peer=@var{on|off}
+@item -object tls-creds-x509,id=@var{id},endpoint=@var{endpoint},dir=@var{/path/to/cred/dir},verify-peer=@var{on|off},passwordid=@var{id}
Creates a TLS anonymous credentials object, which can be used to provide
TLS support on network backends. The @option{id} parameter is a unique
@@ -3653,6 +3653,12 @@ in PEM format, in filenames @var{ca-cert.pem}, @var{ca-crl.pem} (optional),
@var{server-cert.pem} (only servers), @var{server-key.pem} (only servers),
@var{client-cert.pem} (only clients), and @var{client-key.pem} (only clients).
+For the @var{server-key.pem} and @var{client-key.pem} files which
+contain sensitive private keys, it is possible to use an encrypted
+version by providing the @var{passwordid} parameter. This provides
+the ID of a previously created @code{secret} object containing the
+password for decryption.
+
@item -object filter-buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue=@var{all|rx|tx}]
Interval @var{t} can't be 0, this filter batches the packet delivery: all
--
2.5.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/5] util: add base64 decoding function
2015-11-27 16:30 ` [Qemu-devel] [PATCH v3 1/5] util: add base64 decoding function Daniel P. Berrange
@ 2015-12-08 16:18 ` Eric Blake
2015-12-09 12:50 ` Daniel P. Berrange
0 siblings, 1 reply; 11+ messages in thread
From: Eric Blake @ 2015-12-08 16:18 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel; +Cc: Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 4353 bytes --]
On 11/27/2015 09:30 AM, Daniel P. Berrange wrote:
> The standard glib provided g_base64_decode doesn't provide any
> kind of sensible error checking on its input. Add a QEMU custom
> wrapper qbase64_decode which can be used with untrustworthy
> input that can contain invalid base64 characters, embedded
> NUL characters, or not be NUL terminated at all.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> +
> +/**
> + * qbase64_decode:
> + * @input: the (possibly) base64 encoded text
> + * @in_len: length of @input or -1 if NUL terminated
> + * @out_len: filled with length of decoded data
> + * @errp: pointer to uninitialized error object
That almost implies that I could do:
Error *err;
qbase64_decode(,&err);
In reality, it should be the NULL-initialized error object, as in:
Error *err = NULL;
but I don't know if there is a better way to represent it in text. At
any rate, that phrase exists elsewhere, so it would be easier to do a
tree-wide cleanup if we have a better terminology to use, and I won't
hold up review on this patch for it.
> + *
> + * Attempt to decode the (possibly) base64 encoded
> + * text provided in @input. If the @input text may
> + * contain embedded NUL characters, or may not be
> + * NUL terminated, then @in_len must be set to the
> + * known size of the @input buffer.
> + *
> + * Note that embedded NULs, or lack of a NUL terminator
> + * are considered invalid base64 data and errors
> + * will be reported to this effect.
> + *
> + * If decoding is successful, the decoded data will
> + * be returned and @out_len set to indicate the
> + * number of bytes in the decoded data.
> + *
Maybe mention that caller must g_free() the successful result?
> + * Returns: the decoded data or NULL
> + */
> +uint8_t *qbase64_decode(const char *input,
> + size_t in_len,
> + size_t *out_len,
> + Error **errp);
> +
> +
> +++ b/tests/test-base64.c
> @@ -0,0 +1,98 @@
> +static void test_base64_bad(const char *input,
> + size_t input_len)
> +{
> + size_t len;
> + Error *err = NULL;
> + uint8_t *actual = qbase64_decode(input,
> + input_len,
> + &len,
> + &err);
> +
> + g_assert(err != NULL);
Could use &error_abort in the original call instead of a second check
for err != NULL; but that's cosmetic.
> + g_assert(actual == NULL);
> + g_assert_cmpint(len, ==, 0);
So you are testing that we initialize output length even on error,
rather than leaving it uninitialized. That's fair.
> +
> +static void test_base64_embedded_nul(void)
> +{
> + const char input[] = "There's no such\0thing as a free lunch.";
> +
> + test_base64_bad(input, G_N_ELEMENTS(input) - 1);
> +}
> +
This asserts that you have a failure, but doesn't say what that failure
would be...
> +
> +static void test_base64_not_nul_terminated(void)
> +{
> + char input[] = "There's no such thing as a free lunch.";
> + input[G_N_ELEMENTS(input) - 1] = '!';
> +
> + test_base64_bad(input, G_N_ELEMENTS(input) - 1);
> +}
> +
> +
> +static void test_base64_invalid_chars(void)
> +{
> + const char *input = "There's no such thing as a free lunch.";
> +
> + test_base64_bad(input, strlen(input));
> +}
...and this same input already fails because it doesn't match base64,
regardless of whether NUL bytes are mishandled. I wonder if
test_base64_embedded_nul() and test_base64_not_nul_terminated() should
be using variations of your known-good base64 string
("QmVjYXVzZSB3ZSBmb2N1c2VkIG9uIHRoZSBzbmFrZSwgd2UgbW\n"
"lzc2VkIHRoZSBzY29ycGlvbi4="), to prove that they are failing purely
because of the NUL handling and not because of invalid base64 content.
But that's only a weak complaint - because by peering into the black
box, I see that you are fully testing all code paths (that is, the black
box checks for NUL abuse prior to checking for valid base64 data), so
I'm not going to insist on a respin.
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 --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/5] crypto: add QCryptoSecret object class for password/key handling
2015-11-27 16:30 ` [Qemu-devel] [PATCH v3 4/5] crypto: add QCryptoSecret object class for password/key handling Daniel P. Berrange
@ 2015-12-08 16:49 ` Eric Blake
2015-12-09 12:56 ` Daniel P. Berrange
0 siblings, 1 reply; 11+ messages in thread
From: Eric Blake @ 2015-12-08 16:49 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel; +Cc: Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 3581 bytes --]
On 11/27/2015 09:30 AM, Daniel P. Berrange wrote:
> Introduce a new QCryptoSecret object class which will be used
> for providing passwords and keys to other objects which need
> sensitive credentials.
>
> More examples are shown in the updated docs.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> +++ b/crypto/secret.c
> +static void
> +qcrypto_secret_load_data(QCryptoSecret *secret,
> + uint8_t **output,
> + size_t *outputlen,
> + Error **errp)
> +{
> + if (!g_file_get_contents(secret->file, &data, &length, &gerr)) {
> + error_setg(errp,
> + "Unable to read %s: %s",
> + secret->file, gerr->message);
> + g_error_free(gerr);
> + return;
> + }
> + if (length) {
> + /* Even though data is raw 8-bit, so may contain
> + * arbitrary NULs, ensure it is explicitly NUL
> + * terminated */
> + *output = g_renew(uint8_t, data, length + 1);
> + (*output)[length] = '\0';
These two lines are dead code. g_file_get_contents() guarantees that on
success, contents is malloc'd large enough and that contents[length] == 0.
https://developer.gnome.org/glib/stable/glib-File-Utilities.html#g-file-get-contents
> + *outputlen = length;
> + } else {
> + error_setg(errp, "Secret file %s is empty",
> + secret->file);
Is there any technical reason why we must forbid a 0-length password?
(Sometimes, having the empty string as a password can be a useful for
development tests). I'm not opposed to rejecting it, especially if
doing so now avoids a more cryptic error message later because there is
indeed a technical reason; but just want to make sure it is not an
arbitrary limitation.
> + g_free(data);
> + }
> + } else if (secret->data) {
> + *outputlen = strlen(secret->data);
> + *output = g_new(uint8_t, *outputlen + 1);
> + memcpy(*output, secret->data, *outputlen + 1);
These two lines could be shortened to:
*output = g_strdup(secret->data);
> +
> +static void qcrypto_secret_decrypt(QCryptoSecret *secret,
> + const uint8_t *input,
> + size_t inputlen,
> + uint8_t **output,
> + size_t *outputlen,
> + Error **errp)
> +{
> + if (secret->format == QCRYPTO_SECRET_FORMAT_BASE64) {
> + ciphertext = qbase64_decode((const gchar*)input,
> + inputlen,
> + &ciphertextlen,
> + errp);
> + if (!ciphertext) {
> + goto cleanup;
> + }
> + plaintext = g_new0(uint8_t, ciphertextlen + 1);
> + } else {
> + ciphertextlen = inputlen;
> + plaintext = g_new0(uint8_t, inputlen + 1);
g_new0(uint8_t, value) is the same as g_malloc0(value); I don't know if
it is worth the distinction. But not worth a respin on its own.
I found some style or efficiency things you might want to touch up, but
no actual bugs that would prevent this patch from being usable as-is.
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 --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/5] util: add base64 decoding function
2015-12-08 16:18 ` Eric Blake
@ 2015-12-09 12:50 ` Daniel P. Berrange
2015-12-09 15:18 ` Eric Blake
0 siblings, 1 reply; 11+ messages in thread
From: Daniel P. Berrange @ 2015-12-09 12:50 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Markus Armbruster
On Tue, Dec 08, 2015 at 09:18:30AM -0700, Eric Blake wrote:
> On 11/27/2015 09:30 AM, Daniel P. Berrange wrote:
> > The standard glib provided g_base64_decode doesn't provide any
> > kind of sensible error checking on its input. Add a QEMU custom
> > wrapper qbase64_decode which can be used with untrustworthy
> > input that can contain invalid base64 characters, embedded
> > NUL characters, or not be NUL terminated at all.
> >
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
>
> > +
> > +/**
> > + * qbase64_decode:
> > + * @input: the (possibly) base64 encoded text
> > + * @in_len: length of @input or -1 if NUL terminated
> > + * @out_len: filled with length of decoded data
> > + * @errp: pointer to uninitialized error object
>
> That almost implies that I could do:
>
> Error *err;
> qbase64_decode(,&err);
>
> In reality, it should be the NULL-initialized error object, as in:
>
> Error *err = NULL;
>
> but I don't know if there is a better way to represent it in text. At
> any rate, that phrase exists elsewhere, so it would be easier to do a
> tree-wide cleanup if we have a better terminology to use, and I won't
> hold up review on this patch for it.
Yep, most of those comments will be my fault, since that's the standard
doc text I've been adding for @errp parameters. I'll fix this one
and also do a general cleanup patch for others separately.
> > +static void test_base64_bad(const char *input,
> > + size_t input_len)
> > +{
> > + size_t len;
> > + Error *err = NULL;
> > + uint8_t *actual = qbase64_decode(input,
> > + input_len,
> > + &len,
> > + &err);
> > +
> > + g_assert(err != NULL);
>
> Could use &error_abort in the original call instead of a second check
> for err != NULL; but that's cosmetic.
Actually we can't use &error_abort, as we don't want the test
function to abort on error as that would indicate test failure.
We want to make sure the call completes & returns an error
set without aborting.
> > + g_assert(actual == NULL);
> > + g_assert_cmpint(len, ==, 0);
>
> So you are testing that we initialize output length even on error,
> rather than leaving it uninitialized. That's fair.
> > +static void test_base64_embedded_nul(void)
> > +{
> > + const char input[] = "There's no such\0thing as a free lunch.";
> > +
> > + test_base64_bad(input, G_N_ELEMENTS(input) - 1);
> > +}
> > +
>
> This asserts that you have a failure, but doesn't say what that failure
> would be...
>
> > +
> > +static void test_base64_not_nul_terminated(void)
> > +{
> > + char input[] = "There's no such thing as a free lunch.";
> > + input[G_N_ELEMENTS(input) - 1] = '!';
> > +
> > + test_base64_bad(input, G_N_ELEMENTS(input) - 1);
> > +}
> > +
> > +
> > +static void test_base64_invalid_chars(void)
> > +{
> > + const char *input = "There's no such thing as a free lunch.";
> > +
> > + test_base64_bad(input, strlen(input));
> > +}
>
> ...and this same input already fails because it doesn't match base64,
> regardless of whether NUL bytes are mishandled. I wonder if
> test_base64_embedded_nul() and test_base64_not_nul_terminated() should
> be using variations of your known-good base64 string
> ("QmVjYXVzZSB3ZSBmb2N1c2VkIG9uIHRoZSBzbmFrZSwgd2UgbW\n"
> "lzc2VkIHRoZSBzY29ycGlvbi4="), to prove that they are failing purely
> because of the NUL handling and not because of invalid base64 content.
That's a good idea - I'll respin with that an add more comments
about what we're testing for in each case.
> But that's only a weak complaint - because by peering into the black
> box, I see that you are fully testing all code paths (that is, the black
> box checks for NUL abuse prior to checking for valid base64 data), so
> I'm not going to insist on a respin.
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 :|
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/5] crypto: add QCryptoSecret object class for password/key handling
2015-12-08 16:49 ` Eric Blake
@ 2015-12-09 12:56 ` Daniel P. Berrange
0 siblings, 0 replies; 11+ messages in thread
From: Daniel P. Berrange @ 2015-12-09 12:56 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Markus Armbruster
On Tue, Dec 08, 2015 at 09:49:40AM -0700, Eric Blake wrote:
> On 11/27/2015 09:30 AM, Daniel P. Berrange wrote:
> > Introduce a new QCryptoSecret object class which will be used
> > for providing passwords and keys to other objects which need
> > sensitive credentials.
> >
>
> > More examples are shown in the updated docs.
> >
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
>
> > +++ b/crypto/secret.c
>
> > +static void
> > +qcrypto_secret_load_data(QCryptoSecret *secret,
> > + uint8_t **output,
> > + size_t *outputlen,
> > + Error **errp)
> > +{
>
> > + if (!g_file_get_contents(secret->file, &data, &length, &gerr)) {
> > + error_setg(errp,
> > + "Unable to read %s: %s",
> > + secret->file, gerr->message);
> > + g_error_free(gerr);
> > + return;
> > + }
> > + if (length) {
> > + /* Even though data is raw 8-bit, so may contain
> > + * arbitrary NULs, ensure it is explicitly NUL
> > + * terminated */
> > + *output = g_renew(uint8_t, data, length + 1);
> > + (*output)[length] = '\0';
>
> These two lines are dead code. g_file_get_contents() guarantees that on
> success, contents is malloc'd large enough and that contents[length] == 0.
>
> https://developer.gnome.org/glib/stable/glib-File-Utilities.html#g-file-get-contents
Yep, I just checked the source too and it matches the docs
>
> > + *outputlen = length;
> > + } else {
> > + error_setg(errp, "Secret file %s is empty",
> > + secret->file);
>
> Is there any technical reason why we must forbid a 0-length password?
> (Sometimes, having the empty string as a password can be a useful for
> development tests). I'm not opposed to rejecting it, especially if
> doing so now avoids a more cryptic error message later because there is
> indeed a technical reason; but just want to make sure it is not an
> arbitrary limitation.
It was fairly arbitrary, motivated by a desire to give an error
message if user accidentally pointed to a bad file, but based
on your point about empty passwords I'll remove this and leave
it upto the callers to decide if they'll accept empty passwords.
>
> > + g_free(data);
> > + }
> > + } else if (secret->data) {
> > + *outputlen = strlen(secret->data);
> > + *output = g_new(uint8_t, *outputlen + 1);
> > + memcpy(*output, secret->data, *outputlen + 1);
>
> These two lines could be shortened to:
> *output = g_strdup(secret->data);
Yes, indeed, I'll fix that.
>
> > +
> > +static void qcrypto_secret_decrypt(QCryptoSecret *secret,
> > + const uint8_t *input,
> > + size_t inputlen,
> > + uint8_t **output,
> > + size_t *outputlen,
> > + Error **errp)
> > +{
>
> > + if (secret->format == QCRYPTO_SECRET_FORMAT_BASE64) {
> > + ciphertext = qbase64_decode((const gchar*)input,
> > + inputlen,
> > + &ciphertextlen,
> > + errp);
> > + if (!ciphertext) {
> > + goto cleanup;
> > + }
> > + plaintext = g_new0(uint8_t, ciphertextlen + 1);
> > + } else {
> > + ciphertextlen = inputlen;
> > + plaintext = g_new0(uint8_t, inputlen + 1);
>
> g_new0(uint8_t, value) is the same as g_malloc0(value); I don't know if
> it is worth the distinction. But not worth a respin on its own.
I just prefer to always use g_new0 so it is explicit what type we're
using, though I wish glib's allocators worked like libvirt's so it
got the compiler to provide the correct type.
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 :|
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/5] util: add base64 decoding function
2015-12-09 12:50 ` Daniel P. Berrange
@ 2015-12-09 15:18 ` Eric Blake
0 siblings, 0 replies; 11+ messages in thread
From: Eric Blake @ 2015-12-09 15:18 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: qemu-devel, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 1454 bytes --]
On 12/09/2015 05:50 AM, Daniel P. Berrange wrote:
> On Tue, Dec 08, 2015 at 09:18:30AM -0700, Eric Blake wrote:
>> On 11/27/2015 09:30 AM, Daniel P. Berrange wrote:
>>> The standard glib provided g_base64_decode doesn't provide any
>>> kind of sensible error checking on its input. Add a QEMU custom
>>> wrapper qbase64_decode which can be used with untrustworthy
>>> input that can contain invalid base64 characters, embedded
>>> NUL characters, or not be NUL terminated at all.
>>>
>>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>>> ---
>>
>>> + uint8_t *actual = qbase64_decode(input,
>>> + input_len,
>>> + &len,
>>> + &err);
>>> +
>>> + g_assert(err != NULL);
>>
>> Could use &error_abort in the original call instead of a second check
>> for err != NULL; but that's cosmetic.
>
> Actually we can't use &error_abort, as we don't want the test
> function to abort on error as that would indicate test failure.
> We want to make sure the call completes & returns an error
> set without aborting.
Whoops; confusing it with g_assert(err == NULL). (Which is why I tend
to just do g_assert(err) or g_assert(!err) instead of having to think
about the comparison with NULL, oh well).
--
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 --]
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-12-09 15:18 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-11-27 16:30 [Qemu-devel] [PATCH v3 0/5] Add framework for passing secrets to QEMU Daniel P. Berrange
2015-11-27 16:30 ` [Qemu-devel] [PATCH v3 1/5] util: add base64 decoding function Daniel P. Berrange
2015-12-08 16:18 ` Eric Blake
2015-12-09 12:50 ` Daniel P. Berrange
2015-12-09 15:18 ` Eric Blake
2015-11-27 16:30 ` [Qemu-devel] [PATCH v3 2/5] qemu-char: convert to use error checked base64 decode Daniel P. Berrange
2015-11-27 16:30 ` [Qemu-devel] [PATCH v3 3/5] qga: " Daniel P. Berrange
2015-11-27 16:30 ` [Qemu-devel] [PATCH v3 4/5] crypto: add QCryptoSecret object class for password/key handling Daniel P. Berrange
2015-12-08 16:49 ` Eric Blake
2015-12-09 12:56 ` Daniel P. Berrange
2015-11-27 16:30 ` [Qemu-devel] [PATCH v3 5/5] crypto: add support for loading encrypted x509 keys Daniel P. Berrange
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).