Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security] Do not engineer vulnerabilities into implementations without public discussions #204

Open
database64128 opened this issue Sep 11, 2022 · 7 comments

Comments

@database64128
Copy link
Contributor

The security-iv-printable-prefix feature in shadowsocks-rust, implemented by shadowsocks/shadowsocks-rust@53aab48 and shadowsocks/shadowsocks-rust@312faea, and the ReducedIVEntropy option in v2ray, introduced in v2fly/v2ray-core#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:

  1. It makes Shadowsocks identifiable.
  2. The server has no control of whether a client can use this feature, let alone knowing whether a client is using it.
  3. 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

@Mygod
Copy link
Contributor

Mygod commented Sep 11, 2022

I think it was added as a temporary workaround. I do agree that people should be better informed about its negative effects in the long term.

@zonyitoo
Copy link
Contributor

I decided to add it into shadowsocks-rust was just for allowing users to "test" whether this strategy would help for preventing servers to be detected. I do agree that this is not a perfect solution and possibly increases the risk of servers to be detected by adding identities. It is Ok for me to remove it immediately, because it seems that there is no obvious benefit gain from this testing feature.

@chuxi
Copy link

chuxi commented Sep 27, 2022

  1. It does not need to update the client side to get prefix printable chars. It just updated nonce generation.
  2. for my version, I changed the length of prefix 6 to the whole nonce length printable.
  3. now TCP can not bypass GFW block with current default version. So my friends are using other tools instead.

But the feature helps me. I looked into the normal tcp stream. and it has a range for prefix bytes. So I just make it the same as some kind of header "/GET /home.index", instead of random nonce salt. It does help.

Anyway. it is your pleasure to remove the code. I have my own branch just for personal usage.

@database64128
Copy link
Contributor Author

I implemented my proposed equivalent as a pair of pre-shared stream prefixes in shadowsocks-go. To enable this feature, specify the prefixes in base64 encoding as "unsafeRequestStreamPrefix" and "unsafeResponseStreamPrefix" in server and client config.

A simple example of pretending to be a dead HTTP parrot:

printf 'GET / HTTP/1.1\r\nHost: localhost\r\n\r\n' | base64
R0VUIC8gSFRUUC8xLjENCkhvc3Q6IGxvY2FsaG9zdA0KDQo=
printf 'HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n' | base64
SFRUUC8xLjEgMjAwIE9LDQpDb25uZWN0aW9uOiBjbG9zZQ0KDQo=

Then you can add:

{
    "unsafeRequestStreamPrefix": "R0VUIC8gSFRUUC8xLjENCkhvc3Q6IGxvY2FsaG9zdA0KDQo=",
    "unsafeResponseStreamPrefix": "SFRUUC8xLjEgMjAwIE9LDQpDb25uZWN0aW9uOiBjbG9zZQ0KDQo="
}

If you are feeling generous, configure a fallback address:

{
    "unsafeFallbackAddress": "[::1]:80"
}

Now the stupid firewall you have been trying to bypass might actually let you through!

@fortuna
Copy link
Contributor

fortuna commented Oct 25, 2022

Mitigation
Unless it's for testing, I agree it's a mistake to have the circumvention mitigation enabled all the time, as it does make the protocol vulnerable.

However, there's value in making it an opt-in. The assumption in that case is that the server connection is already blocked, so you have nothing to lose by enabling the mitigation.

For Outline, we are not only making it opt-in, but also we'll let the server administrator specify the prefix they would like the user to use in the access key and online config. This way they can be modified quickly as blocking evolves, and the censor won't be able to easily tell what prefixes are being used.

Multi-hop approach
The proposal of adding a new pair of client and server between the original pair is helpful, but often not feasible, as you need to acquire servers on both sides of the firewall. Right now, in Iran, it's hard to pay for servers because of sanctions. Same in Russia.

It also doubles your traffic costs, which is one of the biggest issues for service providers.

@database64128
Copy link
Contributor Author

The proposal of adding a new pair of client and server between the original pair is helpful, but often not feasible, as you need to acquire servers on both sides of the firewall.

It also doubles your traffic costs, which is one of the biggest issues for service providers.

You don't need separate servers for this. Just run the new pair of client and server beside existing ones on the same systems. Some prefer to call it "SIP003 plugins".

Outline's implementation (Jigsaw-Code/outline-ss-server#127) uses part of the salt as the prefix. While I understand the consideration for backwards compatibility, there are quite a few drawbacks:

  1. The entropy of the salt is reduced.
  2. The length of the prefix is limited to the salt length.
  3. Prefix-unaware Shadowsocks servers cannot detect the presence of such prefixes.

For Outline, we are not only making it opt-in, but also we'll let the server administrator specify the prefix they would like the user to use in the access key and online config.

Since server admins are going to have to get involved anyway, why not just put the prefixes ahead of the actual Shadowsocks stream, like my implementation did.

@database64128
Copy link
Contributor Author

It's also trivial to implement the prefix pair as an optional part of the stream: If the server sees the request prefix in the request stream, then the response prefix may be prepended to the response stream. Otherwise it's processed exactly like before. This approach won't break existing prefix-unaware clients, while giving server admins full control of what to use as prefixes.

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

No branches or pull requests

5 participants