shadowsocks / shadowsocks/shadowsocks-org
[Security] Do not engineer vulnerabilities into implementations without public discussions
Nobody has claimed this yet.
- Dominant language
- HTML
- Stars
- 1k
- Forks
- 694
- PR merge metrics
- No merged PRs in 30d
Description
The security-iv-printable-prefix feature in shadowsocks-rust, implemented by https://github.com/shadowsocks/shadowsocks-rust/commit/53aab484f8daba6f5cee6896b034af943cc3d406 and https://github.com/shadowsocks/shadowsocks-rust/commit/312faea756eee2c170903689a8ab3c162f10480c, and the ReducedIVEntropy option in v2ray, introduced in https://github.com/v2fly/v2ray-core/pull/1552, were rushed into existing implementations without public discussions.
Why were these features added?
During that period of time, the GFW was blocking a lot of Shadowsocks servers. It was then discovered by @gfw-report that prepending 6 bytes of printable charaters to the stream could exempt the TCP connection from blocking. @gfw-report notified several developers about the discovery. A few of them made the decision to implement the "feature". It was kind of understandable, since not getting blocked is one of the most important goals of censorship circumvention protocols. But the way it was executed was horrible and extremely short-sighted.
Why is this bad?
First of all, there are better ways to achieve the desired effect. We could have worked out a better solution in the beginning if we held public discussions.
This workaround already requires modified clients. Instead of modifying existing clients, we could just run another pair of client and server. The client accepts a connection, opens a connection, writes out the pre-shared prefix, then splice(2)'s the connections together. The server accepts a connection, reads and validates the prefix, opens a connection, then splice(2)'s the connections together. On non-Linux platforms we have to fallback to socket reads and writes, but otherwise this has almost zero overhead, does not introduce vulnerabilities to the protocol itself, and requires coordination of both client and server (which is important and you'll see why later). One can easily implement this in less than 100 lines of Go code, since Go already took care of the splice(2) and fallback part.
But instead of doing all that, we had a bunch of email exchanges behind closed doors, then a subpar solution was implemented by two of the most used Shadowsocks implementations. There are so many things wrong with the current implementation:
- It makes Shadowsocks identifiable.
- The server has no control of whether a client can use this feature, let alone knowing whether a client is using it.
- It's easy to enable and use. It's not at all uncommon for shadowsocks-rust users to build their own binaries. All it takes is adding
--features security-iv-printable-prefix. With V2Ray it's as easy as flipping a switch.
What we were doing is basically planting vulnerabilities into our implementations. You might get away with the reduced salt entropy without measurable impacts on the encryption strength, but implementing it solely on the client side allows any user to compromise Shadowsocks servers by simply toggling this feature on. Your average user can now elect to become active probes for the adversary. Imagine the friend who's been freeloading your Shadowsocks server suddenly decides to enable this feature, without telling you as usual, of course.
What should be done about it?
- Remove this engineered vulnerability from implementations.
- In the future, any significant deviations from the spec should be discussed out in the open.
/cc @zonyitoo @xiaokangwang
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by reviewing the referenced shadowsocks-rust commits and the v2ray ReducedIVEntropy pull request, then trace how these options are exposed in each implementation. Done means the engineered vulnerability is removed from the implementations and future significant deviations are handled through public discussion.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, rust
- Domain
- networking, security
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100