Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Right to left text and security #2981

Open
alevesely opened this issue Nov 5, 2021 · 9 comments
Open

Right to left text and security #2981

alevesely opened this issue Nov 5, 2021 · 9 comments

Comments

@alevesely
Copy link

I copied and pasted the following code from trojan-source
geany-right-to-left
As you see, line 6 is not rendered properly.
The curly braces inside RLI are unreachable by mouse or arrow keys movements.
When the same code is saved as text, the curly brace on line 8 changes from close to open as the cursor moves to the curly brace on line 9.

And, yes, there are security concerns involved. Read more

@elextr
Copy link
Member

elextr commented Nov 5, 2021

This is not something Geany can do anything about.

Right to left text is not supported by the Scintilla editing widget on the GTK backend used by Geany. As its documentation says:

"Scintilla only correctly displays bidirectional text on some platforms. Currently, there is experimental support for bidirectional text on Win32 using DirectWrite and on macOS using Cocoa."

and even on those platforms

"bidirectional features are experimental and incomplete"

Exactly how bidirectional text displays in Scintilla is undefined on GTK backends and any behaviour is luck from the behaviour of system libraries (Pango for example).

Essentially users copying text from untrusted sources need to be careful.

@alevesely
Copy link
Author

alevesely commented Nov 5, 2021

What about a popup warning about unsupported characters in the file to be opened, and offering to either remove them or abort editing?

AFAIK it's just four code points:

RLO (U+202E): treat the following text as right-to-left
LRI (U+2066): treat the following text as left-to-right, without affecting the surrounding text
RLI (U+2067): treat the following text as right-to-left, without affecting the surrounding text
PDI (U+2069): terminate the most recent LRI or RLI 

@elextr
Copy link
Member

elextr commented Nov 5, 2021

Offering only remove or close would prevent any LTR code to be edited in Geany just because some text in the file is RTL.

Remember its entirely valid for at least comments and strings to have RTL content in many/most programming languages.

And just testing on opening is insufficient as you demonstrated by pasting such text.

Possibly a better way would be for someone to provide a plugin to scan for such things and indicate where they occur (maybe by highlighting) so the user can ensure they only occur inside legal places.

@eht16
Copy link
Member

eht16 commented Nov 7, 2021

What about bringing up this topic in Scintilla first and check if Neil can maybe handle such cases in Scintilla generally?
Then all Scintilla users would benefit, not only Geany and we don't need to implement any workarounds or custom limitations.

@alevesely would you mind opening an issue about this topic for Scintilla directly?

@alevesely
Copy link
Author

@alevesely would you mind opening an issue about this topic for Scintilla directly?

I'm not interested in RTL editing. It is undoubtedly a relevant topic which will better editing for a lot of people who speak those languages. However, I'm not interested in that.

The main point is the security concerns that Trojan Source brought up. Geany fails to handle that code properly, which might happen to alert a user that there's something wrong with that file. Thus, by chance, Geany provides a rickety protection against that kind of attacks. Until scintilla handles RTL, Geany could just fail more elegantly and provide a more solid protection.

@eht16
Copy link
Member

eht16 commented Nov 7, 2021

I agree with your points.
But my point was that it should be handled by Scintilla, ideally. Then Geany doesn't need to do anything special.
If the Scintilla project rejects or postpones it, we'll need to implement a warning or the like. But if it is already handled in Scintilla, this would be better for Geany and for the other projects using it.

@alevesely
Copy link
Author

Here

@alevesely
Copy link
Author

So, Neil Hodgson pointed out that this can be solved by setting representations for the Unicode points RLO (U+202E), LRI (U+2066), RLI (U+2067), and PDI (U+2069). Can this be done by a plugin?.

@elextr
Copy link
Member

elextr commented Nov 8, 2021

Since Geany does not use Scintilla representations AFAICT, a plugin could set them without conflicting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants