diff mbox series

ui/clipboard: avoid crash upon request when clipboard peer is not initialized

Message ID 20240112135527.57212-1-f.ebner@proxmox.com
State New
Headers show
Series ui/clipboard: avoid crash upon request when clipboard peer is not initialized | expand

Commit Message

Fiona Ebner Jan. 12, 2024, 1:55 p.m. UTC 
With VNC, it can be that a client sends a VNC_MSG_CLIENT_CUT_TEXT
message before sending a VNC_MSG_CLIENT_SET_ENCODINGS message with
VNC_ENCODING_CLIPBOARD_EXT for configuring the clipboard extension.

This means that qemu_clipboard_request() can be reached (via
vnc_client_cut_text_ext()) before vnc_server_cut_text_caps() was
called and had the chance to initialize the clipboard peer. In that
case, info->owner->request is NULL instead of a function and so
attempting to call it in qemu_clipboard_request() results in a
segfault.

In particular, this can happen when using the KRDC (22.12.3) VNC
client on Wayland.

It is not enough to check in ui/vnc.c's protocol_client_msg() if the
VNC_FEATURE_CLIPBOARD_EXT feature is enabled before handling an
extended clipboard message with vnc_client_cut_text_ext(), because of
the following scenario with two clients (say noVNC and KRDC):

The noVNC client sets the extension VNC_FEATURE_CLIPBOARD_EXT and
initializes its cbpeer.

