Re-evaluate unicode tricks in comments

[chriseth]

We should re-evaluate which unicode characters we disallow in comments and strings. Especially RTL markers come to mind.

[axic]

Here are the lists of different control blocks: https://en.wikipedia.org/wiki/Unicode_control_characters

Perhaps we should disallow control blocks and add some explicit exceptions for the ones we want to allow. As for RTL it possibly is something we want to support.

[christianparpart]

Interesting site: https://www.compart.com/en/unicode/bidiclass

• RLO (right-to-left override)
• LRO (left-to-right override)
• PDF (pop directional override)

There are some "Isolation" marks that seem to be stackable. I'm not sure yet we should take care about those.

[Meekohi]

I assume people have seen the relation to this, but thought I'd re-post it here to emphasize this issue's importance: https://github.com/ethereum/solidity-underhanded-contest/tree/master/submissions_2020/submission11_RobertMCForster

[cameel]

Yeah, it's this submission that actually prompted the issue :) Thanks for posting the link.

As for the current status, #10326 patched the specific problem with LRO/RLO/PDF and #10607 is a pending proposal of a more general approach to such problems. Feedback is welcome.

[axic]

This was fixed with #10326, wasn't it?

[axic]

Rust (CVE-2021-42574) and Go (issue #20209) is also affected.

[maurelian]

This was fixed with #10326, wasn't it?

Would be nice to get confirmation!

[cameel]

#10326 added checks against LRO/RLO/PDF but Unicode is a large beast and I'm sure there are other edge cases that can be used maliciously. The issue description/title sounds like it was meant to cover it in general, not just this specific case.

[maurelian]

Hmmm, it sounds like the checks added in #10326 are focused on unbalanced codepoints, which I interpret as similar to imbalanced brackets (ie. () :: RLO LRO)?

FWIW, the attack shown in the Rust release doesn't look to be out of balance.

Either way, given the prominence of the announcement today, it'd be very helpful to get a statement from the Solidity team clarifying the status of compiler WRT to this issue.

[tin-z]

Hi, i link here a poc of CVE-2021-42574 for solidity and solc https://github.com/tin-z/solidity_CVE-2021-42574-POC

[cameel]

Hmmm, it sounds like the checks added in #10326 are focused on unbalanced codepoints, which I interpret as similar to imbalanced brackets (ie. () :: RLO LRO)?

Yes. That was the case used in the underhanded contest. Banning unbalanced checks looked like it was enough because it prevented mixing the content of strings/comments with the outside text.

I see that the Rust example uses a slightly different trick - with the isolate characters LRI/RLI. Looks they make possible to just take out a bit of text out of the comment/string and render it at the beginning/end of the line. I think we should disallow these. Created a separate issue covering it: #13936.

Hi, i link here a poc of CVE-2021-42574 for solidity and solc https://github.com/tin-z/solidity_CVE-2021-42574-POC

Yeah, not really sure how far we should take it. Maybe we should just disallow all the override/embed chars like Rust did. What's your opinion?

This should not trick the highlighter so the problem should be easily discovered but I guess you could say the same about the trick from the underhanded contest.

[github-actions]

This issue has been marked as stale due to inactivity for the last 90 days.
It will be automatically closed in 7 days.

[github-actions]

Hi everyone! This issue has been automatically closed due to inactivity.
If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen.
However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.