Add github.com/breml/bidichk linter

[breml]

bidichk checks for dangerous unicode character sequences

The following unicode characters are considered dangerous:

• U+202A: LEFT-TO-RIGHT-EMBEDDING
• U+202B: RIGHT-TO-LEFT-EMBEDDING
• U+202C: POP-DIRECTIONAL-FORMATTING
• U+202D: LEFT-TO-RIGHT-OVERRIDE
• U+202E: RIGHT-TO-LEFT-OVERRIDE
• U+2066: LEFT-TO-RIGHT-ISOLATE
• U+2067: RIGHT-TO-LEFT-ISOLATE
• U+2068: FIRST-STRONG-ISOLATE
• U+2069: POP-DIRECTIONAL-ISOLATE

See following links for background:

• 'Trojan Source' Bug Threatens the Security of All Code
• Trojan Source - Proofs-of-Concept
• Go proposal: disallow LTR/RTL characters in string literals?
• cockroachdb/cockroach - PR: lint: add linter for unicode directional formatting characters

[boring-cyborg]

Hey, thank you for opening your first Pull Request !

[CLAassistant]

All committers have signed the CLA.

[SVilgelm]

Hi, I have checks the code of the linter. Here is my thoughts:

1. this code https://github.com/breml/bidichk/blob/master/pkg/bidichk/bidichk.go#L26-L30 canoe simplified:

return check(f.Name(), f.Pos(0), pass) == nil

2. It will be great to have an ability to configure the list of forbidden codes:

func SetCodes(codes map[string]rune) &analysis.Analyzer {
    disallowedRunes = codes
    return Analyzer
}

[SVilgelm]

3. please bump a release, in this case the Dependabot will be able to upgrade the version of the linter

[ldez]

Hello,

I think that asciicheck already checks those cases.

https://golangci-lint.run/usage/linters/#disabled-by-default-linters--e--enable

Quick example:

.golangci.yml

linters:
  disable-all: true
  enable:
    - asciicheck

example.go

package sandbox

import "fmt"

func gitһub() {
	fmt.Println("Hello")
}

$ golangci-lint run
foo.go:7:6: identifier "gitһub" contain non-ASCII character: U+04BB 'һ' (asciicheck)
func gitһub() {
     ^

#2329 (comment)

[ldez]

I checked and it seems that asciicheck doesn't analyze comments, but I'm not sure how to exploit a comment with this kind of character.
It also doesn't check literal string, maybe asciicheck can be improved.

[breml]

@ldez Have a look at https://github.com/nickboucher/trojan-source/blob/main/Go/commenting-out.go for examples how to exploit with comments. And yes, asciicheck does not check this case. bidichk does not operate on the parsed/tokenized Go code. It inspects the body of the file it self and therefore also string literals and comments. The whole attack works sort of similar to SQL injection attacks.

[ldez]

I tried to run the file, but on Linux with go1.17 or go1.16, I don't reproduce.
Maybe it's only working on MacOS.

[ldez]

bidichk does not operate on the parsed/tokenized Go code. It inspects the body of the file it self and therefore also string literals and comments.

I think there will be a performance issue, I check that.

[breml]

@SVilgelm created an initial release v0.1.0. I will have a look at your other suggestions as well.

[breml]

I tried to run the file, but on Linux with go1.17 or go1.16, I don't reproduce. Maybe it's only working on MacOS.

@ldez I am on Linux as well, using Go 1.16 and I can clearly see the bug.

I cloned the repo, entered the directory Go.

If you look at the file, you see:

package main

import "fmt"

func main() {
	var isAdmin = false
	var isSuperAdmin = false
	isAdmin = isAdmin || isSuperAdmin
	/*‮ } ⁦if (isAdmin)⁩ ⁦ begin admins only */
		fmt.Println("You are an admin.")
	/* end admins only ‮ { ⁦*/
}

If you run the code with e.g. go run commenting-out.go, you get:

$ go run commenting-out.go 
You are an admin.

But this is not what you expect from the code, because both variables, isAdmin and isSuperAdmin are initialized to false, so the condition also resolves to false. Therefore the if shouldn't evaluate to true nor should the line You are an admin. be printed.

So what the compiler sees is (more like):

package main

import "fmt"

func main() {
	var isAdmin = false
	var isSuperAdmin = false
	isAdmin = isAdmin || isSuperAdmin
	/* } if (isAdmin) begin admin only */
		fmt.Println("You are an admin.")
	/* end admins only } */
}

So the complete if is commented out.

[ldez]

I tested the implementation and the performance seems ok.

[SVilgelm]

checked the code again and found one more place for improvement:

			if !bytes.ContainsRune(body[start:], r) {
				break
			}
			start += bytes.IndexRune(body[start:], r)

this code scans a file two times: !bytes.ContainsRune and bytes.IndexRune, I would recommend to improve it to scan only one time, it will increase performance.

			idx := bytes.IndexRune(body[start:], r)
			if idx == -1 {
				break
			}
			start += idx

[ldez]

@SVilgelm maybe you can open a PR or an issue on the bidichk repo?
It can be easier to follow.

[knz]

See this comment: it may not be wise including the isolate characters in this list.

[SVilgelm]

@SVilgelm maybe you can open a PR or an issue on the bidichk repo?
It can be easier to follow.

maybe, but It is hard to find a time for golangci-lint, I just need to add more details in the comments :)

[ldez]

@SVilgelm I will help you and I will open a PR 😄

[ldez]

LGTM

[golangci-automator]

Hey, @breml — we just merged your PR to golangci-lint! 🔥🚀

golangci-lint is built by awesome people like you. Let us say “thanks”: we just invited you to join the GolangCI organization on GitHub.
This will add you to our team of maintainers. Accept the invite by visiting this link.

By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.
More information about contributing is here.

Thanks again!