Option to filter-out entire ANSI escape sequences for `qrexec-client(-vm)` and/or `qvm-run`

alimirjamali commented 1 month ago

The problem you're addressing (if any)

The current existing code of libqrexec-utils/do_replace_chars merely replaces escape characters with underline. This is problematic for the programs which do not have the option/configuration for avoiding ANSI escape sequences. Most specifically text coloring and graphic rendition sequences. Just filtering the escape character and not the entire sequence renders the output hard to read/parse.

One of the examples is DNF5 which will be the default in Fedora 41 (discussed here on forum)

The solution you'd like

Either improving the existing -t, -T, --filter-escape-chars* options to filter-out the entire known sequences instead of just replacing the escape character with underline. Or introducing new options for proper filtering.

The value to a user, and who that user might be

Users will not require to learn/specify plain-text output option for individual programs (if applicable) and could use a generic option for all.
Without this option, qubes-dom0-update output in R4.3 will be bad (the only existing alternative is to disable the progress bar).

Completion criteria checklist

(This section is for developer use only. Please do not modify it.)

marmarek commented 1 month ago

The current option is meant for safety, not for data transformation. Remote side should not send any control sequences, but the option ensures even if it would do, it would not be effective. It's all to protect terminal emulator from potential attacks (there were many vulnerabilities like this in the past, and some are still applicable). Technically, I'd also avoid variable-length sequence processing in the receiving side - there are many corner cases, like detecting where such sequence would end, handling sequences split across multiple data chunks etc. Getting it wrong may mean for example allowing some sequence that should be filtered otherwise. That's extra attack surface on the trusted side (especially since in this case it would apply to the part running in dom0).

The proper solution is for the remote side to not send those sequences. If the application in question (DNF5) doesn't support turning it off, it can be wrapped in a tool that does filtering on the remote side. Maybe even sed would suffice?

But also, what about (asking upstream to) adding an option to disable coloring? Or, stop wrapping the call in script, so that DNF doesn't have PTY, but a socket/pipe for stdin/out and shouldn't use ANSI sequences then. This will likely break real time download progress bar, though...

alimirjamali commented 1 month ago

Maybe even sed would suffice?

Yes. Sed could do that. There are ready to use Regex formulas for that.

But also, what about (asking upstream to) adding an option to disable coloring?

I can work on that. But other programs suffer from the same symptom. Gentoo's emerge is one of them.

Or, stop wrapping the call in script

This is what I am currently doing personally. Maybe this should be the default in R4.3? Most people are using GUI updater anyways and do not see progress bar at all.

alimirjamali commented 1 month ago

One more idea on how to make this safe and eliminate any possible extra attack surface. The filtering code could do its job based on Regex formulas and well known/well tested regex replace libraries. Finally the old code could replace any possible/remaining instances of escape codes with underlines.

marmarek commented 1 month ago

This is what I am currently doing personally. Maybe this should be the default in R4.3? Most people are using GUI updater anyways and do not see progress bar at all.

I guess that's a short term solution. The proper solution is to refactor dom0 update mechanism for a proper stdout/err signaling protocol (so, it reports machine-readable info that qubes-dom0-update or any frontend like qubes-update-gui can use for a proper progress bar). But that's obviously a lot more work. Hopefully we'll be there one day @piotrbartman :wink:

marmarek commented 1 month ago

One more idea on how to make this safe and eliminate any possible extra attack surface. The filtering code could do its job based on Regex formulas and well known/well tested regex replace libraries.

Still, I'd prefer this "advanced" filtering be done on the sending side. It could be even part of the qrexec (either built-in, or a helper wrapper that any service could use in their implementation).

alimirjamali commented 1 month ago

BTW, I explored the possibility of using an ANSI to Gtk's Pango markup converter. So the output in GUI Updater could be in full color. But I rejected the idea as all available libraries are not a part of Python official libraries and it might introduce further attack surface.

Even though it is replatively simple to write one from scratch, I guess it is better to avoid the idea since Qubes is a security oriented project.

alimirjamali commented 1 month ago

alimirjamali commented 1 week ago

If the application in question (DNF5) doesn't support turning it off, it can be wrapped in a tool that does filtering on the remote side.

My PR to DNF5 to re-implement the --color=<always|never|auto> flag has been pending review for over a month. It is a low priority issue for DNF5 team. I guess it would be better to write the remote side wrapper tool.

QubesOS / qubes-issues