Shadowsocks2022 Client Support #2767
Conversation
|
Example config: |
xiaokangwang
force-pushed
the
dev-ss2022
branch
from
f3fc930
to
a0ab5a0
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #2767 +/- ##
==========================================
- Coverage 37.65% 36.97% -0.69%
==========================================
Files 659 671 +12
Lines 38894 39685 +791
==========================================
+ Hits 14647 14675 +28
- Misses 22631 23390 +759
- Partials 1616 1620 +4 ☔ View full report in Codecov by Sentry. |
xiaokangwang
force-pushed
the
dev-ss2022
branch
from
16f1e01
to
c85e8d5
Compare
xiaokangwang
force-pushed
the
dev-ss2022
branch
from
c85e8d5
to
7511c91
Compare
There was a problem hiding this comment.
Thanks for taking the time to read the spec and test against our implementations. While the implementation in this PR is correct overall, I have found a few problems.
Based on the severity of these problems, my recommendation is to pull the release, or at least update the release notes to say that the current implementation MUST NOT be used. I know the release is a user preview, but the lack of basic security guarantees should not make into any kind of release.
| paddingLength := TCPMinPaddingLength | ||
| if initialPayload == nil { | ||
| initialPayload = []byte{} | ||
| paddingLength += rand.Intn(TCPMaxPaddingLength) // TODO INSECURE RANDOM USED |
There was a problem hiding this comment.
In section 3.1.3 of the spec:
When making a request header, if payload is not available, add non-zero random length padding.
It's fine to use simple RNGs. But sending a request with no initial payload and no padding will get yourself rejected by spec-compliant servers.
| paddingLength += rand.Intn(TCPMaxPaddingLength) // TODO INSECURE RANDOM USED | |
| paddingLength += 1 + rand.Intn(TCPMaxPaddingLength) |
| } | ||
| timeDifference := int64(fixedLengthHeader.Timestamp) - time.Now().Unix() | ||
| if timeDifference < -30 || timeDifference > 30 { | ||
| return newError("timestamp is too far away") |
There was a problem hiding this comment.
It can be useful to include the exact time difference in the error.
| { | ||
| timeDifference := int64(udpResp.TimeStamp) - time.Now().Unix() | ||
| if timeDifference < -30 || timeDifference > 30 { | ||
| newError("udp packet timestamp difference too large, packet discarded").WriteToLog() |
There was a problem hiding this comment.
It can be useful to include the exact time difference in the error.
| resp.PacketID = separateHeaderStruct.PacketID | ||
| resp.SessionID = separateHeaderStruct.SessionID | ||
| { | ||
| mainPacketAEADMaterialized := p.mainPacketAEAD(separateHeaderBuffer.Bytes()[0:8]) |
There was a problem hiding this comment.
It looks to me that you derive keys and create a new cipher instance on every new packet. This is extremely inefficient.
Maintaining server-to-client session state is mandatory on the client side, because it is crucial that each server-to-client session has its own sliding window filter for protecting against replay attacks. Please fix this and cache the AEAD cipher in the state.
|
Thanks for your review. The sliding window filter will be considered a stable release block and will be fixed before any release is marked as stable. Other suggestions will be applied in a less urgent manner. Do you consider this client implementation spec confirming, after all suggestions are implemented? |
This merge request adds shadowsocks2022 client support to V2Ray. It is current implementation lacks check against UDP duplicate packets.