[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qemu-bridge-helper: restrict bridge name to IFN
From: |
Daniel P . Berrangé |
Subject: |
Re: [Qemu-devel] [PATCH] qemu-bridge-helper: restrict bridge name to IFNAMSIZ |
Date: |
Fri, 28 Jun 2019 12:04:57 +0100 |
User-agent: |
Mutt/1.12.0 (2019-05-25) |
On Fri, Jun 28, 2019 at 03:19:01PM +0530, P J P wrote:
> From: Prasad J Pandit <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
> 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 :|