On 8/6/26 18:14, Daniel P. Berrangé via Devel wrote:
> From: Daniel P. Berrangé <[email protected]>
>
> In a recent commit we introduced checking for newlines in certain
> dnsmasq.conf values that are expected to be fed through from
> untrusted user input. Newlines could be used to inject malicious
> config statements so must be forbidden.
>
> We have many other dnsmasq.conf entries, however, that have no
> checking, and while they're not considered to be security sensitive,
> from a robustness POV we should exclude newlines there too to
> guarantee a well-formed config file.
>
> Instead of using virBufferAsprintf to directly format config
> entries, introduce some helpers that wrap virBuffer APIs.
>
> static void
> networkDnsmasqConfAddKey(virBuffer *buf,
> const char *key)
> static int
> networkDnsmasqConfAddValue(virBuffer *buf,
> const char *key,
> const char *val)
> G_GNUC_WARN_UNUSED_RESULT;
> #define networkDnsmasqConfAddValueLit(buf, key, val) \
> ignore_value(networkDnsmasqConfAddValue(buf, key, "" val ""))
> static int
> networkDnsmasqConfAddValueFmt(virBuffer *buf,
> const char *key,
> const char *fmt,
> ...)
> G_GNUC_PRINTF(3, 4)
> G_GNUC_WARN_UNUSED_RESULT;
>
> This ensures that every config file entry we write out will
> always be well-formed.
>
> Reported-by: Vivek Parikh <[email protected]>
> Signed-off-by: Daniel P. Berrangé <[email protected]>
> ---
> src/network/bridge_driver.c | 256 ++++++++++++++++++++++--------------
> 1 file changed, 155 insertions(+), 101 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 6ebdc27e76..011b28b82e 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -120,22 +120,6 @@ networkDnsmasqDefNamespaceFree(void *nsdata)
> G_DEFINE_AUTOPTR_CLEANUP_FUNC(networkDnsmasqXmlNsDef,
> networkDnsmasqDefNamespaceFree);
>
>
> -static int
> -networkDnsmasqConfCheckLineBreaks(const char *record,
> - const char *field,
> - const char *value)
> -{
> - if (virStringHasChars(value, "\r\n")) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("DNS %1$s record %2$s must not contain line
> breaks"),
> - record, field);
> - return -1;
> - }
> -
> - return 0;
> -}
> -
> -
> static int
> networkDnsmasqDefNamespaceParseOptions(networkDnsmasqXmlNsDef *nsdef,
> xmlXPathContextPtr ctxt)
> @@ -924,6 +908,60 @@ networkConnectSupportsFeature(virConnectPtr conn, int
> feature)
> }
>
>
> +static void
> +networkDnsmasqConfAddKey(virBuffer *buf,
> + const char *key)
> +{
> + virBufferAsprintf(buf, "%s\n", key);
> +}
> +
> +static int
> +networkDnsmasqConfAddValue(virBuffer *buf,
> + const char *key,
> + const char *val)
> + G_GNUC_WARN_UNUSED_RESULT;
There's no need for forward declaration. This can be rewritten as:
static int
G_GNUC_WARN_UNUSED_RESULT
networkDnsmasqConfAddValue(virBuffer *buf,
const char *key,
const char *val)
{
...
> +static int
> +networkDnsmasqConfAddValue(virBuffer *buf,
> + const char *key,
> + const char *val)
> +{
> + if (virStringHasChars(val, "\r\n")) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Field '%1$s' value '%2$s' must not contain line
> breaks"),
> + key, val);
> + return -1;
> + }
> +
> + virBufferAsprintf(buf, "%s=%s\n", key, val);
> + return 0;
> +}
> +
> +#define networkDnsmasqConfAddValueLit(buf, key, val) \
> + ignore_value(networkDnsmasqConfAddValue(buf, key, "" val ""))
> +
> +static int
> +networkDnsmasqConfAddValueFmt(virBuffer *buf,
> + const char *key,
> + const char *fmt,
> + ...)
> + G_GNUC_PRINTF(3, 4)
> + G_GNUC_WARN_UNUSED_RESULT;
Same here.
> +static int
> +networkDnsmasqConfAddValueFmt(virBuffer *buf,
> + const char *key,
> + const char *fmt,
> + ...)
> +{
> + va_list args;
> + g_autofree char *val = NULL;
> +
> + va_start(args, fmt);
> + val = g_strdup_vprintf(fmt, args);
> + va_end(args);
> +
> + return networkDnsmasqConfAddValue(buf, key, val);
> +}
> +
Please consider squashing this in:
diff --git i/src/network/bridge_driver.c w/src/network/bridge_driver.c
index 011b28b82e..c435e21bed 100644
--- i/src/network/bridge_driver.c
+++ w/src/network/bridge_driver.c
@@ -915,12 +915,9 @@ networkDnsmasqConfAddKey(virBuffer *buf,
virBufferAsprintf(buf, "%s\n", key);
}
+
static int
-networkDnsmasqConfAddValue(virBuffer *buf,
- const char *key,
- const char *val)
- G_GNUC_WARN_UNUSED_RESULT;
-static int
+G_GNUC_WARN_UNUSED_RESULT
networkDnsmasqConfAddValue(virBuffer *buf,
const char *key,
const char *val)
@@ -940,13 +937,8 @@ networkDnsmasqConfAddValue(virBuffer *buf,
ignore_value(networkDnsmasqConfAddValue(buf, key, "" val ""))
static int
-networkDnsmasqConfAddValueFmt(virBuffer *buf,
- const char *key,
- const char *fmt,
- ...)
- G_GNUC_PRINTF(3, 4)
- G_GNUC_WARN_UNUSED_RESULT;
-static int
+G_GNUC_PRINTF(3, 4)
+G_GNUC_WARN_UNUSED_RESULT
networkDnsmasqConfAddValueFmt(virBuffer *buf,
const char *key,
const char *fmt,
Reviewed-by: Michal Privoznik <[email protected]>
Michal
