[PATCH 0/7] src: hardening of dnsmasq config file hostname handling
Daniel P. Berrangé (7): src: add virHostnameValidate helper for validation conf: perform full validation of hostnames / domain names src: remove fallback when opening dnsmasq addnhosts file fails src: use virFileRewrite for dnsmasq addnhosts file src: remove fallback when opening dnsmasq hosts file fails src: use virFileRewrite for dnsmasq hosts file util: sanity check dnsmasq hosts/addnhosts lines for no whitespace src/conf/network_conf.c | 31 ++- src/libvirt_private.syms | 1 + src/util/virdnsmasq.c | 179 +++++++----------- src/util/virutil.c | 50 +++++ src/util/virutil.h | 2 + .../nat-network-bad-hostname.xml | 22 +++ .../nat-network-dhcp-hosts-bad.xml | 21 ++ .../nat-network-dns-hosts-bad.xml | 26 +++ tests/networkxmlconftest.c | 3 + tests/utiltest.c | 72 +++++++ tests/virschematest.c | 3 + 11 files changed, 286 insertions(+), 124 deletions(-) create mode 100644 tests/networkxmlconfdata/nat-network-bad-hostname.xml create mode 100644 tests/networkxmlconfdata/nat-network-dhcp-hosts-bad.xml create mode 100644 tests/networkxmlconfdata/nat-network-dns-hosts-bad.xml -- 2.56.0
From: Daniel P. Berrangé <berrange@redhat.com> Hostnames must comply with certain rules about maximum length and permitted characters. Since these need to be validated in several places, introduce a helper to centralize the rules and provide testing. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virutil.c | 50 ++++++++++++++++++++++++++++ src/util/virutil.h | 2 ++ tests/utiltest.c | 72 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 125 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 798d2d28c8..66520a3e35 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3791,6 +3791,7 @@ virGetUserRuntimeDirectory; virGetUserShell; virHostGetDRMRenderNode; virHostHasIOMMU; +virHostnameValidate; virIndexToDiskName; virKernelCmdlineMatchParam; virKernelCmdlineNextParam; diff --git a/src/util/virutil.c b/src/util/virutil.c index 2be9a460ac..75ecb6da5a 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1979,3 +1979,53 @@ virPipeNonBlock(int fds[2]) { return virPipeImpl(fds, true, true); } + +#define VIR_HOSTNAME_VALID_CHARS \ + "abcdefghijklmnopqrstuvwxyz" \ + "ABCDEFGHIJKLMNOPQRSTUVWXYZ" \ + "0123456789-" + +int virHostnameValidate(const char *hostname, bool qualified) +{ + g_auto(GStrv) labels = g_strsplit(hostname, ".", -1); + size_t i; + + if (!qualified && g_strv_length(labels) > 1) { + virReportError(VIR_ERR_INVALID_ARG, + _("Hostname '%1$s' must not be qualified"), + hostname); + return -1; + } + if (strlen(hostname) > 253) { + virReportError(VIR_ERR_INVALID_ARG, + _("Hostname '%1$s' must be 253 characters or less"), + hostname); + return -1; + } + + for (i = 0; labels[i] != NULL; i++) { + size_t labellen = strlen(labels[i]); + if (labellen == 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Hostname '%1$s' components must be non-empty"), + hostname); + return -1; + } + + if (labellen > 63) { + virReportError(VIR_ERR_INVALID_ARG, + _("Hostname '%1$s' components must be 63 characters or less"), + hostname); + return -1; + } + + if (strspn(labels[i], + VIR_HOSTNAME_VALID_CHARS) != labellen) { + virReportError(VIR_ERR_INVALID_ARG, + _("Hostname '%1$s' components must only contain a-z, A-Z, 0-9 and -"), + hostname); + return -1; + } + } + return 0; +} diff --git a/src/util/virutil.h b/src/util/virutil.h index 5678c339ba..b2718fa1df 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -230,3 +230,5 @@ int virPipeQuiet(int fds[2]); * Returns: -1 on error, 0 on success */ int virPipeNonBlock(int fds[2]); + +int virHostnameValidate(const char *hostname, bool qualified); diff --git a/tests/utiltest.c b/tests/utiltest.c index 0bef502a84..7e14c0a161 100644 --- a/tests/utiltest.c +++ b/tests/utiltest.c @@ -423,6 +423,77 @@ testGetSubIDs(const void *data G_GNUC_UNUSED) } +struct testHostnamesData +{ + const char *name; + bool qualified; + bool valid; +}; + +static const struct testHostnamesData testHostnamesData[] = { + { .name = "foo", .qualified = false, .valid = true }, + { .name = "foo", .qualified = true, .valid = true }, + { .name = "foo.example.com", .qualified = false, .valid = false }, + { .name = "foo.example.com", .qualified = true, .valid = true }, + { .name = "foo ", .qualified = false, .valid = false }, + { .name = "foo\n", .qualified = false, .valid = false }, + { .name = "foo\t", .qualified = false, .valid = false }, + { .name = "0foo", .qualified = false, .valid = true }, + { .name = "foo\n", .qualified = false, .valid = false }, + { .name = "foo.example.com\t", .qualified = true, .valid = false }, + { .name = ".foo", .qualified = false, .valid = false }, + { .name = "foo.", .qualified = false, .valid = false }, + { .name = "foo.example.com.", .qualified = false, .valid = false }, + { .name = "foo..example.com", .qualified = false, .valid = false }, + { .name = "foo1234567foo1234567foo1234567foo1234567foo1234567foo1234567abc", + .qualified = false, .valid = true }, + { .name = "foo1234567foo1234567foo1234567foo1234567foo1234567foo1234567abcd", + .qualified = false, .valid = false }, + { .name = + "foo1234567foo1234567foo1234567foo1234567foo123456." + "foo1234567foo1234567foo1234567foo1234567foo123456." + "foo1234567foo1234567foo1234567foo1234567foo123456." + "foo1234567foo1234567foo1234567foo1234567foo123456." + "foo1234567foo1234567foo1234567foo1234567foo123456." + "abc", + .qualified = true, .valid = true }, + { .name = + "foo1234567foo1234567foo1234567foo1234567foo123456." + "foo1234567foo1234567foo1234567foo1234567foo123456." + "foo1234567foo1234567foo1234567foo1234567foo123456." + "foo1234567foo1234567foo1234567foo1234567foo123456." + "foo1234567foo1234567foo1234567foo1234567foo123456." + "abcd", + .qualified = true, .valid = false }, +}; + + +static int +testHostnames(const void *data G_GNUC_UNUSED) +{ + size_t i; + for (i = 0; i < G_N_ELEMENTS(testHostnamesData); i++) { + int ret = virHostnameValidate(testHostnamesData[i].name, + testHostnamesData[i].qualified); + if (ret < 0) { + if (testHostnamesData[i].valid) { + g_printerr("Hostname '%s' should be valid: %s", + testHostnamesData[i].name, + virGetLastErrorMessage()); + return -1; + } + } else { + if (!testHostnamesData[i].valid) { + g_printerr("Hostname '%s' should be invalid", + testHostnamesData[i].name); + return -1; + } + } + } + return 0; +} + + static int mymain(void) { @@ -447,6 +518,7 @@ mymain(void) DO_TEST(KernelCmdlineNextParam); DO_TEST(KernelCmdlineMatchParam); DO_TEST(GetSubIDs); + DO_TEST(Hostnames); return result == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.56.0
On 10/26/26 8:18 AM, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
Hostnames must comply with certain rules about maximum length and permitted characters. Since these need to be validated in several places, introduce a helper to centralize the rules and provide testing.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virutil.c | 50 ++++++++++++++++++++++++++++ src/util/virutil.h | 2 ++ tests/utiltest.c | 72 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 125 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 798d2d28c8..66520a3e35 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3791,6 +3791,7 @@ virGetUserRuntimeDirectory; virGetUserShell; virHostGetDRMRenderNode; virHostHasIOMMU; +virHostnameValidate; virIndexToDiskName; virKernelCmdlineMatchParam; virKernelCmdlineNextParam; diff --git a/src/util/virutil.c b/src/util/virutil.c index 2be9a460ac..75ecb6da5a 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1979,3 +1979,53 @@ virPipeNonBlock(int fds[2]) { return virPipeImpl(fds, true, true); } + +#define VIR_HOSTNAME_VALID_CHARS \ + "abcdefghijklmnopqrstuvwxyz" \ + "ABCDEFGHIJKLMNOPQRSTUVWXYZ" \ + "0123456789-" + +int virHostnameValidate(const char *hostname, bool qualified) +{ + g_auto(GStrv) labels = g_strsplit(hostname, ".", -1); + size_t i; + + if (!qualified && g_strv_length(labels) > 1) { + virReportError(VIR_ERR_INVALID_ARG, + _("Hostname '%1$s' must not be qualified"), + hostname); + return -1; + } + if (strlen(hostname) > 253) { + virReportError(VIR_ERR_INVALID_ARG, + _("Hostname '%1$s' must be 253 characters or less"), + hostname); + return -1; + } + + for (i = 0; labels[i] != NULL; i++) { + size_t labellen = strlen(labels[i]); + if (labellen == 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Hostname '%1$s' components must be non-empty"), + hostname); + return -1; + } + + if (labellen > 63) { + virReportError(VIR_ERR_INVALID_ARG, + _("Hostname '%1$s' components must be 63 characters or less"), + hostname); + return -1; + } + + if (strspn(labels[i], + VIR_HOSTNAME_VALID_CHARS) != labellen) { + virReportError(VIR_ERR_INVALID_ARG, + _("Hostname '%1$s' components must only contain a-z, A-Z, 0-9 and -"), + hostname); + return -1; + }
1) Additionally, although a hostname is now allowed to start with a number (originally it wasn't), it cannot start or end with a "-" (more specifically, each individual label cannot start or end with a "-". 2) In general, a domainname's octets can't be interpreted or limited by a DNS server; that is declared to be the responsibility of the application that is actually using the information. (e.g. a DNS A or AAAA record key can completely legitimately contain a "_", or any other character and the DNS server is supposed to accept that and forward it on or whatever). It's only when an application decides to use a domainname. And actually I've been reading RFC2181 for the first time and section 11 says "any binary string whatever can be used as the label of any resource record" (after first listing the length limitations you've checked for above). It also says "DNS server must not refuse to serve a zone because it contains labels that might not be acceptable to some DNS client programs" (oddly the "must" isn't capitalized, which has always had a special meaning in RFCs (at least to me). (https://www.rfc-editor.org/info/rfc2181/#section-11). So technically we *could* allow anything in a hostname and rely on the receiving client to validate. Not that I'm suggesting that, I just found it interesting that was the case :-) 3) I was curious about what happens with hostnames/domain names containing international characters. I've known for several years that it's now supported, but never looked at the details before. This is my chance! So, on the wire, only the characters you've specified are supported (in a hostname), and any character beyond 0x80 will cause the name to be translated into "Punycode"[*], which I won't try to describe fully, but it ends up with a string that starts with "xn--" and then does a special encoding of the source string that I'll let you look up for yourself. We *could* just say that libvirt only allows using the Punycode-encoded version of international hostnames in its config files (and thus your validation would remain the same, but with addition of checking leading and trailing "-"), but dnsmasq actually has support for international characters in hostnames and domainnames in its config files, and will automatically translate any hostname/domainname with non-ascii characters to Punycode before putting it on the wire. And certainly it would be much easier to recognize: mötleycrüe.com in the config file rather than xn-mtleycre-n4a5pc.com :-P. (and possibly there are even already people doing this in their libvirt config files, so adding the validation as you've written it could break someone's setup.) A qualifier - it's possible to compile this support out of dnsmasq to save space, and in that case dnsmasq *should* flag an error if it encounters any international characters. A further qualifier - you can check if the dnsmasq on the current host has this support by looking for "IDN" or "IDN2" in the output of "dnsmasq -v", so we could conditionally allow international characters based on that flag. Or we could just always translate to Punycode ourselves. g_hostname_to_ascii() does that (glib has several functions to handle hostnames with international characters, including validation; along with g_hostname_to_ascii(), there is also g_hostname_is_non_ascii(), g_hostname_is_ascii_encoded() (I would include a link to a webpage with all the related functions listed together, but it ends in .php and I don't want to be the reason your laptop gets PHPwned :-P). I think using these functions would also allow you to get rid of the opencoded VIR_HOSTNAME_VALID_CHARS, and possibly even the length checks and leading/trailing "-" checks) In the end, ACK if check for leading/trailing "-" is added. But we might want to be more inclusive about international characters, and it could be possible to replace the opencoded stuff with appropriate g_hostname_*() calls.
+ } + return 0; +} diff --git a/src/util/virutil.h b/src/util/virutil.h index 5678c339ba..b2718fa1df 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -230,3 +230,5 @@ int virPipeQuiet(int fds[2]); * Returns: -1 on error, 0 on success */ int virPipeNonBlock(int fds[2]); + +int virHostnameValidate(const char *hostname, bool qualified); diff --git a/tests/utiltest.c b/tests/utiltest.c index 0bef502a84..7e14c0a161 100644 --- a/tests/utiltest.c +++ b/tests/utiltest.c @@ -423,6 +423,77 @@ testGetSubIDs(const void *data G_GNUC_UNUSED) }
+struct testHostnamesData +{ + const char *name; + bool qualified; + bool valid; +}; + +static const struct testHostnamesData testHostnamesData[] = { + { .name = "foo", .qualified = false, .valid = true }, + { .name = "foo", .qualified = true, .valid = true }, + { .name = "foo.example.com", .qualified = false, .valid = false }, + { .name = "foo.example.com", .qualified = true, .valid = true }, + { .name = "foo ", .qualified = false, .valid = false }, + { .name = "foo\n", .qualified = false, .valid = false }, + { .name = "foo\t", .qualified = false, .valid = false }, + { .name = "0foo", .qualified = false, .valid = true }, + { .name = "foo\n", .qualified = false, .valid = false }, + { .name = "foo.example.com\t", .qualified = true, .valid = false }, + { .name = ".foo", .qualified = false, .valid = false }, + { .name = "foo.", .qualified = false, .valid = false }, + { .name = "foo.example.com.", .qualified = false, .valid = false }, + { .name = "foo..example.com", .qualified = false, .valid = false }, + { .name = "foo1234567foo1234567foo1234567foo1234567foo1234567foo1234567abc", + .qualified = false, .valid = true }, + { .name = "foo1234567foo1234567foo1234567foo1234567foo1234567foo1234567abcd", + .qualified = false, .valid = false }, + { .name = + "foo1234567foo1234567foo1234567foo1234567foo123456." + "foo1234567foo1234567foo1234567foo1234567foo123456." + "foo1234567foo1234567foo1234567foo1234567foo123456." + "foo1234567foo1234567foo1234567foo1234567foo123456." + "foo1234567foo1234567foo1234567foo1234567foo123456." + "abc", + .qualified = true, .valid = true }, + { .name = + "foo1234567foo1234567foo1234567foo1234567foo123456." + "foo1234567foo1234567foo1234567foo1234567foo123456." + "foo1234567foo1234567foo1234567foo1234567foo123456." + "foo1234567foo1234567foo1234567foo1234567foo123456." + "foo1234567foo1234567foo1234567foo1234567foo123456." + "abcd", + .qualified = true, .valid = false }, +}; + + +static int +testHostnames(const void *data G_GNUC_UNUSED) +{ + size_t i; + for (i = 0; i < G_N_ELEMENTS(testHostnamesData); i++) { + int ret = virHostnameValidate(testHostnamesData[i].name, + testHostnamesData[i].qualified); + if (ret < 0) { + if (testHostnamesData[i].valid) { + g_printerr("Hostname '%s' should be valid: %s", + testHostnamesData[i].name, + virGetLastErrorMessage()); + return -1; + } + } else { + if (!testHostnamesData[i].valid) { + g_printerr("Hostname '%s' should be invalid", + testHostnamesData[i].name); + return -1; + } + } + } + return 0; +} + + static int mymain(void) { @@ -447,6 +518,7 @@ mymain(void) DO_TEST(KernelCmdlineNextParam); DO_TEST(KernelCmdlineMatchParam); DO_TEST(GetSubIDs); + DO_TEST(Hostnames);
return result == 0 ? EXIT_SUCCESS : EXIT_FAILURE; }
On Thu, Oct 29, 2026 at 11:58:14PM -0400, Laine Stump wrote:
On 10/26/26 8:18 AM, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
Hostnames must comply with certain rules about maximum length and permitted characters. Since these need to be validated in several places, introduce a helper to centralize the rules and provide testing.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virutil.c | 50 ++++++++++++++++++++++++++++ src/util/virutil.h | 2 ++ tests/utiltest.c | 72 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 125 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 798d2d28c8..66520a3e35 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3791,6 +3791,7 @@ virGetUserRuntimeDirectory; virGetUserShell; virHostGetDRMRenderNode; virHostHasIOMMU; +virHostnameValidate; virIndexToDiskName; virKernelCmdlineMatchParam; virKernelCmdlineNextParam; diff --git a/src/util/virutil.c b/src/util/virutil.c index 2be9a460ac..75ecb6da5a 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1979,3 +1979,53 @@ virPipeNonBlock(int fds[2]) { return virPipeImpl(fds, true, true); } + +#define VIR_HOSTNAME_VALID_CHARS \ + "abcdefghijklmnopqrstuvwxyz" \ + "ABCDEFGHIJKLMNOPQRSTUVWXYZ" \ + "0123456789-" + +int virHostnameValidate(const char *hostname, bool qualified) +{ + g_auto(GStrv) labels = g_strsplit(hostname, ".", -1); + size_t i; + + if (!qualified && g_strv_length(labels) > 1) { + virReportError(VIR_ERR_INVALID_ARG, + _("Hostname '%1$s' must not be qualified"), + hostname); + return -1; + } + if (strlen(hostname) > 253) { + virReportError(VIR_ERR_INVALID_ARG, + _("Hostname '%1$s' must be 253 characters or less"), + hostname); + return -1; + } + + for (i = 0; labels[i] != NULL; i++) { + size_t labellen = strlen(labels[i]); + if (labellen == 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Hostname '%1$s' components must be non-empty"), + hostname); + return -1; + } + + if (labellen > 63) { + virReportError(VIR_ERR_INVALID_ARG, + _("Hostname '%1$s' components must be 63 characters or less"), + hostname); + return -1; + } + + if (strspn(labels[i], + VIR_HOSTNAME_VALID_CHARS) != labellen) { + virReportError(VIR_ERR_INVALID_ARG, + _("Hostname '%1$s' components must only contain a-z, A-Z, 0-9 and -"), + hostname); + return -1; + }
1) Additionally, although a hostname is now allowed to start with a number (originally it wasn't), it cannot start or end with a "-" (more specifically, each individual label cannot start or end with a "-".
Ok, we need the '-' validation. Also for TXT/SRV records I need to allow a leading '_'
2) In general, a domainname's octets can't be interpreted or limited by a DNS server; that is declared to be the responsibility of the application that is actually using the information. (e.g. a DNS A or AAAA record key can completely legitimately contain a "_", or any other character and the DNS server is supposed to accept that and forward it on or whatever). It's only when an application decides to use a domainname.
And actually I've been reading RFC2181 for the first time and section 11 says "any binary string whatever can be used as the label of any resource record" (after first listing the length limitations you've checked for above). It also says "DNS server must not refuse to serve a zone because it contains labels that might not be acceptable to some DNS client programs" (oddly the "must" isn't capitalized, which has always had a special meaning in RFCs (at least to me). (https://www.rfc-editor.org/info/rfc2181/#section-11). So technically we *could* allow anything in a hostname and rely on the receiving client to validate. Not that I'm suggesting that, I just found it interesting that was the case :-)
3) I was curious about what happens with hostnames/domain names containing international characters. I've known for several years that it's now supported, but never looked at the details before. This is my chance!
So, on the wire, only the characters you've specified are supported (in a hostname), and any character beyond 0x80 will cause the name to be translated into "Punycode"[*], which I won't try to describe fully, but it ends up with a string that starts with "xn--" and then does a special encoding of the source string that I'll let you look up for yourself.
We *could* just say that libvirt only allows using the Punycode-encoded version of international hostnames in its config files (and thus your validation would remain the same, but with addition of checking leading and trailing "-"), but dnsmasq actually has support for international characters in hostnames and domainnames in its config files, and will automatically translate any hostname/domainname with non-ascii characters to Punycode before putting it on the wire. And certainly it would be much easier to recognize:
mötleycrüe.com
in the config file rather than
xn-mtleycre-n4a5pc.com
:-P. (and possibly there are even already people doing this in their libvirt config files, so adding the validation as you've written it could break someone's setup.)
Yes, you're right, we must not restrict what is already permitted by dnsmasq. What matters is that we don't break the config file format ultimately. I guess I need to throw most of this away and just allow anything that is not whitespace, or control characters. With regards, Daniel -- |: https://berrange.com ~~ https://hachyderm.io/@berrange :| |: https://libvirt.org ~~ https://entangle-photo.org :| |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
From: Daniel P. Berrangé <berrange@redhat.com> The current network XML parser only validates that a DHCP hostname starts with a-Z character or a digit, or that a TXT record name doesn't contain space or comma. This is way too weak, and misses validation from other parts of the schema. This can allow invalid characters that result in invalid syntax in dnsmasq config files. Use the new hostname validation API to check all places that accept a hostname or domain name as a hardening task to prevent accidental creation of malformed dnsmasq cnofig files. Fixes: https://gitlab.com/libvirt/libvirt/-/work_items/929 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/network_conf.c | 31 +++++++++---------- .../nat-network-bad-hostname.xml | 22 +++++++++++++ .../nat-network-dhcp-hosts-bad.xml | 21 +++++++++++++ .../nat-network-dns-hosts-bad.xml | 26 ++++++++++++++++ tests/networkxmlconftest.c | 3 ++ tests/virschematest.c | 3 ++ 6 files changed, 90 insertions(+), 16 deletions(-) create mode 100644 tests/networkxmlconfdata/nat-network-bad-hostname.xml create mode 100644 tests/networkxmlconfdata/nat-network-dhcp-hosts-bad.xml create mode 100644 tests/networkxmlconfdata/nat-network-dns-hosts-bad.xml diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 8eba1321d6..2fec4d2b91 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -35,6 +35,7 @@ #include "viruuid.h" #include "virbuffer.h" #include "virstring.h" +#include "virutil.h" #define VIR_FROM_THIS VIR_FROM_NETWORK @@ -533,10 +534,7 @@ virNetworkDHCPHostDefParseXML(const char *networkName, } name = virXMLPropString(node, "name"); - if (name && !(g_ascii_isalpha(name[0]) || g_ascii_isdigit(name[0]))) { - virReportError(VIR_ERR_XML_ERROR, - _("Cannot use host name '%1$s' in network '%2$s'"), - name, networkName); + if (name && virHostnameValidate(name, true) < 0) { return -1; } @@ -664,10 +662,7 @@ virNetworkDNSHostDefParseXML(const char *networkName, if (!(hostnames[i] = virXMLNodeContentString(g_ptr_array_index(hostnameNodes, i)))) return -1; - if (*hostnames[i] == '\0') { - virReportError(VIR_ERR_XML_DETAIL, - _("Missing hostname in network '%1$s' DNS HOST record"), - networkName); + if (virHostnameValidate(hostnames[i], true) < 0) { return -1; } } @@ -792,8 +787,11 @@ virNetworkDNSSrvDefParseXML(const char *networkName, def->domain = virXMLPropString(node, "domain"); def->target = virXMLPropString(node, "target"); - if (virNetworkDNSDefCheckLineBreaks("SRV", "domain", def->domain) < 0 || - virNetworkDNSDefCheckLineBreaks("SRV", "target", def->target) < 0) + if (def->domain && + virHostnameValidate(def->domain, true) < 0) + goto error; + if (def->target && STRNEQ(def->target, ".") && + virHostnameValidate(def->target, true) < 0) goto error; ret = virXPathUInt("string(./@port)", ctxt, &def->port); @@ -852,18 +850,13 @@ virNetworkDNSTxtDefParseXML(const char *networkName, virNetworkDNSTxtDef *def, bool partialOkay) { - const char *bad = " ,"; - if (!(def->name = virXMLPropString(node, "name"))) { virReportError(VIR_ERR_XML_DETAIL, _("missing required name attribute in DNS TXT record of network %1$s"), networkName); goto error; } - if (strcspn(def->name, bad) != strlen(def->name)) { - virReportError(VIR_ERR_XML_DETAIL, - _("prohibited character in DNS TXT record name '%1$s' of network %2$s"), - def->name, networkName); + if (virHostnameValidate(def->name, true) < 0) { goto error; } if (!(def->value = virXMLPropString(node, "value")) && !partialOkay) { @@ -958,6 +951,9 @@ virNetworkDNSDefParseXML(const char *networkName, _("Invalid forwarder element, must contain at least one of addr or domain")); return -1; } + if (def->forwarders[i].domain && + virHostnameValidate(def->forwarders[i].domain, true) < 0) + return -1; def->nfwds++; } } @@ -1621,6 +1617,9 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt, /* Parse network domain information */ domain_node = virXPathNode("./domain[1]", ctxt); def->domain = virXMLPropString(domain_node, "name"); + if (def->domain && + virHostnameValidate(def->domain, true) < 0) + return NULL; if (virXMLPropTristateBool(domain_node, "localOnly", VIR_XML_PROP_NONE, &def->domainLocalOnly) < 0) diff --git a/tests/networkxmlconfdata/nat-network-bad-hostname.xml b/tests/networkxmlconfdata/nat-network-bad-hostname.xml new file mode 100644 index 0000000000..24f823f0e6 --- /dev/null +++ b/tests/networkxmlconfdata/nat-network-bad-hostname.xml @@ -0,0 +1,22 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <bridge name="virbr0"/> + <forward mode="nat" dev="eth1"/> + <domain name="example.com fish"/> + <ip address="192.168.122.1" netmask="255.255.255.0"> + <dhcp> + <range start="192.168.122.2" end="192.168.122.254"/> + <host mac="00:16:3e:77:e2:ed" name="a.example.com" ip="192.168.122.10"/> + <host mac="00:16:3e:3e:a9:1a" name="b.example.com" ip="192.168.122.11"/> + </dhcp> + </ip> + <ip family="ipv4" address="192.168.123.1" netmask="255.255.255.0"> + </ip> + <ip family="ipv6" address="2001:db8:ac10:fe01::1" prefix="64"> + </ip> + <ip family="ipv6" address="2001:db8:ac10:fd01::1" prefix="64"> + </ip> + <ip family="ipv4" address="10.24.10.1"> + </ip> +</network> diff --git a/tests/networkxmlconfdata/nat-network-dhcp-hosts-bad.xml b/tests/networkxmlconfdata/nat-network-dhcp-hosts-bad.xml new file mode 100644 index 0000000000..9d9711c7bf --- /dev/null +++ b/tests/networkxmlconfdata/nat-network-dhcp-hosts-bad.xml @@ -0,0 +1,21 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <bridge name="virbr0"/> + <forward mode="nat" dev="eth1"/> + <ip address="192.168.122.1" netmask="255.255.255.0"> + <dhcp> + <range start="192.168.122.2" end="192.168.122.254"/> + <host mac="00:16:3e:77:e2:ed" name="a.example.com fish" ip="192.168.122.10"/> + <host mac="00:16:3e:3e:a9:1a" name="b.example.com" ip="192.168.122.11"/> + </dhcp> + </ip> + <ip family="ipv4" address="192.168.123.1" netmask="255.255.255.0"> + </ip> + <ip family="ipv6" address="2001:db8:ac10:fe01::1" prefix="64"> + </ip> + <ip family="ipv6" address="2001:db8:ac10:fd01::1" prefix="64"> + </ip> + <ip family="ipv4" address="10.24.10.1"> + </ip> +</network> diff --git a/tests/networkxmlconfdata/nat-network-dns-hosts-bad.xml b/tests/networkxmlconfdata/nat-network-dns-hosts-bad.xml new file mode 100644 index 0000000000..b5f43b21dc --- /dev/null +++ b/tests/networkxmlconfdata/nat-network-dns-hosts-bad.xml @@ -0,0 +1,26 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9c</uuid> + <forward dev='eth0' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <domain name="example.com"/> + <dns forwardPlainNames='no'> + <host ip='192.168.122.122'> + <hostname>pudding fish</hostname> + </host> + <host ip='192.168.122.1'> + <hostname>host</hostname> + <hostname>gateway</hostname> + </host> + <host ip='192.168.122.2'> + <hostname>shared</hostname> + <hostname>names</hostname> + </host> + <host ip='fd8f:1391:3a82:150::c0a8:9603'> + <hostname>shared</hostname> + <hostname>names</hostname> + </host> + </dns> + <ip address='192.168.122.1' netmask='255.255.255.0'> + </ip> +</network> diff --git a/tests/networkxmlconftest.c b/tests/networkxmlconftest.c index c0d3886a69..eb60c19604 100644 --- a/tests/networkxmlconftest.c +++ b/tests/networkxmlconftest.c @@ -319,6 +319,7 @@ mymain(void) DO_TEST("open-network"); DO_TEST_PARSE_ERROR("open-network-with-forward-dev"); DO_TEST("nat-network"); + DO_TEST_PARSE_ERROR("nat-network-bad-hostname"); DO_TEST("netboot-network"); DO_TEST("netboot-proxy-network"); DO_TEST("netboot-tftp"); @@ -330,6 +331,8 @@ mymain(void) DO_TEST("nat-network-dns-srv-records"); DO_TEST("nat-network-dns-srv-record-minimal"); DO_TEST("nat-network-dns-hosts"); + DO_TEST_PARSE_ERROR("nat-network-dns-hosts-bad"); + DO_TEST_PARSE_ERROR("nat-network-dhcp-hosts-bad"); DO_TEST("nat-network-dns-forward-plain"); DO_TEST("nat-network-dns-forwarders"); DO_TEST("nat-network-dns-forwarder-no-resolv"); diff --git a/tests/virschematest.c b/tests/virschematest.c index fabe45b65d..e1fa82aa97 100644 --- a/tests/virschematest.c +++ b/tests/virschematest.c @@ -306,6 +306,9 @@ static const char *skip_networkxmlconfdata[] = { "nat-network-dns-srv-record-domain-newline.xml", "nat-network-dns-srv-record-target-newline.xml", "nat-network-dns-txt-recor-record-newline.xml", + "nat-network-bad-hostname.xml", + "nat-network-dhcp-hosts-bad.xml", + "nat-network-dns-hosts-bad.xml", NULL, }; -- 2.56.0
On 10/26/26 8:18 AM, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
The current network XML parser only validates that a DHCP hostname starts with a-Z character or a digit, or that a TXT record name doesn't contain space or comma. This is way too weak, and misses validation from other parts of the schema.
This can allow invalid characters that result in invalid syntax in dnsmasq config files.
Use the new hostname validation API to check all places that accept a hostname or domain name as a hardening task to prevent accidental creation of malformed dnsmasq cnofig files.
Fixes: https://gitlab.com/libvirt/libvirt/-/work_items/929 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/network_conf.c | 31 +++++++++---------- .../nat-network-bad-hostname.xml | 22 +++++++++++++ .../nat-network-dhcp-hosts-bad.xml | 21 +++++++++++++ .../nat-network-dns-hosts-bad.xml | 26 ++++++++++++++++ tests/networkxmlconftest.c | 3 ++ tests/virschematest.c | 3 ++ 6 files changed, 90 insertions(+), 16 deletions(-) create mode 100644 tests/networkxmlconfdata/nat-network-bad-hostname.xml create mode 100644 tests/networkxmlconfdata/nat-network-dhcp-hosts-bad.xml create mode 100644 tests/networkxmlconfdata/nat-network-dns-hosts-bad.xml
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 8eba1321d6..2fec4d2b91 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -35,6 +35,7 @@ #include "viruuid.h" #include "virbuffer.h" #include "virstring.h" +#include "virutil.h"
#define VIR_FROM_THIS VIR_FROM_NETWORK
@@ -533,10 +534,7 @@ virNetworkDHCPHostDefParseXML(const char *networkName, }
name = virXMLPropString(node, "name"); - if (name && !(g_ascii_isalpha(name[0]) || g_ascii_isdigit(name[0]))) { - virReportError(VIR_ERR_XML_ERROR, - _("Cannot use host name '%1$s' in network '%2$s'"), - name, networkName); + if (name && virHostnameValidate(name, true) < 0) { return -1; }
@@ -664,10 +662,7 @@ virNetworkDNSHostDefParseXML(const char *networkName, if (!(hostnames[i] = virXMLNodeContentString(g_ptr_array_index(hostnameNodes, i)))) return -1;
- if (*hostnames[i] == '\0') { - virReportError(VIR_ERR_XML_DETAIL, - _("Missing hostname in network '%1$s' DNS HOST record"), - networkName); + if (virHostnameValidate(hostnames[i], true) < 0) { return -1; } } @@ -792,8 +787,11 @@ virNetworkDNSSrvDefParseXML(const char *networkName, def->domain = virXMLPropString(node, "domain"); def->target = virXMLPropString(node, "target");
- if (virNetworkDNSDefCheckLineBreaks("SRV", "domain", def->domain) < 0 || - virNetworkDNSDefCheckLineBreaks("SRV", "target", def->target) < 0) + if (def->domain && + virHostnameValidate(def->domain, true) < 0) + goto error; + if (def->target && STRNEQ(def->target, ".") && + virHostnameValidate(def->target, true) < 0) goto error;
ret = virXPathUInt("string(./@port)", ctxt, &def->port); @@ -852,18 +850,13 @@ virNetworkDNSTxtDefParseXML(const char *networkName, virNetworkDNSTxtDef *def, bool partialOkay) { - const char *bad = " ,"; - if (!(def->name = virXMLPropString(node, "name"))) { virReportError(VIR_ERR_XML_DETAIL, _("missing required name attribute in DNS TXT record of network %1$s"), networkName); goto error; } - if (strcspn(def->name, bad) != strlen(def->name)) { - virReportError(VIR_ERR_XML_DETAIL, - _("prohibited character in DNS TXT record name '%1$s' of network %2$s"), - def->name, networkName);
I think it might be "a bridge too far" to put all these hostname restrictions on a TXT record. According to the RFCs any octet is allowed in TXT records (technically any octet is allowed even in A and AAAA records, and it's up to the application retrieving the info from the DNS server to validate it, but for those I think we can safely say that we're an application using a DNS server and so it's okay for us to fully validate A and AAAA records as hostnames. I don't think that's true for TXT records. Other than that: Reviewed-by: Laine Stump <laine@redhat.com>
+ if (virHostnameValidate(def->name, true) < 0) { goto error; } if (!(def->value = virXMLPropString(node, "value")) && !partialOkay) { @@ -958,6 +951,9 @@ virNetworkDNSDefParseXML(const char *networkName, _("Invalid forwarder element, must contain at least one of addr or domain")); return -1; } + if (def->forwarders[i].domain && + virHostnameValidate(def->forwarders[i].domain, true) < 0) + return -1; def->nfwds++; } } @@ -1621,6 +1617,9 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt, /* Parse network domain information */ domain_node = virXPathNode("./domain[1]", ctxt); def->domain = virXMLPropString(domain_node, "name"); + if (def->domain && + virHostnameValidate(def->domain, true) < 0) + return NULL; if (virXMLPropTristateBool(domain_node, "localOnly", VIR_XML_PROP_NONE, &def->domainLocalOnly) < 0) diff --git a/tests/networkxmlconfdata/nat-network-bad-hostname.xml b/tests/networkxmlconfdata/nat-network-bad-hostname.xml new file mode 100644 index 0000000000..24f823f0e6 --- /dev/null +++ b/tests/networkxmlconfdata/nat-network-bad-hostname.xml @@ -0,0 +1,22 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <bridge name="virbr0"/> + <forward mode="nat" dev="eth1"/> + <domain name="example.com fish"/> + <ip address="192.168.122.1" netmask="255.255.255.0"> + <dhcp> + <range start="192.168.122.2" end="192.168.122.254"/> + <host mac="00:16:3e:77:e2:ed" name="a.example.com" ip="192.168.122.10"/> + <host mac="00:16:3e:3e:a9:1a" name="b.example.com" ip="192.168.122.11"/> + </dhcp> + </ip> + <ip family="ipv4" address="192.168.123.1" netmask="255.255.255.0"> + </ip> + <ip family="ipv6" address="2001:db8:ac10:fe01::1" prefix="64"> + </ip> + <ip family="ipv6" address="2001:db8:ac10:fd01::1" prefix="64"> + </ip> + <ip family="ipv4" address="10.24.10.1"> + </ip> +</network> diff --git a/tests/networkxmlconfdata/nat-network-dhcp-hosts-bad.xml b/tests/networkxmlconfdata/nat-network-dhcp-hosts-bad.xml new file mode 100644 index 0000000000..9d9711c7bf --- /dev/null +++ b/tests/networkxmlconfdata/nat-network-dhcp-hosts-bad.xml @@ -0,0 +1,21 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <bridge name="virbr0"/> + <forward mode="nat" dev="eth1"/> + <ip address="192.168.122.1" netmask="255.255.255.0"> + <dhcp> + <range start="192.168.122.2" end="192.168.122.254"/> + <host mac="00:16:3e:77:e2:ed" name="a.example.com fish" ip="192.168.122.10"/> + <host mac="00:16:3e:3e:a9:1a" name="b.example.com" ip="192.168.122.11"/> + </dhcp> + </ip> + <ip family="ipv4" address="192.168.123.1" netmask="255.255.255.0"> + </ip> + <ip family="ipv6" address="2001:db8:ac10:fe01::1" prefix="64"> + </ip> + <ip family="ipv6" address="2001:db8:ac10:fd01::1" prefix="64"> + </ip> + <ip family="ipv4" address="10.24.10.1"> + </ip> +</network> diff --git a/tests/networkxmlconfdata/nat-network-dns-hosts-bad.xml b/tests/networkxmlconfdata/nat-network-dns-hosts-bad.xml new file mode 100644 index 0000000000..b5f43b21dc --- /dev/null +++ b/tests/networkxmlconfdata/nat-network-dns-hosts-bad.xml @@ -0,0 +1,26 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9c</uuid> + <forward dev='eth0' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <domain name="example.com"/> + <dns forwardPlainNames='no'> + <host ip='192.168.122.122'> + <hostname>pudding fish</hostname> + </host> + <host ip='192.168.122.1'> + <hostname>host</hostname> + <hostname>gateway</hostname> + </host> + <host ip='192.168.122.2'> + <hostname>shared</hostname> + <hostname>names</hostname> + </host> + <host ip='fd8f:1391:3a82:150::c0a8:9603'> + <hostname>shared</hostname> + <hostname>names</hostname> + </host> + </dns> + <ip address='192.168.122.1' netmask='255.255.255.0'> + </ip> +</network> diff --git a/tests/networkxmlconftest.c b/tests/networkxmlconftest.c index c0d3886a69..eb60c19604 100644 --- a/tests/networkxmlconftest.c +++ b/tests/networkxmlconftest.c @@ -319,6 +319,7 @@ mymain(void) DO_TEST("open-network"); DO_TEST_PARSE_ERROR("open-network-with-forward-dev"); DO_TEST("nat-network"); + DO_TEST_PARSE_ERROR("nat-network-bad-hostname"); DO_TEST("netboot-network"); DO_TEST("netboot-proxy-network"); DO_TEST("netboot-tftp"); @@ -330,6 +331,8 @@ mymain(void) DO_TEST("nat-network-dns-srv-records"); DO_TEST("nat-network-dns-srv-record-minimal"); DO_TEST("nat-network-dns-hosts"); + DO_TEST_PARSE_ERROR("nat-network-dns-hosts-bad"); + DO_TEST_PARSE_ERROR("nat-network-dhcp-hosts-bad"); DO_TEST("nat-network-dns-forward-plain"); DO_TEST("nat-network-dns-forwarders"); DO_TEST("nat-network-dns-forwarder-no-resolv"); diff --git a/tests/virschematest.c b/tests/virschematest.c index fabe45b65d..e1fa82aa97 100644 --- a/tests/virschematest.c +++ b/tests/virschematest.c @@ -306,6 +306,9 @@ static const char *skip_networkxmlconfdata[] = { "nat-network-dns-srv-record-domain-newline.xml", "nat-network-dns-srv-record-target-newline.xml", "nat-network-dns-txt-recor-record-newline.xml", + "nat-network-bad-hostname.xml", + "nat-network-dhcp-hosts-bad.xml", + "nat-network-dns-hosts-bad.xml", NULL, };
On Fri, Oct 30, 2026 at 12:04:56AM -0400, Laine Stump wrote:
On 10/26/26 8:18 AM, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
The current network XML parser only validates that a DHCP hostname starts with a-Z character or a digit, or that a TXT record name doesn't contain space or comma. This is way too weak, and misses validation from other parts of the schema.
This can allow invalid characters that result in invalid syntax in dnsmasq config files.
Use the new hostname validation API to check all places that accept a hostname or domain name as a hardening task to prevent accidental creation of malformed dnsmasq cnofig files.
Fixes: https://gitlab.com/libvirt/libvirt/-/work_items/929 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/network_conf.c | 31 +++++++++---------- .../nat-network-bad-hostname.xml | 22 +++++++++++++ .../nat-network-dhcp-hosts-bad.xml | 21 +++++++++++++ .../nat-network-dns-hosts-bad.xml | 26 ++++++++++++++++ tests/networkxmlconftest.c | 3 ++ tests/virschematest.c | 3 ++ 6 files changed, 90 insertions(+), 16 deletions(-) create mode 100644 tests/networkxmlconfdata/nat-network-bad-hostname.xml create mode 100644 tests/networkxmlconfdata/nat-network-dhcp-hosts-bad.xml create mode 100644 tests/networkxmlconfdata/nat-network-dns-hosts-bad.xml
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 8eba1321d6..2fec4d2b91 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -35,6 +35,7 @@ #include "viruuid.h" #include "virbuffer.h" #include "virstring.h" +#include "virutil.h" #define VIR_FROM_THIS VIR_FROM_NETWORK @@ -533,10 +534,7 @@ virNetworkDHCPHostDefParseXML(const char *networkName, } name = virXMLPropString(node, "name"); - if (name && !(g_ascii_isalpha(name[0]) || g_ascii_isdigit(name[0]))) { - virReportError(VIR_ERR_XML_ERROR, - _("Cannot use host name '%1$s' in network '%2$s'"), - name, networkName); + if (name && virHostnameValidate(name, true) < 0) { return -1; } @@ -664,10 +662,7 @@ virNetworkDNSHostDefParseXML(const char *networkName, if (!(hostnames[i] = virXMLNodeContentString(g_ptr_array_index(hostnameNodes, i)))) return -1; - if (*hostnames[i] == '\0') { - virReportError(VIR_ERR_XML_DETAIL, - _("Missing hostname in network '%1$s' DNS HOST record"), - networkName); + if (virHostnameValidate(hostnames[i], true) < 0) { return -1; } } @@ -792,8 +787,11 @@ virNetworkDNSSrvDefParseXML(const char *networkName, def->domain = virXMLPropString(node, "domain"); def->target = virXMLPropString(node, "target"); - if (virNetworkDNSDefCheckLineBreaks("SRV", "domain", def->domain) < 0 || - virNetworkDNSDefCheckLineBreaks("SRV", "target", def->target) < 0) + if (def->domain && + virHostnameValidate(def->domain, true) < 0) + goto error; + if (def->target && STRNEQ(def->target, ".") && + virHostnameValidate(def->target, true) < 0) goto error; ret = virXPathUInt("string(./@port)", ctxt, &def->port); @@ -852,18 +850,13 @@ virNetworkDNSTxtDefParseXML(const char *networkName, virNetworkDNSTxtDef *def, bool partialOkay) { - const char *bad = " ,"; - if (!(def->name = virXMLPropString(node, "name"))) { virReportError(VIR_ERR_XML_DETAIL, _("missing required name attribute in DNS TXT record of network %1$s"), networkName); goto error; } - if (strcspn(def->name, bad) != strlen(def->name)) { - virReportError(VIR_ERR_XML_DETAIL, - _("prohibited character in DNS TXT record name '%1$s' of network %2$s"), - def->name, networkName);
I think it might be "a bridge too far" to put all these hostname restrictions on a TXT record. According to the RFCs any octet is allowed in TXT records (technically any octet is allowed even in A and AAAA records, and it's up to the application retrieving the info from the DNS server to validate it, but for those I think we can safely say that we're an application using a DNS server and so it's okay for us to fully validate A and AAAA records as hostnames. I don't think that's true for TXT records.
This is validating the name part of the TXT record, which IIUC, still has to comply with the naming rules for DNS records. Only the value is free-form unrestricted octets. I did, however, create a bug. A "_" is permitted in the TXT record name and required in a SRV record. With regards, Daniel -- |: https://berrange.com ~~ https://hachyderm.io/@berrange :| |: https://libvirt.org ~~ https://entangle-photo.org :| |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
From: Daniel P. Berrangé <berrange@redhat.com> When writing the dnsmasq addnhosts file to a temporary file, there is a fallback which tries to open the original file. We expect creating the temporary file to always succeed, and if it fails something serious has gone wrong that we want to have reported. So this fallback is expected to be dead code. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virdnsmasq.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 7d6de943cf..817e060a44 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -161,7 +161,6 @@ addnhostsWrite(const char *path, { g_autofree char *tmp = NULL; FILE *f; - bool istmp = true; size_t i, j; int rc = 0; @@ -171,20 +170,14 @@ addnhostsWrite(const char *path, tmp = g_strdup_printf("%s.new", path); - if (!(f = fopen(tmp, "w"))) { - istmp = false; - if (!(f = fopen(path, "w"))) - return -errno; - } + if (!(f = fopen(tmp, "w"))) + return -errno; for (i = 0; i < nhosts; i++) { if (fputs(hosts[i].ip, f) == EOF || fputc('\t', f) == EOF) { rc = -errno; VIR_FORCE_FCLOSE(f); - - if (istmp) - unlink(tmp); - + unlink(tmp); return rc; } @@ -192,10 +185,7 @@ addnhostsWrite(const char *path, if (fputs(hosts[i].hostnames[j], f) == EOF || fputc('\t', f) == EOF) { rc = -errno; VIR_FORCE_FCLOSE(f); - - if (istmp) - unlink(tmp); - + unlink(tmp); return rc; } } @@ -203,10 +193,7 @@ addnhostsWrite(const char *path, if (fputc('\n', f) == EOF) { rc = -errno; VIR_FORCE_FCLOSE(f); - - if (istmp) - unlink(tmp); - + unlink(tmp); return rc; } } @@ -214,7 +201,7 @@ addnhostsWrite(const char *path, if (VIR_FCLOSE(f) == EOF) return -errno; - if (istmp && rename(tmp, path) < 0) { + if (rename(tmp, path) < 0) { rc = -errno; unlink(tmp); return rc; -- 2.56.0
On 10/26/26 8:18 AM, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
When writing the dnsmasq addnhosts file to a temporary file, there is a fallback which tries to open the original file. We expect creating the temporary file to always succeed, and if it fails something serious has gone wrong that we want to have reported. So this fallback is expected to be dead code.
I guess the point of writing to the temp file is either 1) so that the original is still there in the case that writing the new one fails, or 2) to make the change atomic? This code has been around so long that I only barely recognize the original author's name, much less have any idea why it is the way it is :-P (admittedly I didn't spend much time thinking about it). But anyway if it's being done, there must have been some reason for it, and I can't think of any good reason why we would fail to open and write to "x.tmp" but then succeed to write to "x".
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
--- src/util/virdnsmasq.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-)
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 7d6de943cf..817e060a44 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -161,7 +161,6 @@ addnhostsWrite(const char *path, { g_autofree char *tmp = NULL; FILE *f; - bool istmp = true; size_t i, j; int rc = 0;
@@ -171,20 +170,14 @@ addnhostsWrite(const char *path,
tmp = g_strdup_printf("%s.new", path);
- if (!(f = fopen(tmp, "w"))) { - istmp = false; - if (!(f = fopen(path, "w"))) - return -errno; - } + if (!(f = fopen(tmp, "w"))) + return -errno;
for (i = 0; i < nhosts; i++) { if (fputs(hosts[i].ip, f) == EOF || fputc('\t', f) == EOF) { rc = -errno; VIR_FORCE_FCLOSE(f); - - if (istmp) - unlink(tmp); - + unlink(tmp); return rc; }
@@ -192,10 +185,7 @@ addnhostsWrite(const char *path, if (fputs(hosts[i].hostnames[j], f) == EOF || fputc('\t', f) == EOF) { rc = -errno; VIR_FORCE_FCLOSE(f); - - if (istmp) - unlink(tmp); - + unlink(tmp); return rc; } } @@ -203,10 +193,7 @@ addnhostsWrite(const char *path, if (fputc('\n', f) == EOF) { rc = -errno; VIR_FORCE_FCLOSE(f); - - if (istmp) - unlink(tmp); - + unlink(tmp); return rc; } } @@ -214,7 +201,7 @@ addnhostsWrite(const char *path, if (VIR_FCLOSE(f) == EOF) return -errno;
- if (istmp && rename(tmp, path) < 0) { + if (rename(tmp, path) < 0) { rc = -errno; unlink(tmp); return rc;
On Thu, Oct 29, 2026 at 11:09:01AM -0400, Laine Stump wrote:
On 10/26/26 8:18 AM, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
When writing the dnsmasq addnhosts file to a temporary file, there is a fallback which tries to open the original file. We expect creating the temporary file to always succeed, and if it fails something serious has gone wrong that we want to have reported. So this fallback is expected to be dead code.
I guess the point of writing to the temp file is either 1) so that the original is still there in the case that writing the new one fails, or 2) to make the change atomic? This code has been around so long that I only barely recognize the original author's name, much less have any idea why it is the way it is :-P (admittedly I didn't spend much time thinking about it). But anyway if it's being done, there must have been some reason for it, and I can't think of any good reason why we would fail to open and write to "x.tmp" but then succeed to write to "x".
Mostly I think it is the atomicity that's important, as well as greater robustness on host crash, but this hand written code fails at the latter since it lacks the fsync() that virFileRewrite does.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
--- src/util/virdnsmasq.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-)
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 7d6de943cf..817e060a44 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -161,7 +161,6 @@ addnhostsWrite(const char *path, { g_autofree char *tmp = NULL; FILE *f; - bool istmp = true; size_t i, j; int rc = 0; @@ -171,20 +170,14 @@ addnhostsWrite(const char *path, tmp = g_strdup_printf("%s.new", path); - if (!(f = fopen(tmp, "w"))) { - istmp = false; - if (!(f = fopen(path, "w"))) - return -errno; - } + if (!(f = fopen(tmp, "w"))) + return -errno; for (i = 0; i < nhosts; i++) { if (fputs(hosts[i].ip, f) == EOF || fputc('\t', f) == EOF) { rc = -errno; VIR_FORCE_FCLOSE(f); - - if (istmp) - unlink(tmp); - + unlink(tmp); return rc; } @@ -192,10 +185,7 @@ addnhostsWrite(const char *path, if (fputs(hosts[i].hostnames[j], f) == EOF || fputc('\t', f) == EOF) { rc = -errno; VIR_FORCE_FCLOSE(f); - - if (istmp) - unlink(tmp); - + unlink(tmp); return rc; } } @@ -203,10 +193,7 @@ addnhostsWrite(const char *path, if (fputc('\n', f) == EOF) { rc = -errno; VIR_FORCE_FCLOSE(f); - - if (istmp) - unlink(tmp); - + unlink(tmp); return rc; } } @@ -214,7 +201,7 @@ addnhostsWrite(const char *path, if (VIR_FCLOSE(f) == EOF) return -errno; - if (istmp && rename(tmp, path) < 0) { + if (rename(tmp, path) < 0) { rc = -errno; unlink(tmp); return rc;
With regards, Daniel -- |: https://berrange.com ~~ https://hachyderm.io/@berrange :| |: https://libvirt.org ~~ https://entangle-photo.org :| |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
From: Daniel P. Berrangé <berrange@redhat.com> The current addnhostsWrite() method implements logic that is already present in virFileRewrite. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virdnsmasq.c | 82 ++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 51 deletions(-) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 817e060a44..656665e174 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -154,77 +154,57 @@ addnhostsNew(const char *name, return NULL; } + static int -addnhostsWrite(const char *path, - dnsmasqAddnHost *hosts, - unsigned int nhosts) +addnhostsWrite(int fd, + const char *path, + const void *opaque) { - g_autofree char *tmp = NULL; - FILE *f; + const dnsmasqAddnHostsfile *addnhostsfile = opaque; size_t i, j; - int rc = 0; /* even if there are 0 hosts, create a 0 length file, to allow * for runtime addition. */ - tmp = g_strdup_printf("%s.new", path); - - if (!(f = fopen(tmp, "w"))) - return -errno; - - for (i = 0; i < nhosts; i++) { - if (fputs(hosts[i].ip, f) == EOF || fputc('\t', f) == EOF) { - rc = -errno; - VIR_FORCE_FCLOSE(f); - unlink(tmp); - return rc; - } - - for (j = 0; j < hosts[i].nhostnames; j++) { - if (fputs(hosts[i].hostnames[j], f) == EOF || fputc('\t', f) == EOF) { - rc = -errno; - VIR_FORCE_FCLOSE(f); - unlink(tmp); - return rc; - } - } - - if (fputc('\n', f) == EOF) { - rc = -errno; - VIR_FORCE_FCLOSE(f); - unlink(tmp); - return rc; + for (i = 0; i < addnhostsfile->nhosts; i++) { + if (safewrite(fd, addnhostsfile->hosts[i].ip, + strlen(addnhostsfile->hosts[i].ip)) < 0) + goto error; + if (safewrite(fd, "\t", 1) < 0) + goto error; + + for (j = 0; j < addnhostsfile->hosts[i].nhostnames; j++) { + if (safewrite(fd, addnhostsfile->hosts[i].hostnames[j], + strlen(addnhostsfile->hosts[i].hostnames[j])) < 0) + goto error; + if (safewrite(fd, "\t", 1) < 0) + goto error; } - } - - if (VIR_FCLOSE(f) == EOF) - return -errno; - if (rename(tmp, path) < 0) { - rc = -errno; - unlink(tmp); - return rc; + if (safewrite(fd, "\n", 1) < 0) + goto error; } return 0; + + error: + virReportSystemError(errno, + _("cannot write config file '%1$s'"), + path); + return -1; } static int addnhostsSave(dnsmasqAddnHostsfile *addnhostsfile) { - int err = addnhostsWrite(addnhostsfile->path, addnhostsfile->hosts, - addnhostsfile->nhosts); - - if (err < 0) { - virReportSystemError(-err, _("cannot write config file '%1$s'"), - addnhostsfile->path); - return -1; - } - - return 0; + return virFileRewrite(addnhostsfile->path, + 0600, -1, -1, + addnhostsWrite, + addnhostsfile); } + static int genericFileDelete(char *path) { -- 2.56.0
On 10/26/26 8:18 AM, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
The current addnhostsWrite() method implements logic that is already present in virFileRewrite.
...and then some - it also does an fsync() before closing the new file, I guess to make sure that the new file really truly is all on disk and ready to use by the time the rename happens. Am I correct in thinking you included patch 3 (and also upcoming patch 5) just to make it more obvious that this was just doing a straight-across replacement of functionality? Because otherwise it would totally be okay with me to squash 3&4 together, and also 5&6. BTW, I also noticed that these patches remove the only uses of fputs() that gets compiled into any daemons (all that's left is a couple of tests, and a few uses in tools/vsh.c). Is fputs() one of those problematic (not async-signal safe) functions that we should ban via a syntax-check rule?
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
--- src/util/virdnsmasq.c | 82 ++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 51 deletions(-)
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 817e060a44..656665e174 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -154,77 +154,57 @@ addnhostsNew(const char *name, return NULL; }
+ static int -addnhostsWrite(const char *path, - dnsmasqAddnHost *hosts, - unsigned int nhosts) +addnhostsWrite(int fd, + const char *path, + const void *opaque) { - g_autofree char *tmp = NULL; - FILE *f; + const dnsmasqAddnHostsfile *addnhostsfile = opaque; size_t i, j; - int rc = 0;
/* even if there are 0 hosts, create a 0 length file, to allow * for runtime addition. */
- tmp = g_strdup_printf("%s.new", path); - - if (!(f = fopen(tmp, "w"))) - return -errno; - - for (i = 0; i < nhosts; i++) { - if (fputs(hosts[i].ip, f) == EOF || fputc('\t', f) == EOF) { - rc = -errno; - VIR_FORCE_FCLOSE(f); - unlink(tmp); - return rc; - } - - for (j = 0; j < hosts[i].nhostnames; j++) { - if (fputs(hosts[i].hostnames[j], f) == EOF || fputc('\t', f) == EOF) { - rc = -errno; - VIR_FORCE_FCLOSE(f); - unlink(tmp); - return rc; - } - } - - if (fputc('\n', f) == EOF) { - rc = -errno; - VIR_FORCE_FCLOSE(f); - unlink(tmp); - return rc; + for (i = 0; i < addnhostsfile->nhosts; i++) { + if (safewrite(fd, addnhostsfile->hosts[i].ip, + strlen(addnhostsfile->hosts[i].ip)) < 0) + goto error; + if (safewrite(fd, "\t", 1) < 0) + goto error; + + for (j = 0; j < addnhostsfile->hosts[i].nhostnames; j++) { + if (safewrite(fd, addnhostsfile->hosts[i].hostnames[j], + strlen(addnhostsfile->hosts[i].hostnames[j])) < 0) + goto error; + if (safewrite(fd, "\t", 1) < 0) + goto error; } - } - - if (VIR_FCLOSE(f) == EOF) - return -errno;
- if (rename(tmp, path) < 0) { - rc = -errno; - unlink(tmp); - return rc; + if (safewrite(fd, "\n", 1) < 0) + goto error; }
return 0; + + error: + virReportSystemError(errno, + _("cannot write config file '%1$s'"), + path); + return -1; }
static int addnhostsSave(dnsmasqAddnHostsfile *addnhostsfile) { - int err = addnhostsWrite(addnhostsfile->path, addnhostsfile->hosts, - addnhostsfile->nhosts); - - if (err < 0) { - virReportSystemError(-err, _("cannot write config file '%1$s'"), - addnhostsfile->path); - return -1; - } - - return 0; + return virFileRewrite(addnhostsfile->path, + 0600, -1, -1, + addnhostsWrite, + addnhostsfile); }
+ static int genericFileDelete(char *path) {
On Thu, Oct 29, 2026 at 11:42:25PM -0400, Laine Stump wrote:
On 10/26/26 8:18 AM, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
The current addnhostsWrite() method implements logic that is already present in virFileRewrite.
...and then some - it also does an fsync() before closing the new file, I guess to make sure that the new file really truly is all on disk and ready to use by the time the rename happens.
The fsync() combined with rename() is important for crash safety. Helps ensure that after host OS crash, you should get either the old file, or the new file, and not some intermediate half written mess.
Am I correct in thinking you included patch 3 (and also upcoming patch 5) just to make it more obvious that this was just doing a straight-across replacement of functionality? Because otherwise it would totally be okay with me to squash 3&4 together, and also 5&6.
Yeah, I wanted to separate a clear functional behaviour change from what is effectively just a refactoring here.
BTW, I also noticed that these patches remove the only uses of fputs() that gets compiled into any daemons (all that's left is a couple of tests, and a few uses in tools/vsh.c). Is fputs() one of those problematic (not async-signal safe) functions that we should ban via a syntax-check rule?
All of stdio (ie FILE* APIs) is unsafe from signal handlers and fork/exec as it has internal mutexes that are held. This dnsmasq code usage was harmless.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
--- src/util/virdnsmasq.c | 82 ++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 51 deletions(-)
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 817e060a44..656665e174 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -154,77 +154,57 @@ addnhostsNew(const char *name, return NULL; } + static int -addnhostsWrite(const char *path, - dnsmasqAddnHost *hosts, - unsigned int nhosts) +addnhostsWrite(int fd, + const char *path, + const void *opaque) { - g_autofree char *tmp = NULL; - FILE *f; + const dnsmasqAddnHostsfile *addnhostsfile = opaque; size_t i, j; - int rc = 0; /* even if there are 0 hosts, create a 0 length file, to allow * for runtime addition. */ - tmp = g_strdup_printf("%s.new", path); - - if (!(f = fopen(tmp, "w"))) - return -errno; - - for (i = 0; i < nhosts; i++) { - if (fputs(hosts[i].ip, f) == EOF || fputc('\t', f) == EOF) { - rc = -errno; - VIR_FORCE_FCLOSE(f); - unlink(tmp); - return rc; - } - - for (j = 0; j < hosts[i].nhostnames; j++) { - if (fputs(hosts[i].hostnames[j], f) == EOF || fputc('\t', f) == EOF) { - rc = -errno; - VIR_FORCE_FCLOSE(f); - unlink(tmp); - return rc; - } - } - - if (fputc('\n', f) == EOF) { - rc = -errno; - VIR_FORCE_FCLOSE(f); - unlink(tmp); - return rc; + for (i = 0; i < addnhostsfile->nhosts; i++) { + if (safewrite(fd, addnhostsfile->hosts[i].ip, + strlen(addnhostsfile->hosts[i].ip)) < 0) + goto error; + if (safewrite(fd, "\t", 1) < 0) + goto error; + + for (j = 0; j < addnhostsfile->hosts[i].nhostnames; j++) { + if (safewrite(fd, addnhostsfile->hosts[i].hostnames[j], + strlen(addnhostsfile->hosts[i].hostnames[j])) < 0) + goto error; + if (safewrite(fd, "\t", 1) < 0) + goto error; } - } - - if (VIR_FCLOSE(f) == EOF) - return -errno; - if (rename(tmp, path) < 0) { - rc = -errno; - unlink(tmp); - return rc; + if (safewrite(fd, "\n", 1) < 0) + goto error; } return 0; + + error: + virReportSystemError(errno, + _("cannot write config file '%1$s'"), + path); + return -1; } static int addnhostsSave(dnsmasqAddnHostsfile *addnhostsfile) { - int err = addnhostsWrite(addnhostsfile->path, addnhostsfile->hosts, - addnhostsfile->nhosts); - - if (err < 0) { - virReportSystemError(-err, _("cannot write config file '%1$s'"), - addnhostsfile->path); - return -1; - } - - return 0; + return virFileRewrite(addnhostsfile->path, + 0600, -1, -1, + addnhostsWrite, + addnhostsfile); } + static int genericFileDelete(char *path) {
With regards, Daniel -- |: https://berrange.com ~~ https://hachyderm.io/@berrange :| |: https://libvirt.org ~~ https://entangle-photo.org :| |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
From: Daniel P. Berrangé <berrange@redhat.com> When writing the dnsmasq hosts file to a temporary file, there is a fallback which tries to open the original file. We expect creating the temporary file to always succeed, and if it fails something serious has gone wrong that we want to have reported. So this fallback is expected to be dead code. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virdnsmasq.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 656665e174..7bb1188516 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -320,7 +320,6 @@ hostsfileWrite(const char *path, { g_autofree char *tmp = NULL; FILE *f; - bool istmp = true; size_t i; int rc = 0; @@ -331,19 +330,14 @@ hostsfileWrite(const char *path, tmp = g_strdup_printf("%s.new", path); if (!(f = fopen(tmp, "w"))) { - istmp = false; - if (!(f = fopen(path, "w"))) - return -errno; + return -errno; } for (i = 0; i < nhosts; i++) { if (fputs(hosts[i].host, f) == EOF || fputc('\n', f) == EOF) { rc = -errno; VIR_FORCE_FCLOSE(f); - - if (istmp) - unlink(tmp); - + unlink(tmp); return rc; } } @@ -351,7 +345,7 @@ hostsfileWrite(const char *path, if (VIR_FCLOSE(f) == EOF) return -errno; - if (istmp && rename(tmp, path) < 0) { + if (rename(tmp, path) < 0) { rc = -errno; unlink(tmp); return rc; -- 2.56.0
On 10/26/26 8:18 AM, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
When writing the dnsmasq hosts file to a temporary file, there is a fallback which tries to open the original file. We expect creating the temporary file to always succeed, and if it fails something serious has gone wrong that we want to have reported. So this fallback is expected to be dead code.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
From: Daniel P. Berrangé <berrange@redhat.com> The current hostsfileWrite() method implements logic that is already present in virFileRewrite. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virdnsmasq.c | 62 ++++++++++++++----------------------------- 1 file changed, 20 insertions(+), 42 deletions(-) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 7bb1188516..0e12e9ab38 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -314,59 +314,37 @@ hostsfileNew(const char *name, } static int -hostsfileWrite(const char *path, - dnsmasqDhcpHost *hosts, - unsigned int nhosts) +hostsfileWrite(int fd, + const char *path, + const void *opaque) { - g_autofree char *tmp = NULL; - FILE *f; + const dnsmasqHostsfile *hostsfile = opaque; size_t i; - int rc = 0; - - /* even if there are 0 hosts, create a 0 length file, to allow - * for runtime addition. - */ - - tmp = g_strdup_printf("%s.new", path); - if (!(f = fopen(tmp, "w"))) { - return -errno; - } - - for (i = 0; i < nhosts; i++) { - if (fputs(hosts[i].host, f) == EOF || fputc('\n', f) == EOF) { - rc = -errno; - VIR_FORCE_FCLOSE(f); - unlink(tmp); - return rc; - } - } - - if (VIR_FCLOSE(f) == EOF) - return -errno; - - if (rename(tmp, path) < 0) { - rc = -errno; - unlink(tmp); - return rc; + for (i = 0; i < hostsfile->nhosts; i++) { + if (safewrite(fd, hostsfile->hosts[i].host, + strlen(hostsfile->hosts[i].host)) < 0) + goto error; + if (safewrite(fd, "\n", 1) < 0) + goto error; } return 0; + + error: + virReportSystemError(errno, + _("cannot write config file '%1$s'"), + path); + return -1; } static int hostsfileSave(dnsmasqHostsfile *hostsfile) { - int err = hostsfileWrite(hostsfile->path, hostsfile->hosts, - hostsfile->nhosts); - - if (err < 0) { - virReportSystemError(-err, _("cannot write config file '%1$s'"), - hostsfile->path); - return -1; - } - - return 0; + return virFileRewrite(hostsfile->path, + 0600, -1, -1, + hostsfileWrite, + hostsfile); } /** -- 2.56.0
On 10/26/26 8:18 AM, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
The current hostsfileWrite() method implements logic that is already present in virFileRewrite.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com> (same comments as patch 4)> ---
src/util/virdnsmasq.c | 62 ++++++++++++++----------------------------- 1 file changed, 20 insertions(+), 42 deletions(-)
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 7bb1188516..0e12e9ab38 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -314,59 +314,37 @@ hostsfileNew(const char *name, }
static int -hostsfileWrite(const char *path, - dnsmasqDhcpHost *hosts, - unsigned int nhosts) +hostsfileWrite(int fd, + const char *path, + const void *opaque) { - g_autofree char *tmp = NULL; - FILE *f; + const dnsmasqHostsfile *hostsfile = opaque; size_t i; - int rc = 0; - - /* even if there are 0 hosts, create a 0 length file, to allow - * for runtime addition. - */ - - tmp = g_strdup_printf("%s.new", path);
- if (!(f = fopen(tmp, "w"))) { - return -errno; - } - - for (i = 0; i < nhosts; i++) { - if (fputs(hosts[i].host, f) == EOF || fputc('\n', f) == EOF) { - rc = -errno; - VIR_FORCE_FCLOSE(f); - unlink(tmp); - return rc; - } - } - - if (VIR_FCLOSE(f) == EOF) - return -errno; - - if (rename(tmp, path) < 0) { - rc = -errno; - unlink(tmp); - return rc; + for (i = 0; i < hostsfile->nhosts; i++) { + if (safewrite(fd, hostsfile->hosts[i].host, + strlen(hostsfile->hosts[i].host)) < 0) + goto error; + if (safewrite(fd, "\n", 1) < 0) + goto error; }
return 0; + + error: + virReportSystemError(errno, + _("cannot write config file '%1$s'"), + path); + return -1; }
static int hostsfileSave(dnsmasqHostsfile *hostsfile) { - int err = hostsfileWrite(hostsfile->path, hostsfile->hosts, - hostsfile->nhosts); - - if (err < 0) { - virReportSystemError(-err, _("cannot write config file '%1$s'"), - hostsfile->path); - return -1; - } - - return 0; + return virFileRewrite(hostsfile->path, + 0600, -1, -1, + hostsfileWrite, + hostsfile); }
/**
From: Daniel P. Berrangé <berrange@redhat.com> Although the network XML parser should already be rejecting hostnames with forbidden characters, add sanity checking before writing the dnsmasq hostsfile / addhosts files that no unexpected whitespace is present. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virdnsmasq.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 0e12e9ab38..a66577fd67 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -198,6 +198,19 @@ addnhostsWrite(int fd, static int addnhostsSave(dnsmasqAddnHostsfile *addnhostsfile) { + size_t i, j; + for (i = 0; i < addnhostsfile->nhosts; i++) { + for (j = 0; j < addnhostsfile->hosts[i].nhostnames; j++) { + if (strcspn(addnhostsfile->hosts[i].hostnames[j], "\t\n\r ") != + strlen(addnhostsfile->hosts[i].hostnames[j])) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("dnsmasq hostname '%1$s' contains forbidden whitespace"), + addnhostsfile->hosts[i].hostnames[j]); + return -1; + } + } + } + return virFileRewrite(addnhostsfile->path, 0600, -1, -1, addnhostsWrite, @@ -341,6 +354,17 @@ hostsfileWrite(int fd, static int hostsfileSave(dnsmasqHostsfile *hostsfile) { + size_t i; + for (i = 0; i < hostsfile->nhosts; i++) { + if (strcspn(hostsfile->hosts[i].host, "\t\n\r ") != + strlen(hostsfile->hosts[i].host)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("dnsmasq hostname '%1$s' contains forbidden whitespace"), + hostsfile->hosts[i].host); + return -1; + } + } + return virFileRewrite(hostsfile->path, 0600, -1, -1, hostsfileWrite, -- 2.56.0
On 10/26/26 8:18 AM, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
Although the network XML parser should already be rejecting hostnames with forbidden characters, add sanity checking before writing the dnsmasq hostsfile / addhosts files that no unexpected whitespace is present.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
--- src/util/virdnsmasq.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 0e12e9ab38..a66577fd67 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -198,6 +198,19 @@ addnhostsWrite(int fd, static int addnhostsSave(dnsmasqAddnHostsfile *addnhostsfile) { + size_t i, j; + for (i = 0; i < addnhostsfile->nhosts; i++) { + for (j = 0; j < addnhostsfile->hosts[i].nhostnames; j++) { + if (strcspn(addnhostsfile->hosts[i].hostnames[j], "\t\n\r ") != + strlen(addnhostsfile->hosts[i].hostnames[j])) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("dnsmasq hostname '%1$s' contains forbidden whitespace"), + addnhostsfile->hosts[i].hostnames[j]); + return -1; + } + } + } + return virFileRewrite(addnhostsfile->path, 0600, -1, -1, addnhostsWrite, @@ -341,6 +354,17 @@ hostsfileWrite(int fd, static int hostsfileSave(dnsmasqHostsfile *hostsfile) { + size_t i; + for (i = 0; i < hostsfile->nhosts; i++) { + if (strcspn(hostsfile->hosts[i].host, "\t\n\r ") != + strlen(hostsfile->hosts[i].host)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("dnsmasq hostname '%1$s' contains forbidden whitespace"), + hostsfile->hosts[i].host); + return -1; + } + } + return virFileRewrite(hostsfile->path, 0600, -1, -1, hostsfileWrite,
participants (2)
-
Daniel P. Berrangé -
Laine Stump