The KRDC client does not, but triggers a vnc_client_cut_text() (note
it's not the _ext variant)). There, a new clipboard info with it as
the 'owner' is created and via qemu_clipboard_set_data() is called,
which in turn calls qemu_clipboard_update() with that info.

In qemu_clipboard_update(), the notifier for the noVNC client will be
called, i.e. vnc_clipboard_notify() and also set vs->cbinfo for the
noVNC client. The 'owner' in that clipboard info is the clipboard peer
for the KRDC client, which did not initialize the 'request' function.
That sounds correct to me, it is the owner of that clipboard info.

Then when noVNC sends a VNC_MSG_CLIENT_CUT_TEXT message (it did set
the feature correctly, so the check added by your patch passes), that
clipboard info is passed to qemu_clipboard_request() and the original
segfault still happens.

Fixes: CVE-2023-6683
Reported-by: Markus Frank <m.frank@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Tested-by: Markus Frank <m.frank@proxmox.com>
---

This is just a minimal fix. Happy to add some warning/error to not
hide the issue with the missing initialization completely and/or go
for a different approach with a check somewhere in the VNC code.

 ui/clipboard.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Fiona Ebner Jan. 12, 2024, 2:11 p.m. UTC | #1 
Am 12.01.24 um 14:55 schrieb Fiona Ebner:
> 
> Then when noVNC sends a VNC_MSG_CLIENT_CUT_TEXT message (it did set
> the feature correctly, so the check added by your patch passes),

Sorry, forgot to adapt this part. This should read "it did set the
VNC_FEATURE_CLIPBOARD_EXT feature correctly, so a check for it passes".

> that
> clipboard info is passed to qemu_clipboard_request() and the original
> segfault still happens.
>
Marc-André Lureau Jan. 14, 2024, 1:51 p.m. UTC | #2 
Hi

On Fri, Jan 12, 2024 at 5:57 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>
> With VNC, it can be that a client sends a VNC_MSG_CLIENT_CUT_TEXT
> message before sending a VNC_MSG_CLIENT_SET_ENCODINGS message with
> VNC_ENCODING_CLIPBOARD_EXT for configuring the clipboard extension.
>
> This means that qemu_clipboard_request() can be reached (via
> vnc_client_cut_text_ext()) before vnc_server_cut_text_caps() was
> called and had the chance to initialize the clipboard peer. In that
> case, info->owner->request is NULL instead of a function and so
> attempting to call it in qemu_clipboard_request() results in a
> segfault.
>
> In particular, this can happen when using the KRDC (22.12.3) VNC
> client on Wayland.
>
> It is not enough to check in ui/vnc.c's protocol_client_msg() if the
> VNC_FEATURE_CLIPBOARD_EXT feature is enabled before handling an
> extended clipboard message with vnc_client_cut_text_ext(), because of
> the following scenario with two clients (say noVNC and KRDC):
>
> The noVNC client sets the extension VNC_FEATURE_CLIPBOARD_EXT and
> initializes its cbpeer.
>
> The KRDC client does not, but triggers a vnc_client_cut_text() (note
> it's not the _ext variant)). There, a new clipboard info with it as
> the 'owner' is created and via qemu_clipboard_set_data() is called,
> which in turn calls qemu_clipboard_update() with that info.
>
> In qemu_clipboard_update(), the notifier for the noVNC client will be
> called, i.e. vnc_clipboard_notify() and also set vs->cbinfo for the
> noVNC client. The 'owner' in that clipboard info is the clipboard peer
> for the KRDC client, which did not initialize the 'request' function.
> That sounds correct to me, it is the owner of that clipboard info.
>
> Then when noVNC sends a VNC_MSG_CLIENT_CUT_TEXT message (it did set
> the feature correctly, so the check added by your patch passes), that
> clipboard info is passed to qemu_clipboard_request() and the original
> segfault still happens.
>
> Fixes: CVE-2023-6683
> Reported-by: Markus Frank <m.frank@proxmox.com>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> Tested-by: Markus Frank <m.frank@proxmox.com>
> ---
>
> This is just a minimal fix. Happy to add some warning/error to not
> hide the issue with the missing initialization completely and/or go
> for a different approach with a check somewhere in the VNC code.
>
>  ui/clipboard.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/ui/clipboard.c b/ui/clipboard.c
> index 3d14bffaf8..c13b54d2e9 100644
> --- a/ui/clipboard.c
> +++ b/ui/clipboard.c
> @@ -129,7 +129,8 @@ void qemu_clipboard_request(QemuClipboardInfo *info,
>      if (info->types[type].data ||
>          info->types[type].requested ||
>          !info->types[type].available ||
> -        !info->owner)
> +        !info->owner ||
> +        !info->owner->request)
>          return;

While that fixes the crash, I think we should handle the situation
earlier. A clipboard peer shouldn't be allowed to hold the clipboard
if it doesn't have the data available or a "request" callback set.

Iow, we should have an assert(info->owner->request != NULL) here instead.

>      info->types[type].requested = true;
> --
> 2.39.2
>
>
>
Fiona Ebner Jan. 15, 2024, 10:44 a.m. UTC | #3 
Am 14.01.24 um 14:51 schrieb Marc-André Lureau:
>>
>> diff --git a/ui/clipboard.c b/ui/clipboard.c
>> index 3d14bffaf8..c13b54d2e9 100644
>> --- a/ui/clipboard.c
>> +++ b/ui/clipboard.c
>> @@ -129,7 +129,8 @@ void qemu_clipboard_request(QemuClipboardInfo *info,
>>      if (info->types[type].data ||
>>          info->types[type].requested ||
>>          !info->types[type].available ||
>> -        !info->owner)
>> +        !info->owner ||
>> +        !info->owner->request)
>>          return;
> 
> While that fixes the crash, I think we should handle the situation
> earlier. A clipboard peer shouldn't be allowed to hold the clipboard
> if it doesn't have the data available or a "request" callback set.
> 

Where should initialization of the cbpeer happen so that we are
guaranteed to do it also for clients that do not set the
VNC_FEATURE_CLIPBOARD_EXT feature? Can the vnc_clipboard_request
function be re-used for clients without that feature or will it be
necessary to add some kind of "dummy" request callback for those clients?

> Iow, we should have an assert(info->owner->request != NULL) here instead.
>
Your choice of course, but it would be a crash again should the issue
ever re-appear. Would error message (so the issue gets noticed) + return
be an option too?

Best Regards,
Fiona
Marc-André Lureau Jan. 15, 2024, 11:15 a.m. UTC | #4 
Hi

On Mon, Jan 15, 2024 at 2:45 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>
> Am 14.01.24 um 14:51 schrieb Marc-André Lureau:
> >>
> >> diff --git a/ui/clipboard.c b/ui/clipboard.c
> >> index 3d14bffaf8..c13b54d2e9 100644
> >> --- a/ui/clipboard.c
> >> +++ b/ui/clipboard.c
> >> @@ -129,7 +129,8 @@ void qemu_clipboard_request(QemuClipboardInfo *info,
> >>      if (info->types[type].data ||
> >>          info->types[type].requested ||
> >>          !info->types[type].available ||
> >> -        !info->owner)
> >> +        !info->owner ||
> >> +        !info->owner->request)
> >>          return;
> >
> > While that fixes the crash, I think we should handle the situation
> > earlier. A clipboard peer shouldn't be allowed to hold the clipboard
> > if it doesn't have the data available or a "request" callback set.
> >
>
> Where should initialization of the cbpeer happen so that we are
> guaranteed to do it also for clients that do not set the
> VNC_FEATURE_CLIPBOARD_EXT feature? Can the vnc_clipboard_request
> function be re-used for clients without that feature or will it be
> necessary to add some kind of "dummy" request callback for those clients?

qemu_clipboard_update() shouldn't accept info as current clipboard if
the owner doesn't have the data available or a "request" callback set.
This should also be assert() somehow and handled earlier.

In vnc_client_cut_text_ext() we could detect that situation, but with
Daniel's "[PATCH] ui: reject extended clipboard message if not
activated", this shouldn't happen anymore iiuc.

>
> > Iow, we should have an assert(info->owner->request != NULL) here instead.
> >
> Your choice of course, but it would be a crash again should the issue
> ever re-appear. Would error message (so the issue gets noticed) + return
> be an option too?
>
> Best Regards,
> Fiona
>
Fiona Ebner Jan. 15, 2024, 11:26 a.m. UTC | #5 
Am 15.01.24 um 12:15 schrieb Marc-André Lureau:
> Hi
> 
> On Mon, Jan 15, 2024 at 2:45 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>>
>> Am 14.01.24 um 14:51 schrieb Marc-André Lureau:
>>>>
>>>> diff --git a/ui/clipboard.c b/ui/clipboard.c
>>>> index 3d14bffaf8..c13b54d2e9 100644
>>>> --- a/ui/clipboard.c
>>>> +++ b/ui/clipboard.c
>>>> @@ -129,7 +129,8 @@ void qemu_clipboard_request(QemuClipboardInfo *info,
>>>>      if (info->types[type].data ||
>>>>          info->types[type].requested ||
>>>>          !info->types[type].available ||
>>>> -        !info->owner)
>>>> +        !info->owner ||
>>>> +        !info->owner->request)
>>>>          return;
>>>
>>> While that fixes the crash, I think we should handle the situation
>>> earlier. A clipboard peer shouldn't be allowed to hold the clipboard
>>> if it doesn't have the data available or a "request" callback set.
>>>
>>
>> Where should initialization of the cbpeer happen so that we are
>> guaranteed to do it also for clients that do not set the
>> VNC_FEATURE_CLIPBOARD_EXT feature? Can the vnc_clipboard_request
>> function be re-used for clients without that feature or will it be
>> necessary to add some kind of "dummy" request callback for those clients?
> 
> qemu_clipboard_update() shouldn't accept info as current clipboard if
> the owner doesn't have the data available or a "request" callback set.
> This should also be assert() somehow and handled earlier.
> 

The request callback is only initialized in vnc_server_cut_text_caps()
when the VNC_FEATURE_CLIPBOARD_EXT is enabled. AFAIU, it's perfectly
fine for clients to use the clipboard with non-extended messages and
qemu_clipboard_update() should (and currently does) accept those.

> In vnc_client_cut_text_ext() we could detect that situation, but with
> Daniel's "[PATCH] ui: reject extended clipboard message if not
> activated", this shouldn't happen anymore iiuc.
> 

Daniel's patch doesn't change the behavior for non-extended messages.
The problem can still happen with two VNC clients. This is the scenario
described in the lower half of my commit message (and why Daniel
mentions in his patch that it's not sufficient to fix the CVE).

In short: client A does not set the VNC_FEATURE_CLIPBOARD_EXT feature
and then uses a non-extended VNC_MSG_CLIENT_CUT_TEXT message. This leads
to vnc_client_cut_text() being called and setting the clipboard info
referencing that client. But here, no request callback is initialized,
that only happens in vnc_server_cut_text_caps() when the
VNC_FEATURE_CLIPBOARD_EXT is enabled.

When client B does set the VNC_FEATURE_CLIPBOARD_EXT feature and does
send an extended VNC_MSG_CLIENT_CUT_TEXT message, the request callback
will be attempted but it isn't set.

Best Regards,
Fiona
Marc-André Lureau Jan. 15, 2024, 11:33 a.m. UTC | #6 
Hi

On Mon, Jan 15, 2024 at 3:26 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>
> Am 15.01.24 um 12:15 schrieb Marc-André Lureau:
> > Hi
> >
> > On Mon, Jan 15, 2024 at 2:45 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
> >>
> >> Am 14.01.24 um 14:51 schrieb Marc-André Lureau:
> >>>>
> >>>> diff --git a/ui/clipboard.c b/ui/clipboard.c
> >>>> index 3d14bffaf8..c13b54d2e9 100644
> >>>> --- a/ui/clipboard.c
> >>>> +++ b/ui/clipboard.c
> >>>> @@ -129,7 +129,8 @@ void qemu_clipboard_request(QemuClipboardInfo *info,
> >>>>      if (info->types[type].data ||
> >>>>          info->types[type].requested ||
> >>>>          !info->types[type].available ||
> >>>> -        !info->owner)
> >>>> +        !info->owner ||
> >>>> +        !info->owner->request)
> >>>>          return;
> >>>
> >>> While that fixes the crash, I think we should handle the situation
> >>> earlier. A clipboard peer shouldn't be allowed to hold the clipboard
> >>> if it doesn't have the data available or a "request" callback set.
> >>>
> >>
> >> Where should initialization of the cbpeer happen so that we are
> >> guaranteed to do it also for clients that do not set the
> >> VNC_FEATURE_CLIPBOARD_EXT feature? Can the vnc_clipboard_request
> >> function be re-used for clients without that feature or will it be
> >> necessary to add some kind of "dummy" request callback for those clients?
> >
> > qemu_clipboard_update() shouldn't accept info as current clipboard if
> > the owner doesn't have the data available or a "request" callback set.
> > This should also be assert() somehow and handled earlier.
> >
>
> The request callback is only initialized in vnc_server_cut_text_caps()
> when the VNC_FEATURE_CLIPBOARD_EXT is enabled. AFAIU, it's perfectly
> fine for clients to use the clipboard with non-extended messages and
> qemu_clipboard_update() should (and currently does) accept those.
>
> > In vnc_client_cut_text_ext() we could detect that situation, but with
> > Daniel's "[PATCH] ui: reject extended clipboard message if not
> > activated", this shouldn't happen anymore iiuc.
> >
>
> Daniel's patch doesn't change the behavior for non-extended messages.
> The problem can still happen with two VNC clients. This is the scenario
> described in the lower half of my commit message (and why Daniel
> mentions in his patch that it's not sufficient to fix the CVE).
>
> In short: client A does not set the VNC_FEATURE_CLIPBOARD_EXT feature
> and then uses a non-extended VNC_MSG_CLIENT_CUT_TEXT message. This leads
> to vnc_client_cut_text() being called and setting the clipboard info
> referencing that client. But here, no request callback is initialized,
> that only happens in vnc_server_cut_text_caps() when the
> VNC_FEATURE_CLIPBOARD_EXT is enabled.
>
> When client B does set the VNC_FEATURE_CLIPBOARD_EXT feature and does
> send an extended VNC_MSG_CLIENT_CUT_TEXT message, the request callback
> will be attempted but it isn't set.
>

The trouble is when qemu_clipboard_update() is called without data &
without a request callback set. We shouldn't allow that as we have no
means to get the clipboard data then.
Fiona Ebner Jan. 15, 2024, 11:48 a.m. UTC | #7 
Am 15.01.24 um 12:33 schrieb Marc-André Lureau:
> Hi
> 
> On Mon, Jan 15, 2024 at 3:26 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>>
>> Am 15.01.24 um 12:15 schrieb Marc-André Lureau:
>>> Hi
>>>
>>> On Mon, Jan 15, 2024 at 2:45 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>>>>
>>>> Am 14.01.24 um 14:51 schrieb Marc-André Lureau:
>>>>>>
>>>>>> diff --git a/ui/clipboard.c b/ui/clipboard.c
>>>>>> index 3d14bffaf8..c13b54d2e9 100644
>>>>>> --- a/ui/clipboard.c
>>>>>> +++ b/ui/clipboard.c
>>>>>> @@ -129,7 +129,8 @@ void qemu_clipboard_request(QemuClipboardInfo *info,
>>>>>>      if (info->types[type].data ||
>>>>>>          info->types[type].requested ||
>>>>>>          !info->types[type].available ||
>>>>>> -        !info->owner)
>>>>>> +        !info->owner ||
>>>>>> +        !info->owner->request)
>>>>>>          return;
>>>>>
>>>>> While that fixes the crash, I think we should handle the situation
>>>>> earlier. A clipboard peer shouldn't be allowed to hold the clipboard
>>>>> if it doesn't have the data available or a "request" callback set.
>>>>>
>>>>
>>>> Where should initialization of the cbpeer happen so that we are
>>>> guaranteed to do it also for clients that do not set the
>>>> VNC_FEATURE_CLIPBOARD_EXT feature? Can the vnc_clipboard_request
>>>> function be re-used for clients without that feature or will it be
>>>> necessary to add some kind of "dummy" request callback for those clients?
>>>
>>> qemu_clipboard_update() shouldn't accept info as current clipboard if
>>> the owner doesn't have the data available or a "request" callback set.
>>> This should also be assert() somehow and handled earlier.
>>>
>>
>> The request callback is only initialized in vnc_server_cut_text_caps()
>> when the VNC_FEATURE_CLIPBOARD_EXT is enabled. AFAIU, it's perfectly
>> fine for clients to use the clipboard with non-extended messages and
>> qemu_clipboard_update() should (and currently does) accept those.
>>
>>> In vnc_client_cut_text_ext() we could detect that situation, but with
>>> Daniel's "[PATCH] ui: reject extended clipboard message if not
>>> activated", this shouldn't happen anymore iiuc.
>>>
>>
>> Daniel's patch doesn't change the behavior for non-extended messages.
>> The problem can still happen with two VNC clients. This is the scenario
>> described in the lower half of my commit message (and why Daniel
>> mentions in his patch that it's not sufficient to fix the CVE).
>>
>> In short: client A does not set the VNC_FEATURE_CLIPBOARD_EXT feature
>> and then uses a non-extended VNC_MSG_CLIENT_CUT_TEXT message. This leads
>> to vnc_client_cut_text() being called and setting the clipboard info
>> referencing that client. But here, no request callback is initialized,
>> that only happens in vnc_server_cut_text_caps() when the
>> VNC_FEATURE_CLIPBOARD_EXT is enabled.
>>
>> When client B does set the VNC_FEATURE_CLIPBOARD_EXT feature and does
>> send an extended VNC_MSG_CLIENT_CUT_TEXT message, the request callback
>> will be attempted but it isn't set.
>>
> 
> The trouble is when qemu_clipboard_update() is called without data &
> without a request callback set. We shouldn't allow that as we have no
> means to get the clipboard data then.
> 

In the above scenario, I'm pretty sure there is data when
qemu_clipboard_update() is called. Just no request callback. If we'd
reject this, won't that break clients that do not set the
VNC_FEATURE_CLIPBOARD_EXT feature and only use non-extended
VNC_MSG_CLIENT_CUT_TEXT messages?

Best Regards,
Fiona
Marc-André Lureau Jan. 15, 2024, noon UTC | #8 
Hi

On Mon, Jan 15, 2024 at 3:48 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>
> Am 15.01.24 um 12:33 schrieb Marc-André Lureau:
> > Hi
> >
> > On Mon, Jan 15, 2024 at 3:26 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
> >>
> >> Am 15.01.24 um 12:15 schrieb Marc-André Lureau:
> >>> Hi
> >>>
> >>> On Mon, Jan 15, 2024 at 2:45 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
> >>>>
> >>>> Am 14.01.24 um 14:51 schrieb Marc-André Lureau:
> >>>>>>
> >>>>>> diff --git a/ui/clipboard.c b/ui/clipboard.c
> >>>>>> index 3d14bffaf8..c13b54d2e9 100644
> >>>>>> --- a/ui/clipboard.c
> >>>>>> +++ b/ui/clipboard.c
> >>>>>> @@ -129,7 +129,8 @@ void qemu_clipboard_request(QemuClipboardInfo *info,
> >>>>>>      if (info->types[type].data ||
> >>>>>>          info->types[type].requested ||
> >>>>>>          !info->types[type].available ||
> >>>>>> -        !info->owner)
> >>>>>> +        !info->owner ||
> >>>>>> +        !info->owner->request)
> >>>>>>          return;
> >>>>>
> >>>>> While that fixes the crash, I think we should handle the situation
> >>>>> earlier. A clipboard peer shouldn't be allowed to hold the clipboard
> >>>>> if it doesn't have the data available or a "request" callback set.
> >>>>>
> >>>>
> >>>> Where should initialization of the cbpeer happen so that we are
> >>>> guaranteed to do it also for clients that do not set the
> >>>> VNC_FEATURE_CLIPBOARD_EXT feature? Can the vnc_clipboard_request
> >>>> function be re-used for clients without that feature or will it be
> >>>> necessary to add some kind of "dummy" request callback for those clients?
> >>>
> >>> qemu_clipboard_update() shouldn't accept info as current clipboard if
> >>> the owner doesn't have the data available or a "request" callback set.
> >>> This should also be assert() somehow and handled earlier.
> >>>
> >>
> >> The request callback is only initialized in vnc_server_cut_text_caps()
> >> when the VNC_FEATURE_CLIPBOARD_EXT is enabled. AFAIU, it's perfectly
> >> fine for clients to use the clipboard with non-extended messages and
> >> qemu_clipboard_update() should (and currently does) accept those.
> >>
> >>> In vnc_client_cut_text_ext() we could detect that situation, but with
> >>> Daniel's "[PATCH] ui: reject extended clipboard message if not
> >>> activated", this shouldn't happen anymore iiuc.
> >>>
> >>
> >> Daniel's patch doesn't change the behavior for non-extended messages.
> >> The problem can still happen with two VNC clients. This is the scenario
> >> described in the lower half of my commit message (and why Daniel
> >> mentions in his patch that it's not sufficient to fix the CVE).
> >>
> >> In short: client A does not set the VNC_FEATURE_CLIPBOARD_EXT feature
> >> and then uses a non-extended VNC_MSG_CLIENT_CUT_TEXT message. This leads
> >> to vnc_client_cut_text() being called and setting the clipboard info
> >> referencing that client. But here, no request callback is initialized,
> >> that only happens in vnc_server_cut_text_caps() when the
> >> VNC_FEATURE_CLIPBOARD_EXT is enabled.
> >>
> >> When client B does set the VNC_FEATURE_CLIPBOARD_EXT feature and does
> >> send an extended VNC_MSG_CLIENT_CUT_TEXT message, the request callback
> >> will be attempted but it isn't set.
> >>
> >
> > The trouble is when qemu_clipboard_update() is called without data &
> > without a request callback set. We shouldn't allow that as we have no
> > means to get the clipboard data then.
> >
>
> In the above scenario, I'm pretty sure there is data when
> qemu_clipboard_update() is called. Just no request callback. If we'd
> reject this, won't that break clients that do not set the
> VNC_FEATURE_CLIPBOARD_EXT feature and only use non-extended
> VNC_MSG_CLIENT_CUT_TEXT messages?

If "data" is already set, then qemu_clipboard_request() returns.

Inverting the condition I suggested above: it's allowed to be the
clipboard owner if either "data" is set, or a request callback is set.
Fiona Ebner Jan. 16, 2024, 12:11 p.m. UTC | #9 
Am 15.01.24 um 13:00 schrieb Marc-André Lureau:
>>>>
>>>
>>> The trouble is when qemu_clipboard_update() is called without data &
>>> without a request callback set. We shouldn't allow that as we have no
>>> means to get the clipboard data then.
>>>
>>
>> In the above scenario, I'm pretty sure there is data when
>> qemu_clipboard_update() is called. Just no request callback. If we'd
>> reject this, won't that break clients that do not set the
>> VNC_FEATURE_CLIPBOARD_EXT feature and only use non-extended
>> VNC_MSG_CLIENT_CUT_TEXT messages?
> 
> If "data" is already set, then qemu_clipboard_request() returns.
> 
> Inverting the condition I suggested above: it's allowed to be the
> clipboard owner if either "data" is set, or a request callback is set.
> 

Oh, sorry. Yes, it seems the problematic case is where data is not set.
But isn't that legitimate when clearing the clipboard? Or is a
VNC_MSG_CLIENT_CUT_TEXT message not valid when len is 0 and should be
rejected? In my testing KRDC does send such a message when the clipboard
is cleared:

> #1  0x0000558f1e6a0dac in vnc_client_cut_text (vs=0x558f207754d0, len=0, 
>     text=0x558f2046e008 "\003 \002\377\005") at ../ui/vnc-clipboard.c:313
> #2  0x0000558f1e68e067 in protocol_client_msg (vs=0x558f207754d0, 
>     data=0x558f2046e000 "\006", len=8) at ../ui/vnc.c:2454

Your suggestion would disallow this for clients that do not set the
VNC_FEATURE_CLIPBOARD_EXT feature (and only use non-extended
VNC_MSG_CLIENT_CUT_TEXT messages).

Best Regards,
Fiona
Fiona Ebner Jan. 17, 2024, 10:59 a.m. UTC | #10 
Am 16.01.24 um 13:11 schrieb Fiona Ebner:
> Am 15.01.24 um 13:00 schrieb Marc-André Lureau:
>>>>>
>>>>
>>>> The trouble is when qemu_clipboard_update() is called without data &
>>>> without a request callback set. We shouldn't allow that as we have no
>>>> means to get the clipboard data then.
>>>>
>>>
>>> In the above scenario, I'm pretty sure there is data when
>>> qemu_clipboard_update() is called. Just no request callback. If we'd
>>> reject this, won't that break clients that do not set the
>>> VNC_FEATURE_CLIPBOARD_EXT feature and only use non-extended
>>> VNC_MSG_CLIENT_CUT_TEXT messages?
>>
>> If "data" is already set, then qemu_clipboard_request() returns.
>>
>> Inverting the condition I suggested above: it's allowed to be the
>> clipboard owner if either "data" is set, or a request callback is set.
>>
> 
> Oh, sorry. Yes, it seems the problematic case is where data is not set.
> But isn't that legitimate when clearing the clipboard? Or is a
> VNC_MSG_CLIENT_CUT_TEXT message not valid when len is 0 and should be
> rejected? In my testing KRDC does send such a message when the clipboard
> is cleared:
> 
>> #1  0x0000558f1e6a0dac in vnc_client_cut_text (vs=0x558f207754d0, len=0, 
>>     text=0x558f2046e008 "\003 \002\377\005") at ../ui/vnc-clipboard.c:313
>> #2  0x0000558f1e68e067 in protocol_client_msg (vs=0x558f207754d0, 
>>     data=0x558f2046e000 "\006", len=8) at ../ui/vnc.c:2454
> 
> Your suggestion would disallow this for clients that do not set the
> VNC_FEATURE_CLIPBOARD_EXT feature (and only use non-extended
> VNC_MSG_CLIENT_CUT_TEXT messages).
> 

I noticed that there is yet another code path leading to
qemu_clipboard_request() and dereferencing the NULL owner->request even
if only non-extended VNC_MSG_CLIENT_CUT_TEXT messages are used:

> Thread 1 "qemu-system-x86" hit Breakpoint 1, vnc_client_cut_text (
>     vs=0x5555593e6c00, len=0, 
>     text=0x5555575ea608 "404078,bus=ahci0.0,drive=drive-sata0,id=sata0,bootindex=100\377\001") at ../ui/vnc-clipboard.c:310
> (gdb) c
> Continuing.

And later:

> qemu_clipboard_request (info=0x555557e576e0, type=QEMU_CLIPBOARD_TYPE_TEXT) at ../ui/clipboard.c:129
> 129	    if (info->types[type].data ||
> (gdb) p *info->owner
> $65 = {name = 0x0, notifier = {notify = 0x0, node = {le_next = 0x0, le_prev = 0x0}}, request = 0x0}
> (gdb) bt
> #0  qemu_clipboard_request (info=0x555557e576e0, type=QEMU_CLIPBOARD_TYPE_TEXT) at ../ui/clipboard.c:129
> #1  0x00005555558952ce in vdagent_chr_recv_chunk (vd=0x555556f67800) at ../ui/vdagent.c:769
> #2  vdagent_chr_write (chr=<optimized out>, buf=0x7fff4ab263e4 "", len=0) at ../ui/vdagent.c:840
> #3  0x0000555555d98830 in qemu_chr_write_buffer (s=s@entry=0x555556f67800, buf=buf@entry=0x7fff4ab263c0 "\001", len=36, 
>     offset=offset@entry=0x7fffffffd030, write_all=false) at ../chardev/char.c:122
> #4  0x0000555555d98c3c in qemu_chr_write (s=0x555556f67800, buf=buf@entry=0x7fff4ab263c0 "\001", len=len@entry=36, 
>     write_all=<optimized out>, write_all@entry=false) at ../chardev/char.c:186
> #5  0x0000555555d9109f in qemu_chr_fe_write (be=be@entry=0x555556e59040, buf=buf@entry=0x7fff4ab263c0 "\001", len=len@entry=36)
>     at ../chardev/char-fe.c:42
> #6  0x0000555555900045 in flush_buf (port=0x555556e58f40, buf=0x7fff4ab263c0 "\001", len=36) at ../hw/char/virtio-console.c:63
> #7  0x0000555555bea4f3 in do_flush_queued_data (port=0x555556e58f40, vq=0x555559272558, vdev=0x55555926a4d0)
>     at ../hw/char/virtio-serial-bus.c:188
> #8  0x0000555555c201ff in virtio_queue_notify_vq (vq=0x555559272558) at ../hw/virtio/virtio.c:2268
> #9  0x0000555555e36dd5 in aio_dispatch_handler (ctx=ctx@entry=0x555556e51b10, node=0x7ffed812b9f0) at ../util/aio-posix.c:372
> #10 0x0000555555e37662 in aio_dispatch_handlers (ctx=0x555556e51b10) at ../util/aio-posix.c:414
> #11 aio_dispatch (ctx=0x555556e51b10) at ../util/aio-posix.c:424
> #12 0x0000555555e4d44e in aio_ctx_dispatch (source=<optimized out>, callback=<optimized out>, user_data=<optimized out>)
>     at ../util/async.c:358
> #13 0x00007ffff753e7a9 in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> #14 0x0000555555e4ecb8 in glib_pollfds_poll () at ../util/main-loop.c:287
> #15 os_host_main_loop_wait (timeout=58675786) at ../util/main-loop.c:310
> #16 main_loop_wait (nonblocking=nonblocking@entry=0) at ../util/main-loop.c:589
> #17 0x0000555555aa5f63 in qemu_main_loop () at ../system/runstate.c:791
> #18 0x0000555555cadc16 in qemu_default_main () at ../system/main.c:37
> #19 0x00007ffff60801ca in __libc_start_call_main (main=main@entry=0x5555558819d0 <main>, argc=argc@entry=102, 
>     argv=argv@entry=0x7fffffffd458) at ../sysdeps/nptl/libc_start_call_main.h:58
> #20 0x00007ffff6080285 in __libc_start_main_impl (main=0x5555558819d0 <main>, argc=102, argv=0x7fffffffd458, init=<optimized out>, 
>     fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffd448) at ../csu/libc-start.c:360
> #21 0x0000555555883581 in _start ()

So such non-extended VNC_MSG_CLIENT_CUT_TEXT messages with len=0 are
already problematic. Is clearing the clipboard supposed to be done
differently?

Your suggestion would prevent this scenario too. I'll send a patch with
that shortly.

Best Regards,
Fiona
diff mbox series

Patch

diff --git a/ui/clipboard.c b/ui/clipboard.c
index 3d14bffaf8..c13b54d2e9 100644
--- a/ui/clipboard.c
+++ b/ui/clipboard.c
@@ -129,7 +129,8 @@  void qemu_clipboard_request(QemuClipboardInfo *info,
     if (info->types[type].data ||
         info->types[type].requested ||
         !info->types[type].available ||
-        !info->owner)
+        !info->owner ||
+        !info->owner->request)
         return;
 
     info->types[type].requested = true;