From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50598) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a6LSX-00050C-S4 for qemu-devel@nongnu.org; Tue, 08 Dec 2015 11:49:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a6LSU-0000IL-Em for qemu-devel@nongnu.org; Tue, 08 Dec 2015 11:49:49 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34932) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a6LSU-0000I4-77 for qemu-devel@nongnu.org; Tue, 08 Dec 2015 11:49:46 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 81A6142E5CE for ; Tue, 8 Dec 2015 16:49:45 +0000 (UTC) References: <1448641837-1632-1-git-send-email-berrange@redhat.com> <1448641837-1632-5-git-send-email-berrange@redhat.com> From: Eric Blake Message-ID: <56670A24.8030302@redhat.com> Date: Tue, 8 Dec 2015 09:49:40 -0700 MIME-Version: 1.0 In-Reply-To: <1448641837-1632-5-git-send-email-berrange@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="JwvSB0B4BdD2lcGOJG9ki3au7nS9UvJJK" Subject: Re: [Qemu-devel] [PATCH v3 4/5] crypto: add QCryptoSecret object class for password/key handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" , qemu-devel@nongnu.org Cc: Markus Armbruster This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --JwvSB0B4BdD2lcGOJG9ki3au7nS9UvJJK Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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. >=20 > More examples are shown in the updated docs. >=20 > Signed-off-by: Daniel P. Berrange > --- > +++ 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 =3D g_renew(uint8_t, data, length + 1); > + (*output)[length] =3D '\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] =3D=3D= 0. https://developer.gnome.org/glib/stable/glib-File-Utilities.html#g-file-g= et-contents > + *outputlen =3D 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 =3D strlen(secret->data); > + *output =3D g_new(uint8_t, *outputlen + 1); > + memcpy(*output, secret->data, *outputlen + 1); These two lines could be shortened to: *output =3D 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 =3D=3D QCRYPTO_SECRET_FORMAT_BASE64) { > + ciphertext =3D qbase64_decode((const gchar*)input, > + inputlen, > + &ciphertextlen, > + errp); > + if (!ciphertext) { > + goto cleanup; > + } > + plaintext =3D g_new0(uint8_t, ciphertextlen + 1); > + } else { > + ciphertextlen =3D inputlen; > + plaintext =3D 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 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --JwvSB0B4BdD2lcGOJG9ki3au7nS9UvJJK Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWZwokAAoJEKeha0olJ0NqRJoH/3Z27FS+zutH1P90CmBrgTeu Ngg1bW+x3+WjfbpfusSFsUkiBuv5t86S4Dq/1kfnL0RoRHc/sLF2D/IgWFU04i8w xpkO8Kph5xGq/xLYhRLEzS3iQ7FFgE63/yo2JpRaf361oVteOoa1kFMalFGQliJ5 LtIk8aIktmurP/aOW1x6P2P8AwLqjrZsXE2tn22xUNA964jroDWVDiEaf+23i2g0 QoPubqXIin0GUUS7cYU2FH+FQm57toXRDiYTZl76q/oSTKC2sTjjgHqmikYnsLqt FeJTQEhlcgXKErwGa8bZqxgibVxniAOX1Pgyc+R2kRHOJKDY3nb6Wp+d0MiP0ug= =XsRO -----END PGP SIGNATURE----- --JwvSB0B4BdD2lcGOJG9ki3au7nS9UvJJK--