Allow comm open messages

[saulshanabrook]

I have a mimerender extension that opens comms from the client side to communicate with the server (https://github.com/Quansight/ibis-vega-transform).

xref vidartf/phoila#11 vidartf/phoila#7 Quansight/ibis-vega-transform#15

[vidartf]

Relevant parts of previous discussion:

@vidartf:

Note: There might be an argument against opening comm's from the client side (although there are clearly strong arguments for allowing it), as it might allow arbitrary code execution from the client side.

@saulshanabrook:

Aren't we kinda forced to open them from the client side in a mime renderer? Otherwise, when would the server open the comm? We need a listener on the client registered before the server would open it.

I mean it doesn't matter to me really how it's done, I just need bidirectional communication from frontend to backend. Whatever way we decide is the sanctioned way to do that, I am happy to adopt. See some longer thoughts here ("Protocol Agnostic") Quansight/ibis-vega-transform#14

[SylvainCorlay]

Thanks @saulshanabrook !

Note: There might be an argument against opening comm's from the client side (although there are clearly strong arguments for allowing it), as it might allow arbitrary code execution from the client side.

Not including comm_open was more of an oversight by me orignially. Is comm_open more likely to execute arbitrary code than other comm messages?

[saulshanabrook]

Not including comm_open was more of an oversight by me orignially. Is comm_open more likely to execute arbitrary code than other comm messages?

AFAIK you have to have a comm handler registered on the backend for a comm_open so it won't allow arbitrary code execution unless your handler does (which is the same as other comm messages)

[SylvainCorlay]

I am fine with this change! Leaving it open for a bit to leave a chance to comment for more people.

[vidartf]

so it won't allow arbitrary code execution unless your handler does (which is the same as other comm messages)

I guess there is only a super construed corner case where it would be an issue:

• A widgets module registers some targets that directly/indirectly allows some remote execution.
• The notebook code imports that module.

Possible at-risk cases:

• Widgets that take a file-path as a synced trait, and do something with it kernel side (e.g. read/write).
• Widgets that take a URL as a synced trait, and do something with it kernel side.
• Widgets that exec/eval something kernel side that includes a synced trait.

I haven't been able to find any such cases in core ipywidgets, and a few other libs I searched tough.

[saulshanabrook]

I guess there is only a super construed corner case where it would be an issue:

Yeah that is an issue. Basically it means you have to vet all your python code for registered comm handlers. Voila could print this as well when it starts up, so at least you know the attack area. And if you see one that you don't recognize, then you can dig into it.

[SylvainCorlay]

I guess there is only a super construed corner case where it would be an issue:

A widgets module registers some targets that directly/indirectly allows some remote execution.
The notebook code imports that module.

Simply a notebook calling eval on the content of a textarea is not something we can prevent either, unless we assume that the notebook and content of the execution environment are trusted. If the latter is true, I think this PR is fine as well.

[maartenbreddels]

Interesting security issue, let me think about it before we merge.

I also noticed we don't allow comm close events, I guess that is less of a security issue.

[SylvainCorlay]

We discussed this at our daily standup today.

• comm_open is somewhat different from comm_msg in that
  • the latter requires the notebook to be trusted code (and not include a widget allowing arbitrary code execution).
  • the former can trigger the instantiation of any widget that is installed in the kernel environment.

So the requirement for comm_open to be safe is that the environment only contains trusted code while the requirement for comm_msg to be safe is that the notebook only contains trusted code.

Now, we think that creating widgets from the frontend is a much needed feature in voila. We think that we should merge this and make jupyter_server have user warnings for when execute_request and comm_open are allowed messages.

[vidartf]

Note: I think the comm targets will only be registered when the python code that registers it is imported. Right?

[vidartf]

and make jupyter_server have user warnings for when execute_request and comm_open are allowed messages.

This seems strange to me. Should it give these warnings when starting notebook/lab ?

[saulshanabrook]

Note: I think the comm targets will only be registered when the python code that registers it is imported. Right?

Yeah.