Skip to content
This repository has been archived by the owner on Nov 2, 2018. It is now read-only.

Tomfoolery #1703

Closed
Closed

Conversation

VoidingWarranties
Copy link
Collaborator

This PR is intended to look benign, but isn't :) Can you see the vulnerability this change introduces? (hint: look at why TestAcceptPeer fails) It's an admittedly contrived example, but nonetheless possible. What do you think?

@lukechampine
Copy link
Member

wow, that is sneaky. At first I thought p was shadowing the p *peer argument, but that seemed too easy. Then I flipped back because it worked when I changed the variable name. Then I got REALLY wigged out when I started getting undefined: р in р.NetAddress errors. I thought maybe there was some lexical scoping rule in Go that I wasn't aware of. I won't give it away so David and John have a chance to try.

@DavidVorick
Copy link
Member

Going to admit here, @lukechampine had to tell me what was wrong. I stared at it for probably half an hour without figuring it out.

  1. Thank god we have a test to check correctness
  2. I'm sure there are places in our code where you could get something this awful through without triggering a build failure.

Bravo, this is really clever. Thanks for sharing :)

@lukechampine
Copy link
Member

Inspired by this, I'm going to write a linter that checks for suspicious unicode. Easiest approach would be to simply ban any character larger than a certain value, but that could be overreaching for non-english programmers. Maybe that's acceptable, not sure.

@VoidingWarranties
Copy link
Collaborator Author

The linter is great! Thanks for writing it. It's much better than my kludgy grep-foo. I'm glad that homographic attacks have been getting more attention recently, although this is still the first example I've seen of a homographic attack applied to source code.

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

Successfully merging this pull request may close these issues.

None yet

3 participants