From 3938bf1065f5389e8757b2c7c7a6d76175850627 Mon Sep 17 00:00:00 2001 From: Vinicius Fortuna Date: Mon, 17 Aug 2020 23:10:40 -0400 Subject: [PATCH 01/14] Record server salts --- shadowsocks/client.go | 2 +- shadowsocks/client_test.go | 2 +- shadowsocks/integration_test.go | 5 +++++ shadowsocks/stream.go | 34 ++++++++++++++++++++++++++------ shadowsocks/stream_test.go | 10 +++++----- shadowsocks/tcp.go | 35 ++++++++++++++++++++++++--------- shadowsocks/tcp_test.go | 12 ++++++----- 7 files changed, 73 insertions(+), 27 deletions(-) diff --git a/shadowsocks/client.go b/shadowsocks/client.go index d06b937d..dcf6f990 100644 --- a/shadowsocks/client.go +++ b/shadowsocks/client.go @@ -69,7 +69,7 @@ func (c *ssClient) DialTCP(laddr *net.TCPAddr, raddr string) (onet.DuplexConn, e if err != nil { return nil, err } - ssw := NewShadowsocksWriter(proxyConn, c.cipher) + ssw := NewShadowsocksWriter(proxyConn, c.cipher, RandomSaltGenerator) _, err = ssw.LazyWrite(socksTargetAddr) if err != nil { proxyConn.Close() diff --git a/shadowsocks/client_test.go b/shadowsocks/client_test.go index 492bc071..6e4e1899 100644 --- a/shadowsocks/client_test.go +++ b/shadowsocks/client_test.go @@ -224,7 +224,7 @@ func startShadowsocksTCPEchoProxy(expectedTgtAddr string, t testing.TB) (net.Lis defer running.Done() defer clientConn.Close() ssr := NewShadowsocksReader(clientConn, cipher) - ssw := NewShadowsocksWriter(clientConn, cipher) + ssw := NewShadowsocksWriter(clientConn, cipher, RandomSaltGenerator) ssClientConn := onet.WrapConn(clientConn, ssr, ssw) tgtAddr, err := socks.ReadAddr(ssClientConn) diff --git a/shadowsocks/integration_test.go b/shadowsocks/integration_test.go index c77ff5a5..572882ac 100644 --- a/shadowsocks/integration_test.go +++ b/shadowsocks/integration_test.go @@ -152,6 +152,11 @@ func TestTCPEcho(t *testing.T) { t.Fatal("Echo mismatch") } + // Check for client and server salts. + if len(replayCache.active) != 2 { + t.Fatalf("Replay cache has wrong number of salts: %d", len(replayCache.active)) + } + conn.Close() proxy.Stop() echoListener.Close() diff --git a/shadowsocks/stream.go b/shadowsocks/stream.go index 9c6b4d0e..50b30dd9 100644 --- a/shadowsocks/stream.go +++ b/shadowsocks/stream.go @@ -29,6 +29,24 @@ import ( // payloadSizeMask is the maximum size of payload in bytes. const payloadSizeMask = 0x3FFF // 16*1024 - 1 +// SaltGenerator generates unique salts to use in Shadowsocks connections. +type SaltGenerator interface { + // Writes a new salt into the input slice + GetSalt(salt []byte) error +} + +// randomSaltGenerator generates a new random salt. +type randomSaltGenerator struct{} + +// GetSalt outputs a random salt. +func (*randomSaltGenerator) GetSalt(salt []byte) error { + _, err := io.ReadFull(rand.Reader, salt) + return err +} + +// RandomSaltGenerator is a SaltGenerator that generates a new random salt. +var RandomSaltGenerator SaltGenerator = &randomSaltGenerator{} + // Writer is an io.Writer that also implements io.ReaderFrom to // allow for piping the data without extra allocations and copies. // The LazyWrite and Flush methods allow a header to be @@ -52,9 +70,10 @@ type shadowsocksWriter struct { // else while needFlush could be true. mu sync.Mutex // Indicates that a concurrent flush is currently allowed. - needFlush bool - writer io.Writer - ssCipher shadowaead.Cipher + needFlush bool + writer io.Writer + ssCipher shadowaead.Cipher + saltGenerator SaltGenerator // Wrapper for input that arrives as a slice. byteWrapper bytes.Reader // Number of plaintext bytes that are currently buffered. @@ -68,8 +87,11 @@ type shadowsocksWriter struct { // NewShadowsocksWriter creates a Writer that encrypts the given Writer using // the shadowsocks protocol with the given shadowsocks cipher. -func NewShadowsocksWriter(writer io.Writer, ssCipher shadowaead.Cipher) Writer { - return &shadowsocksWriter{writer: writer, ssCipher: ssCipher} +func NewShadowsocksWriter(writer io.Writer, ssCipher shadowaead.Cipher, saltGenerator SaltGenerator) Writer { + if saltGenerator == nil { + saltGenerator = RandomSaltGenerator + } + return &shadowsocksWriter{writer: writer, ssCipher: ssCipher, saltGenerator: saltGenerator} } // init generates a random salt, sets up the AEAD object and writes @@ -77,7 +99,7 @@ func NewShadowsocksWriter(writer io.Writer, ssCipher shadowaead.Cipher) Writer { func (sw *shadowsocksWriter) init() (err error) { if sw.aead == nil { salt := make([]byte, sw.ssCipher.SaltSize()) - if _, err := io.ReadFull(rand.Reader, salt); err != nil { + if err := sw.saltGenerator.GetSalt(salt); err != nil { return fmt.Errorf("failed to generate salt: %v", err) } sw.aead, err = sw.ssCipher.Encrypter(salt) diff --git a/shadowsocks/stream_test.go b/shadowsocks/stream_test.go index 4d1bcafe..f20c3e82 100644 --- a/shadowsocks/stream_test.go +++ b/shadowsocks/stream_test.go @@ -157,7 +157,7 @@ func TestEndToEnd(t *testing.T) { cipher := newTestCipher(t) connReader, connWriter := io.Pipe() - writer := NewShadowsocksWriter(connWriter, cipher) + writer := NewShadowsocksWriter(connWriter, cipher, RandomSaltGenerator) reader := NewShadowsocksReader(connReader, cipher) expected := "Test" go func() { @@ -180,7 +180,7 @@ func TestEndToEnd(t *testing.T) { func TestLazyWriteFlush(t *testing.T) { cipher := newTestCipher(t) buf := new(bytes.Buffer) - writer := NewShadowsocksWriter(buf, cipher) + writer := NewShadowsocksWriter(buf, cipher, RandomSaltGenerator) header := []byte{1, 2, 3, 4} n, err := writer.LazyWrite(header) if n != len(header) { @@ -241,7 +241,7 @@ func TestLazyWriteFlush(t *testing.T) { func TestLazyWriteConcat(t *testing.T) { cipher := newTestCipher(t) buf := new(bytes.Buffer) - writer := NewShadowsocksWriter(buf, cipher) + writer := NewShadowsocksWriter(buf, cipher, RandomSaltGenerator) header := []byte{1, 2, 3, 4} n, err := writer.LazyWrite(header) if n != len(header) { @@ -295,7 +295,7 @@ func TestLazyWriteConcat(t *testing.T) { func TestLazyWriteOversize(t *testing.T) { cipher := newTestCipher(t) buf := new(bytes.Buffer) - writer := NewShadowsocksWriter(buf, cipher) + writer := NewShadowsocksWriter(buf, cipher, RandomSaltGenerator) N := 25000 // More than one block, less than two. data := make([]byte, N) for i := range data { @@ -335,7 +335,7 @@ func TestLazyWriteOversize(t *testing.T) { func TestLazyWriteConcurrentFlush(t *testing.T) { cipher := newTestCipher(t) buf := new(bytes.Buffer) - writer := NewShadowsocksWriter(buf, cipher) + writer := NewShadowsocksWriter(buf, cipher, RandomSaltGenerator) header := []byte{1, 2, 3, 4} n, err := writer.LazyWrite(header) if n != len(header) { diff --git a/shadowsocks/tcp.go b/shadowsocks/tcp.go index 09eee52d..69c194c3 100644 --- a/shadowsocks/tcp.go +++ b/shadowsocks/tcp.go @@ -29,6 +29,7 @@ import ( onet "github.com/Jigsaw-Code/outline-ss-server/net" logging "github.com/op/go-logging" + "github.com/shadowsocks/go-shadowsocks2/shadowaead" "github.com/shadowsocks/go-shadowsocks2/socks" ) @@ -56,13 +57,27 @@ func debugTCP(cipherID, template string, val interface{}) { } } -func findAccessKey(clientConn onet.DuplexConn, cipherList CipherList) (string, onet.DuplexConn, []byte, time.Duration, error) { - clientIP := remoteIP(clientConn) +type recordingSaltGenerator struct { + saltGenerator SaltGenerator + replayCache *ReplayCache + keyID string +} + +func (sg *recordingSaltGenerator) GetSalt(salt []byte) error { + err := sg.saltGenerator.GetSalt(salt) + if err != nil { + return err + } + _ = sg.replayCache.Add(sg.keyID, salt) + return nil +} + +func findAccessKey(clientReader io.Reader, clientIP net.IP, cipherList CipherList) (string, shadowaead.Cipher, io.Reader, []byte, time.Duration, error) { // We snapshot the list because it may be modified while we use it. tcpTrialSize, ciphers := cipherList.SnapshotForClientIP(clientIP) firstBytes := make([]byte, tcpTrialSize) - if n, err := io.ReadFull(clientConn, firstBytes); err != nil { - return "", clientConn, nil, 0, fmt.Errorf("Reading header failed after %d bytes: %v", n, err) + if n, err := io.ReadFull(clientReader, firstBytes); err != nil { + return "", nil, clientReader, nil, 0, fmt.Errorf("Reading header failed after %d bytes: %v", n, err) } findStartTime := time.Now() @@ -70,16 +85,14 @@ func findAccessKey(clientConn onet.DuplexConn, cipherList CipherList) (string, o timeToCipher := time.Now().Sub(findStartTime) if entry == nil { // TODO: Ban and log client IPs with too many failures too quick to protect against DoS. - return "", clientConn, nil, timeToCipher, fmt.Errorf("Could not find valid TCP cipher") + return "", nil, clientReader, nil, timeToCipher, fmt.Errorf("Could not find valid TCP cipher") } // Move the active cipher to the front, so that the search is quicker next time. cipherList.MarkUsedByClientIP(elt, clientIP) id, cipher := entry.ID, entry.Cipher - ssr := NewShadowsocksReader(io.MultiReader(bytes.NewReader(firstBytes), clientConn), cipher) - ssw := NewShadowsocksWriter(clientConn, cipher) salt := firstBytes[:cipher.SaltSize()] - return id, onet.WrapConn(clientConn, ssr, ssw).(onet.DuplexConn), salt, timeToCipher, nil + return id, cipher, io.MultiReader(bytes.NewReader(firstBytes), clientReader), salt, timeToCipher, nil } // Implements a trial decryption search. This assumes that all ciphers are AEAD. @@ -234,7 +247,7 @@ func (s *tcpService) handleConnection(listenerPort int, clientConn onet.DuplexCo clientConn.SetReadDeadline(connStart.Add(s.readTimeout)) var proxyMetrics metrics.ProxyMetrics clientConn = metrics.MeasureConn(clientConn, &proxyMetrics.ProxyClient, &proxyMetrics.ClientProxy) - keyID, clientConn, salt, timeToCipher, keyErr := findAccessKey(clientConn, s.ciphers) + keyID, cipher, clientReader, salt, timeToCipher, keyErr := findAccessKey(clientConn, remoteIP(clientConn), s.ciphers) connError := func() *onet.ConnectionError { if keyErr != nil { @@ -250,6 +263,10 @@ func (s *tcpService) handleConnection(listenerPort int, clientConn onet.DuplexCo } // Clear the authentication deadline clientConn.SetReadDeadline(time.Time{}) + + ssr := NewShadowsocksReader(clientReader, cipher) + ssw := NewShadowsocksWriter(clientConn, cipher, &recordingSaltGenerator{saltGenerator: RandomSaltGenerator, replayCache: s.replayCache, keyID: keyID}) + clientConn = onet.WrapConn(clientConn, ssr, ssw) return proxyConnection(clientConn, &proxyMetrics, s.checkAllowedIP) }() diff --git a/shadowsocks/tcp_test.go b/shadowsocks/tcp_test.go index 3c619c4b..621ba63f 100644 --- a/shadowsocks/tcp_test.go +++ b/shadowsocks/tcp_test.go @@ -59,8 +59,9 @@ func BenchmarkTCPFindCipherFail(b *testing.B) { if err != nil { b.Fatalf("AcceptTCP failed: %v", err) } + clientIP := clientConn.RemoteAddr().(*net.TCPAddr).IP b.StartTimer() - findAccessKey(clientConn, cipherList) + findAccessKey(clientConn, clientIP, cipherList) b.StopTimer() } } @@ -139,12 +140,13 @@ func BenchmarkTCPFindCipherRepeat(b *testing.B) { for n := 0; n < b.N; n++ { cipherNumber := byte(n % numCiphers) reader, writer := io.Pipe() - addr := &net.TCPAddr{IP: net.IPv4(192, 0, 2, cipherNumber), Port: 54321} + clientIP := net.IPv4(192, 0, 2, cipherNumber) + addr := &net.TCPAddr{IP: clientIP, Port: 54321} c := conn{clientAddr: addr, reader: reader, writer: writer} cipher := cipherEntries[cipherNumber].Cipher - go NewShadowsocksWriter(writer, cipher).Write(MakeTestPayload(50)) + go NewShadowsocksWriter(writer, cipher, RandomSaltGenerator).Write(MakeTestPayload(50)) b.StartTimer() - _, _, _, _, err := findAccessKey(&c, cipherList) + _, _, _, _, _, err := findAccessKey(&c, clientIP, cipherList) b.StopTimer() if err != nil { b.Error(err) @@ -205,7 +207,7 @@ func TestReplayDefense(t *testing.T) { cipherEntry := snapshot[0].Value.(*CipherEntry) cipher := cipherEntry.Cipher reader, writer := io.Pipe() - go NewShadowsocksWriter(writer, cipher).Write([]byte{0}) + go NewShadowsocksWriter(writer, cipher, RandomSaltGenerator).Write([]byte{0}) preamble := make([]byte, 32+2+16) if _, err := io.ReadFull(reader, preamble); err != nil { t.Fatal(err) From 1c62281cb0ef400c4ab11eb8cd029a3489688f15 Mon Sep 17 00:00:00 2001 From: Vinicius Fortuna Date: Tue, 18 Aug 2020 12:09:08 -0400 Subject: [PATCH 02/14] Add SetSaltGenerator --- shadowsocks/client.go | 2 +- shadowsocks/client_test.go | 2 +- shadowsocks/stream.go | 48 +++++++++++++++++--------------------- shadowsocks/stream_test.go | 10 ++++---- shadowsocks/tcp.go | 3 ++- shadowsocks/tcp_test.go | 4 ++-- 6 files changed, 32 insertions(+), 37 deletions(-) diff --git a/shadowsocks/client.go b/shadowsocks/client.go index dcf6f990..d06b937d 100644 --- a/shadowsocks/client.go +++ b/shadowsocks/client.go @@ -69,7 +69,7 @@ func (c *ssClient) DialTCP(laddr *net.TCPAddr, raddr string) (onet.DuplexConn, e if err != nil { return nil, err } - ssw := NewShadowsocksWriter(proxyConn, c.cipher, RandomSaltGenerator) + ssw := NewShadowsocksWriter(proxyConn, c.cipher) _, err = ssw.LazyWrite(socksTargetAddr) if err != nil { proxyConn.Close() diff --git a/shadowsocks/client_test.go b/shadowsocks/client_test.go index 6e4e1899..492bc071 100644 --- a/shadowsocks/client_test.go +++ b/shadowsocks/client_test.go @@ -224,7 +224,7 @@ func startShadowsocksTCPEchoProxy(expectedTgtAddr string, t testing.TB) (net.Lis defer running.Done() defer clientConn.Close() ssr := NewShadowsocksReader(clientConn, cipher) - ssw := NewShadowsocksWriter(clientConn, cipher, RandomSaltGenerator) + ssw := NewShadowsocksWriter(clientConn, cipher) ssClientConn := onet.WrapConn(clientConn, ssr, ssw) tgtAddr, err := socks.ReadAddr(ssClientConn) diff --git a/shadowsocks/stream.go b/shadowsocks/stream.go index 50b30dd9..760d0366 100644 --- a/shadowsocks/stream.go +++ b/shadowsocks/stream.go @@ -52,19 +52,7 @@ var RandomSaltGenerator SaltGenerator = &randomSaltGenerator{} // The LazyWrite and Flush methods allow a header to be // added but delayed until the first write, for concatenation. // All methods except Flush must be called from a single thread. -type Writer interface { - io.Writer - io.ReaderFrom - // LazyWrite queues p to be written, but doesn't send it until - // Flush() is called, a non-lazy write is made, or the buffer - // is filled. - LazyWrite(p []byte) (int, error) - // Flush sends the pending data, if any. This method is - // thread-safe. - Flush() error -} - -type shadowsocksWriter struct { +type Writer struct { // This type is single-threaded except when needFlush is true. // mu protects needFlush, and also protects everything // else while needFlush could be true. @@ -87,16 +75,18 @@ type shadowsocksWriter struct { // NewShadowsocksWriter creates a Writer that encrypts the given Writer using // the shadowsocks protocol with the given shadowsocks cipher. -func NewShadowsocksWriter(writer io.Writer, ssCipher shadowaead.Cipher, saltGenerator SaltGenerator) Writer { - if saltGenerator == nil { - saltGenerator = RandomSaltGenerator - } - return &shadowsocksWriter{writer: writer, ssCipher: ssCipher, saltGenerator: saltGenerator} +func NewShadowsocksWriter(writer io.Writer, ssCipher shadowaead.Cipher) *Writer { + return &Writer{writer: writer, ssCipher: ssCipher, saltGenerator: RandomSaltGenerator} +} + +// SetSaltGenerator sets the salt generator to be used. Must be called before the first write. +func (sw *Writer) SetSaltGenerator(saltGenerator SaltGenerator) { + sw.saltGenerator = saltGenerator } // init generates a random salt, sets up the AEAD object and writes // the salt to the inner Writer. -func (sw *shadowsocksWriter) init() (err error) { +func (sw *Writer) init() (err error) { if sw.aead == nil { salt := make([]byte, sw.ssCipher.SaltSize()) if err := sw.saltGenerator.GetSalt(salt); err != nil { @@ -120,19 +110,21 @@ func (sw *shadowsocksWriter) init() (err error) { // encryptBlock encrypts `plaintext` in-place. The slice must have enough capacity // for the tag. Returns the total ciphertext length. -func (sw *shadowsocksWriter) encryptBlock(plaintext []byte) int { +func (sw *Writer) encryptBlock(plaintext []byte) int { out := sw.aead.Seal(plaintext[:0], sw.counter, plaintext, nil) increment(sw.counter) return len(out) } -func (sw *shadowsocksWriter) Write(p []byte) (int, error) { +func (sw *Writer) Write(p []byte) (int, error) { sw.byteWrapper.Reset(p) n, err := sw.ReadFrom(&sw.byteWrapper) return int(n), err } -func (sw *shadowsocksWriter) LazyWrite(p []byte) (int, error) { +// LazyWrite queues p to be written, but doesn't send it until Flush() is +// called, a non-lazy write is made, or the buffer is filled. +func (sw *Writer) LazyWrite(p []byte) (int, error) { if err := sw.init(); err != nil { return 0, err } @@ -159,7 +151,8 @@ func (sw *shadowsocksWriter) LazyWrite(p []byte) (int, error) { } } -func (sw *shadowsocksWriter) Flush() error { +// Flush sends the pending data, if any. This method is thread-safe. +func (sw *Writer) Flush() error { sw.mu.Lock() defer sw.mu.Unlock() if !sw.needFlush { @@ -178,7 +171,7 @@ func isZero(b []byte) bool { } // Returns the slices of sw.buf in which to place plaintext for encryption. -func (sw *shadowsocksWriter) buffers() (sizeBuf, payloadBuf []byte) { +func (sw *Writer) buffers() (sizeBuf, payloadBuf []byte) { // sw.buf starts with the salt. saltSize := sw.ssCipher.SaltSize() @@ -190,7 +183,8 @@ func (sw *shadowsocksWriter) buffers() (sizeBuf, payloadBuf []byte) { return } -func (sw *shadowsocksWriter) ReadFrom(r io.Reader) (int64, error) { +// ReadFrom implements the io.ReaderFrom interface. +func (sw *Writer) ReadFrom(r io.Reader) (int64, error) { if err := sw.init(); err != nil { return 0, err } @@ -240,7 +234,7 @@ func (sw *shadowsocksWriter) ReadFrom(r io.Reader) (int64, error) { // Adds as much of `plaintext` into the buffer as will fit, and increases // sw.pending accordingly. Returns the number of bytes consumed. -func (sw *shadowsocksWriter) enqueue(plaintext []byte) int { +func (sw *Writer) enqueue(plaintext []byte) int { _, payloadBuf := sw.buffers() n := copy(payloadBuf[sw.pending:], plaintext) sw.pending += n @@ -248,7 +242,7 @@ func (sw *shadowsocksWriter) enqueue(plaintext []byte) int { } // Encrypts all pending data and writes it to the output. -func (sw *shadowsocksWriter) flush() error { +func (sw *Writer) flush() error { if sw.pending == 0 { return nil } diff --git a/shadowsocks/stream_test.go b/shadowsocks/stream_test.go index f20c3e82..4d1bcafe 100644 --- a/shadowsocks/stream_test.go +++ b/shadowsocks/stream_test.go @@ -157,7 +157,7 @@ func TestEndToEnd(t *testing.T) { cipher := newTestCipher(t) connReader, connWriter := io.Pipe() - writer := NewShadowsocksWriter(connWriter, cipher, RandomSaltGenerator) + writer := NewShadowsocksWriter(connWriter, cipher) reader := NewShadowsocksReader(connReader, cipher) expected := "Test" go func() { @@ -180,7 +180,7 @@ func TestEndToEnd(t *testing.T) { func TestLazyWriteFlush(t *testing.T) { cipher := newTestCipher(t) buf := new(bytes.Buffer) - writer := NewShadowsocksWriter(buf, cipher, RandomSaltGenerator) + writer := NewShadowsocksWriter(buf, cipher) header := []byte{1, 2, 3, 4} n, err := writer.LazyWrite(header) if n != len(header) { @@ -241,7 +241,7 @@ func TestLazyWriteFlush(t *testing.T) { func TestLazyWriteConcat(t *testing.T) { cipher := newTestCipher(t) buf := new(bytes.Buffer) - writer := NewShadowsocksWriter(buf, cipher, RandomSaltGenerator) + writer := NewShadowsocksWriter(buf, cipher) header := []byte{1, 2, 3, 4} n, err := writer.LazyWrite(header) if n != len(header) { @@ -295,7 +295,7 @@ func TestLazyWriteConcat(t *testing.T) { func TestLazyWriteOversize(t *testing.T) { cipher := newTestCipher(t) buf := new(bytes.Buffer) - writer := NewShadowsocksWriter(buf, cipher, RandomSaltGenerator) + writer := NewShadowsocksWriter(buf, cipher) N := 25000 // More than one block, less than two. data := make([]byte, N) for i := range data { @@ -335,7 +335,7 @@ func TestLazyWriteOversize(t *testing.T) { func TestLazyWriteConcurrentFlush(t *testing.T) { cipher := newTestCipher(t) buf := new(bytes.Buffer) - writer := NewShadowsocksWriter(buf, cipher, RandomSaltGenerator) + writer := NewShadowsocksWriter(buf, cipher) header := []byte{1, 2, 3, 4} n, err := writer.LazyWrite(header) if n != len(header) { diff --git a/shadowsocks/tcp.go b/shadowsocks/tcp.go index 69c194c3..2ab34687 100644 --- a/shadowsocks/tcp.go +++ b/shadowsocks/tcp.go @@ -265,7 +265,8 @@ func (s *tcpService) handleConnection(listenerPort int, clientConn onet.DuplexCo clientConn.SetReadDeadline(time.Time{}) ssr := NewShadowsocksReader(clientReader, cipher) - ssw := NewShadowsocksWriter(clientConn, cipher, &recordingSaltGenerator{saltGenerator: RandomSaltGenerator, replayCache: s.replayCache, keyID: keyID}) + ssw := NewShadowsocksWriter(clientConn, cipher) + ssw.SetSaltGenerator(&recordingSaltGenerator{saltGenerator: RandomSaltGenerator, replayCache: s.replayCache, keyID: keyID}) clientConn = onet.WrapConn(clientConn, ssr, ssw) return proxyConnection(clientConn, &proxyMetrics, s.checkAllowedIP) }() diff --git a/shadowsocks/tcp_test.go b/shadowsocks/tcp_test.go index 621ba63f..ad6ef4fa 100644 --- a/shadowsocks/tcp_test.go +++ b/shadowsocks/tcp_test.go @@ -144,7 +144,7 @@ func BenchmarkTCPFindCipherRepeat(b *testing.B) { addr := &net.TCPAddr{IP: clientIP, Port: 54321} c := conn{clientAddr: addr, reader: reader, writer: writer} cipher := cipherEntries[cipherNumber].Cipher - go NewShadowsocksWriter(writer, cipher, RandomSaltGenerator).Write(MakeTestPayload(50)) + go NewShadowsocksWriter(writer, cipher).Write(MakeTestPayload(50)) b.StartTimer() _, _, _, _, _, err := findAccessKey(&c, clientIP, cipherList) b.StopTimer() @@ -207,7 +207,7 @@ func TestReplayDefense(t *testing.T) { cipherEntry := snapshot[0].Value.(*CipherEntry) cipher := cipherEntry.Cipher reader, writer := io.Pipe() - go NewShadowsocksWriter(writer, cipher, RandomSaltGenerator).Write([]byte{0}) + go NewShadowsocksWriter(writer, cipher).Write([]byte{0}) preamble := make([]byte, 32+2+16) if _, err := io.ReadFull(reader, preamble); err != nil { t.Fatal(err) From 1c6ebb57b039d929cd15ed3df9177f3149428ab7 Mon Sep 17 00:00:00 2001 From: Ben Schwartz Date: Tue, 18 Aug 2020 15:53:59 -0400 Subject: [PATCH 03/14] Use the shadowsocks cipher to mark the salt --- shadowsocks/integration_test.go | 5 --- shadowsocks/stream.go | 80 ++++++++++++++++++++++++++++----- shadowsocks/tcp.go | 38 ++++++++-------- 3 files changed, 89 insertions(+), 34 deletions(-) diff --git a/shadowsocks/integration_test.go b/shadowsocks/integration_test.go index 572882ac..c77ff5a5 100644 --- a/shadowsocks/integration_test.go +++ b/shadowsocks/integration_test.go @@ -152,11 +152,6 @@ func TestTCPEcho(t *testing.T) { t.Fatal("Echo mismatch") } - // Check for client and server salts. - if len(replayCache.active) != 2 { - t.Fatalf("Replay cache has wrong number of salts: %d", len(replayCache.active)) - } - conn.Close() proxy.Stop() echoListener.Close() diff --git a/shadowsocks/stream.go b/shadowsocks/stream.go index 760d0366..ec3341c1 100644 --- a/shadowsocks/stream.go +++ b/shadowsocks/stream.go @@ -31,21 +31,81 @@ const payloadSizeMask = 0x3FFF // 16*1024 - 1 // SaltGenerator generates unique salts to use in Shadowsocks connections. type SaltGenerator interface { - // Writes a new salt into the input slice - GetSalt(salt []byte) error + // Returns a new salt + GetSalt() ([]byte, error) } // randomSaltGenerator generates a new random salt. -type randomSaltGenerator struct{} +type randomSaltGenerator struct { + saltSize int +} + +// ServerSaltGenerator generates unique salts that are secretly marked. +type ServerSaltGenerator struct { + saltSize int + encrypter cipher.AEAD +} // GetSalt outputs a random salt. -func (*randomSaltGenerator) GetSalt(salt []byte) error { +func (sg randomSaltGenerator) GetSalt() ([]byte, error) { + salt := make([]byte, sg.saltSize) _, err := io.ReadFull(rand.Reader, salt) - return err + return salt, err +} + +// Number of bytes of salt to use as a marker. +const markLen = 4 + +// Constant to identify this marking scheme. +var serverIndication = []byte("outline-salt-mark") + +func NewServerSaltGenerator(cipher shadowaead.Cipher) (ServerSaltGenerator, error) { + saltSize := cipher.SaltSize() + zerosalt := make([]byte, saltSize) + encrypter, err := cipher.Encrypter(zerosalt) + if err != nil { + return ServerSaltGenerator{}, err + } + return ServerSaltGenerator{saltSize, encrypter}, nil +} + +func (sg ServerSaltGenerator) splitSalt(salt []byte) (prefix, mark []byte) { + prefixLen := sg.saltSize - markLen + prefix = salt[:prefixLen] + mark = salt[prefixLen:] + return } -// RandomSaltGenerator is a SaltGenerator that generates a new random salt. -var RandomSaltGenerator SaltGenerator = &randomSaltGenerator{} +// getTag takes in a salt prefix and writes out the tag. +// len(prefix) must be saltSize - markLen +func (sg ServerSaltGenerator) getTag(prefix []byte) []byte { + nonce := make([]byte, sg.encrypter.NonceSize()) + n := copy(nonce, prefix) + plaintext := prefix[n:] + encrypted := sg.encrypter.Seal(nil, nonce, plaintext, serverIndication) + return encrypted[len(plaintext):] +} + +// GetSalt returns an apparently random salt that can be identified +// as server-originated by anyone who knows the Shadowsocks key. +func (sg ServerSaltGenerator) GetSalt() ([]byte, error) { + salt := make([]byte, sg.saltSize) + prefix, mark := sg.splitSalt(salt) + _, err := io.ReadFull(rand.Reader, prefix) + if err != nil { + return nil, err + } + tag := sg.getTag(prefix) + copy(mark, tag) + return salt, nil +} + +// IsMarked returns true if the salt is marked as server-originated. +func (sg ServerSaltGenerator) IsMarked(salt []byte) bool { + prefix, mark := sg.splitSalt(salt) + tag := sg.getTag(prefix) + return bytes.Equal(tag[:markLen], mark) +} // Writer is an io.Writer that also implements io.ReaderFrom to // allow for piping the data without extra allocations and copies. @@ -76,7 +136,7 @@ type Writer struct { // NewShadowsocksWriter creates a Writer that encrypts the given Writer using // the shadowsocks protocol with the given shadowsocks cipher. func NewShadowsocksWriter(writer io.Writer, ssCipher shadowaead.Cipher) *Writer { - return &Writer{writer: writer, ssCipher: ssCipher, saltGenerator: RandomSaltGenerator} + return &Writer{writer: writer, ssCipher: ssCipher, saltGenerator: randomSaltGenerator{ssCipher.SaltSize()}} } // SetSaltGenerator sets the salt generator to be used. Must be called before the first write. @@ -88,8 +148,8 @@ func (sw *Writer) SetSaltGenerator(saltGenerator SaltGenerator) { // the salt to the inner Writer. func (sw *Writer) init() (err error) { if sw.aead == nil { - salt := make([]byte, sw.ssCipher.SaltSize()) - if err := sw.saltGenerator.GetSalt(salt); err != nil { + salt, err := sw.saltGenerator.GetSalt() + if err != nil { return fmt.Errorf("failed to generate salt: %v", err) } sw.aead, err = sw.ssCipher.Encrypter(salt) diff --git a/shadowsocks/tcp.go b/shadowsocks/tcp.go index 2ab34687..c74e721c 100644 --- a/shadowsocks/tcp.go +++ b/shadowsocks/tcp.go @@ -57,21 +57,6 @@ func debugTCP(cipherID, template string, val interface{}) { } } -type recordingSaltGenerator struct { - saltGenerator SaltGenerator - replayCache *ReplayCache - keyID string -} - -func (sg *recordingSaltGenerator) GetSalt(salt []byte) error { - err := sg.saltGenerator.GetSalt(salt) - if err != nil { - return err - } - _ = sg.replayCache.Add(sg.keyID, salt) - return nil -} - func findAccessKey(clientReader io.Reader, clientIP net.IP, cipherList CipherList) (string, shadowaead.Cipher, io.Reader, []byte, time.Duration, error) { // We snapshot the list because it may be modified while we use it. tcpTrialSize, ciphers := cipherList.SnapshotForClientIP(clientIP) @@ -255,18 +240,33 @@ func (s *tcpService) handleConnection(listenerPort int, clientConn onet.DuplexCo const status = "ERR_CIPHER" s.absorbProbe(listenerPort, clientConn, clientLocation, status, &proxyMetrics) return onet.NewConnectionError(status, "Failed to find a valid cipher", keyErr) - } else if !s.replayCache.Add(keyID, salt) { // Only check the cache if findAccessKey succeeded. + } + + saltGenerator, err := NewServerSaltGenerator(cipher) + if err != nil { + return onet.NewConnectionError("ERR_SALTGEN", "Failed to construct salt generator", err) + } + + isMarked := saltGenerator.IsMarked(salt) + // Only check the cache if findAccessKey succeeded and the salt is unmarked. + if isMarked || !s.replayCache.Add(keyID, salt) { const status = "ERR_REPLAY" s.absorbProbe(listenerPort, clientConn, clientLocation, status, &proxyMetrics) - logger.Debugf("Replay: %v in %s sent %d bytes", clientConn.RemoteAddr(), clientLocation, proxyMetrics.ClientProxy) - return onet.NewConnectionError(status, "Replay detected", nil) + var msg string + if isMarked { + msg = "Server replay detected" + } else { + msg = "Client replay detected" + } + logger.Debugf(msg+": %v in %s sent %d bytes", clientConn.RemoteAddr(), clientLocation, proxyMetrics.ClientProxy) + return onet.NewConnectionError(status, msg, nil) } // Clear the authentication deadline clientConn.SetReadDeadline(time.Time{}) ssr := NewShadowsocksReader(clientReader, cipher) ssw := NewShadowsocksWriter(clientConn, cipher) - ssw.SetSaltGenerator(&recordingSaltGenerator{saltGenerator: RandomSaltGenerator, replayCache: s.replayCache, keyID: keyID}) + ssw.SetSaltGenerator(saltGenerator) clientConn = onet.WrapConn(clientConn, ssr, ssw) return proxyConnection(clientConn, &proxyMetrics, s.checkAllowedIP) }() From 5c265e80b242afde2ab660c3d6cfbac42cd2cc7b Mon Sep 17 00:00:00 2001 From: Ben Schwartz Date: Wed, 19 Aug 2020 17:34:27 -0400 Subject: [PATCH 04/14] Respond to comments and various improvements --- server.go | 6 +- shadowsocks/cipher_list.go | 20 +- shadowsocks/cipher_list_test.go | 36 +--- shadowsocks/cipher_testing.go | 49 ++++- shadowsocks/salt_generator.go | 100 +++++++++ shadowsocks/salt_generator_test.go | 313 +++++++++++++++++++++++++++++ shadowsocks/stream.go | 86 +------- shadowsocks/tcp.go | 52 +++-- shadowsocks/tcp_test.go | 2 +- 9 files changed, 516 insertions(+), 148 deletions(-) create mode 100644 shadowsocks/salt_generator.go create mode 100644 shadowsocks/salt_generator_test.go diff --git a/server.go b/server.go index 3b8b6efb..a01fb60b 100644 --- a/server.go +++ b/server.go @@ -139,7 +139,11 @@ func (s *ssServer) loadConfig(filename string) error { if !ok { return fmt.Errorf("Only AEAD ciphers are supported. Found %v", keyConfig.Cipher) } - cipherList.PushBack(&shadowsocks.CipherEntry{ID: keyConfig.ID, Cipher: aead}) + entry, err := shadowsocks.MakeCipherEntry(keyConfig.ID, aead) + if err != nil { + return fmt.Errorf("Failed to register cipher: %v", err) + } + cipherList.PushBack(&entry) } for port := range s.ports { portChanges[port] = portChanges[port] - 1 diff --git a/shadowsocks/cipher_list.go b/shadowsocks/cipher_list.go index 815359e4..ef6c8da4 100644 --- a/shadowsocks/cipher_list.go +++ b/shadowsocks/cipher_list.go @@ -30,9 +30,23 @@ const maxNonceSize = 12 // CipherEntry holds a Cipher with an identifier. // The public fields are constant, but lastAddress is mutable under cipherList.mu. type CipherEntry struct { - ID string - Cipher shadowaead.Cipher - lastClientIP net.IP + ID string + Cipher shadowaead.Cipher + SaltGenerator ServerSaltGenerator + lastClientIP net.IP +} + +// MakeCipherEntry constructs a CipherEntry. +func MakeCipherEntry(id string, cipher shadowaead.Cipher) (CipherEntry, error) { + saltGenerator, err := NewServerSaltGenerator(cipher) + if err != nil { + return CipherEntry{}, err + } + return CipherEntry{ + ID: id, + Cipher: cipher, + SaltGenerator: saltGenerator, + }, nil } // CipherList is a thread-safe collection of CipherEntry elements that allows for diff --git a/shadowsocks/cipher_list_test.go b/shadowsocks/cipher_list_test.go index fb120012..30eea51f 100644 --- a/shadowsocks/cipher_list_test.go +++ b/shadowsocks/cipher_list_test.go @@ -16,45 +16,17 @@ package shadowsocks import ( "container/list" - "crypto/cipher" "testing" "github.com/shadowsocks/go-shadowsocks2/shadowaead" ) -type fakeAEAD struct { - cipher.AEAD - overhead, nonceSize int -} - -func (a *fakeAEAD) NonceSize() int { - return a.nonceSize -} - -func (a *fakeAEAD) Overhead() int { - return a.overhead -} - -type fakeCipher struct { - shadowaead.Cipher - saltsize int - decrypter *fakeAEAD -} - -func (c *fakeCipher) SaltSize() int { - return c.saltsize -} - -func (c *fakeCipher) Decrypter(b []byte) (cipher.AEAD, error) { - return c.decrypter, nil -} - func TestIncompatibleCiphers(t *testing.T) { l := list.New() l.PushBack(&CipherEntry{ ID: "short", - Cipher: &fakeCipher{saltsize: 5, decrypter: &fakeAEAD{overhead: 3}}}) - l.PushBack(&CipherEntry{ID: "long", Cipher: &fakeCipher{saltsize: 50, decrypter: &fakeAEAD{overhead: 30}}}) + Cipher: &fakeCipher{saltSize: 5, aead: &fakeAEAD{overhead: 3}}}) + l.PushBack(&CipherEntry{ID: "long", Cipher: &fakeCipher{saltSize: 50, aead: &fakeAEAD{overhead: 30}}}) cipherList := NewCipherList() err := cipherList.Update(l) if err == nil { @@ -66,8 +38,8 @@ func TestMaxNonceSize(t *testing.T) { l := list.New() l.PushBack(&CipherEntry{ ID: "oversize nonce", - Cipher: &fakeCipher{saltsize: 5, decrypter: &fakeAEAD{overhead: 3, nonceSize: 13}}}) - l.PushBack(&CipherEntry{ID: "long", Cipher: &fakeCipher{saltsize: 50, decrypter: &fakeAEAD{overhead: 30}}}) + Cipher: &fakeCipher{saltSize: 5, aead: &fakeAEAD{overhead: 3, nonceSize: 13}}}) + l.PushBack(&CipherEntry{ID: "long", Cipher: &fakeCipher{saltSize: 50, aead: &fakeAEAD{overhead: 30}}}) cipherList := NewCipherList() err := cipherList.Update(l) if err == nil { diff --git a/shadowsocks/cipher_testing.go b/shadowsocks/cipher_testing.go index 75c63a8e..79fcd2b3 100644 --- a/shadowsocks/cipher_testing.go +++ b/shadowsocks/cipher_testing.go @@ -16,6 +16,7 @@ package shadowsocks import ( "container/list" + "crypto/cipher" "fmt" "github.com/shadowsocks/go-shadowsocks2/core" @@ -43,7 +44,11 @@ func MakeTestCiphers(secrets []string) (CipherList, error) { if err != nil { return nil, fmt.Errorf("Failed to create cipher %v: %v", i, err) } - l.PushBack(&CipherEntry{ID: cipherID, Cipher: cipher.(shadowaead.Cipher)}) + entry, err := MakeCipherEntry(cipherID, cipher.(shadowaead.Cipher)) + if err != nil { + return nil, fmt.Errorf("Failed to create entry: %v", err) + } + l.PushBack(&entry) } cipherList := NewCipherList() cipherList.Update(l) @@ -58,3 +63,45 @@ func MakeTestPayload(size int) []byte { } return payload } + +type fakeAEAD struct { + cipher.AEAD + overhead, nonceSize int + salt []byte +} + +func (a *fakeAEAD) NonceSize() int { + return a.nonceSize +} + +func (a *fakeAEAD) Overhead() int { + return a.overhead +} + +type fakeCipher struct { + keySize int + saltSize int + aead cipher.AEAD +} + +func (c *fakeCipher) KeySize() int { + return c.keySize +} + +func (c *fakeCipher) SaltSize() int { + return c.saltSize +} + +func (c *fakeCipher) Decrypter(salt []byte) (cipher.AEAD, error) { + if len(salt) != c.saltSize { + return nil, fmt.Errorf("Wrong salt size: %d != %d", len(salt), c.saltSize) + } + return c.aead, nil +} + +func (c *fakeCipher) Encrypter(salt []byte) (cipher.AEAD, error) { + if len(salt) != c.saltSize { + return nil, fmt.Errorf("Wrong salt size: %d != %d", len(salt), c.saltSize) + } + return c.aead, nil +} diff --git a/shadowsocks/salt_generator.go b/shadowsocks/salt_generator.go new file mode 100644 index 00000000..7059c0ce --- /dev/null +++ b/shadowsocks/salt_generator.go @@ -0,0 +1,100 @@ +package shadowsocks + +import ( + "bytes" + "crypto/cipher" + "crypto/rand" + "fmt" + "io" + + "github.com/shadowsocks/go-shadowsocks2/shadowaead" +) + +// SaltGenerator generates unique salts to use in Shadowsocks connections. +type SaltGenerator interface { + // Returns a new salt + GetSalt(salt []byte) error +} + +// randomSaltGenerator generates a new random salt. +type randomSaltGenerator struct{} + +// GetSalt outputs a random salt. +func (randomSaltGenerator) GetSalt(salt []byte) error { + _, err := io.ReadFull(rand.Reader, salt) + return err +} + +// RandomSaltGenerator is a basic SaltGenerator. +var RandomSaltGenerator SaltGenerator = randomSaltGenerator{} + +// ServerSaltGenerator generates unique salts that are secretly marked. +type ServerSaltGenerator struct { + saltSize int + encrypter cipher.AEAD +} + +// Number of bytes of salt to use as a marker. Increasing this value reduces +// the false positive rate, but increases the likelihood of salt collisions. +// Must be less than or equal to the cipher overhead. +const markLen = 4 + +// Constant to identify this marking scheme. +var serverSaltLabel = []byte("outline-server-salt") + +// NewServerSaltGenerator returns a SaltGenerator whose output is apparently +// random, but is secretly marked as being issued by the server. +// This is useful to prevent the server from accepting its own output in a +// reflection attack. +func NewServerSaltGenerator(cipher shadowaead.Cipher) (ServerSaltGenerator, error) { + saltSize := cipher.SaltSize() + zeroSalt := make([]byte, saltSize) + encrypter, err := cipher.Encrypter(zeroSalt) + if err != nil { + return ServerSaltGenerator{}, err + } + return ServerSaltGenerator{saltSize, encrypter}, nil +} + +func (sg ServerSaltGenerator) splitSalt(salt []byte) (prefix, mark []byte) { + prefixLen := sg.saltSize - markLen + prefix = salt[:prefixLen] + mark = salt[prefixLen:] + return +} + +// getTag takes in a salt prefix and returns the tag. +// len(prefix) must be saltSize - markLen, which must be at least nonceSize. +// prefix must be random to avoid nonce reuse. +func (sg ServerSaltGenerator) getTag(prefix []byte) []byte { + // Only the first nonceSize bytes are used to compute the tag. + // In the event of a nonce collision (p=2^-33 after 2^32 messages), + // the only effect will be to reveal a pattern in the handshakes, not + // to reuse the same nonce on different inputs (which can cause more + // serious problems: https://www.imperialviolet.org/2015/05/16/aeads.html). + nonce := prefix[:sg.encrypter.NonceSize()] + return sg.encrypter.Seal(nil, nonce, nil, serverSaltLabel) +} + +// GetSalt returns an apparently random salt that can be identified +// as server-originated by anyone who knows the Shadowsocks key. +func (sg ServerSaltGenerator) GetSalt(salt []byte) error { + if len(salt) != sg.saltSize { + return fmt.Errorf("Wrong salt size: %d != %d", len(salt), sg.saltSize) + } + prefix, mark := sg.splitSalt(salt) + _, err := io.ReadFull(rand.Reader, prefix) + if err != nil { + return err + } + tag := sg.getTag(prefix) + copy(mark, tag) + return nil +} + +// IsServerSalt returns true if the salt is marked as server-originated. +func (sg ServerSaltGenerator) IsServerSalt(salt []byte) bool { + prefix, mark := sg.splitSalt(salt) + tag := sg.getTag(prefix) + return bytes.Equal(tag[:markLen], mark) +} diff --git a/shadowsocks/salt_generator_test.go b/shadowsocks/salt_generator_test.go new file mode 100644 index 00000000..69763906 --- /dev/null +++ b/shadowsocks/salt_generator_test.go @@ -0,0 +1,313 @@ +package shadowsocks + +import ( + "bytes" + "testing" + + "github.com/shadowsocks/go-shadowsocks2/core" + "github.com/shadowsocks/go-shadowsocks2/shadowaead" +) + +func TestRandomSaltGenerator(t *testing.T) { + if err := RandomSaltGenerator.GetSalt(nil); err != nil { + t.Error(err) + } + salt := make([]byte, 16) + if err := RandomSaltGenerator.GetSalt(salt); err != nil { + t.Error(err) + } + if bytes.Equal(salt, make([]byte, 16)) { + t.Error("Salt is all zeros") + } +} + +type mockAEAD struct { + fakeAEAD + t *testing.T + sealed bool + tag []byte +} + +func (a *mockAEAD) Seal(dst, nonce, plaintext, additionalData []byte) []byte { + if len(nonce) != a.NonceSize() { + a.t.Errorf("Wrong nonce length: %d != %d", len(nonce), a.NonceSize()) + } + if len(plaintext) != 0 { + a.t.Errorf("Wrong plaintext length: %d != 0", len(plaintext)) + } + if !bytes.Equal(additionalData, serverSaltLabel) { + a.t.Errorf("Wrong additional data: %v", additionalData) + } + a.sealed = true + dst = append(dst, a.tag...) + return dst +} + +// Test that ServerSaltGenerator works as expected using a fake cipher. +func TestServerSaltFake(t *testing.T) { + tag := []byte("1234567890123456") + mockCipher := &fakeCipher{ + saltSize: 32, + aead: &mockAEAD{ + fakeAEAD: fakeAEAD{ + nonceSize: 12, + overhead: 16, + }, + t: t, + tag: tag, + }, + } + + ssg, err := NewServerSaltGenerator(mockCipher) + if err != nil { + t.Fatal(err) + } + salt := make([]byte, mockCipher.saltSize) + if err := ssg.GetSalt(salt); err != nil { + t.Fatal(err) + } + if bytes.Equal(salt, make([]byte, len(salt))) { + t.Error("Salt is zero") + } + if !bytes.Equal(salt[len(salt)-markLen:], tag[:markLen]) { + t.Error("Tag mismatch") + } + if !ssg.IsServerSalt(salt) { + t.Error("Tag was not recognized") + } + + // Make another random salt with the same tag + salt2 := make([]byte, mockCipher.saltSize) + if err := ssg.GetSalt(salt2); err != nil { + t.Fatal(err) + } + if bytes.Equal(salt, salt2) { + t.Error("Salts should be different") + } + if !bytes.Equal(salt2[len(salt2)-markLen:], tag[:markLen]) { + t.Error("Tag mismatch") + } + if !ssg.IsServerSalt(salt2) { + t.Error("Tag was not recognized") + } + + // Alter tag + salt[len(salt)-1]++ + if ssg.IsServerSalt(salt) { + t.Error("Altered tag was still recognized") + } +} + +// Test that ServerSaltGenerator recognizes its own salts +func TestServerSaltRecognized(t *testing.T) { + cipher, err := core.PickCipher(testCipher, nil, "test") + if err != nil { + t.Fatal(err) + } + aead := cipher.(shadowaead.Cipher) + ssg, err := NewServerSaltGenerator(aead) + if err != nil { + t.Fatal(err) + } + + salt := make([]byte, aead.SaltSize()) + if err := ssg.GetSalt(salt); err != nil { + t.Fatal(err) + } + if !ssg.IsServerSalt(salt) { + t.Error("Server salt was not recognized") + } +} + +// Test that ServerSaltGenerator doesn't recognize random salts +func TestServerSaltUnrecognized(t *testing.T) { + cipher, err := core.PickCipher(testCipher, nil, "test") + if err != nil { + t.Fatal(err) + } + aead := cipher.(shadowaead.Cipher) + ssg, err := NewServerSaltGenerator(aead) + if err != nil { + t.Fatal(err) + } + + salt := make([]byte, aead.SaltSize()) + if err := RandomSaltGenerator.GetSalt(salt); err != nil { + t.Fatal(err) + } + if ssg.IsServerSalt(salt) { + t.Error("Client salt was recognized as a server salt") + } +} + +// Test that ServerSaltGenerator produces different output on each call +func TestServerSaltDifferent(t *testing.T) { + cipher, err := core.PickCipher(testCipher, nil, "test") + if err != nil { + t.Fatal(err) + } + aead := cipher.(shadowaead.Cipher) + ssg, err := NewServerSaltGenerator(aead) + if err != nil { + t.Fatal(err) + } + + salt1 := make([]byte, aead.SaltSize()) + if err := ssg.GetSalt(salt1); err != nil { + t.Fatal(err) + } + salt2 := make([]byte, aead.SaltSize()) + if err := ssg.GetSalt(salt2); err != nil { + t.Fatal(err) + } + + if bytes.Equal(salt1, salt2) { + t.Error("salts should be random") + } +} + +// Test that two ServerSaltGenerators derived from the same cipher +// produce different outputs and recognize each other's output. +func TestServerSaltSameCipher(t *testing.T) { + cipher, err := core.PickCipher(testCipher, nil, "test") + if err != nil { + t.Fatal(err) + } + aead := cipher.(shadowaead.Cipher) + ssg1, err := NewServerSaltGenerator(aead) + if err != nil { + t.Fatal(err) + } + ssg2, err := NewServerSaltGenerator(aead) + if err != nil { + t.Fatal(err) + } + + salt1 := make([]byte, aead.SaltSize()) + if err := ssg1.GetSalt(salt1); err != nil { + t.Fatal(err) + } + salt2 := make([]byte, aead.SaltSize()) + if err := ssg2.GetSalt(salt2); err != nil { + t.Fatal(err) + } + + if bytes.Equal(salt1, salt2) { + t.Error("salts should be random") + } + + if !ssg1.IsServerSalt(salt2) || !ssg2.IsServerSalt(salt1) { + t.Error("Cross-recognition failed") + } +} + +// Test that two ServerSaltGenerators derived from the same secret +// produce different outputs and recognize each other's output. +func TestServerSaltSameSecret(t *testing.T) { + cipher1, err := core.PickCipher(testCipher, nil, "test") + if err != nil { + t.Fatal(err) + } + aead1 := cipher1.(shadowaead.Cipher) + cipher2, err := core.PickCipher(testCipher, nil, "test") + if err != nil { + t.Fatal(err) + } + aead2 := cipher2.(shadowaead.Cipher) + ssg1, err := NewServerSaltGenerator(aead1) + if err != nil { + t.Fatal(err) + } + ssg2, err := NewServerSaltGenerator(aead2) + if err != nil { + t.Fatal(err) + } + + salt1 := make([]byte, aead1.SaltSize()) + if err := ssg1.GetSalt(salt1); err != nil { + t.Fatal(err) + } + salt2 := make([]byte, aead2.SaltSize()) + if err := ssg2.GetSalt(salt2); err != nil { + t.Fatal(err) + } + + if bytes.Equal(salt1, salt2) { + t.Error("salts should be random") + } + + if !ssg1.IsServerSalt(salt2) || !ssg2.IsServerSalt(salt1) { + t.Error("Cross-recognition failed") + } +} + +// Test that two ServerSaltGenerators derived from different secrets +// do not recognize each other's output. +func TestServerSaltDifferentCiphers(t *testing.T) { + cipher1, err := core.PickCipher(testCipher, nil, "test1") + if err != nil { + t.Fatal(err) + } + aead1 := cipher1.(shadowaead.Cipher) + cipher2, err := core.PickCipher(testCipher, nil, "test2") + if err != nil { + t.Fatal(err) + } + aead2 := cipher2.(shadowaead.Cipher) + ssg1, err := NewServerSaltGenerator(aead1) + if err != nil { + t.Fatal(err) + } + ssg2, err := NewServerSaltGenerator(aead2) + if err != nil { + t.Fatal(err) + } + + salt1 := make([]byte, aead1.SaltSize()) + if err := ssg1.GetSalt(salt1); err != nil { + t.Fatal(err) + } + salt2 := make([]byte, aead2.SaltSize()) + if err := ssg2.GetSalt(salt2); err != nil { + t.Fatal(err) + } + + if bytes.Equal(salt1, salt2) { + t.Error("salts should be random") + } + + if ssg1.IsServerSalt(salt2) || ssg2.IsServerSalt(salt1) { + t.Error("Different ciphers should not recognize each other") + } +} + +func BenchmarkRandomSaltGenerator(b *testing.B) { + salt := make([]byte, 32) + for i := 0; i < b.N; i++ { + if err := RandomSaltGenerator.GetSalt(salt); err != nil { + b.Fatal(err) + } + } +} + +func BenchmarkServerSaltGenerator(b *testing.B) { + cipher, err := core.PickCipher(testCipher, nil, "test") + if err != nil { + b.Fatal(err) + } + aead := cipher.(shadowaead.Cipher) + ssg, err := NewServerSaltGenerator(aead) + if err != nil { + b.Fatal(err) + } + salt := make([]byte, aead.SaltSize()) + b.ResetTimer() + for i := 0; i < b.N; i++ { + if err := ssg.GetSalt(salt); err != nil { + b.Fatal(err) + } + if !ssg.IsServerSalt(salt) { + b.Fatal("Failed to recognize salt") + } + } +} diff --git a/shadowsocks/stream.go b/shadowsocks/stream.go index ec3341c1..02a41dc9 100644 --- a/shadowsocks/stream.go +++ b/shadowsocks/stream.go @@ -17,7 +17,6 @@ package shadowsocks import ( "bytes" "crypto/cipher" - "crypto/rand" "encoding/binary" "fmt" "io" @@ -29,84 +28,6 @@ import ( // payloadSizeMask is the maximum size of payload in bytes. const payloadSizeMask = 0x3FFF // 16*1024 - 1 -// SaltGenerator generates unique salts to use in Shadowsocks connections. -type SaltGenerator interface { - // Returns a new salt - GetSalt() ([]byte, error) -} - -// randomSaltGenerator generates a new random salt. -type randomSaltGenerator struct { - saltSize int -} - -// ServerSaltGenerator generates unique salts that are secretly marked. -type ServerSaltGenerator struct { - saltSize int - encrypter cipher.AEAD -} - -// GetSalt outputs a random salt. -func (sg randomSaltGenerator) GetSalt() ([]byte, error) { - salt := make([]byte, sg.saltSize) - _, err := io.ReadFull(rand.Reader, salt) - return salt, err -} - -// Number of bytes of salt to use as a marker. -const markLen = 4 - -// Constant to identify this marking scheme. -var serverIndication = []byte("outline-salt-mark") - -func NewServerSaltGenerator(cipher shadowaead.Cipher) (ServerSaltGenerator, error) { - saltSize := cipher.SaltSize() - zerosalt := make([]byte, saltSize) - encrypter, err := cipher.Encrypter(zerosalt) - if err != nil { - return ServerSaltGenerator{}, err - } - return ServerSaltGenerator{saltSize, encrypter}, nil -} - -func (sg ServerSaltGenerator) splitSalt(salt []byte) (prefix, mark []byte) { - prefixLen := sg.saltSize - markLen - prefix = salt[:prefixLen] - mark = salt[prefixLen:] - return -} - -// getTag takes in a salt prefix and writes out the tag. -// len(prefix) must be saltSize - markLen -func (sg ServerSaltGenerator) getTag(prefix []byte) []byte { - nonce := make([]byte, sg.encrypter.NonceSize()) - n := copy(nonce, prefix) - plaintext := prefix[n:] - encrypted := sg.encrypter.Seal(nil, nonce, plaintext, serverIndication) - return encrypted[len(plaintext):] -} - -// GetSalt returns an apparently random salt that can be identified -// as server-originated by anyone who knows the Shadowsocks key. -func (sg ServerSaltGenerator) GetSalt() ([]byte, error) { - salt := make([]byte, sg.saltSize) - prefix, mark := sg.splitSalt(salt) - _, err := io.ReadFull(rand.Reader, prefix) - if err != nil { - return nil, err - } - tag := sg.getTag(prefix) - copy(mark, tag) - return salt, nil -} - -// IsMarked returns true if the salt is marked as server-originated. -func (sg ServerSaltGenerator) IsMarked(salt []byte) bool { - prefix, mark := sg.splitSalt(salt) - tag := sg.getTag(prefix) - return bytes.Equal(tag[:markLen], mark) -} - // Writer is an io.Writer that also implements io.ReaderFrom to // allow for piping the data without extra allocations and copies. // The LazyWrite and Flush methods allow a header to be @@ -136,7 +57,7 @@ type Writer struct { // NewShadowsocksWriter creates a Writer that encrypts the given Writer using // the shadowsocks protocol with the given shadowsocks cipher. func NewShadowsocksWriter(writer io.Writer, ssCipher shadowaead.Cipher) *Writer { - return &Writer{writer: writer, ssCipher: ssCipher, saltGenerator: randomSaltGenerator{ssCipher.SaltSize()}} + return &Writer{writer: writer, ssCipher: ssCipher, saltGenerator: RandomSaltGenerator} } // SetSaltGenerator sets the salt generator to be used. Must be called before the first write. @@ -148,14 +69,15 @@ func (sw *Writer) SetSaltGenerator(saltGenerator SaltGenerator) { // the salt to the inner Writer. func (sw *Writer) init() (err error) { if sw.aead == nil { - salt, err := sw.saltGenerator.GetSalt() - if err != nil { + salt := make([]byte, sw.ssCipher.SaltSize()) + if err := sw.saltGenerator.GetSalt(salt); err != nil { return fmt.Errorf("failed to generate salt: %v", err) } sw.aead, err = sw.ssCipher.Encrypter(salt) if err != nil { return fmt.Errorf("failed to create AEAD: %v", err) } + sw.saltGenerator = nil // No longer needed, so release reference. sw.counter = make([]byte, sw.aead.NonceSize()) // The maximum length message is the salt (first message only), length, length tag, // payload, and payload tag. diff --git a/shadowsocks/tcp.go b/shadowsocks/tcp.go index c74e721c..a6611a24 100644 --- a/shadowsocks/tcp.go +++ b/shadowsocks/tcp.go @@ -29,7 +29,6 @@ import ( onet "github.com/Jigsaw-Code/outline-ss-server/net" logging "github.com/op/go-logging" - "github.com/shadowsocks/go-shadowsocks2/shadowaead" "github.com/shadowsocks/go-shadowsocks2/socks" ) @@ -57,12 +56,12 @@ func debugTCP(cipherID, template string, val interface{}) { } } -func findAccessKey(clientReader io.Reader, clientIP net.IP, cipherList CipherList) (string, shadowaead.Cipher, io.Reader, []byte, time.Duration, error) { +func findAccessKey(clientReader io.Reader, clientIP net.IP, cipherList CipherList) (*CipherEntry, io.Reader, []byte, time.Duration, error) { // We snapshot the list because it may be modified while we use it. tcpTrialSize, ciphers := cipherList.SnapshotForClientIP(clientIP) firstBytes := make([]byte, tcpTrialSize) if n, err := io.ReadFull(clientReader, firstBytes); err != nil { - return "", nil, clientReader, nil, 0, fmt.Errorf("Reading header failed after %d bytes: %v", n, err) + return nil, clientReader, nil, 0, fmt.Errorf("Reading header failed after %d bytes: %v", n, err) } findStartTime := time.Now() @@ -70,14 +69,13 @@ func findAccessKey(clientReader io.Reader, clientIP net.IP, cipherList CipherLis timeToCipher := time.Now().Sub(findStartTime) if entry == nil { // TODO: Ban and log client IPs with too many failures too quick to protect against DoS. - return "", nil, clientReader, nil, timeToCipher, fmt.Errorf("Could not find valid TCP cipher") + return nil, clientReader, nil, timeToCipher, fmt.Errorf("Could not find valid TCP cipher") } // Move the active cipher to the front, so that the search is quicker next time. cipherList.MarkUsedByClientIP(elt, clientIP) - id, cipher := entry.ID, entry.Cipher - salt := firstBytes[:cipher.SaltSize()] - return id, cipher, io.MultiReader(bytes.NewReader(firstBytes), clientReader), salt, timeToCipher, nil + salt := firstBytes[:entry.Cipher.SaltSize()] + return entry, io.MultiReader(bytes.NewReader(firstBytes), clientReader), salt, timeToCipher, nil } // Implements a trial decryption search. This assumes that all ciphers are AEAD. @@ -232,7 +230,7 @@ func (s *tcpService) handleConnection(listenerPort int, clientConn onet.DuplexCo clientConn.SetReadDeadline(connStart.Add(s.readTimeout)) var proxyMetrics metrics.ProxyMetrics clientConn = metrics.MeasureConn(clientConn, &proxyMetrics.ProxyClient, &proxyMetrics.ClientProxy) - keyID, cipher, clientReader, salt, timeToCipher, keyErr := findAccessKey(clientConn, remoteIP(clientConn), s.ciphers) + cipherEntry, clientReader, clientSalt, timeToCipher, keyErr := findAccessKey(clientConn, remoteIP(clientConn), s.ciphers) connError := func() *onet.ConnectionError { if keyErr != nil { @@ -242,31 +240,25 @@ func (s *tcpService) handleConnection(listenerPort int, clientConn onet.DuplexCo return onet.NewConnectionError(status, "Failed to find a valid cipher", keyErr) } - saltGenerator, err := NewServerSaltGenerator(cipher) - if err != nil { - return onet.NewConnectionError("ERR_SALTGEN", "Failed to construct salt generator", err) - } - - isMarked := saltGenerator.IsMarked(salt) - // Only check the cache if findAccessKey succeeded and the salt is unmarked. - if isMarked || !s.replayCache.Add(keyID, salt) { - const status = "ERR_REPLAY" - s.absorbProbe(listenerPort, clientConn, clientLocation, status, &proxyMetrics) - var msg string - if isMarked { - msg = "Server replay detected" + isServerSalt := cipherEntry.SaltGenerator.IsServerSalt(clientSalt) + // Only check the cache if findAccessKey succeeded and the salt is unrecognized. + if isServerSalt || !s.replayCache.Add(cipherEntry.ID, clientSalt) { + var status string + if isServerSalt { + status = "ERR_REPLAY_SERVER" } else { - msg = "Client replay detected" + status = "ERR_REPLAY" } - logger.Debugf(msg+": %v in %s sent %d bytes", clientConn.RemoteAddr(), clientLocation, proxyMetrics.ClientProxy) - return onet.NewConnectionError(status, msg, nil) + s.absorbProbe(listenerPort, clientConn, clientLocation, status, &proxyMetrics) + logger.Debugf(status+": %v in %s sent %d bytes", clientConn.RemoteAddr(), clientLocation, proxyMetrics.ClientProxy) + return onet.NewConnectionError(status, "Replay detected", nil) } // Clear the authentication deadline clientConn.SetReadDeadline(time.Time{}) - ssr := NewShadowsocksReader(clientReader, cipher) - ssw := NewShadowsocksWriter(clientConn, cipher) - ssw.SetSaltGenerator(saltGenerator) + ssr := NewShadowsocksReader(clientReader, cipherEntry.Cipher) + ssw := NewShadowsocksWriter(clientConn, cipherEntry.Cipher) + ssw.SetSaltGenerator(cipherEntry.SaltGenerator) clientConn = onet.WrapConn(clientConn, ssr, ssw) return proxyConnection(clientConn, &proxyMetrics, s.checkAllowedIP) }() @@ -277,7 +269,11 @@ func (s *tcpService) handleConnection(listenerPort int, clientConn onet.DuplexCo logger.Debugf("TCP Error: %v: %v", connError.Message, connError.Cause) status = connError.Status } - s.m.AddClosedTCPConnection(clientLocation, keyID, status, proxyMetrics, timeToCipher, connDuration) + var id string + if cipherEntry != nil { + id = cipherEntry.ID + } + s.m.AddClosedTCPConnection(clientLocation, id, status, proxyMetrics, timeToCipher, connDuration) clientConn.Close() // Closing after the metrics are added aids integration testing. logger.Debugf("Done with status %v, duration %v", status, connDuration) } diff --git a/shadowsocks/tcp_test.go b/shadowsocks/tcp_test.go index ad6ef4fa..2556e7c6 100644 --- a/shadowsocks/tcp_test.go +++ b/shadowsocks/tcp_test.go @@ -146,7 +146,7 @@ func BenchmarkTCPFindCipherRepeat(b *testing.B) { cipher := cipherEntries[cipherNumber].Cipher go NewShadowsocksWriter(writer, cipher).Write(MakeTestPayload(50)) b.StartTimer() - _, _, _, _, _, err := findAccessKey(&c, clientIP, cipherList) + _, _, _, _, err := findAccessKey(&c, clientIP, cipherList) b.StopTimer() if err != nil { b.Error(err) From bda76c72fc7617390c7b66ba81402a2327c4ac74 Mon Sep 17 00:00:00 2001 From: Ben Schwartz Date: Wed, 19 Aug 2020 21:12:30 -0400 Subject: [PATCH 05/14] Clarify probability claims --- shadowsocks/salt_generator.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/shadowsocks/salt_generator.go b/shadowsocks/salt_generator.go index 7059c0ce..ae03510e 100644 --- a/shadowsocks/salt_generator.go +++ b/shadowsocks/salt_generator.go @@ -67,8 +67,8 @@ func (sg ServerSaltGenerator) splitSalt(salt []byte) (prefix, mark []byte) { // len(prefix) must be saltSize - markLen, which must be at least nonceSize. // prefix must be random to avoid nonce reuse. func (sg ServerSaltGenerator) getTag(prefix []byte) []byte { - // Only the first nonceSize bytes are used to compute the tag. - // In the event of a nonce collision (p=2^-33 after 2^32 messages), + // Only the first nonceSize bytes are used to compute the tag. In the event + // of a nonce collision (p=2^-33 after 2^32 messages for nonceSize==12), // the only effect will be to reveal a pattern in the handshakes, not // to reuse the same nonce on different inputs (which can cause more // serious problems: https://www.imperialviolet.org/2015/05/16/aeads.html). From b447202989abb7b34bfcefa413bb56a6dee5b97c Mon Sep 17 00:00:00 2001 From: Ben Schwartz Date: Thu, 20 Aug 2020 10:00:45 -0400 Subject: [PATCH 06/14] Remove unused field from fakeAEAD --- shadowsocks/cipher_testing.go | 1 - 1 file changed, 1 deletion(-) diff --git a/shadowsocks/cipher_testing.go b/shadowsocks/cipher_testing.go index 79fcd2b3..e344718f 100644 --- a/shadowsocks/cipher_testing.go +++ b/shadowsocks/cipher_testing.go @@ -67,7 +67,6 @@ func MakeTestPayload(size int) []byte { type fakeAEAD struct { cipher.AEAD overhead, nonceSize int - salt []byte } func (a *fakeAEAD) NonceSize() int { From cfd8e000229028078946ef37190a20895dab6959 Mon Sep 17 00:00:00 2001 From: Ben Schwartz Date: Thu, 20 Aug 2020 10:03:04 -0400 Subject: [PATCH 07/14] Add license headers --- shadowsocks/salt_generator.go | 14 ++++++++++++++ shadowsocks/salt_generator_test.go | 14 ++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/shadowsocks/salt_generator.go b/shadowsocks/salt_generator.go index ae03510e..a9c1e7aa 100644 --- a/shadowsocks/salt_generator.go +++ b/shadowsocks/salt_generator.go @@ -1,3 +1,17 @@ +// Copyright 2020 Jigsaw Operations LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package shadowsocks import ( diff --git a/shadowsocks/salt_generator_test.go b/shadowsocks/salt_generator_test.go index 69763906..94d1dadf 100644 --- a/shadowsocks/salt_generator_test.go +++ b/shadowsocks/salt_generator_test.go @@ -1,3 +1,17 @@ +// Copyright 2020 Jigsaw Operations LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package shadowsocks import ( From 25cbc2f6908743c40af653666507e29ff56bcb9d Mon Sep 17 00:00:00 2001 From: Ben Schwartz Date: Fri, 21 Aug 2020 16:26:42 -0400 Subject: [PATCH 08/14] Switch to HMAC-SHA1 and HKDF-SHA1 --- server.go | 6 +- shadowsocks/cipher_list.go | 12 +- shadowsocks/cipher_list_test.go | 36 ++++- shadowsocks/cipher_testing.go | 47 +----- shadowsocks/salt_generator.go | 70 ++++---- shadowsocks/salt_generator_test.go | 247 ++++++----------------------- 6 files changed, 122 insertions(+), 296 deletions(-) diff --git a/server.go b/server.go index a01fb60b..2ef19d38 100644 --- a/server.go +++ b/server.go @@ -139,11 +139,7 @@ func (s *ssServer) loadConfig(filename string) error { if !ok { return fmt.Errorf("Only AEAD ciphers are supported. Found %v", keyConfig.Cipher) } - entry, err := shadowsocks.MakeCipherEntry(keyConfig.ID, aead) - if err != nil { - return fmt.Errorf("Failed to register cipher: %v", err) - } - cipherList.PushBack(&entry) + cipherList.PushBack(shadowsocks.MakeCipherEntry(keyConfig.ID, aead, keyConfig.Secret)) } for port := range s.ports { portChanges[port] = portChanges[port] - 1 diff --git a/shadowsocks/cipher_list.go b/shadowsocks/cipher_list.go index ef6c8da4..7bf6ff64 100644 --- a/shadowsocks/cipher_list.go +++ b/shadowsocks/cipher_list.go @@ -32,21 +32,17 @@ const maxNonceSize = 12 type CipherEntry struct { ID string Cipher shadowaead.Cipher - SaltGenerator ServerSaltGenerator + SaltGenerator *ServerSaltGenerator lastClientIP net.IP } // MakeCipherEntry constructs a CipherEntry. -func MakeCipherEntry(id string, cipher shadowaead.Cipher) (CipherEntry, error) { - saltGenerator, err := NewServerSaltGenerator(cipher) - if err != nil { - return CipherEntry{}, err - } +func MakeCipherEntry(id string, cipher shadowaead.Cipher, secret string) CipherEntry { return CipherEntry{ ID: id, Cipher: cipher, - SaltGenerator: saltGenerator, - }, nil + SaltGenerator: NewServerSaltGenerator(secret), + } } // CipherList is a thread-safe collection of CipherEntry elements that allows for diff --git a/shadowsocks/cipher_list_test.go b/shadowsocks/cipher_list_test.go index 30eea51f..fb120012 100644 --- a/shadowsocks/cipher_list_test.go +++ b/shadowsocks/cipher_list_test.go @@ -16,17 +16,45 @@ package shadowsocks import ( "container/list" + "crypto/cipher" "testing" "github.com/shadowsocks/go-shadowsocks2/shadowaead" ) +type fakeAEAD struct { + cipher.AEAD + overhead, nonceSize int +} + +func (a *fakeAEAD) NonceSize() int { + return a.nonceSize +} + +func (a *fakeAEAD) Overhead() int { + return a.overhead +} + +type fakeCipher struct { + shadowaead.Cipher + saltsize int + decrypter *fakeAEAD +} + +func (c *fakeCipher) SaltSize() int { + return c.saltsize +} + +func (c *fakeCipher) Decrypter(b []byte) (cipher.AEAD, error) { + return c.decrypter, nil +} + func TestIncompatibleCiphers(t *testing.T) { l := list.New() l.PushBack(&CipherEntry{ ID: "short", - Cipher: &fakeCipher{saltSize: 5, aead: &fakeAEAD{overhead: 3}}}) - l.PushBack(&CipherEntry{ID: "long", Cipher: &fakeCipher{saltSize: 50, aead: &fakeAEAD{overhead: 30}}}) + Cipher: &fakeCipher{saltsize: 5, decrypter: &fakeAEAD{overhead: 3}}}) + l.PushBack(&CipherEntry{ID: "long", Cipher: &fakeCipher{saltsize: 50, decrypter: &fakeAEAD{overhead: 30}}}) cipherList := NewCipherList() err := cipherList.Update(l) if err == nil { @@ -38,8 +66,8 @@ func TestMaxNonceSize(t *testing.T) { l := list.New() l.PushBack(&CipherEntry{ ID: "oversize nonce", - Cipher: &fakeCipher{saltSize: 5, aead: &fakeAEAD{overhead: 3, nonceSize: 13}}}) - l.PushBack(&CipherEntry{ID: "long", Cipher: &fakeCipher{saltSize: 50, aead: &fakeAEAD{overhead: 30}}}) + Cipher: &fakeCipher{saltsize: 5, decrypter: &fakeAEAD{overhead: 3, nonceSize: 13}}}) + l.PushBack(&CipherEntry{ID: "long", Cipher: &fakeCipher{saltsize: 50, decrypter: &fakeAEAD{overhead: 30}}}) cipherList := NewCipherList() err := cipherList.Update(l) if err == nil { diff --git a/shadowsocks/cipher_testing.go b/shadowsocks/cipher_testing.go index e344718f..93bbd612 100644 --- a/shadowsocks/cipher_testing.go +++ b/shadowsocks/cipher_testing.go @@ -16,7 +16,6 @@ package shadowsocks import ( "container/list" - "crypto/cipher" "fmt" "github.com/shadowsocks/go-shadowsocks2/core" @@ -44,10 +43,7 @@ func MakeTestCiphers(secrets []string) (CipherList, error) { if err != nil { return nil, fmt.Errorf("Failed to create cipher %v: %v", i, err) } - entry, err := MakeCipherEntry(cipherID, cipher.(shadowaead.Cipher)) - if err != nil { - return nil, fmt.Errorf("Failed to create entry: %v", err) - } + entry := MakeCipherEntry(cipherID, cipher.(shadowaead.Cipher), secrets[i]) l.PushBack(&entry) } cipherList := NewCipherList() @@ -63,44 +59,3 @@ func MakeTestPayload(size int) []byte { } return payload } - -type fakeAEAD struct { - cipher.AEAD - overhead, nonceSize int -} - -func (a *fakeAEAD) NonceSize() int { - return a.nonceSize -} - -func (a *fakeAEAD) Overhead() int { - return a.overhead -} - -type fakeCipher struct { - keySize int - saltSize int - aead cipher.AEAD -} - -func (c *fakeCipher) KeySize() int { - return c.keySize -} - -func (c *fakeCipher) SaltSize() int { - return c.saltSize -} - -func (c *fakeCipher) Decrypter(salt []byte) (cipher.AEAD, error) { - if len(salt) != c.saltSize { - return nil, fmt.Errorf("Wrong salt size: %d != %d", len(salt), c.saltSize) - } - return c.aead, nil -} - -func (c *fakeCipher) Encrypter(salt []byte) (cipher.AEAD, error) { - if len(salt) != c.saltSize { - return nil, fmt.Errorf("Wrong salt size: %d != %d", len(salt), c.saltSize) - } - return c.aead, nil -} diff --git a/shadowsocks/salt_generator.go b/shadowsocks/salt_generator.go index a9c1e7aa..218f7692 100644 --- a/shadowsocks/salt_generator.go +++ b/shadowsocks/salt_generator.go @@ -16,12 +16,12 @@ package shadowsocks import ( "bytes" - "crypto/cipher" + "crypto" + "crypto/hmac" "crypto/rand" - "fmt" "io" - "github.com/shadowsocks/go-shadowsocks2/shadowaead" + "golang.org/x/crypto/hkdf" ) // SaltGenerator generates unique salts to use in Shadowsocks connections. @@ -35,7 +35,7 @@ type randomSaltGenerator struct{} // GetSalt outputs a random salt. func (randomSaltGenerator) GetSalt(salt []byte) error { - _, err := io.ReadFull(rand.Reader, salt) + _, err := rand.Read(salt) return err } @@ -44,8 +44,7 @@ var RandomSaltGenerator SaltGenerator = randomSaltGenerator{} // ServerSaltGenerator generates unique salts that are secretly marked. type ServerSaltGenerator struct { - saltSize int - encrypter cipher.AEAD + key []byte } // Number of bytes of salt to use as a marker. Increasing this value reduces @@ -53,6 +52,11 @@ type ServerSaltGenerator struct { // Must be less than or equal to the cipher overhead. const markLen = 4 +// For a random salt to be secure, it needs at least 16 bytes (128 bits) of +// entropy. If adding the mark would reduce the entropy below this level, +// we generate an unmarked random salt. +const minEntropy = 16 + // Constant to identify this marking scheme. var serverSaltLabel = []byte("outline-server-salt") @@ -60,45 +64,38 @@ var serverSaltLabel = []byte("outline-server-salt") // random, but is secretly marked as being issued by the server. // This is useful to prevent the server from accepting its own output in a // reflection attack. -func NewServerSaltGenerator(cipher shadowaead.Cipher) (ServerSaltGenerator, error) { - saltSize := cipher.SaltSize() - zeroSalt := make([]byte, saltSize) - encrypter, err := cipher.Encrypter(zeroSalt) - if err != nil { - return ServerSaltGenerator{}, err - } - return ServerSaltGenerator{saltSize, encrypter}, nil +func NewServerSaltGenerator(secret string) *ServerSaltGenerator { + // Shadowsocks already uses HKDF-SHA1 to derive the AEAD key, so we use + // the same derivation with a different "info" to generate our HMAC key. + keySource := hkdf.New(crypto.SHA1.New, []byte(secret), nil, serverSaltLabel) + // The key can be any size, but matching the block size is most efficient. + key := make([]byte, crypto.SHA1.Size()) + io.ReadFull(keySource, key) + return &ServerSaltGenerator{key} } -func (sg ServerSaltGenerator) splitSalt(salt []byte) (prefix, mark []byte) { - prefixLen := sg.saltSize - markLen - prefix = salt[:prefixLen] - mark = salt[prefixLen:] - return +func (sg *ServerSaltGenerator) splitSalt(salt []byte) (prefix, mark []byte) { + prefixLen := len(salt) - markLen + return salt[:prefixLen], salt[prefixLen:] } // getTag takes in a salt prefix and returns the tag. -// len(prefix) must be saltSize - markLen, which must be at least nonceSize. -// prefix must be random to avoid nonce reuse. -func (sg ServerSaltGenerator) getTag(prefix []byte) []byte { - // Only the first nonceSize bytes are used to compute the tag. In the event - // of a nonce collision (p=2^-33 after 2^32 messages for nonceSize==12), - // the only effect will be to reveal a pattern in the handshakes, not - // to reuse the same nonce on different inputs (which can cause more - // serious problems: https://www.imperialviolet.org/2015/05/16/aeads.html). - nonce := prefix[:sg.encrypter.NonceSize()] - return sg.encrypter.Seal(nil, nonce, nil, serverSaltLabel) +func (sg *ServerSaltGenerator) getTag(prefix []byte) []byte { + // Use HMAC-SHA1, even though SHA1 is broken, because HMAC-SHA1 is still + // secure, and we're already using HKDF-SHA1. + hmac := hmac.New(crypto.SHA1.New, sg.key) + hmac.Write(prefix) // Hash.Write never returns an error. + return hmac.Sum(nil) } // GetSalt returns an apparently random salt that can be identified // as server-originated by anyone who knows the Shadowsocks key. -func (sg ServerSaltGenerator) GetSalt(salt []byte) error { - if len(salt) != sg.saltSize { - return fmt.Errorf("Wrong salt size: %d != %d", len(salt), sg.saltSize) +func (sg *ServerSaltGenerator) GetSalt(salt []byte) error { + if len(salt)-markLen < minEntropy { + return RandomSaltGenerator.GetSalt(salt) } prefix, mark := sg.splitSalt(salt) - _, err := io.ReadFull(rand.Reader, prefix) - if err != nil { + if _, err := rand.Read(prefix); err != nil { return err } tag := sg.getTag(prefix) @@ -107,7 +104,10 @@ func (sg ServerSaltGenerator) GetSalt(salt []byte) error { } // IsServerSalt returns true if the salt is marked as server-originated. -func (sg ServerSaltGenerator) IsServerSalt(salt []byte) bool { +func (sg *ServerSaltGenerator) IsServerSalt(salt []byte) bool { + if len(salt) < markLen { + return false + } prefix, mark := sg.splitSalt(salt) tag := sg.getTag(prefix) return bytes.Equal(tag[:markLen], mark) diff --git a/shadowsocks/salt_generator_test.go b/shadowsocks/salt_generator_test.go index 94d1dadf..069fee31 100644 --- a/shadowsocks/salt_generator_test.go +++ b/shadowsocks/salt_generator_test.go @@ -17,9 +17,6 @@ package shadowsocks import ( "bytes" "testing" - - "github.com/shadowsocks/go-shadowsocks2/core" - "github.com/shadowsocks/go-shadowsocks2/shadowaead" ) func TestRandomSaltGenerator(t *testing.T) { @@ -35,96 +32,11 @@ func TestRandomSaltGenerator(t *testing.T) { } } -type mockAEAD struct { - fakeAEAD - t *testing.T - sealed bool - tag []byte -} - -func (a *mockAEAD) Seal(dst, nonce, plaintext, additionalData []byte) []byte { - if len(nonce) != a.NonceSize() { - a.t.Errorf("Wrong nonce length: %d != %d", len(nonce), a.NonceSize()) - } - if len(plaintext) != 0 { - a.t.Errorf("Wrong plaintext length: %d != 0", len(plaintext)) - } - if !bytes.Equal(additionalData, serverSaltLabel) { - a.t.Errorf("Wrong additional data: %v", additionalData) - } - a.sealed = true - dst = append(dst, a.tag...) - return dst -} - -// Test that ServerSaltGenerator works as expected using a fake cipher. -func TestServerSaltFake(t *testing.T) { - tag := []byte("1234567890123456") - mockCipher := &fakeCipher{ - saltSize: 32, - aead: &mockAEAD{ - fakeAEAD: fakeAEAD{ - nonceSize: 12, - overhead: 16, - }, - t: t, - tag: tag, - }, - } - - ssg, err := NewServerSaltGenerator(mockCipher) - if err != nil { - t.Fatal(err) - } - salt := make([]byte, mockCipher.saltSize) - if err := ssg.GetSalt(salt); err != nil { - t.Fatal(err) - } - if bytes.Equal(salt, make([]byte, len(salt))) { - t.Error("Salt is zero") - } - if !bytes.Equal(salt[len(salt)-markLen:], tag[:markLen]) { - t.Error("Tag mismatch") - } - if !ssg.IsServerSalt(salt) { - t.Error("Tag was not recognized") - } - - // Make another random salt with the same tag - salt2 := make([]byte, mockCipher.saltSize) - if err := ssg.GetSalt(salt2); err != nil { - t.Fatal(err) - } - if bytes.Equal(salt, salt2) { - t.Error("Salts should be different") - } - if !bytes.Equal(salt2[len(salt2)-markLen:], tag[:markLen]) { - t.Error("Tag mismatch") - } - if !ssg.IsServerSalt(salt2) { - t.Error("Tag was not recognized") - } - - // Alter tag - salt[len(salt)-1]++ - if ssg.IsServerSalt(salt) { - t.Error("Altered tag was still recognized") - } -} - // Test that ServerSaltGenerator recognizes its own salts func TestServerSaltRecognized(t *testing.T) { - cipher, err := core.PickCipher(testCipher, nil, "test") - if err != nil { - t.Fatal(err) - } - aead := cipher.(shadowaead.Cipher) - ssg, err := NewServerSaltGenerator(aead) - if err != nil { - t.Fatal(err) - } + ssg := NewServerSaltGenerator("test") - salt := make([]byte, aead.SaltSize()) + salt := make([]byte, 32) if err := ssg.GetSalt(salt); err != nil { t.Fatal(err) } @@ -135,17 +47,9 @@ func TestServerSaltRecognized(t *testing.T) { // Test that ServerSaltGenerator doesn't recognize random salts func TestServerSaltUnrecognized(t *testing.T) { - cipher, err := core.PickCipher(testCipher, nil, "test") - if err != nil { - t.Fatal(err) - } - aead := cipher.(shadowaead.Cipher) - ssg, err := NewServerSaltGenerator(aead) - if err != nil { - t.Fatal(err) - } + ssg := NewServerSaltGenerator("test") - salt := make([]byte, aead.SaltSize()) + salt := make([]byte, 32) if err := RandomSaltGenerator.GetSalt(salt); err != nil { t.Fatal(err) } @@ -156,21 +60,13 @@ func TestServerSaltUnrecognized(t *testing.T) { // Test that ServerSaltGenerator produces different output on each call func TestServerSaltDifferent(t *testing.T) { - cipher, err := core.PickCipher(testCipher, nil, "test") - if err != nil { - t.Fatal(err) - } - aead := cipher.(shadowaead.Cipher) - ssg, err := NewServerSaltGenerator(aead) - if err != nil { - t.Fatal(err) - } + ssg := NewServerSaltGenerator("test") - salt1 := make([]byte, aead.SaltSize()) + salt1 := make([]byte, 32) if err := ssg.GetSalt(salt1); err != nil { t.Fatal(err) } - salt2 := make([]byte, aead.SaltSize()) + salt2 := make([]byte, 32) if err := ssg.GetSalt(salt2); err != nil { t.Fatal(err) } @@ -180,28 +76,17 @@ func TestServerSaltDifferent(t *testing.T) { } } -// Test that two ServerSaltGenerators derived from the same cipher +// Test that two ServerSaltGenerators derived from the same secret // produce different outputs and recognize each other's output. -func TestServerSaltSameCipher(t *testing.T) { - cipher, err := core.PickCipher(testCipher, nil, "test") - if err != nil { - t.Fatal(err) - } - aead := cipher.(shadowaead.Cipher) - ssg1, err := NewServerSaltGenerator(aead) - if err != nil { - t.Fatal(err) - } - ssg2, err := NewServerSaltGenerator(aead) - if err != nil { - t.Fatal(err) - } +func TestServerSaltSameSecret(t *testing.T) { + ssg1 := NewServerSaltGenerator("test") + ssg2 := NewServerSaltGenerator("test") - salt1 := make([]byte, aead.SaltSize()) + salt1 := make([]byte, 32) if err := ssg1.GetSalt(salt1); err != nil { t.Fatal(err) } - salt2 := make([]byte, aead.SaltSize()) + salt2 := make([]byte, 32) if err := ssg2.GetSalt(salt2); err != nil { t.Fatal(err) } @@ -215,33 +100,17 @@ func TestServerSaltSameCipher(t *testing.T) { } } -// Test that two ServerSaltGenerators derived from the same secret -// produce different outputs and recognize each other's output. -func TestServerSaltSameSecret(t *testing.T) { - cipher1, err := core.PickCipher(testCipher, nil, "test") - if err != nil { - t.Fatal(err) - } - aead1 := cipher1.(shadowaead.Cipher) - cipher2, err := core.PickCipher(testCipher, nil, "test") - if err != nil { - t.Fatal(err) - } - aead2 := cipher2.(shadowaead.Cipher) - ssg1, err := NewServerSaltGenerator(aead1) - if err != nil { - t.Fatal(err) - } - ssg2, err := NewServerSaltGenerator(aead2) - if err != nil { - t.Fatal(err) - } +// Test that two ServerSaltGenerators derived from different secrets +// do not recognize each other's output. +func TestServerSaltDifferentCiphers(t *testing.T) { + ssg1 := NewServerSaltGenerator("test1") + ssg2 := NewServerSaltGenerator("test2") - salt1 := make([]byte, aead1.SaltSize()) + salt1 := make([]byte, 32) if err := ssg1.GetSalt(salt1); err != nil { t.Fatal(err) } - salt2 := make([]byte, aead2.SaltSize()) + salt2 := make([]byte, 32) if err := ssg2.GetSalt(salt2); err != nil { t.Fatal(err) } @@ -250,48 +119,36 @@ func TestServerSaltSameSecret(t *testing.T) { t.Error("salts should be random") } - if !ssg1.IsServerSalt(salt2) || !ssg2.IsServerSalt(salt1) { - t.Error("Cross-recognition failed") + if ssg1.IsServerSalt(salt2) || ssg2.IsServerSalt(salt1) { + t.Error("Different ciphers should not recognize each other") } } -// Test that two ServerSaltGenerators derived from different secrets -// do not recognize each other's output. -func TestServerSaltDifferentCiphers(t *testing.T) { - cipher1, err := core.PickCipher(testCipher, nil, "test1") - if err != nil { - t.Fatal(err) - } - aead1 := cipher1.(shadowaead.Cipher) - cipher2, err := core.PickCipher(testCipher, nil, "test2") - if err != nil { - t.Fatal(err) - } - aead2 := cipher2.(shadowaead.Cipher) - ssg1, err := NewServerSaltGenerator(aead1) - if err != nil { +func TestServerSaltShort(t *testing.T) { + ssg := NewServerSaltGenerator("test") + + salt20 := make([]byte, 20) + if err := ssg.GetSalt(salt20); err != nil { t.Fatal(err) } - ssg2, err := NewServerSaltGenerator(aead2) - if err != nil { - t.Fatal(err) + if !ssg.IsServerSalt(salt20) { + t.Error("Server salt was not recognized") } - salt1 := make([]byte, aead1.SaltSize()) - if err := ssg1.GetSalt(salt1); err != nil { + salt19 := make([]byte, 19) + if err := ssg.GetSalt(salt19); err != nil { t.Fatal(err) } - salt2 := make([]byte, aead2.SaltSize()) - if err := ssg2.GetSalt(salt2); err != nil { - t.Fatal(err) + if ssg.IsServerSalt(salt19) { + t.Error("Short salt was marked") } - if bytes.Equal(salt1, salt2) { - t.Error("salts should be random") + salt2 := make([]byte, 2) + if err := ssg.GetSalt(salt2); err != nil { + t.Fatal(err) } - - if ssg1.IsServerSalt(salt2) || ssg2.IsServerSalt(salt1) { - t.Error("Different ciphers should not recognize each other") + if ssg.IsServerSalt(salt2) { + t.Error("Very short salt was marked") } } @@ -305,23 +162,17 @@ func BenchmarkRandomSaltGenerator(b *testing.B) { } func BenchmarkServerSaltGenerator(b *testing.B) { - cipher, err := core.PickCipher(testCipher, nil, "test") - if err != nil { - b.Fatal(err) - } - aead := cipher.(shadowaead.Cipher) - ssg, err := NewServerSaltGenerator(aead) - if err != nil { - b.Fatal(err) - } - salt := make([]byte, aead.SaltSize()) + ssg := NewServerSaltGenerator("test") b.ResetTimer() - for i := 0; i < b.N; i++ { - if err := ssg.GetSalt(salt); err != nil { - b.Fatal(err) - } - if !ssg.IsServerSalt(salt) { - b.Fatal("Failed to recognize salt") + b.RunParallel(func(pb *testing.PB) { + salt := make([]byte, 32) + for pb.Next() { + if err := ssg.GetSalt(salt); err != nil { + b.Fatal(err) + } + if !ssg.IsServerSalt(salt) { + b.Fatal("Failed to recognize salt") + } } - } + }) } From 463afb06f106a1201062e1995f53431206c057fe Mon Sep 17 00:00:00 2001 From: Ben Schwartz Date: Fri, 21 Aug 2020 16:40:33 -0400 Subject: [PATCH 09/14] Rename ERR_REPLAY and add a test --- shadowsocks/tcp.go | 2 +- shadowsocks/tcp_test.go | 66 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/shadowsocks/tcp.go b/shadowsocks/tcp.go index a6611a24..dc428e96 100644 --- a/shadowsocks/tcp.go +++ b/shadowsocks/tcp.go @@ -247,7 +247,7 @@ func (s *tcpService) handleConnection(listenerPort int, clientConn onet.DuplexCo if isServerSalt { status = "ERR_REPLAY_SERVER" } else { - status = "ERR_REPLAY" + status = "ERR_REPLAY_CLIENT" } s.absorbProbe(listenerPort, clientConn, clientLocation, status, &proxyMetrics) logger.Debugf(status+": %v in %s sent %d bytes", clientConn.RemoteAddr(), clientLocation, proxyMetrics.ClientProxy) diff --git a/shadowsocks/tcp_test.go b/shadowsocks/tcp_test.go index 2556e7c6..b26fcb43 100644 --- a/shadowsocks/tcp_test.go +++ b/shadowsocks/tcp_test.go @@ -255,7 +255,7 @@ func TestReplayDefense(t *testing.T) { t.Errorf("Unexpected probe data: %v", data) } status := testMetrics.probeStatus[0] - if status != "ERR_REPLAY" { + if status != "ERR_REPLAY_CLIENT" { t.Errorf("Unexpected TCP probe status: %s", status) } } else { @@ -263,7 +263,69 @@ func TestReplayDefense(t *testing.T) { } if len(testMetrics.closeStatus) == 2 { status := testMetrics.closeStatus[1] - if status != "ERR_REPLAY" { + if status != "ERR_REPLAY_CLIENT" { + t.Errorf("Unexpected TCP close status: %s", status) + } + } else { + t.Error("Replay should have reported an error status") + } +} + +func TestReverseReplayDefense(t *testing.T) { + listener, err := net.ListenTCP("tcp", &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 0}) + if err != nil { + t.Fatalf("ListenTCP failed: %v", err) + } + cipherList, err := MakeTestCiphers(MakeTestSecrets(1)) + if err != nil { + t.Fatal(err) + } + replayCache := NewReplayCache(5) + testMetrics := &probeTestMetrics{} + const testTimeout = 200 * time.Millisecond + s := NewTCPService(cipherList, &replayCache, testMetrics, testTimeout) + _, snapshot := cipherList.SnapshotForClientIP(nil) + cipherEntry := snapshot[0].Value.(*CipherEntry) + cipher := cipherEntry.Cipher + reader, writer := io.Pipe() + ssWriter := NewShadowsocksWriter(writer, cipher) + // Use a server-marked salt in the client's preamble. + ssWriter.SetSaltGenerator(cipherEntry.SaltGenerator) + go ssWriter.Write([]byte{0}) + preamble := make([]byte, 32+2+16) + if _, err := io.ReadFull(reader, preamble); err != nil { + t.Fatal(err) + } + + go s.Serve(listener) + + conn, err := net.Dial(listener.Addr().Network(), listener.Addr().String()) + if err != nil { + t.Fatal(err) + } + n, err := conn.Write(preamble) + if n < len(preamble) { + t.Error(err) + } + conn.Close() + s.GracefulStop() + + // The preamble should have been marked as a server replay. + if len(testMetrics.probeData) == 1 { + data := testMetrics.probeData[0] + if data.ClientProxy != int64(len(preamble)) { + t.Errorf("Unexpected probe data: %v", data) + } + status := testMetrics.probeStatus[0] + if status != "ERR_REPLAY_SERVER" { + t.Errorf("Unexpected TCP probe status: %s", status) + } + } else { + t.Error("Replay should have triggered probe detection") + } + if len(testMetrics.closeStatus) == 1 { + status := testMetrics.closeStatus[0] + if status != "ERR_REPLAY_SERVER" { t.Errorf("Unexpected TCP close status: %s", status) } } else { From 958c818b3316dcc116b7199d2eb3881c0b771812 Mon Sep 17 00:00:00 2001 From: Ben Schwartz Date: Mon, 24 Aug 2020 11:21:30 -0400 Subject: [PATCH 10/14] Restructure salt size validity checks --- shadowsocks/cipher_list.go | 4 +- shadowsocks/cipher_list_test.go | 28 -------------- shadowsocks/cipher_testing.go | 28 ++++++++++++++ shadowsocks/salt_generator.go | 46 ++++++++++++++++------- shadowsocks/salt_generator_test.go | 59 ++++++++++++++++++------------ 5 files changed, 97 insertions(+), 68 deletions(-) diff --git a/shadowsocks/cipher_list.go b/shadowsocks/cipher_list.go index 7bf6ff64..f6dbba91 100644 --- a/shadowsocks/cipher_list.go +++ b/shadowsocks/cipher_list.go @@ -32,7 +32,7 @@ const maxNonceSize = 12 type CipherEntry struct { ID string Cipher shadowaead.Cipher - SaltGenerator *ServerSaltGenerator + SaltGenerator ServerSaltGenerator lastClientIP net.IP } @@ -41,7 +41,7 @@ func MakeCipherEntry(id string, cipher shadowaead.Cipher, secret string) CipherE return CipherEntry{ ID: id, Cipher: cipher, - SaltGenerator: NewServerSaltGenerator(secret), + SaltGenerator: NewServerSaltGenerator(cipher, secret), } } diff --git a/shadowsocks/cipher_list_test.go b/shadowsocks/cipher_list_test.go index fb120012..e6f1e9c2 100644 --- a/shadowsocks/cipher_list_test.go +++ b/shadowsocks/cipher_list_test.go @@ -16,39 +16,11 @@ package shadowsocks import ( "container/list" - "crypto/cipher" "testing" "github.com/shadowsocks/go-shadowsocks2/shadowaead" ) -type fakeAEAD struct { - cipher.AEAD - overhead, nonceSize int -} - -func (a *fakeAEAD) NonceSize() int { - return a.nonceSize -} - -func (a *fakeAEAD) Overhead() int { - return a.overhead -} - -type fakeCipher struct { - shadowaead.Cipher - saltsize int - decrypter *fakeAEAD -} - -func (c *fakeCipher) SaltSize() int { - return c.saltsize -} - -func (c *fakeCipher) Decrypter(b []byte) (cipher.AEAD, error) { - return c.decrypter, nil -} - func TestIncompatibleCiphers(t *testing.T) { l := list.New() l.PushBack(&CipherEntry{ diff --git a/shadowsocks/cipher_testing.go b/shadowsocks/cipher_testing.go index 93bbd612..2778156e 100644 --- a/shadowsocks/cipher_testing.go +++ b/shadowsocks/cipher_testing.go @@ -16,6 +16,7 @@ package shadowsocks import ( "container/list" + "crypto/cipher" "fmt" "github.com/shadowsocks/go-shadowsocks2/core" @@ -59,3 +60,30 @@ func MakeTestPayload(size int) []byte { } return payload } + +type fakeAEAD struct { + cipher.AEAD + overhead, nonceSize int +} + +func (a *fakeAEAD) NonceSize() int { + return a.nonceSize +} + +func (a *fakeAEAD) Overhead() int { + return a.overhead +} + +type fakeCipher struct { + shadowaead.Cipher + saltsize int + decrypter *fakeAEAD +} + +func (c *fakeCipher) SaltSize() int { + return c.saltsize +} + +func (c *fakeCipher) Decrypter(b []byte) (cipher.AEAD, error) { + return c.decrypter, nil +} diff --git a/shadowsocks/salt_generator.go b/shadowsocks/salt_generator.go index 218f7692..e679d5bb 100644 --- a/shadowsocks/salt_generator.go +++ b/shadowsocks/salt_generator.go @@ -19,8 +19,10 @@ import ( "crypto" "crypto/hmac" "crypto/rand" + "fmt" "io" + "github.com/shadowsocks/go-shadowsocks2/shadowaead" "golang.org/x/crypto/hkdf" ) @@ -30,6 +32,13 @@ type SaltGenerator interface { GetSalt(salt []byte) error } +type ServerSaltGenerator interface { + SaltGenerator + // IsServerSalt returns true if the salt was created by this generator + // and is marked as server-originated. + IsServerSalt(salt []byte) bool +} + // randomSaltGenerator generates a new random salt. type randomSaltGenerator struct{} @@ -39,12 +48,17 @@ func (randomSaltGenerator) GetSalt(salt []byte) error { return err } +func (randomSaltGenerator) IsServerSalt(salt []byte) bool { + return false +} + // RandomSaltGenerator is a basic SaltGenerator. -var RandomSaltGenerator SaltGenerator = randomSaltGenerator{} +var RandomSaltGenerator ServerSaltGenerator = randomSaltGenerator{} -// ServerSaltGenerator generates unique salts that are secretly marked. -type ServerSaltGenerator struct { - key []byte +// serverSaltGenerator generates unique salts that are secretly marked. +type serverSaltGenerator struct { + saltSize int + key []byte } // Number of bytes of salt to use as a marker. Increasing this value reduces @@ -64,23 +78,28 @@ var serverSaltLabel = []byte("outline-server-salt") // random, but is secretly marked as being issued by the server. // This is useful to prevent the server from accepting its own output in a // reflection attack. -func NewServerSaltGenerator(secret string) *ServerSaltGenerator { +func NewServerSaltGenerator(cipher shadowaead.Cipher, secret string) ServerSaltGenerator { + if cipher.SaltSize()-markLen < minEntropy { + // This cipher doesn't support server marking. + return RandomSaltGenerator + } + // Shadowsocks already uses HKDF-SHA1 to derive the AEAD key, so we use // the same derivation with a different "info" to generate our HMAC key. keySource := hkdf.New(crypto.SHA1.New, []byte(secret), nil, serverSaltLabel) // The key can be any size, but matching the block size is most efficient. key := make([]byte, crypto.SHA1.Size()) io.ReadFull(keySource, key) - return &ServerSaltGenerator{key} + return serverSaltGenerator{cipher.SaltSize(), key} } -func (sg *ServerSaltGenerator) splitSalt(salt []byte) (prefix, mark []byte) { +func (sg serverSaltGenerator) splitSalt(salt []byte) (prefix, mark []byte) { prefixLen := len(salt) - markLen return salt[:prefixLen], salt[prefixLen:] } // getTag takes in a salt prefix and returns the tag. -func (sg *ServerSaltGenerator) getTag(prefix []byte) []byte { +func (sg serverSaltGenerator) getTag(prefix []byte) []byte { // Use HMAC-SHA1, even though SHA1 is broken, because HMAC-SHA1 is still // secure, and we're already using HKDF-SHA1. hmac := hmac.New(crypto.SHA1.New, sg.key) @@ -90,9 +109,9 @@ func (sg *ServerSaltGenerator) getTag(prefix []byte) []byte { // GetSalt returns an apparently random salt that can be identified // as server-originated by anyone who knows the Shadowsocks key. -func (sg *ServerSaltGenerator) GetSalt(salt []byte) error { - if len(salt)-markLen < minEntropy { - return RandomSaltGenerator.GetSalt(salt) +func (sg serverSaltGenerator) GetSalt(salt []byte) error { + if len(salt) != sg.saltSize { + return fmt.Errorf("Wrong salt size: %d != %d", len(salt), sg.saltSize) } prefix, mark := sg.splitSalt(salt) if _, err := rand.Read(prefix); err != nil { @@ -103,9 +122,8 @@ func (sg *ServerSaltGenerator) GetSalt(salt []byte) error { return nil } -// IsServerSalt returns true if the salt is marked as server-originated. -func (sg *ServerSaltGenerator) IsServerSalt(salt []byte) bool { - if len(salt) < markLen { +func (sg serverSaltGenerator) IsServerSalt(salt []byte) bool { + if len(salt) != sg.saltSize { return false } prefix, mark := sg.splitSalt(salt) diff --git a/shadowsocks/salt_generator_test.go b/shadowsocks/salt_generator_test.go index 069fee31..f5aa882f 100644 --- a/shadowsocks/salt_generator_test.go +++ b/shadowsocks/salt_generator_test.go @@ -30,11 +30,14 @@ func TestRandomSaltGenerator(t *testing.T) { if bytes.Equal(salt, make([]byte, 16)) { t.Error("Salt is all zeros") } + if RandomSaltGenerator.IsServerSalt(salt) { + t.Error("RandomSaltGenerator.IsServerSalt is always false") + } } // Test that ServerSaltGenerator recognizes its own salts func TestServerSaltRecognized(t *testing.T) { - ssg := NewServerSaltGenerator("test") + ssg := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test") salt := make([]byte, 32) if err := ssg.GetSalt(salt); err != nil { @@ -47,7 +50,7 @@ func TestServerSaltRecognized(t *testing.T) { // Test that ServerSaltGenerator doesn't recognize random salts func TestServerSaltUnrecognized(t *testing.T) { - ssg := NewServerSaltGenerator("test") + ssg := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test") salt := make([]byte, 32) if err := RandomSaltGenerator.GetSalt(salt); err != nil { @@ -60,7 +63,7 @@ func TestServerSaltUnrecognized(t *testing.T) { // Test that ServerSaltGenerator produces different output on each call func TestServerSaltDifferent(t *testing.T) { - ssg := NewServerSaltGenerator("test") + ssg := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test") salt1 := make([]byte, 32) if err := ssg.GetSalt(salt1); err != nil { @@ -79,8 +82,8 @@ func TestServerSaltDifferent(t *testing.T) { // Test that two ServerSaltGenerators derived from the same secret // produce different outputs and recognize each other's output. func TestServerSaltSameSecret(t *testing.T) { - ssg1 := NewServerSaltGenerator("test") - ssg2 := NewServerSaltGenerator("test") + ssg1 := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test") + ssg2 := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test") salt1 := make([]byte, 32) if err := ssg1.GetSalt(salt1); err != nil { @@ -103,8 +106,8 @@ func TestServerSaltSameSecret(t *testing.T) { // Test that two ServerSaltGenerators derived from different secrets // do not recognize each other's output. func TestServerSaltDifferentCiphers(t *testing.T) { - ssg1 := NewServerSaltGenerator("test1") - ssg2 := NewServerSaltGenerator("test2") + ssg1 := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test1") + ssg2 := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test2") salt1 := make([]byte, 32) if err := ssg1.GetSalt(salt1); err != nil { @@ -125,44 +128,52 @@ func TestServerSaltDifferentCiphers(t *testing.T) { } func TestServerSaltShort(t *testing.T) { - ssg := NewServerSaltGenerator("test") - + ssg20 := NewServerSaltGenerator(&fakeCipher{saltsize: 20}, "test") salt20 := make([]byte, 20) - if err := ssg.GetSalt(salt20); err != nil { + if err := ssg20.GetSalt(salt20); err != nil { t.Fatal(err) } - if !ssg.IsServerSalt(salt20) { + if !ssg20.IsServerSalt(salt20) { t.Error("Server salt was not recognized") } + ssg19 := NewServerSaltGenerator(&fakeCipher{saltsize: 19}, "test") salt19 := make([]byte, 19) - if err := ssg.GetSalt(salt19); err != nil { + if err := ssg19.GetSalt(salt19); err != nil { t.Fatal(err) } - if ssg.IsServerSalt(salt19) { + if ssg19.IsServerSalt(salt19) { t.Error("Short salt was marked") } +} - salt2 := make([]byte, 2) - if err := ssg.GetSalt(salt2); err != nil { - t.Fatal(err) +func TestServerSaltWrongSize(t *testing.T) { + ssg := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test") + + salt33 := make([]byte, 33) + if err := ssg.GetSalt(salt33); err == nil { + t.Error("Expected error for wrong size salt") } - if ssg.IsServerSalt(salt2) { - t.Error("Very short salt was marked") + + salt31 := make([]byte, 31) + if err := ssg.GetSalt(salt31); err == nil { + t.Error("Expected error for wrong size salt") } } func BenchmarkRandomSaltGenerator(b *testing.B) { - salt := make([]byte, 32) - for i := 0; i < b.N; i++ { - if err := RandomSaltGenerator.GetSalt(salt); err != nil { - b.Fatal(err) + b.RunParallel(func(pb *testing.PB) { + salt := make([]byte, 32) + for pb.Next() { + if err := RandomSaltGenerator.GetSalt(salt); err != nil { + b.Fatal(err) + } } - } + }) } func BenchmarkServerSaltGenerator(b *testing.B) { - ssg := NewServerSaltGenerator("test") + ssg := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test") b.ResetTimer() b.RunParallel(func(pb *testing.PB) { salt := make([]byte, 32) From 64c3a21841d66b975e629254da1bc31847c73488 Mon Sep 17 00:00:00 2001 From: Ben Schwartz Date: Tue, 25 Aug 2020 12:03:31 -0400 Subject: [PATCH 11/14] Add documentation on probing defenses --- README.md | 2 +- shadowsocks/PROBES.md | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) create mode 100644 shadowsocks/PROBES.md diff --git a/README.md b/README.md index f6b9a180..513a6e2e 100644 --- a/README.md +++ b/README.md @@ -10,7 +10,7 @@ The Outline Shadowsocks service allows for: - Whitebox monitoring of the service using [prometheus.io](https://prometheus.io) - Includes traffic measurements and other health indicators. - Live updates via config change + SIGHUP -- Experimental: optional replay defense (--replay_history). +- Replay defense (add `--replay_history 10000`). See [PROBES](shadowsocks/PROBES.md) for details. ![Graphana Dashboard](https://user-images.githubusercontent.com/113565/44177062-419d7700-a0ba-11e8-9621-db519692ff6c.png "Graphana Dashboard") diff --git a/shadowsocks/PROBES.md b/shadowsocks/PROBES.md new file mode 100644 index 00000000..c34bf614 --- /dev/null +++ b/shadowsocks/PROBES.md @@ -0,0 +1,35 @@ +# Outline Shadowsocks Probing and Replay Defenses + +## Attacks + +To ensure that proxied connections have not been modified in transit, the Outline implementation of Shadowsocks only supports modern [AEAD cipher suites](https://shadowsocks.org/en/spec/AEAD-Ciphers.html). This protects users from a wide range of potential attacks. However, even with [AEAD's authenticity guarantees](https://en.wikipedia.org/wiki/Authenticated_encryption), there are still ways for an attacker to abuse the Shadowsocks protocol. + +One category of attacks are "probing" attacks, in which the adversary sends test data to the proxy in order to confirm that it is actually a Shadowsocks proxy. This is a violation of the Shadowsocks security design, which is intended to ensure that only an authenticated user can identify the proxy. For example, one [probing attack against Shadowsocks](https://scholar.google.com/scholar?cluster=8542824533765048218) sends different numbers of random bytes to a target server, and identifies how many bytes the server reads before detecting an error and closing the connection. This number can be distinctive, identifying the server software. + +Another [reported](https://gfw.report/blog/gfw_shadowsocks/) category of attacks are "replay" attacks, in which an adversary records a conversation between a Shadowsocks client and server, then replays the contents of that connection. The contents are valid Shadowsocks AEAD data, so the proxy will forward the connection to the specified destination, as usual. In some cases, this can cause a duplicated action (e.g. uploading a file twice with HTTP POST). However, modern secure protocols such as HTTPS are not replayable, so this will normally have no ill effect. + +A greater concern for Outline is the use of replays in probing attacks to identify Shadowsocks proxies. By sending modified and unmodified replays, an attacker might be able to confirm that a server is in fact a Shadowsocks proxy, by observing distinctive behaviors. + +## Outline's defenses + +Outline contains several defenses against probing and replay attacks. + +### Invalid probe data + +If Outline detects that the initial data is invalid, it will continue to read data (exactly as if it were valid), but will not reply, and will not close the connection until a timeout. This leaves the attacker with minimal information about the server. + +### Client replays + +When client replay protection is enabled, every incoming valid handshake is reduced to a 32-bit checksum and stored in a hash table. When the table is full, it is archived and replaced with a fresh one, ensuring that the recent history is always in memory. Using 32-bit checksums results in a false-positive detection rate of 1 in 4 billion for each entry in the history. At the maximum history size (two sets of 20,000 checksums each), that results in a false-positive failure rate of 1 in 100,000 sockets ... still far lower than the error rate expected from network unreliability. + +This feature is on by default in Outline. Admins who are using outline-ss-server directly can enable this feature by adding "--replay_history 10000" to their outline-ss-server invocation. This costs approximately 20 bytes of memory per checksum. + +### Server replays + +Shadowsocks uses the same Key Derivation Function for both upstream and downstream flows, so in principle an attacker could record data sent from the server to the client, and use it in a "reflected replay" attack as simulated client->server data. The data would appear to be valid and authenticated to the server, but the connection would most likely fail when attempting to parse the destination address header, perhaps leading to a distinctive failure behavior. + +To avoid this class of attacks, outline-ss-server uses an [HMAC](https://en.wikipedia.org/wiki/HMAC) with a 32-bit tag to mark all server handshakes, and checks for the presence of this tag in all incoming handshakes. If the tag is present, the connection is a reflected replay, with a false positive probability of 1 in 4 billion. + +## Metrics + +Outline provides server operators with metrics on a variety of aspects of server activity, including any detected attacks. To observe attacks detected by your server, look at the `tcp_probes` histogram vector in Prometheus. The `status` field will be `"ERR_CIPHER"` (indicating invalid probe data), `"ERR_REPLAY_CLIENT"`, or `"ERR_REPLAY_SERVER"`, depending on the kind of attack your server observed. You can also see what country each probe appeared to originate from, and approximately how many bytes were sent before giving up. \ No newline at end of file From 78d6b15b58d2f7e8e6cf4b6e41bd3c04113a3d87 Mon Sep 17 00:00:00 2001 From: Ben Schwartz Date: Tue, 25 Aug 2020 12:13:15 -0400 Subject: [PATCH 12/14] Revert "Restructure salt size validity checks" This reverts commit 958c818b3316dcc116b7199d2eb3881c0b771812. --- shadowsocks/cipher_list.go | 4 +- shadowsocks/cipher_list_test.go | 28 ++++++++++++++ shadowsocks/cipher_testing.go | 28 -------------- shadowsocks/salt_generator.go | 46 +++++++---------------- shadowsocks/salt_generator_test.go | 59 ++++++++++++------------------ 5 files changed, 68 insertions(+), 97 deletions(-) diff --git a/shadowsocks/cipher_list.go b/shadowsocks/cipher_list.go index f6dbba91..7bf6ff64 100644 --- a/shadowsocks/cipher_list.go +++ b/shadowsocks/cipher_list.go @@ -32,7 +32,7 @@ const maxNonceSize = 12 type CipherEntry struct { ID string Cipher shadowaead.Cipher - SaltGenerator ServerSaltGenerator + SaltGenerator *ServerSaltGenerator lastClientIP net.IP } @@ -41,7 +41,7 @@ func MakeCipherEntry(id string, cipher shadowaead.Cipher, secret string) CipherE return CipherEntry{ ID: id, Cipher: cipher, - SaltGenerator: NewServerSaltGenerator(cipher, secret), + SaltGenerator: NewServerSaltGenerator(secret), } } diff --git a/shadowsocks/cipher_list_test.go b/shadowsocks/cipher_list_test.go index e6f1e9c2..fb120012 100644 --- a/shadowsocks/cipher_list_test.go +++ b/shadowsocks/cipher_list_test.go @@ -16,11 +16,39 @@ package shadowsocks import ( "container/list" + "crypto/cipher" "testing" "github.com/shadowsocks/go-shadowsocks2/shadowaead" ) +type fakeAEAD struct { + cipher.AEAD + overhead, nonceSize int +} + +func (a *fakeAEAD) NonceSize() int { + return a.nonceSize +} + +func (a *fakeAEAD) Overhead() int { + return a.overhead +} + +type fakeCipher struct { + shadowaead.Cipher + saltsize int + decrypter *fakeAEAD +} + +func (c *fakeCipher) SaltSize() int { + return c.saltsize +} + +func (c *fakeCipher) Decrypter(b []byte) (cipher.AEAD, error) { + return c.decrypter, nil +} + func TestIncompatibleCiphers(t *testing.T) { l := list.New() l.PushBack(&CipherEntry{ diff --git a/shadowsocks/cipher_testing.go b/shadowsocks/cipher_testing.go index 2778156e..93bbd612 100644 --- a/shadowsocks/cipher_testing.go +++ b/shadowsocks/cipher_testing.go @@ -16,7 +16,6 @@ package shadowsocks import ( "container/list" - "crypto/cipher" "fmt" "github.com/shadowsocks/go-shadowsocks2/core" @@ -60,30 +59,3 @@ func MakeTestPayload(size int) []byte { } return payload } - -type fakeAEAD struct { - cipher.AEAD - overhead, nonceSize int -} - -func (a *fakeAEAD) NonceSize() int { - return a.nonceSize -} - -func (a *fakeAEAD) Overhead() int { - return a.overhead -} - -type fakeCipher struct { - shadowaead.Cipher - saltsize int - decrypter *fakeAEAD -} - -func (c *fakeCipher) SaltSize() int { - return c.saltsize -} - -func (c *fakeCipher) Decrypter(b []byte) (cipher.AEAD, error) { - return c.decrypter, nil -} diff --git a/shadowsocks/salt_generator.go b/shadowsocks/salt_generator.go index e679d5bb..218f7692 100644 --- a/shadowsocks/salt_generator.go +++ b/shadowsocks/salt_generator.go @@ -19,10 +19,8 @@ import ( "crypto" "crypto/hmac" "crypto/rand" - "fmt" "io" - "github.com/shadowsocks/go-shadowsocks2/shadowaead" "golang.org/x/crypto/hkdf" ) @@ -32,13 +30,6 @@ type SaltGenerator interface { GetSalt(salt []byte) error } -type ServerSaltGenerator interface { - SaltGenerator - // IsServerSalt returns true if the salt was created by this generator - // and is marked as server-originated. - IsServerSalt(salt []byte) bool -} - // randomSaltGenerator generates a new random salt. type randomSaltGenerator struct{} @@ -48,17 +39,12 @@ func (randomSaltGenerator) GetSalt(salt []byte) error { return err } -func (randomSaltGenerator) IsServerSalt(salt []byte) bool { - return false -} - // RandomSaltGenerator is a basic SaltGenerator. -var RandomSaltGenerator ServerSaltGenerator = randomSaltGenerator{} +var RandomSaltGenerator SaltGenerator = randomSaltGenerator{} -// serverSaltGenerator generates unique salts that are secretly marked. -type serverSaltGenerator struct { - saltSize int - key []byte +// ServerSaltGenerator generates unique salts that are secretly marked. +type ServerSaltGenerator struct { + key []byte } // Number of bytes of salt to use as a marker. Increasing this value reduces @@ -78,28 +64,23 @@ var serverSaltLabel = []byte("outline-server-salt") // random, but is secretly marked as being issued by the server. // This is useful to prevent the server from accepting its own output in a // reflection attack. -func NewServerSaltGenerator(cipher shadowaead.Cipher, secret string) ServerSaltGenerator { - if cipher.SaltSize()-markLen < minEntropy { - // This cipher doesn't support server marking. - return RandomSaltGenerator - } - +func NewServerSaltGenerator(secret string) *ServerSaltGenerator { // Shadowsocks already uses HKDF-SHA1 to derive the AEAD key, so we use // the same derivation with a different "info" to generate our HMAC key. keySource := hkdf.New(crypto.SHA1.New, []byte(secret), nil, serverSaltLabel) // The key can be any size, but matching the block size is most efficient. key := make([]byte, crypto.SHA1.Size()) io.ReadFull(keySource, key) - return serverSaltGenerator{cipher.SaltSize(), key} + return &ServerSaltGenerator{key} } -func (sg serverSaltGenerator) splitSalt(salt []byte) (prefix, mark []byte) { +func (sg *ServerSaltGenerator) splitSalt(salt []byte) (prefix, mark []byte) { prefixLen := len(salt) - markLen return salt[:prefixLen], salt[prefixLen:] } // getTag takes in a salt prefix and returns the tag. -func (sg serverSaltGenerator) getTag(prefix []byte) []byte { +func (sg *ServerSaltGenerator) getTag(prefix []byte) []byte { // Use HMAC-SHA1, even though SHA1 is broken, because HMAC-SHA1 is still // secure, and we're already using HKDF-SHA1. hmac := hmac.New(crypto.SHA1.New, sg.key) @@ -109,9 +90,9 @@ func (sg serverSaltGenerator) getTag(prefix []byte) []byte { // GetSalt returns an apparently random salt that can be identified // as server-originated by anyone who knows the Shadowsocks key. -func (sg serverSaltGenerator) GetSalt(salt []byte) error { - if len(salt) != sg.saltSize { - return fmt.Errorf("Wrong salt size: %d != %d", len(salt), sg.saltSize) +func (sg *ServerSaltGenerator) GetSalt(salt []byte) error { + if len(salt)-markLen < minEntropy { + return RandomSaltGenerator.GetSalt(salt) } prefix, mark := sg.splitSalt(salt) if _, err := rand.Read(prefix); err != nil { @@ -122,8 +103,9 @@ func (sg serverSaltGenerator) GetSalt(salt []byte) error { return nil } -func (sg serverSaltGenerator) IsServerSalt(salt []byte) bool { - if len(salt) != sg.saltSize { +// IsServerSalt returns true if the salt is marked as server-originated. +func (sg *ServerSaltGenerator) IsServerSalt(salt []byte) bool { + if len(salt) < markLen { return false } prefix, mark := sg.splitSalt(salt) diff --git a/shadowsocks/salt_generator_test.go b/shadowsocks/salt_generator_test.go index f5aa882f..069fee31 100644 --- a/shadowsocks/salt_generator_test.go +++ b/shadowsocks/salt_generator_test.go @@ -30,14 +30,11 @@ func TestRandomSaltGenerator(t *testing.T) { if bytes.Equal(salt, make([]byte, 16)) { t.Error("Salt is all zeros") } - if RandomSaltGenerator.IsServerSalt(salt) { - t.Error("RandomSaltGenerator.IsServerSalt is always false") - } } // Test that ServerSaltGenerator recognizes its own salts func TestServerSaltRecognized(t *testing.T) { - ssg := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test") + ssg := NewServerSaltGenerator("test") salt := make([]byte, 32) if err := ssg.GetSalt(salt); err != nil { @@ -50,7 +47,7 @@ func TestServerSaltRecognized(t *testing.T) { // Test that ServerSaltGenerator doesn't recognize random salts func TestServerSaltUnrecognized(t *testing.T) { - ssg := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test") + ssg := NewServerSaltGenerator("test") salt := make([]byte, 32) if err := RandomSaltGenerator.GetSalt(salt); err != nil { @@ -63,7 +60,7 @@ func TestServerSaltUnrecognized(t *testing.T) { // Test that ServerSaltGenerator produces different output on each call func TestServerSaltDifferent(t *testing.T) { - ssg := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test") + ssg := NewServerSaltGenerator("test") salt1 := make([]byte, 32) if err := ssg.GetSalt(salt1); err != nil { @@ -82,8 +79,8 @@ func TestServerSaltDifferent(t *testing.T) { // Test that two ServerSaltGenerators derived from the same secret // produce different outputs and recognize each other's output. func TestServerSaltSameSecret(t *testing.T) { - ssg1 := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test") - ssg2 := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test") + ssg1 := NewServerSaltGenerator("test") + ssg2 := NewServerSaltGenerator("test") salt1 := make([]byte, 32) if err := ssg1.GetSalt(salt1); err != nil { @@ -106,8 +103,8 @@ func TestServerSaltSameSecret(t *testing.T) { // Test that two ServerSaltGenerators derived from different secrets // do not recognize each other's output. func TestServerSaltDifferentCiphers(t *testing.T) { - ssg1 := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test1") - ssg2 := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test2") + ssg1 := NewServerSaltGenerator("test1") + ssg2 := NewServerSaltGenerator("test2") salt1 := make([]byte, 32) if err := ssg1.GetSalt(salt1); err != nil { @@ -128,52 +125,44 @@ func TestServerSaltDifferentCiphers(t *testing.T) { } func TestServerSaltShort(t *testing.T) { - ssg20 := NewServerSaltGenerator(&fakeCipher{saltsize: 20}, "test") + ssg := NewServerSaltGenerator("test") + salt20 := make([]byte, 20) - if err := ssg20.GetSalt(salt20); err != nil { + if err := ssg.GetSalt(salt20); err != nil { t.Fatal(err) } - if !ssg20.IsServerSalt(salt20) { + if !ssg.IsServerSalt(salt20) { t.Error("Server salt was not recognized") } - ssg19 := NewServerSaltGenerator(&fakeCipher{saltsize: 19}, "test") salt19 := make([]byte, 19) - if err := ssg19.GetSalt(salt19); err != nil { + if err := ssg.GetSalt(salt19); err != nil { t.Fatal(err) } - if ssg19.IsServerSalt(salt19) { + if ssg.IsServerSalt(salt19) { t.Error("Short salt was marked") } -} - -func TestServerSaltWrongSize(t *testing.T) { - ssg := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test") - salt33 := make([]byte, 33) - if err := ssg.GetSalt(salt33); err == nil { - t.Error("Expected error for wrong size salt") + salt2 := make([]byte, 2) + if err := ssg.GetSalt(salt2); err != nil { + t.Fatal(err) } - - salt31 := make([]byte, 31) - if err := ssg.GetSalt(salt31); err == nil { - t.Error("Expected error for wrong size salt") + if ssg.IsServerSalt(salt2) { + t.Error("Very short salt was marked") } } func BenchmarkRandomSaltGenerator(b *testing.B) { - b.RunParallel(func(pb *testing.PB) { - salt := make([]byte, 32) - for pb.Next() { - if err := RandomSaltGenerator.GetSalt(salt); err != nil { - b.Fatal(err) - } + salt := make([]byte, 32) + for i := 0; i < b.N; i++ { + if err := RandomSaltGenerator.GetSalt(salt); err != nil { + b.Fatal(err) } - }) + } } func BenchmarkServerSaltGenerator(b *testing.B) { - ssg := NewServerSaltGenerator(&fakeCipher{saltsize: 32}, "test") + ssg := NewServerSaltGenerator("test") b.ResetTimer() b.RunParallel(func(pb *testing.PB) { salt := make([]byte, 32) From 62e89ea6e2178fa91774721bfb8e41a644cf61c6 Mon Sep 17 00:00:00 2001 From: Ben Schwartz Date: Tue, 25 Aug 2020 12:36:11 -0400 Subject: [PATCH 13/14] Move entropy check into MakeCipherEntry --- shadowsocks/cipher_list.go | 15 +++++-- shadowsocks/salt_generator.go | 65 +++++++++++++++++------------- shadowsocks/salt_generator_test.go | 38 ++++++++--------- 3 files changed, 70 insertions(+), 48 deletions(-) diff --git a/shadowsocks/cipher_list.go b/shadowsocks/cipher_list.go index 7bf6ff64..cb3c8a94 100644 --- a/shadowsocks/cipher_list.go +++ b/shadowsocks/cipher_list.go @@ -27,21 +27,30 @@ import ( // All ciphers must have a nonce size this big or smaller. const maxNonceSize = 12 +// Don't add a tag if it would reduce the salt entropy below this amount. +const minSaltEntropy = 16 + // CipherEntry holds a Cipher with an identifier. -// The public fields are constant, but lastAddress is mutable under cipherList.mu. +// The public fields are constant, but lastClientIP is mutable under cipherList.mu. type CipherEntry struct { ID string Cipher shadowaead.Cipher - SaltGenerator *ServerSaltGenerator + SaltGenerator ServerSaltGenerator lastClientIP net.IP } // MakeCipherEntry constructs a CipherEntry. func MakeCipherEntry(id string, cipher shadowaead.Cipher, secret string) CipherEntry { + var saltGenerator ServerSaltGenerator + if cipher.SaltSize()-ServerSaltMarkLen >= minSaltEntropy { + saltGenerator = NewServerSaltGenerator(secret) + } else { + saltGenerator = RandomSaltGenerator + } return CipherEntry{ ID: id, Cipher: cipher, - SaltGenerator: NewServerSaltGenerator(secret), + SaltGenerator: saltGenerator, } } diff --git a/shadowsocks/salt_generator.go b/shadowsocks/salt_generator.go index 218f7692..d6880c18 100644 --- a/shadowsocks/salt_generator.go +++ b/shadowsocks/salt_generator.go @@ -19,6 +19,7 @@ import ( "crypto" "crypto/hmac" "crypto/rand" + "fmt" "io" "golang.org/x/crypto/hkdf" @@ -30,6 +31,15 @@ type SaltGenerator interface { GetSalt(salt []byte) error } +// ServerSaltGenerator offers the ability to check if a salt was marked as +// server-originated. +type ServerSaltGenerator interface { + SaltGenerator + // IsServerSalt returns true if the salt was created by this generator + // and is marked as server-originated. + IsServerSalt(salt []byte) bool +} + // randomSaltGenerator generates a new random salt. type randomSaltGenerator struct{} @@ -39,23 +49,22 @@ func (randomSaltGenerator) GetSalt(salt []byte) error { return err } +func (randomSaltGenerator) IsServerSalt(salt []byte) bool { + return false +} + // RandomSaltGenerator is a basic SaltGenerator. -var RandomSaltGenerator SaltGenerator = randomSaltGenerator{} +var RandomSaltGenerator ServerSaltGenerator = randomSaltGenerator{} -// ServerSaltGenerator generates unique salts that are secretly marked. -type ServerSaltGenerator struct { +// serverSaltGenerator generates unique salts that are secretly marked. +type serverSaltGenerator struct { key []byte } -// Number of bytes of salt to use as a marker. Increasing this value reduces -// the false positive rate, but increases the likelihood of salt collisions. -// Must be less than or equal to the cipher overhead. -const markLen = 4 - -// For a random salt to be secure, it needs at least 16 bytes (128 bits) of -// entropy. If adding the mark would reduce the entropy below this level, -// we generate an unmarked random salt. -const minEntropy = 16 +// ServerSaltMarkLen is the number of bytes of salt to use as a marker. +// Increasing this value reduces the false positive rate, but increases +// the likelihood of salt collisions. +const ServerSaltMarkLen = 4 // Must be less than or equal to SHA1.Size() // Constant to identify this marking scheme. var serverSaltLabel = []byte("outline-server-salt") @@ -64,23 +73,26 @@ var serverSaltLabel = []byte("outline-server-salt") // random, but is secretly marked as being issued by the server. // This is useful to prevent the server from accepting its own output in a // reflection attack. -func NewServerSaltGenerator(secret string) *ServerSaltGenerator { +func NewServerSaltGenerator(secret string) ServerSaltGenerator { // Shadowsocks already uses HKDF-SHA1 to derive the AEAD key, so we use // the same derivation with a different "info" to generate our HMAC key. keySource := hkdf.New(crypto.SHA1.New, []byte(secret), nil, serverSaltLabel) // The key can be any size, but matching the block size is most efficient. key := make([]byte, crypto.SHA1.Size()) io.ReadFull(keySource, key) - return &ServerSaltGenerator{key} + return serverSaltGenerator{key} } -func (sg *ServerSaltGenerator) splitSalt(salt []byte) (prefix, mark []byte) { - prefixLen := len(salt) - markLen - return salt[:prefixLen], salt[prefixLen:] +func (sg serverSaltGenerator) splitSalt(salt []byte) (prefix, mark []byte, err error) { + prefixLen := len(salt) - ServerSaltMarkLen + if prefixLen < 0 { + return nil, nil, fmt.Errorf("Salt is too short: %d < %d", len(salt), ServerSaltMarkLen) + } + return salt[:prefixLen], salt[prefixLen:], nil } // getTag takes in a salt prefix and returns the tag. -func (sg *ServerSaltGenerator) getTag(prefix []byte) []byte { +func (sg serverSaltGenerator) getTag(prefix []byte) []byte { // Use HMAC-SHA1, even though SHA1 is broken, because HMAC-SHA1 is still // secure, and we're already using HKDF-SHA1. hmac := hmac.New(crypto.SHA1.New, sg.key) @@ -90,11 +102,11 @@ func (sg *ServerSaltGenerator) getTag(prefix []byte) []byte { // GetSalt returns an apparently random salt that can be identified // as server-originated by anyone who knows the Shadowsocks key. -func (sg *ServerSaltGenerator) GetSalt(salt []byte) error { - if len(salt)-markLen < minEntropy { - return RandomSaltGenerator.GetSalt(salt) +func (sg serverSaltGenerator) GetSalt(salt []byte) error { + prefix, mark, err := sg.splitSalt(salt) + if err != nil { + return err } - prefix, mark := sg.splitSalt(salt) if _, err := rand.Read(prefix); err != nil { return err } @@ -103,12 +115,11 @@ func (sg *ServerSaltGenerator) GetSalt(salt []byte) error { return nil } -// IsServerSalt returns true if the salt is marked as server-originated. -func (sg *ServerSaltGenerator) IsServerSalt(salt []byte) bool { - if len(salt) < markLen { +func (sg serverSaltGenerator) IsServerSalt(salt []byte) bool { + prefix, mark, err := sg.splitSalt(salt) + if err != nil { return false } - prefix, mark := sg.splitSalt(salt) tag := sg.getTag(prefix) - return bytes.Equal(tag[:markLen], mark) + return bytes.Equal(tag[:ServerSaltMarkLen], mark) } diff --git a/shadowsocks/salt_generator_test.go b/shadowsocks/salt_generator_test.go index 069fee31..b750b665 100644 --- a/shadowsocks/salt_generator_test.go +++ b/shadowsocks/salt_generator_test.go @@ -30,6 +30,9 @@ func TestRandomSaltGenerator(t *testing.T) { if bytes.Equal(salt, make([]byte, 16)) { t.Error("Salt is all zeros") } + if RandomSaltGenerator.IsServerSalt(salt) { + t.Error("RandomSaltGenerator.IsServerSalt is always false") + } } // Test that ServerSaltGenerator recognizes its own salts @@ -127,38 +130,37 @@ func TestServerSaltDifferentCiphers(t *testing.T) { func TestServerSaltShort(t *testing.T) { ssg := NewServerSaltGenerator("test") - salt20 := make([]byte, 20) - if err := ssg.GetSalt(salt20); err != nil { + salt5 := make([]byte, 5) + if err := ssg.GetSalt(salt5); err != nil { t.Fatal(err) } - if !ssg.IsServerSalt(salt20) { + if !ssg.IsServerSalt(salt5) { t.Error("Server salt was not recognized") } - salt19 := make([]byte, 19) - if err := ssg.GetSalt(salt19); err != nil { + salt4 := make([]byte, 4) + if err := ssg.GetSalt(salt4); err != nil { t.Fatal(err) } - if ssg.IsServerSalt(salt19) { - t.Error("Short salt was marked") + if !ssg.IsServerSalt(salt4) { + t.Error("Server salt was not recognized") } - salt2 := make([]byte, 2) - if err := ssg.GetSalt(salt2); err != nil { - t.Fatal(err) - } - if ssg.IsServerSalt(salt2) { - t.Error("Very short salt was marked") + salt3 := make([]byte, 3) + if err := ssg.GetSalt(salt3); err == nil { + t.Error("Expected error for too-short salt") } } func BenchmarkRandomSaltGenerator(b *testing.B) { - salt := make([]byte, 32) - for i := 0; i < b.N; i++ { - if err := RandomSaltGenerator.GetSalt(salt); err != nil { - b.Fatal(err) + b.RunParallel(func(pb *testing.PB) { + salt := make([]byte, 32) + for pb.Next() { + if err := RandomSaltGenerator.GetSalt(salt); err != nil { + b.Fatal(err) + } } - } + }) } func BenchmarkServerSaltGenerator(b *testing.B) { From 777815aa203498e69336da938d4a03928cb4afd7 Mon Sep 17 00:00:00 2001 From: Ben Schwartz Date: Thu, 27 Aug 2020 09:52:10 -0400 Subject: [PATCH 14/14] Add comments on entropy check --- shadowsocks/cipher_list.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/shadowsocks/cipher_list.go b/shadowsocks/cipher_list.go index cb3c8a94..dafddf5a 100644 --- a/shadowsocks/cipher_list.go +++ b/shadowsocks/cipher_list.go @@ -43,8 +43,11 @@ type CipherEntry struct { func MakeCipherEntry(id string, cipher shadowaead.Cipher, secret string) CipherEntry { var saltGenerator ServerSaltGenerator if cipher.SaltSize()-ServerSaltMarkLen >= minSaltEntropy { + // Mark salts with a tag for reverse replay protection. saltGenerator = NewServerSaltGenerator(secret) } else { + // Adding a tag would leave too little randomness to protect + // against accidental salt reuse, so don't mark the salts. saltGenerator = RandomSaltGenerator } return CipherEntry{