[Qemu-devel] [PATCH] qemu-bridge-helper: restrict bridge name to IFNAMSIZ

[Qemu-devel] [PATCH] qemu-bridge-helper: restrict bridge 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 bridge 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index f9940deefd..2eca8c5cc4 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -246,7 +246,7 @@ int main(int argc, char **argv)
         if (strcmp(argv[index], "--use-vnet") == 0) {
             use_vnet = 1;
         } else if (strncmp(argv[index], "--br=", 5) == 0) {
-            bridge = &argv[index][5];
+            bridge = strndup(&argv[index][5], IFNAMSIZ - 1);
         } else if (strncmp(argv[index], "--fd=", 5) == 0) {
             unixfd = atoi(&argv[index][5]);
         } else {
-- 
2.21.0

Re: [Qemu-devel] [PATCH] qemu-bridge-helper: restrict bridge name to IFNAMSIZ

[Daniel P. Berrangé]

On Fri, Jun 28, 2019 at 03:19:01PM +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 bridge name to IFNAMSIZ, including null byte.

Can you elaborate on the way to exploit this as I'm not seeing
any way that doesn't involve mis-configuration of the ACL
config file data.

Currently the code has:

    const char *bridge = NULL;
    ...
    bridge = &argv[index][5];
    ...
    if (strcmp(bridge, acl_rule->iface) == 0) {

So the user suplied "--br=bridge" name will never be
truncated.

The ACLRule struct has iface[IFNAMSIZ] so it is possible
that we truncate the bridge name that's in the bridge.conf
file if the admin put in a name that was too long.

This is simply an admin mis-configuration, since it is
impossible for a bridge to actually exist that has a
name longer than IFNAMSIZ.

I think we simply need to report a hard error when
reading the bridge.conf file if the name we find does
not fit into IFNAMSIZ.


> 
> Reported-by: Riccardo Schirone <rschiron@redhat.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  qemu-bridge-helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
> index f9940deefd..2eca8c5cc4 100644
> --- a/qemu-bridge-helper.c
> +++ b/qemu-bridge-helper.c
> @@ -246,7 +246,7 @@ int main(int argc, char **argv)
>          if (strcmp(argv[index], "--use-vnet") == 0) {
>              use_vnet = 1;
>          } else if (strncmp(argv[index], "--br=", 5) == 0) {
> -            bridge = &argv[index][5];
> +            bridge = strndup(&argv[index][5], IFNAMSIZ - 1);

This is not fixing the real problem imho

>          } else if (strncmp(argv[index], "--fd=", 5) == 0) {
>              unixfd = atoi(&argv[index][5]);
>          } else {
> -- 
> 2.21.0
> 

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] qemu-bridge-helper: restrict bridge name to IFNAMSIZ

[P J P]

+-- On Fri, 28 Jun 2019, Daniel P. Berrangé wrote --+
| Can you elaborate on the way to exploit this as I'm not seeing
| any way that doesn't involve mis-configuration of the ACL
| config file data.

True, it depends on having an 'allow all' rule. If the bridge.conf had an 
'allow all' rule below

==
deny BridgeLength0xF
allow all
==

And user supplied name as --br=BridgeLength0xFun

    if (strcmp(bridge, acl_rule->iface) == 0) {

the strcmp(3) above would not match the deny ACL rule, because given bridge 
name is longer. And qemu-bridge-helper would go on to connect the tap device 
with a bridge that is configured to have access denied.


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

Re: [Qemu-devel] [PATCH] qemu-bridge-helper: restrict bridge name to IFNAMSIZ

[Daniel P. Berrangé]

On Fri, Jun 28, 2019 at 04:51:31PM +0530, P J P wrote:
> +-- On Fri, 28 Jun 2019, Daniel P. Berrangé wrote --+
> | Can you elaborate on the way to exploit this as I'm not seeing
> | any way that doesn't involve mis-configuration of the ACL
> | config file data.
> 
> True, it depends on having an 'allow all' rule. If the bridge.conf had an 
> 'allow all' rule below
> 
> ==
> deny BridgeLength0xF
> allow all
> ==
> 
> And user supplied name as --br=BridgeLength0xFun
> 
>     if (strcmp(bridge, acl_rule->iface) == 0) {
> 
> the strcmp(3) above would not match the deny ACL rule, because given bridge 
> name is longer. And qemu-bridge-helper would go on to connect the tap device 
> with a bridge that is configured to have access denied.

Ok, so we should explicitly report an error if the user supplied
bridge name is too long, not silently truncate it.

We should also reoprt an error if config file has too long a bridge
name.

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] qemu-bridge-helper: restrict bridge name to IFNAMSIZ

[P J P]

+-- On Fri, 28 Jun 2019, Daniel P. Berrangé wrote --+
| Ok, so we should explicitly report an error if the user supplied bridge name 
| is too long, not silently truncate it.
| 
| We should also report an error if config file has too long a bridge name.

Okay, ie. report error and exit?

+-- On Fri, 28 Jun 2019, Li Qiang wrote --+
| I think we should cleanup the bridge in the final.

I did free(bridge) first, it showed build error because 'bridge' is declared 
as const char *, not sure why, didn't want to make patch too intrusive. If we 
exit like Dan suggests above, guess it wouldn't be required.


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

Re: [Qemu-devel] [PATCH] qemu-bridge-helper: restrict bridge name to IFNAMSIZ

[Daniel P. Berrangé]

On Fri, Jun 28, 2019 at 05:32:50PM +0530, P J P wrote:
> +-- On Fri, 28 Jun 2019, Daniel P. Berrangé wrote --+
> | Ok, so we should explicitly report an error if the user supplied bridge name 
> | is too long, not silently truncate it.
> | 
> | We should also report an error if config file has too long a bridge name.
> 
> Okay, ie. report error and exit?

Yes, exactly.

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] qemu-bridge-helper: restrict bridge name to IFNAMSIZ

[Li Qiang]

Hello Prasad,

P J P <ppandit@redhat.com> 于2019年6月28日周五 下午5:52写道：

> 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 bridge 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
> index f9940deefd..2eca8c5cc4 100644
> --- a/qemu-bridge-helper.c
> +++ b/qemu-bridge-helper.c
> @@ -246,7 +246,7 @@ int main(int argc, char **argv)
>          if (strcmp(argv[index], "--use-vnet") == 0) {
>              use_vnet = 1;
>          } else if (strncmp(argv[index], "--br=", 5) == 0) {
> -            bridge = &argv[index][5];
> +            bridge = strndup(&argv[index][5], IFNAMSIZ - 1);
>


I thinke we should cleanup the bridge in the final.

Thanks,
Li Qiang



>          } else if (strncmp(argv[index], "--fd=", 5) == 0) {
>              unixfd = atoi(&argv[index][5]);
>          } else {
> --
> 2.21.0
>
>
>