From 958c818b3316dcc116b7199d2eb3881c0b771812 Mon Sep 17 00:00:00 2001 From: Ben Schwartz Date: Mon, 24 Aug 2020 11:21:30 -0400 Subject: [PATCH] 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)