From 751223fecf56c6865080c2dda760a47a9958e49c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 24 Nov 2017 12:19:40 +0100 Subject: [PATCH] Fail on unknown (alphanumerical) specifiers The code intentionally ignored unknown specifiers, treating them as text. This needs to change because otherwise we can never add a new specifier in a backwards compatible way. So just treat an unknown (potential) specifier as an error. In principle this is a break of backwards compatibility, but the previous behaviour was pretty much useless, since the expanded value could change every time we add new specifiers, which we do all the time. As a compromise for backwards compatibility, only fail on alphanumerical characters. This should cover the most cases where an unescaped percent character is used, like size=5% and such, which behave the same as before with this patch. OTOH, this means that we will not be able to use non-alphanumerical specifiers without breaking backwards compatibility again. I think that's an acceptable compromise. v2: - add NEWS entry v3: - only fail on alphanumerical --- NEWS | 7 +++++++ man/systemd.unit.xml | 3 ++- man/tmpfiles.d.xml | 1 + src/shared/specifier.c | 9 ++++++++- src/test/test-systemd-tmpfiles.py | 8 ++++---- src/tmpfiles/tmpfiles.c | 14 +++++++++----- 6 files changed, 31 insertions(+), 11 deletions(-) diff --git a/NEWS b/NEWS index 77b38f5d6c..de3d28d668 100644 --- a/NEWS +++ b/NEWS @@ -7,6 +7,13 @@ CHANGES WITH 236 in spe: numdummies=0, preventing the kernel from automatically creating dummy0. All dummy interfaces must now be explicitly created. + * Unknown specifiers are now rejected. This applies to units and + tmpfiles.d configuration. Any percent characters that are followed by + a letter or digit that are not supposed to be interpreted as the + beginning of a specifier should be escaped by doubling ("%%"). + (So "size=5%" is still accepted, as well as "size=5%,foo=bar", but + not "LABEL=x%y%z" since %y and %z are not valid specifiers today.) + * systemd-resolved now maintains a new dynamic /run/systemd/resolve/stub-resolv.conf compatibility file. It is recommended to make /etc/resolv.conf a symlink to it. This file diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml index 9c40562e08..06a7bbd3ba 100644 --- a/man/systemd.unit.xml +++ b/man/systemd.unit.xml @@ -1297,7 +1297,8 @@ Many settings resolve specifiers which may be used to write generic unit files referring to runtime or unit parameters that - are replaced when the unit files are loaded. The following + are replaced when the unit files are loaded. Specifiers must be known + and resolvable for the setting to be valid. The following specifiers are understood: diff --git a/man/tmpfiles.d.xml b/man/tmpfiles.d.xml index d983984fd6..642a124eac 100644 --- a/man/tmpfiles.d.xml +++ b/man/tmpfiles.d.xml @@ -594,6 +594,7 @@ r! /tmp/.X[0-9]*-lockSpecifiersSpecifiers can be used in the "path" and "argument" fields. + An unknown or unresolvable specifier is treated as invalid configuration. The following expansions are understood:
Specifiers available diff --git a/src/shared/specifier.c b/src/shared/specifier.c index 9bef890346..9034cc20d7 100644 --- a/src/shared/specifier.c +++ b/src/shared/specifier.c @@ -41,6 +41,10 @@ * */ +/* Any ASCII character or digit: our pool of potential specifiers, + * and "%" used for escaping. */ +#define POSSIBLE_SPECIFIERS ALPHANUMERICAL "%" + int specifier_printf(const char *text, const Specifier table[], void *userdata, char **_ret) { char *ret, *t; const char *f; @@ -97,7 +101,10 @@ int specifier_printf(const char *text, const Specifier table[], void *userdata, ret = n; t = n + j + k; - } else { + } else if (strchr(POSSIBLE_SPECIFIERS, *f)) + /* Oops, an unknown specifier. */ + return -EBADSLT; + else { *(t++) = '%'; *(t++) = *f; } diff --git a/src/test/test-systemd-tmpfiles.py b/src/test/test-systemd-tmpfiles.py index 0f45bd4f76..75d8fd3657 100755 --- a/src/test/test-systemd-tmpfiles.py +++ b/src/test/test-systemd-tmpfiles.py @@ -32,10 +32,10 @@ if __name__ == '__main__': test_invalid_line('f++ /too/many/plusses') test_invalid_line('f+!+ /too/many/plusses') test_invalid_line('f!+! /too/many/bangs') - #test_invalid_line('w /unresolved/argument - - - - "%Y"') - #test_invalid_line('w /unresolved/argument/sandwich - - - - "%v%Y%v"') - #test_invalid_line('w /unresolved/filename/%Y - - - - "whatever"') - #test_invalid_line('w /unresolved/filename/sandwich/%v%Y%v - - - - "whatever"') + test_invalid_line('w /unresolved/argument - - - - "%Y"') + test_invalid_line('w /unresolved/argument/sandwich - - - - "%v%Y%v"') + test_invalid_line('w /unresolved/filename/%Y - - - - "whatever"') + test_invalid_line('w /unresolved/filename/sandwich/%v%Y%v - - - - "whatever"') test_invalid_line('w - - - - - "no file specfied"') test_invalid_line('C - - - - - "no file specfied"') test_invalid_line('C non/absolute/path - - - - -') diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index e0d96d612d..4e615288da 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -1867,7 +1867,8 @@ static int parse_line(const char *fname, unsigned line, const char *buffer, bool &age, NULL); if (r < 0) { - if (r == -EINVAL) /* invalid quoting and such */ + if (IN_SET(r, -EINVAL, -EBADSLT)) + /* invalid quoting and such or an unknown specifier */ *invalid_config = true; return log_error_errno(r, "[%s:%u] Failed to parse line: %m", fname, line); } @@ -1916,8 +1917,8 @@ static int parse_line(const char *fname, unsigned line, const char *buffer, bool if (r == -ENOKEY) return log_unresolvable_specifier(fname, line); if (r < 0) { - /* ENOMEM is the only return value left after ENOKEY, - * so *invalid_config should not be set. */ + if (IN_SET(r, -EINVAL, -EBADSLT)) + *invalid_config = true; return log_error_errno(r, "[%s:%u] Failed to replace specifiers: %s", fname, line, path); } @@ -2028,7 +2029,7 @@ static int parse_line(const char *fname, unsigned line, const char *buffer, bool return -EBADMSG; } r = parse_attribute_from_arg(&i); - if (r == -EINVAL) + if (IN_SET(r, -EINVAL, -EBADSLT)) *invalid_config = true; if (r < 0) return r; @@ -2054,9 +2055,12 @@ static int parse_line(const char *fname, unsigned line, const char *buffer, bool r = specifier_expansion_from_arg(&i); if (r == -ENOKEY) return log_unresolvable_specifier(fname, line); - if (r < 0) + if (r < 0) { + if (IN_SET(r, -EINVAL, -EBADSLT)) + *invalid_config = true; return log_error_errno(r, "[%s:%u] Failed to substitute specifiers in argument: %m", fname, line); + } if (arg_root) { char *p;