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

["Daniel P. Berrangé" <berrange@redhat.com>]

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