Restructure salt size validity checks

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),
 	}
 }
 

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{

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
+}

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)

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)