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

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

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