Protect against RTLO problems

[Septias]

fixes #3479 This pr strips all BIDI embedding controls in group names, system messages and file names. Since there is no function like create_system_message which funnels all system message creations I decided to move the safety-check to when the messages are actuall sent because all messages which could do harm will pass that border, right? If this is not the correct place please tell me @reviewees.

[Septias]

This check is actually sender-wise. So attacker with old or no deltachat-client could still create falsy messages. The check should thus be on the receiving side, right?

[Septias]

also, i agree, the receiving side is more important than tweaking sending; maybe altering sending is even superfluous then.

Makes sense to also strip them on the sending side for none-deltachat recepients I guess. @r10s

[r10s]

Makes sense to also strip them on the sending side for none-deltachat recepients I guess.

probably okay, also as otherwise things will be displayed differently for sender and receiver.

but if in doubt, receiving side is far more important.

[Septias]

At the moment I am not quite sure how to implement my solution, maybe you can help me @r10s @hpk42 @Hocuri @link2xt. Basically, we probably don't want any unexpected RTLO-Problems in our whole codebase so protecting against that should be pretty far up in the hierarchy. That's what makes me think that adding a function that protects against RTLO should be added to Mimeparser so that the Mimemessage only has safe fields right from the start. This is a big change though and I don't know how that would affect integration like what happens if some real user X already has some RTLO characters in his name and sends messages. The secured Mimemessage would probably disconnect the user X on the recipient Y's phone because Y only knew X with the name containing RTLO characters. Probably two ways to think about a solution:

1. Don't mind because RTLO characters shouldn't be used anywhere besides for attacks so < 1% of users would be affected.
2. Check the users' name and if it contains RTLO-characters notify the recipients about a change of name.

The displayed problem is probably just one of many cases though because MimeMessage is such a fundamental part of the code. I am not quite sure how big the hassle is to change this and if it is even worth the effort for such a small feature. (Security is an important topic though!)

The other way to secure messages would be to check every occurrence of

• Creating a chat
• Creating an ad-hoc-group
• Creating a group
• Creating a mailing list
• Changing a group name
• Changing a user name
• etc...

and add security measures at the places where it is needed at last. This doesn't change the MimeMessage but can produce imparity as well. Also, adding code in so many places is hard to comprehend - especially for me who is not too familiar with the codebase - so missing an important part or introducing imparities is quite a risk. Furthermore, this approach needs to be maintained as well so every developer changing code has to think about RTLO attacks and how not to introduce new loopholes.

[Hocuri]

Sorry that I didn't respond for such a long time.

This is a big change though and I don't know how that would affect integration like what happens if some real user X already has some RTLO characters in his name and sends messages.

If it's only in the displayname (like, Septias), then it's fine, the contact will simply be renamed. If it's in the email address (like, [email protected]), then, this might create problems, because we use email addresses to identify contacts.

I'm not sure what to do about email addresses - stripping characters can mean that we treat two different email addresses as the same, leading to new possible attacks. Completely treating them as invalid would be possible, but break completely break the app for anyone using such an email address, and make it impossible to communicate with such people.

Also, adding code in so many places is hard to comprehend

Doing this in improve_single_line_input() should cover most of the cases already. Mabye also in normalize_name(), in case that's not called in all places where improve_single_line_input() is called, anyway.

[Septias]

can you take a look at this @link2xt so we can finally close this long open pr?

[link2xt]

:rofl: there is the same problem with GitHub

Maybe create this file dynamically in the test though? Because this may cause troubles checking out the repository if similar protections are implemented, having such filename in a folder is not nice for doing ls, browsing it etc.