[Qemu-devel] [PATCH v2 0/3] restrict bridge interface name to IFNAMSIZ

[Qemu-devel] [PATCH v2 0/3] restrict bridge interface name to IFNAMSIZ

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

Hello,

Linux net_deivce defines network interface name to be of IFNAMSIZE(=16)
bytes, including the terminating null('\0') byte.

Qemu tap deivce, while invoking 'qemu-bridge-helper' tool to set up the
network bridge interface, supplies bridge name of 16 characters, thus
allowing to create an ACL bypass scenario.

This patch series attempts to fix it.

Thank you.
---
Prasad J Pandit (3):
  qemu-bridge-helper: restrict interface name to IFNAMSIZ
  qemu-bridge-helper: move repeating code in parse_acl_file
  net: tap: restrict bridge name to IFNAMSIZ

 net/tap.c            |  2 +-
 qemu-bridge-helper.c | 23 ++++++++++++++++-------
 2 files changed, 17 insertions(+), 8 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v2 1/3] qemu-bridge-helper: restrict interface name to IFNAMSIZ

[P J P]

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));
+            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);
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 1/3] qemu-bridge-helper: restrict interface name to IFNAMSIZ

[Daniel P. Berrangé]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 1/3] qemu-bridge-helper: restrict interface name to IFNAMSIZ

[P J P]

+-- 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

[Qemu-devel] [PATCH v2 2/3] qemu-bridge-helper: move repeating code in parse_acl_file

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

Move repeating error handling sequence in parse_acl_file routine
to an 'err' label.

Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 qemu-bridge-helper.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index 8ec0a65174..da647de38f 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -92,9 +92,7 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
 
         if (arg == NULL) {
             fprintf(stderr, "Invalid config line:\n  %s\n", line);
-            fclose(f);
-            errno = EINVAL;
-            return -1;
+            goto err;
         }
 
         *arg = 0;
@@ -111,9 +109,7 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
 
         if (strcmp(cmd, "include") && strlen(arg) >= IFNAMSIZ) {
             fprintf(stderr, "name `%s' too long: %lu\n", arg, strlen(arg));
-            fclose(f);
-            errno = EINVAL;
-            return -1;
+            goto err;
         }
 
         if (strcmp(cmd, "deny") == 0) {
@@ -139,15 +135,17 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
             parse_acl_file(arg, acl_list);
         } else {
             fprintf(stderr, "Unknown command `%s'\n", cmd);
-            fclose(f);
-            errno = EINVAL;
-            return -1;
+            goto err;
         }
     }
 
     fclose(f);
-
     return 0;
+
+err:
+    fclose(f);
+    errno = EINVAL;
+    return -1;
 }
 
 static bool has_vnet_hdr(int fd)
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 2/3] qemu-bridge-helper: move repeating code in parse_acl_file

[Daniel P. Berrangé]

On Mon, Jul 01, 2019 at 02:39:03PM +0530, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> Move repeating error handling sequence in parse_acl_file routine
> to an 'err' label.
> 
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  qemu-bridge-helper.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[Qemu-devel] [PATCH v2 3/3] net: tap: restrict bridge name to IFNAMSIZ

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

The interface name in Linux interface request struct 'ifreq'
OR in qemu-bridge-helper is defined to be of size IFNAMSIZ(=16),
including the terminating null('\0') byte.

QEMU tap device, while invoking qemu-bridge-helper, supplies bridge
name of 16 characters, restrict it to IFNAMESIZ-1 to accommodate
terminating null('\0') byte.

Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 net/tap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tap.c b/net/tap.c
index e8aadd8d4b..ca8536624c 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -499,7 +499,7 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
     if (pid == 0) {
         int open_max = sysconf(_SC_OPEN_MAX), i;
         char fd_buf[6+10];
-        char br_buf[6+IFNAMSIZ] = {0};
+        char br_buf[5+IFNAMSIZ] = {0};
         char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + 15];
 
         for (i = 3; i < open_max; i++) {
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 3/3] net: tap: restrict bridge name to IFNAMSIZ

[Daniel P. Berrangé]

