[PATCH 0/3] Fix some coding issues
From: jiangjiacheng <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(-) -- 2.33.0
From: jiangjiacheng <jiangjiacheng@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@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
On Tue, Sep 06, 2022 at 21:48:29 +0800, Jiacheng Jiang wrote:
From: jiangjiacheng <jiangjiacheng@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@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.
On 9/6/22 15:48, Jiacheng Jiang wrote:
From: jiangjiacheng <jiangjiacheng@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@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
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@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@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.
From: jiangjiacheng <jiangjiacheng@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@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
From: jiangjiacheng <jiangjiacheng@huawei.com> Fix some coding style issues with alignment and spaces. Signed-off-by: jiangjiacheng <jiangjiacheng@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
On Tue, Sep 6, 2022 at 4:14 PM Jiacheng Jiang <jiangjiacheng@huawei.com> wrote:
From: jiangjiacheng <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@redhat.com> Kristina
On 9/6/22 18:26, Kristina Hanicova wrote:
On Tue, Sep 6, 2022 at 4:14 PM Jiacheng Jiang <jiangjiacheng@huawei.com <mailto:jiangjiacheng@huawei.com>> wrote:
From: jiangjiacheng <jiangjiacheng@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@redhat.com <mailto:khanicov@redhat.com>>
Kristina
Merged now. Michal
participants (4)
-
Jiacheng Jiang -
Kristina Hanicova -
Michal Prívozník -
Peter Krempa