4:31 a.m.
The VM does not need read permission for its own sockets to create(),
bind(), accept() connections or to recv(), send(), etc. on connections.
This was fixed in ab9569e5460d1e4737fe8b625c67687dc2204665
(virt-aa-helper: disallow VNC socket read permissions),
but then b6465e1aa49397367a9cd0f27110b9c2280a7385
(graphics: introduce new listen type 'socket')
and acc83afe333bfadd3f7f79091d38ca3d7da1eeb2
(acc83afe333bfadd3f7f79091d38ca3d7da1eeb2) reverted it.
Unless the read permission is omitted, VMs can connect to each other's
VNC/graphics sockets.
Signed-off-by: Simon Arlott <libvirt(a)octiron.net>
---
src/security/virt-aa-helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 6e6dd1b1db..fddbdafc41 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -1053,7 +1053,7 @@ get_files(vahControl * ctl)
if (listenObj.type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET &&
listenObj.socket &&
- vah_add_file(&buf, listenObj.socket, "rw"))
+ vah_add_file(&buf, listenObj.socket, "w"))
goto cleanup;
}
}
--
2.17.1
--
Simon Arlott
5:44 a.m.
New subject: [PATCH v2] virt-aa-helper: disallow graphics socket read permissions
The VM does not need read permission for its own sockets to create,
bind(), listen(), accept() connections or to recv(), send(), etc. on
those connections.
This was fixed in ab9569e5460d1e4737fe8b625c67687dc2204665
(virt-aa-helper: disallow VNC socket read permissions),
but then b6465e1aa49397367a9cd0f27110b9c2280a7385
(graphics: introduce new listen type 'socket')
and acc83afe333bfadd3f7f79091d38ca3d7da1eeb2
(acc83afe333bfadd3f7f79091d38ca3d7da1eeb2) reverted it.
Unless the read permission is omitted, VMs can connect to each other's
VNC/graphics sockets.
Signed-off-by: Simon Arlott <libvirt(a)octiron.net>
---
Updated version that changes the test case too.
src/security/virt-aa-helper.c | 2 +-
tests/virt-aa-helper-test | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 6e6dd1b1db..fddbdafc41 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -1053,7 +1053,7 @@ get_files(vahControl * ctl)
if (listenObj.type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET &&
listenObj.socket &&
- vah_add_file(&buf, listenObj.socket, "rw"))
+ vah_add_file(&buf, listenObj.socket, "w"))
goto cleanup;
}
}
diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
index 6a6703ecf5..a3b3c01163 100755
--- a/tests/virt-aa-helper-test
+++ b/tests/virt-aa-helper-test
@@ -370,7 +370,7 @@ sed -e "s,###UUID###,$uuid,g" -e
"s,###DISK###,$disk1,g" "$template_xml" > "$tes
testme "0" "hugepages" "-r -u $valid_uuid -F
/run/hugepages/kvm/\*\*" "$test_xml"
"/run/hugepages/kvm/.*rwk,$"
sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
"s,<graphics.*>,<graphics type='vnc'
socket='/var/lib/libvirt/qemu/myself.vnc'><listen type='address'
address='0.0.0.0'/></graphics>,g" "$template_xml" >
"$test_xml"
-testme "0" "vnc socket" "-r -u $valid_uuid"
"$test_xml" "/var/lib/libvirt/qemu/myself.vnc.*rw,$"
+testme "0" "vnc socket" "-r -u $valid_uuid"
"$test_xml" "/var/lib/libvirt/qemu/myself.vnc.*\s\+w,$"
sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
"s,</devices>,<input type='passthrough'
bus='virtio'><source evdev='$disk2'
/></input></devices>,g" "$template_xml" >
"$test_xml"
testme "0" "input dev passthrough" "-r -u $valid_uuid"
"$test_xml" "$disk2.*rw,$"
--
2.17.1
--
Simon Arlott
5:11 a.m.
New subject: [PATCH v2] virt-aa-helper: disallow graphics socket read permissions
On Thu, May 28, 2020 at 12:45 PM Simon Arlott <libvirt(a)octiron.net> wrote:
The VM does not need read permission for its own sockets to create,
bind(), listen(), accept() connections or to recv(), send(), etc. on
those connections.
This was fixed in ab9569e5460d1e4737fe8b625c67687dc2204665
(virt-aa-helper: disallow VNC socket read permissions),
but then b6465e1aa49397367a9cd0f27110b9c2280a7385
(graphics: introduce new listen type 'socket')
and acc83afe333bfadd3f7f79091d38ca3d7da1eeb2
(acc83afe333bfadd3f7f79091d38ca3d7da1eeb2) reverted it.
Unless the read permission is omitted, VMs can connect to each other's
VNC/graphics sockets.
As only the defined path from the XML is allowed that would only apply
if one specified the same path in each VM definition right?
That would be borderline to "intentionally configured that way".
Signed-off-by: Simon Arlott <libvirt(a)octiron.net>
---
Updated version that changes the test case too.
src/security/virt-aa-helper.c | 2 +-
tests/virt-aa-helper-test | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 6e6dd1b1db..fddbdafc41 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
[...]
<graphics type='vnc'
socket='/var/lib/libvirt/qemu/myself.vnc'><listen type='address'
address='0.0.0.0'/></graphics>
Isn't that option above converted to "-vnc unix:path"?
Hmm, no as ./tests/qemuxml2argvdata/graphics-vnc-remove-generated-socket.args
shows it is actually removed later.
But the attribute that virt-aa-helper parses in the lines you touched
is used for good cases as well, cases like these:
./tests/qemuxml2argvdata/graphics-vnc-socket-new-cmdline.args
./tests/qemuxml2argvdata/graphics-vnc-socket.args
In which case qemu would need to write to it as it is meant to [1]:
"... Rather than using listen/port, QEMU supports a socket attribute
for listening on a unix domain socket path Since 0.8.8 . ..."
An example would be:
./tests/qemuxml2argvdata/graphics-vnc-socket.xml-22- <graphics
type='vnc'>
./tests/qemuxml2argvdata/graphics-vnc-socket.xml:23: <listen
type='socket' socket='/tmp/vnc.sock'/>
./tests/qemuxml2argvdata/graphics-vnc-socket.xml-24- </graphics>
=>
./tests/qemuxml2argvdata/graphics-vnc-socket.args:27:-vnc unix:/tmp/vnc.sock \
And if I bluntly try that there are cases where qemu then creates that socket:
$ qemu-system-x86_64 -vnc socket:/tmp/foobar
creates:
srwxrwxr-x 1 paelzer paelzer 0 Sep 1 11:43 /tmp/foobar=
Therefore qemu would need the write permission to that value IMHO.
And as I said the concern of "VMs can connect to each other" would
only be true if the admin specifies the same path in each of them
intentionally.
[1]: https://libvirt.org/formatdomain.html#graphical-framebuffers
--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd
5:19 a.m.
New subject: [PATCH v2] virt-aa-helper: disallow graphics socket read permissions
On Tue, Sep 01, 2020 at 12:11:11PM +0200, Christian Ehrhardt wrote:
On Thu, May 28, 2020 at 12:45 PM Simon Arlott
<libvirt(a)octiron.net> wrote:
>
> The VM does not need read permission for its own sockets to create,
> bind(), listen(), accept() connections or to recv(), send(), etc. on
> those connections.
>
> This was fixed in ab9569e5460d1e4737fe8b625c67687dc2204665
> (virt-aa-helper: disallow VNC socket read permissions),
> but then b6465e1aa49397367a9cd0f27110b9c2280a7385
> (graphics: introduce new listen type 'socket')
> and acc83afe333bfadd3f7f79091d38ca3d7da1eeb2
> (acc83afe333bfadd3f7f79091d38ca3d7da1eeb2) reverted it.
>
> Unless the read permission is omitted, VMs can connect to each other's
> VNC/graphics sockets.
snip
And as I said the concern of "VMs can connect to each
other" would
only be true if the admin specifies the same path in each of them
intentionally.
Protecting against administrator mis-configurations is NOT a goal
of the security drivers. We're only aiming to protect against a
compromised QEMU in whatever configuration the admin requested.
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 :|
8:22 a.m.
New subject: [PATCH v2] virt-aa-helper: disallow graphics socket read permissions
On 01/09/2020 11:19, Daniel P. Berrangé wrote:
On Tue, Sep 01, 2020 at 12:11:11PM +0200, Christian Ehrhardt wrote:
> On Thu, May 28, 2020 at 12:45 PM Simon Arlott <libvirt(a)octiron.net> wrote:
>>
>> The VM does not need read permission for its own sockets to create,
>> bind(), listen(), accept() connections or to recv(), send(), etc. on
>> those connections.
>>
>> This was fixed in ab9569e5460d1e4737fe8b625c67687dc2204665
>> (virt-aa-helper: disallow VNC socket read permissions),
>> but then b6465e1aa49397367a9cd0f27110b9c2280a7385
>> (graphics: introduce new listen type 'socket')
>> and acc83afe333bfadd3f7f79091d38ca3d7da1eeb2
>> (acc83afe333bfadd3f7f79091d38ca3d7da1eeb2) reverted it.
>>
>> Unless the read permission is omitted, VMs can connect to each other's
>> VNC/graphics sockets.
snip
> And as I said the concern of "VMs can connect to each other" would
> only be true if the admin specifies the same path in each of them
> intentionally.
Protecting against administrator mis-configurations is NOT a goal
of the security drivers. We're only aiming to protect against a
compromised QEMU in whatever configuration the admin requested.
Searching the libvirt documentation for "directory", I cannot find any
information that using the same directory for the VNC path of multiple
VMs (or any other files) is an "administrator mis-configuration".
There is no reason to grant more permissions than are necessary. We
don't grant write permission to all files that need to be read so why
should we grant connect (read) permission to all sockets that need to
be used for listening (write permission)?
--
Simon Arlott
8:19 a.m.
New subject: [PATCH v2] virt-aa-helper: disallow graphics socket read permissions
On 01/09/2020 11:11, Christian Ehrhardt wrote:
On Thu, May 28, 2020 at 12:45 PM Simon Arlott
<libvirt(a)octiron.net> wrote:
>
> The VM does not need read permission for its own sockets to create,
> bind(), listen(), accept() connections or to recv(), send(), etc. on
> those connections.
>
> This was fixed in ab9569e5460d1e4737fe8b625c67687dc2204665
> (virt-aa-helper: disallow VNC socket read permissions),
> but then b6465e1aa49397367a9cd0f27110b9c2280a7385
> (graphics: introduce new listen type 'socket')
> and acc83afe333bfadd3f7f79091d38ca3d7da1eeb2
> (acc83afe333bfadd3f7f79091d38ca3d7da1eeb2) reverted it.
>
> Unless the read permission is omitted, VMs can connect to each other's
> VNC/graphics sockets.
As only the defined path from the XML is allowed that would only apply
if one specified the same path in each VM definition right?
That would be borderline to "intentionally configured that way".
Where is it documented that it is unsafe to configure multiple VMs to
use the same directory for VNC files?
The application of AppArmor restrictions to directories instead of files
is an implementation detail that is not mentioned in the configuration
documentation. It is therefore not "intentionally configured that way"
if no users are made aware of potential security problems.
> Signed-off-by: Simon Arlott <libvirt(a)octiron.net>
> ---
> Updated version that changes the test case too.
>
> src/security/virt-aa-helper.c | 2 +-
> tests/virt-aa-helper-test | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 6e6dd1b1db..fddbdafc41 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
[...]
> <graphics type='vnc'
socket='/var/lib/libvirt/qemu/myself.vnc'><listen type='address'
address='0.0.0.0'/></graphics>
Isn't that option above converted to "-vnc unix:path"?
Hmm, no as ./tests/qemuxml2argvdata/graphics-vnc-remove-generated-socket.args
shows it is actually removed later.
But the attribute that virt-aa-helper parses in the lines you touched
is used for good cases as well, cases like these:
./tests/qemuxml2argvdata/graphics-vnc-socket-new-cmdline.args
./tests/qemuxml2argvdata/graphics-vnc-socket.args
In which case qemu would need to write to it as it is meant to [1]:
"... Rather than using listen/port, QEMU supports a socket attribute
for listening on a unix domain socket path Since 0.8.8 . ..."
I'm not trying to remove write permission. I'm trying to remove read
permission.
I sent an updated version 3 of the patch in September that includes a
comment explaining this. This appears to be the main problem with socket
permissions; everyone assumes that write permission should always imply
read permission because that is how files are normally handled.
--
Simon Arlott
3:40 a.m.
New subject: [PATCH v3] virt-aa-helper: disallow graphics socket read permissions
The VM does not need read permission for its own sockets to create,
bind(), listen(), accept() connections or to recv(), send(), etc. on
those connections.
This was fixed in ab9569e5460d1e4737fe8b625c67687dc2204665
(virt-aa-helper: disallow VNC socket read permissions),
but then b6465e1aa49397367a9cd0f27110b9c2280a7385
(graphics: introduce new listen type 'socket')
and acc83afe333bfadd3f7f79091d38ca3d7da1eeb2
(vnc: add support for listen type 'socket') reverted it.
Unless the read permission is omitted, VMs can connect to each other's
VNC/graphics sockets.
Signed-off-by: Simon Arlott <libvirt(a)octiron.net>
---
Updated version that adds a comment to the code to prevent further
weakening of security on sockets.
src/security/virt-aa-helper.c | 7 ++++++-
tests/virt-aa-helper-test | 2 +-
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 12429278fb..e9cc865552 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -1051,9 +1051,14 @@ get_files(vahControl * ctl)
for (n = 0; n < graphics->nListens; n++) {
virDomainGraphicsListenDef listenObj = graphics->listens[n];
+ /* Accepting connections on sockets only requires write
+ * permission. Making connections to them requires read
+ * permission. Don't allow read permission because VMs
+ * should not be connecting to each other.
+ */
if (listenObj.type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET &&
listenObj.socket &&
- vah_add_file(&buf, listenObj.socket, "rw"))
+ vah_add_file(&buf, listenObj.socket, "w"))
goto cleanup;
}
}
diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
index 83f53acef6..776f0b43a6 100755
--- a/tests/virt-aa-helper-test
+++ b/tests/virt-aa-helper-test
@@ -370,7 +370,7 @@ sed -e "s,###UUID###,$uuid,g" -e
"s,###DISK###,$disk1,g" "$template_xml" > "$tes
testme "0" "hugepages" "-r -u $valid_uuid -F
/run/hugepages/kvm/\*\*" "$test_xml"
"/run/hugepages/kvm/.*rwk,$"
sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
"s,<graphics.*>,<graphics type='vnc'
socket='/var/lib/libvirt/qemu/myself.vnc'><listen type='address'
address='0.0.0.0'/></graphics>,g" "$template_xml" >
"$test_xml"
-testme "0" "vnc socket" "-r -u $valid_uuid"
"$test_xml" "/var/lib/libvirt/qemu/myself.vnc.*rw,$"
+testme "0" "vnc socket" "-r -u $valid_uuid"
"$test_xml" "/var/lib/libvirt/qemu/myself.vnc.*\s\+w,$"
sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
"s,</devices>,<input type='passthrough'
bus='virtio'><source evdev='$disk2'
/></input></devices>,g" "$template_xml" >
"$test_xml"
testme "0" "input dev passthrough" "-r -u $valid_uuid"
"$test_xml" "$disk2.*rw,$"
--
2.17.1
--
Simon Arlott
1450
days inactive
1669
days old
6 comments
3 participants
participants (3)
-
Christian Ehrhardt -
Daniel P. Berrangé -
Simon Arlott