On Mon, Jul 01, 2019 at 02:39:04PM +0530, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> The interface name in Linux interface request struct 'ifreq'
> OR in qemu-bridge-helper is defined to be of size IFNAMSIZ(=16),
> including the terminating null('\0') byte.
> 
> QEMU tap device, while invoking qemu-bridge-helper, supplies bridge
> name of 16 characters, restrict it to IFNAMESIZ-1 to accommodate
> terminating null('\0') byte.
> 
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  net/tap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/tap.c b/net/tap.c
> index e8aadd8d4b..ca8536624c 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -499,7 +499,7 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
>      if (pid == 0) {
>          int open_max = sysconf(_SC_OPEN_MAX), i;
>          char fd_buf[6+10];
> -        char br_buf[6+IFNAMSIZ] = {0};
> +        char br_buf[5+IFNAMSIZ] = {0};
>          char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + 15];
>  
>          for (i = 3; i < open_max; i++) {

Playing games with multiple "perfectly" sized static buffers & snprintf
is madness. How about re-writing this method so that it just uses
g_strdup_printf() to dynamically format the helper_cmd string.

Alternatively we could get rid of the use of shell and directly exec
the helper program. This would let us just pass argv[] and avoid the
printf'ing entirely.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 3/3] net: tap: restrict bridge name to IFNAMSIZ

[P J P]

+-- On Mon, 1 Jul 2019, Daniel P. Berrangé wrote --+
| Playing games with multiple "perfectly" sized static buffers & snprintf is 
| madness. How about re-writing this method so that it just uses 
| g_strdup_printf() to dynamically format the helper_cmd string.
| 
| Alternatively we could get rid of the use of shell and directly exec the 
| helper program. This would let us just pass argv[] and avoid the printf'ing 
| entirely.

Okay, makes sense; I'll prepare patch v3.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v2 0/3] restrict bridge interface name to IFNAMSIZ

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190701090904.31312-1-ppandit@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20190701090904.31312-1-ppandit@redhat.com
Type: series
Subject: [Qemu-devel] [PATCH v2 0/3] restrict bridge interface name to IFNAMSIZ

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20190701090904.31312-1-ppandit@redhat.com -> patchew/20190701090904.31312-1-ppandit@redhat.com
Switched to a new branch 'test'
3ef9883 net: tap: restrict bridge name to IFNAMSIZ
681b23b qemu-bridge-helper: move repeating code in parse_acl_file
1293e8c qemu-bridge-helper: restrict interface name to IFNAMSIZ

=== OUTPUT BEGIN ===
1/3 Checking commit 1293e8c77f0d (qemu-bridge-helper: restrict interface name to IFNAMSIZ)
2/3 Checking commit 681b23b37e73 (qemu-bridge-helper: move repeating code in parse_acl_file)
3/3 Checking commit 3ef9883171ae (net: tap: restrict bridge name to IFNAMSIZ)
ERROR: spaces required around that '+' (ctx:VxV)
#27: FILE: net/tap.c:502:
+        char br_buf[5+IFNAMSIZ] = {0};
                      ^

total: 1 errors, 0 warnings, 8 lines checked

Patch 3/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190701090904.31312-1-ppandit@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v2 0/3] restrict bridge interface name to IFNAMSIZ

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190701090904.31312-1-ppandit@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20190701090904.31312-1-ppandit@redhat.com
Subject: [Qemu-devel] [PATCH v2 0/3] restrict bridge interface name to IFNAMSIZ
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20190621064615.20099-1-mst@redhat.com -> patchew/20190621064615.20099-1-mst@redhat.com
 - [tag update]      patchew/20190701090904.31312-1-ppandit@redhat.com -> patchew/20190701090904.31312-1-ppandit@redhat.com
 - [tag update]      patchew/20190701093034.18873-1-eric.auger@redhat.com -> patchew/20190701093034.18873-1-eric.auger@redhat.com
 - [tag update]      patchew/fc5404f7-4d1d-c28f-6e48-d8799c82acc0@web.de -> patchew/fc5404f7-4d1d-c28f-6e48-d8799c82acc0@web.de
Switched to a new branch 'test'
bb29827 net: tap: restrict bridge name to IFNAMSIZ
d7b4e94 qemu-bridge-helper: move repeating code in parse_acl_file
cced821 qemu-bridge-helper: restrict interface name to IFNAMSIZ

=== OUTPUT BEGIN ===
1/3 Checking commit cced82104140 (qemu-bridge-helper: restrict interface name to IFNAMSIZ)
2/3 Checking commit d7b4e9480bbe (qemu-bridge-helper: move repeating code in parse_acl_file)
3/3 Checking commit bb298278f680 (net: tap: restrict bridge name to IFNAMSIZ)
ERROR: spaces required around that '+' (ctx:VxV)
#27: FILE: net/tap.c:502:
+        char br_buf[5+IFNAMSIZ] = {0};
                      ^

total: 1 errors, 0 warnings, 8 lines checked

Patch 3/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190701090904.31312-1-ppandit@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com