diff mbox series

qemu-bridge-helper: restrict bridge name to IFNAMSIZ

Message ID 20190628094901.13347-1-ppandit@redhat.com (mailing list archive)
State New, archived
Headers show
Series qemu-bridge-helper: restrict bridge name to IFNAMSIZ | expand

Commit Message

Prasad Pandit June 28, 2019, 9:49 a.m. UTC 
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(-)

Comments

Daniel P. Berrangé June 28, 2019, 11:04 a.m. UTC | #1 
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
Prasad Pandit June 28, 2019, 11:21 a.m. UTC | #2 
+-- 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
Daniel P. Berrangé June 28, 2019, 11:32 a.m. UTC | #3 
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
Li Qiang June 28, 2019, 11:42 a.m. UTC | #4 
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
>
>
>
Prasad Pandit June 28, 2019, 12:02 p.m. UTC | #5 
+-- 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
Daniel P. Berrangé June 28, 2019, 12:18 p.m. UTC | #6 
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
diff mbox series

Patch

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 {