| Message ID | 20190701090904.31312-2-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | restrict bridge interface name to IFNAMSIZ | expand |
On Mon, Jul 01, 2019 at 02:39:02PM +0530, P J P wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > The interface names in qemu-bridge-helper are defined to be > of size IFNAMSIZ(=16), including the terminating null('\0') byte. > The same is applied to interface names read from 'bridge.conf' > file to form ACLs rules. If user supplied '--br=bridge' name > is not restricted to the same length, it could lead to ACL bypass > issue. Restrict interface name to IFNAMSIZ, including null byte. > > Reported-by: Riccardo Schirone <rschiron@redhat.com> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > qemu-bridge-helper.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > Update v2: report an error and exit > -> https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg06239.html > > diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c > index f9940deefd..8ec0a65174 100644 > --- a/qemu-bridge-helper.c > +++ b/qemu-bridge-helper.c > @@ -109,6 +109,13 @@ static int parse_acl_file(const char *filename, ACLList *acl_list) > } > *argend = 0; > > + if (strcmp(cmd, "include") && strlen(arg) >= IFNAMSIZ) { > + fprintf(stderr, "name `%s' too long: %lu\n", arg, strlen(arg)); strlen returns size_t, which does not match %lu - it needs %zu - we can ignore the non-portability of %zu to windows, since this code is UNIX only. I'd prefer also !g_str_equal(cmd, "include") as to me it reads more easily. > + fclose(f); > + errno = EINVAL; > + return -1; > + } > + > if (strcmp(cmd, "deny") == 0) { > acl_rule = g_malloc(sizeof(*acl_rule)); > if (strcmp(arg, "all") == 0) { > @@ -259,6 +266,10 @@ int main(int argc, char **argv) > usage(); > return EXIT_FAILURE; > } > + if (strlen(bridge) >= IFNAMSIZ) { > + fprintf(stderr, "name `%s' too long: %lu\n", bridge, strlen(bridge)); > + return EXIT_FAILURE; > + } %zu too Regards, Daniel
+-- On Mon, 1 Jul 2019, Daniel P. Berrangé wrote --+
| > + if (strcmp(cmd, "include") && strlen(arg) >= IFNAMSIZ) {
| > + fprintf(stderr, "name `%s' too long: %lu\n", arg, strlen(arg));
|
| strlen returns size_t, which does not match %lu - it needs %zu - we can
| ignore the non-portability of %zu to windows, since this code is UNIX
| only.
|
| I'd prefer also !g_str_equal(cmd, "include") as to me it reads more
| easily.
Okay. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c index f9940deefd..8ec0a65174 100644 --- a/qemu-bridge-helper.c +++ b/qemu-bridge-helper.c @@ -109,6 +109,13 @@ static int parse_acl_file(const char *filename, ACLList *acl_list) } *argend = 0; + if (strcmp(cmd, "include") && strlen(arg) >= IFNAMSIZ) { + fprintf(stderr, "name `%s' too long: %lu\n", arg, strlen(arg)); + fclose(f); + errno = EINVAL; + return -1; + } + if (strcmp(cmd, "deny") == 0) { acl_rule = g_malloc(sizeof(*acl_rule)); if (strcmp(arg, "all") == 0) { @@ -259,6 +266,10 @@ int main(int argc, char **argv) usage(); return EXIT_FAILURE; } + if (strlen(bridge) >= IFNAMSIZ) { + fprintf(stderr, "name `%s' too long: %lu\n", bridge, strlen(bridge)); + return EXIT_FAILURE; + } /* parse default acl file */ QSIMPLEQ_INIT(&acl_list);