disallow unbalanced bidirectional formatting in strings and comments #42918
Conversation
JeffBezanson
added
unicode
|
Since it's so easy, might as well check to see if this occurs in any packages :) @nanosoldier |
|
Would be good to backport this to 1.6 and 1.7. |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
|
Looks good! Are there other places where bidi overrides are allowed, other than strings and comments? A quick cursory test with
Agreed - having this in the upcoming LTS is definitely a good thing.
A number of these seem to be due to DNS failures 🤔 Is there a way to check whether any of the failed runs threw the new parse error? |
|
Just checking in what the status/plan for this is - since the fix seems proper, let's make sure this doesn't get forgotten and backported to 1.6 & 1.7. |
fredrikekre
added
backport 1.6
JeffBezanson
force-pushed
the
jb/bidi
branch
from
9f10661
to
ad57dcd
Compare
JeffBezanson
added
merge me
DilumAluthge
removed
the
merge me
|
|
||
| @test_throws ParseError Meta.parse(""" | ||
| function checkUserAccess(u::User) | ||
| if u.accessLevel != "user\u202e \u2066# users are not allowed\u2069\u2066" |
There was a problem hiding this comment.
Any reason this was indented with tabs?
There was a problem hiding this comment.
That may be from the initial report/gist I sent - I just checked my local version and it seems to have a tab character there as well, though that wasn't intentional. Not sure how it got there. It certainly isn't a functional part of the vulnerability though.
KristofferC
removed
backport 1.6
…uliaLang#42918) (cherry picked from commit 2cfebad)
|
I'm not sure how much of an issue this is in practice, but I thought it worth mentioning that the homoglyph-function (from the trojan-source repository) approach still works in 1.6.4: |
|
Hmmm. That's unfortunate. Even if the risk is mostly theoretical, it would be nice for us to get the mitigation working in 1.6.x. |
|
(To mention, just incase my comment may be seen to imply that it doesn't work in 1.7, this may also work for 1.7 I just haven't checked yet as my tiny laptop doesn't have sufficient SSD space left for multiple Julia installs simultaneously.)
Sounds good to me 👍 I'm happy to create an issue if you think it might be missed here - I don't think I'm knowledgeable enough about the parser to push a fix myself. |
Just checked, also works on 1.7. The trouble with homoglyph attacks are that to us, that's a valid identifier right now. IIRC we basically allow some Unicode Letter categories in identifiers, which has the sideeffect of allowing those. I think those would best be mitigated with either a parser warning (which I don't think we have right now) or in a linter. |
|
How breaking would it be to just disallow (i.e. make it a parser error) to have any of the relevant characters? We could run PkgEval and just fix any cases that we find. |
|
I'm not sure I'd use "fixing" here - it's imo perfectly valid to write some piece of code in a non-western script. "Fixing" would just mean arbitrarily favoring "our" script over another, to make it easier on some people while maybe being alienating to someone who just uses the letter their keyboard produces (rightfully so!). That's why I'm in favor of a linter or parser warning and not making this a hard error, preferably only when there really is a visual collision, instead of outright forbidding some characters. So I think making that an error would be extremely breaking. |
|
From triage: we don't think we can do anything about this, because there are a lot of confusable characters and it is always possible to write confusing or misleading code. IDEs are a good place to help with this though. |
Disallow unbalanced Unicode bidirectional formatting directives within strings and comments, to mitigate the "trojan source" vulnerability https://www.trojansource.codes See also JuliaLang/julia#42918
Disallow unbalanced Unicode bidirectional formatting directives within strings and comments, to mitigate the "trojan source" vulnerability https://www.trojansource.codes See also JuliaLang/julia#42918
Disallow unbalanced Unicode bidirectional formatting directives within strings and comments, to mitigate the "trojan source" vulnerability https://www.trojansource.codes See also JuliaLang/julia#42918
See https://www.trojansource.codes/, rust-lang/rust#90461 for reference.
I don't believe anything needs to be done for single-line comments, since line (paragraph) endings also terminate bidi formatting directives.