8:48 a.m.
From: jiangjiacheng <jiangjiacheng(a)huawei.com>
*** BLURB HERE ***
jiangjiacheng (3):
conf: clean up memory containing secrets before freeing
remote: remove unnecessary return value and if branch
Fix some coding style issues
src/conf/domain_conf.c | 4 +++-
src/remote/remote_daemon.c | 6 +-----
src/remote/remote_daemon_config.c | 4 +---
src/remote/remote_daemon_config.h | 2 +-
src/rpc/virnetserverclient.c | 28 ++++++++++++++--------------
5 files changed, 20 insertions(+), 24 deletions(-)
--
2.33.0
8:48 a.m.
New subject: [PATCH 1/3] conf: clean up memory containing secrets before freeing
From: jiangjiacheng <jiangjiacheng(a)huawei.com>
The password may not be valid in the error branch, but for
higher security, it's better to clean up the memory before
freeing it.
Signed-off-by: jiangjiacheng <jiangjiacheng(a)huawei.com>
---
src/conf/domain_conf.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 970cc85ded..d456fd0067 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -60,6 +60,7 @@
#include "virdomainsnapshotobjlist.h"
#include "virdomaincheckpointobjlist.h"
#include "virutil.h"
+#include "virsecureerase.h"
#define VIR_FROM_THIS VIR_FROM_DOMAIN
@@ -10888,6 +10889,7 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot parse password validity time '%s',
expect YYYY-MM-DDTHH:MM:SS"),
validTo);
+ virSecureEraseString(def->passwd);
VIR_FREE(def->passwd);
return -1;
}
--
2.33.0
2:59 a.m.
New subject: [PATCH 1/3] conf: clean up memory containing secrets before freeing
On Tue, Sep 06, 2022 at 21:48:29 +0800, Jiacheng Jiang wrote:
From: jiangjiacheng <jiangjiacheng(a)huawei.com>
The password may not be valid in the error branch, but for
higher security, it's better to clean up the memory before
freeing it.
Signed-off-by: jiangjiacheng <jiangjiacheng(a)huawei.com>
---
src/conf/domain_conf.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 970cc85ded..d456fd0067 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -60,6 +60,7 @@
#include "virdomainsnapshotobjlist.h"
#include "virdomaincheckpointobjlist.h"
#include "virutil.h"
+#include "virsecureerase.h"
#define VIR_FROM_THIS VIR_FROM_DOMAIN
@@ -10888,6 +10889,7 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot parse password validity time '%s',
expect YYYY-MM-DDTHH:MM:SS"),
validTo);
+ virSecureEraseString(def->passwd);
VIR_FREE(def->passwd);
return -1;
}
In this form the patch is not very useful. When freeing a successfully
parsed definition the 'passwd' field isn't erased either.
In general it makes little sense to do this for the XML parser because
there's another instance in the XML parser structs which isn't erased
either and there are multiple other instances of the code where we don't
do that either.
This patch should not be merged.
4:54 a.m.
New subject: [PATCH 1/3] conf: clean up memory containing secrets before freeing
On 9/6/22 15:48, Jiacheng Jiang wrote:
From: jiangjiacheng <jiangjiacheng(a)huawei.com>
The password may not be valid in the error branch, but for
higher security, it's better to clean up the memory before
freeing it.
Signed-off-by: jiangjiacheng <jiangjiacheng(a)huawei.com>
---
src/conf/domain_conf.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 970cc85ded..d456fd0067 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -60,6 +60,7 @@
#include "virdomainsnapshotobjlist.h"
#include "virdomaincheckpointobjlist.h"
#include "virutil.h"
+#include "virsecureerase.h"
#define VIR_FROM_THIS VIR_FROM_DOMAIN
@@ -10888,6 +10889,7 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot parse password validity time '%s',
expect YYYY-MM-DDTHH:MM:SS"),
validTo);
+ virSecureEraseString(def->passwd);
VIR_FREE(def->passwd);
return -1;
}
There are other 'return -1' statements which leave
virDomainGraphicsAuthDef partially filled. Eventually, the error leads
to virDomainGraphicsDefFree() being called which in turn calls
virDomainGraphicsAuthDefClear() which does not call virSecureEraseString().
I wonder what we can do about it.
Michal
6:06 a.m.
New subject: [PATCH 1/3] conf: clean up memory containing secrets before freeing
On Wed, Sep 07, 2022 at 11:54:59 +0200, Michal Prívozník wrote:
On 9/6/22 15:48, Jiacheng Jiang wrote:
> From: jiangjiacheng <jiangjiacheng(a)huawei.com>
>
> The password may not be valid in the error branch, but for
> higher security, it's better to clean up the memory before
> freeing it.
>
> Signed-off-by: jiangjiacheng <jiangjiacheng(a)huawei.com>
> ---
> src/conf/domain_conf.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 970cc85ded..d456fd0067 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -60,6 +60,7 @@
> #include "virdomainsnapshotobjlist.h"
> #include "virdomaincheckpointobjlist.h"
> #include "virutil.h"
> +#include "virsecureerase.h"
>
> #define VIR_FROM_THIS VIR_FROM_DOMAIN
>
> @@ -10888,6 +10889,7 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("cannot parse password validity time
'%s', expect YYYY-MM-DDTHH:MM:SS"),
> validTo);
> + virSecureEraseString(def->passwd);
> VIR_FREE(def->passwd);
> return -1;
> }
There are other 'return -1' statements which leave
virDomainGraphicsAuthDef partially filled. Eventually, the error leads
to virDomainGraphicsDefFree() being called which in turn calls
virDomainGraphicsAuthDefClear() which does not call virSecureEraseString().
I wonder what we can do about it.
As noted in my reply you've got another copy in the XML parser and yet
another copy in the string that holds the full XML before it was parsed.
Trying to sanitize secrets for anything XML related is pointless.
Even the one piece of code which supposedly seems to make sense
(fetching and encrypting disk secrets) where we sanitize the
un-encrypted secret we got from the secret driver doesn't actually
sanitize the RPC buffers which were used to communicate with the secret
daemon.
8:48 a.m.
New subject: [PATCH 2/3] remote: remove unnecessary return value and if branch
From: jiangjiacheng <jiangjiacheng(a)huawei.com>
Function daemonConfigFilePath() will assign a path to
remote_config_file definitely and the path will be validated
in following codes. So, it's unnecessary to return value
from daemonConfigFilePath() and check the returned value.
Signed-off-by: jiangjiacheng <jiangjiacheng(a)huawei.com>
---
src/remote/remote_daemon.c | 6 +-----
src/remote/remote_daemon_config.c | 4 +---
src/remote/remote_daemon_config.h | 2 +-
3 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index 23a5eeb200..36d95de83d 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -903,11 +903,7 @@ int main(int argc, char **argv) {
/* No explicit config, so try and find a default one */
if (remote_config_file == NULL) {
implicit_conf = true;
- if (daemonConfigFilePath(privileged,
- &remote_config_file) < 0) {
- VIR_ERROR(_("Can't determine config path"));
- exit(EXIT_FAILURE);
- }
+ daemonConfigFilePath(privileged, &remote_config_file);
}
/* Read the config file if it exists */
diff --git a/src/remote/remote_daemon_config.c b/src/remote/remote_daemon_config.c
index 330db54651..3567e337c4 100644
--- a/src/remote/remote_daemon_config.c
+++ b/src/remote/remote_daemon_config.c
@@ -72,7 +72,7 @@ remoteConfigGetAuth(virConf *conf,
return 0;
}
-int
+void
daemonConfigFilePath(bool privileged, char **configfile)
{
if (privileged) {
@@ -84,8 +84,6 @@ daemonConfigFilePath(bool privileged, char **configfile)
*configfile = g_strdup_printf("%s/%s.conf", configdir, DAEMON_NAME);
}
-
- return 0;
}
struct daemonConfig*
diff --git a/src/remote/remote_daemon_config.h b/src/remote/remote_daemon_config.h
index 47839271d3..9f9e54e838 100644
--- a/src/remote/remote_daemon_config.h
+++ b/src/remote/remote_daemon_config.h
@@ -99,7 +99,7 @@ struct daemonConfig {
};
-int daemonConfigFilePath(bool privileged, char **configfile);
+void daemonConfigFilePath(bool privileged, char **configfile);
struct daemonConfig* daemonConfigNew(bool privileged);
void daemonConfigFree(struct daemonConfig *data);
int daemonConfigLoadFile(struct daemonConfig *data,
--
2.33.0
8:48 a.m.
New subject: [PATCH 3/3] Fix some coding style issues
From: jiangjiacheng <jiangjiacheng(a)huawei.com>
Fix some coding style issues with alignment and spaces.
Signed-off-by: jiangjiacheng <jiangjiacheng(a)huawei.com>
---
src/conf/domain_conf.c | 2 +-
src/rpc/virnetserverclient.c | 28 ++++++++++++++--------------
2 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d456fd0067..c904d9c63d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -26234,7 +26234,7 @@ virDomainMemorytuneDefFormat(virBuffer *buf,
&childrenBuf) < 0)
return -1;
- for (i = 0; i< resctrl->nmonitors; i++) {
+ for (i = 0; i < resctrl->nmonitors; i++) {
if (virDomainResctrlMonDefFormatHelper(resctrl->monitors[i],
VIR_RESCTRL_MONITOR_TYPE_MEMBW,
&childrenBuf) < 0)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index d57ca07167..a7d2dfa795 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -416,15 +416,15 @@ virNetServerClientNewInternal(unsigned long long id,
virNetServerClient *virNetServerClientNew(unsigned long long id,
- virNetSocket *sock,
- int auth,
- bool readonly,
- size_t nrequests_max,
- virNetTLSContext *tls,
- virNetServerClientPrivNew privNew,
- virNetServerClientPrivPreExecRestart
privPreExecRestart,
- virFreeCallback privFree,
- void *privOpaque)
+ virNetSocket *sock,
+ int auth,
+ bool readonly,
+ size_t nrequests_max,
+ virNetTLSContext *tls,
+ virNetServerClientPrivNew privNew,
+ virNetServerClientPrivPreExecRestart
privPreExecRestart,
+ virFreeCallback privFree,
+ void *privOpaque)
{
virNetServerClient *client;
time_t now;
@@ -454,11 +454,11 @@ virNetServerClient *virNetServerClientNew(unsigned long long id,
virNetServerClient *virNetServerClientNewPostExecRestart(virNetServer *srv,
- virJSONValue *object,
-
virNetServerClientPrivNewPostExecRestart privNew,
-
virNetServerClientPrivPreExecRestart privPreExecRestart,
- virFreeCallback privFree,
- void *privOpaque)
+ virJSONValue *object,
+
virNetServerClientPrivNewPostExecRestart privNew,
+
virNetServerClientPrivPreExecRestart privPreExecRestart,
+ virFreeCallback privFree,
+ void *privOpaque)
{
virJSONValue *child;
virNetServerClient *client = NULL;
--
2.33.0
11:26 a.m.
On Tue, Sep 6, 2022 at 4:14 PM Jiacheng Jiang <jiangjiacheng(a)huawei.com>
wrote:
From: jiangjiacheng <jiangjiacheng(a)huawei.com>
*** BLURB HERE ***
jiangjiacheng (3):
conf: clean up memory containing secrets before freeing
remote: remove unnecessary return value and if branch
Fix some coding style issues
src/conf/domain_conf.c | 4 +++-
src/remote/remote_daemon.c | 6 +-----
src/remote/remote_daemon_config.c | 4 +---
src/remote/remote_daemon_config.h | 2 +-
src/rpc/virnetserverclient.c | 28 ++++++++++++++--------------
5 files changed, 20 insertions(+), 24 deletions(-)
Reviewed-by: Kristina Hanicova <khanicov(a)redhat.com>
Kristina
4:54 a.m.
On 9/6/22 18:26, Kristina Hanicova wrote:
On Tue, Sep 6, 2022 at 4:14 PM Jiacheng Jiang <jiangjiacheng(a)huawei.com
<mailto:jiangjiacheng@huawei.com>> wrote:
From: jiangjiacheng <jiangjiacheng(a)huawei.com
<mailto:jiangjiacheng@huawei.com>>
*** BLURB HERE ***
jiangjiacheng (3):
conf: clean up memory containing secrets before freeing
remote: remove unnecessary return value and if branch
Fix some coding style issues
src/conf/domain_conf.c | 4 +++-
src/remote/remote_daemon.c | 6 +-----
src/remote/remote_daemon_config.c | 4 +---
src/remote/remote_daemon_config.h | 2 +-
src/rpc/virnetserverclient.c | 28 ++++++++++++++--------------
5 files changed, 20 insertions(+), 24 deletions(-)
Reviewed-by: Kristina Hanicova <khanicov(a)redhat.com
<mailto:khanicov@redhat.com>>
Kristina
Merged now.
Michal
799
days inactive
800
days old
8 comments
4 participants
participants (4)
-
Jiacheng Jiang -
Kristina Hanicova -
Michal Prívozník -
Peter Krempa