Re: [Qemu-devel] [PATCH] monitor: Add whitelist support for QMP commands

[Markus Armbruster]

I apologize for the long delay.

Julia Suvorova via Qemu-devel <address@hidden> writes:

> On 12.02.2019 10:13, Markus Armbruster wrote:
>> Julia Suvorova via Qemu-devel <address@hidden> writes:
>>
>>> On 01.02.2019 12:14, Markus Armbruster wrote:
>>>> Julia Suvorova via Qemu-devel <address@hidden> writes:
>>>>
>>>>> The whitelist option allows to run a reduced monitor with a subset of
>>>>> QMP commands. This allows the monitor to run in secure mode, which is
>>>>
>>>> For a value of "secure".  I'm not saying this can't be useful, just
>>>> tempering expecations.  I guess you intend to use this to restrict the
>>>> monitor to sufficiently harmless commands, such as commands that merely
>>>> return information without changing anything.  However, even such
>>>> commands can be abused for denial of service.  Whether that's an issue
>>>> depends on your use case.
>>>>
>>>>> convenient for sending commands via the WebSocket monitor using the
>>>>> web UI. This is planned to be done on micro:bit board.
>>>>>
>>>>> The list of allowed commands should be written to a file, one per line.
>>>>> The command line will look like this:
>>>>>       -mon chardev_name,mode=control,whitelist=path_to_file
>>>>>
>>>>> Signed-off-by: Julia Suvorova <address@hidden>
>>>>
>>>> Please describe your intended use case in more detail, and provide at
>>>> least a rough security analysis that includes the denial of service
>>>> aspect.
>>>
>>> It is planned to use the web interface for micro:bit board, e.g. send a
>>> button press as a QMP command and send a LED display changing as a QMP
>>> event, and send them via WebSocket protocol from QEMU to a web page.
>>> The monitor will also send commands from some other sensors, for
>>> example, an accelerometer/magnetometer. Therefore, it is convenient to
>>> limit the monitor so that it can send only these commands of
>>> buttons/sensors.
>>
>> Covers "describe your intended use case" nicely, thanks.  Still missing:
>> "a rough security analysis that includes the denial of service aspect".
>> Here are two questions to help you get started.  Is "the web interface"
>> trusted?  If not, what if it floods QEMU with button presses?
>
> In the case of a trusted connection, we don't even need a whitelist.
> The oob execution must be disabled in order to prohibit the direct
> execution of speculative series of commands. The regular command
> suspends the monitor and causes it to stop reading the commands until
> the execution is completed, which prevents the client from capturing too
> many resources.

It could still peg one core, at least in theory.

>                 And since the suspension of the monitor doesn't stop the
> sending of events, the display will work fine.

This is not an objection; I asked for a rough security analysis not to
challenge its premises, but to make sure you have one.  Now put it on
record by working it into your commit message.

I still have to review the actual patch.  You may want to wait for that
before you respin.