[libvirt] [PATCH] storage: Properly terminate secrets

Michal Privoznik posted 1 patch 5 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/e78549665ba4ce6cc2f999ac16b64a70d6ef3bb8.1534149729.git.mprivozn@redhat.com
Test syntax-check passed
src/storage/storage_backend_iscsi.c        | 5 +++++
src/storage/storage_backend_iscsi_direct.c | 5 +++++
2 files changed, 10 insertions(+)
[libvirt] [PATCH] storage: Properly terminate secrets
Posted by Michal Privoznik 5 years, 7 months ago
The virSecretGetSecretString() helper looks up a secret for given
pool and returns its value in @secret_value and its length in
@secret_value_size. However, the trailing '\0' is not included in
either of the variables. This is because usually the value of the
secret is passed to some encoder (usually base64 encoder) where
the trailing zero must not be accounted for.

However, in two places we actually want the string as we don't
process is any further.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---

I wonder if putting this realloc into virSecretGetSecretString() would
be a better fix or not. I mean, without changing @secret_size. Opinions?

 src/storage/storage_backend_iscsi.c        | 5 +++++
 src/storage/storage_backend_iscsi_direct.c | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
index 6242cd0fac..55fe47f5e1 100644
--- a/src/storage/storage_backend_iscsi.c
+++ b/src/storage/storage_backend_iscsi.c
@@ -303,6 +303,11 @@ virStorageBackendISCSISetAuth(const char *portal,
                                  &secret_value, &secret_size) < 0)
         goto cleanup;
 
+    if (VIR_REALLOC_N(secret_value, secret_size + 1) < 0)
+        goto cleanup;
+
+    secret_value[secret_size] = '\0';
+
     if (virISCSINodeUpdate(portal,
                            source->devices[0].path,
                            "node.session.auth.authmethod",
diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c
index 1624066e9c..0d7d6ba9c3 100644
--- a/src/storage/storage_backend_iscsi_direct.c
+++ b/src/storage/storage_backend_iscsi_direct.c
@@ -115,6 +115,11 @@ virStorageBackendISCSIDirectSetAuth(struct iscsi_context *iscsi,
                                  &secret_value, &secret_size) < 0)
         goto cleanup;
 
+    if (VIR_REALLOC_N(secret_value, secret_size + 1) < 0)
+        goto cleanup;
+
+    secret_value[secret_size] = '\0';
+
     if (iscsi_set_initiator_username_pwd(iscsi,
                                          authdef->username,
                                          (const char *)secret_value) < 0) {
-- 
2.16.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: Properly terminate secrets
Posted by Ján Tomko 5 years, 7 months ago
On Mon, Aug 13, 2018 at 10:44:20AM +0200, Michal Privoznik wrote:
>The virSecretGetSecretString() helper looks up a secret for given
>pool and returns its value in @secret_value and its length in
>@secret_value_size. However, the trailing '\0' is not included in
>either of the variables. This is because usually the value of the
>secret is passed to some encoder (usually base64 encoder) where
>the trailing zero must not be accounted for.
>
>However, in two places we actually want the string as we don't
>process is any further.

s/is/it/

>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
>
>I wonder if putting this realloc into virSecretGetSecretString() would
>be a better fix or not. I mean, without changing @secret_size. Opinions?
>

Relying on returning a null-terminated string from a function that also
returns its size would look odd to me.

> src/storage/storage_backend_iscsi.c        | 5 +++++
> src/storage/storage_backend_iscsi_direct.c | 5 +++++
> 2 files changed, 10 insertions(+)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list