Add server entry validation

* Validated after decoded (embedded, remote, and discovery)
* Re-validated with hard error before stored
* Currently only IP address is validated, due to its potential
to cause handshake to fail

ConsoleClient/psiphonClient.go

@@ -99,7 +99,7 @@ func main() {
 			log.Fatalf("error loading embedded server entry list file: %s", err)
 		}
 		// TODO: stream embedded server list data? also, the cast makaes an unnecessary copy of a large buffer?
-		serverEntries, err := psiphon.DecodeServerEntryList(string(serverEntryList))
+		serverEntries, err := psiphon.DecodeAndValidateServerEntryList(string(serverEntryList))
 		if err != nil {
 			log.Fatalf("error decoding embedded server entry list file: %s", err)
 		}

psiphon/dataStore.go

@@ -165,7 +165,17 @@ func serverEntryExists(transaction *sql.Tx, ipAddress string) (bool, error) {
 // rank order, so the largest rank is top rank.
 // When replaceIfExists is true, an existing server entry record is
 // overwritten; otherwise, the existing record is unchanged.
+// If the server entry data is malformed, an alert notice is issued and
+// the entry is skipped; no error is returned.
 func StoreServerEntry(serverEntry *ServerEntry, replaceIfExists bool) error {
+
+	// Server entries should already be validated before this point,
+	// so instead of skipping we fail with an error.
+	err := ValidateServerEntry(serverEntry)
+	if err != nil {
+		return ContextError(errors.New("invalid server entry"))
+	}
+
 	return transactionWithRetry(func(transaction *sql.Tx) error {
 		serverEntryExists, err := serverEntryExists(transaction, serverEntry.IpAddress)
 		if err != nil {

psiphon/remoteServerList.go

@@ -84,7 +84,7 @@ func FetchRemoteServerList(config *Config, pendingConns *Conns) (err error) {
 		return ContextError(err)
 	}
 
-	serverEntries, err := DecodeServerEntryList(remoteServerList.Data)
+	serverEntries, err := DecodeAndValidateServerEntryList(remoteServerList.Data)
 	if err != nil {
 		return ContextError(err)
 	}

psiphon/serverApi.go

@@ -225,6 +225,11 @@ func (session *Session) doHandshakeRequest() error {
 		if err != nil {
 			return ContextError(err)
 		}
+		err = ValidateServerEntry(serverEntry)
+		if err != nil {
+			// Skip this entry and continue with the next one
+			continue
+		}
 		err = StoreServerEntry(serverEntry, true)
 		if err != nil {
 			return ContextError(err)

psiphon/serverEntry.go

@@ -24,6 +24,8 @@ import (
 	"encoding/hex"
 	"encoding/json"
 	"errors"
+	"fmt"
+	"net"
 	"strings"
 )
 
@@ -71,15 +73,38 @@ func DecodeServerEntry(encodedServerEntry string) (serverEntry *ServerEntry, err
 	return serverEntry, nil
 }
 
-// DecodeServerEntryList extracts server entries from the list encoding
+// ValidateServerEntry checks for malformed server entries.
+// Currently, it checks for a valid ipAddress. This is important since
+// handshake requests submit back to the server a list of known server
+// IP addresses and the handshake API expects well-formed inputs.
+// TODO: validate more fields
+func ValidateServerEntry(serverEntry *ServerEntry) error {
+	ipAddr := net.ParseIP(serverEntry.IpAddress)
+	if ipAddr == nil {
+		errMsg := fmt.Sprintf("server entry has invalid IpAddress: '%s'", serverEntry.IpAddress)
+		// Some callers skip invalid server entries without propagating
+		// the error mesage, so issue a notice.
+		Notice(NOTICE_ALERT, errMsg)
+		return ContextError(errors.New(errMsg))
+	}
+	return nil
+}
+
+// DecodeAndValidateServerEntryList extracts server entries from the list encoding
 // used by remote server lists and Psiphon server handshake requests.
-func DecodeServerEntryList(encodedServerEntryList string) (serverEntries []*ServerEntry, err error) {
+// Each server entry is validated and invalid entries are skipped.
+func DecodeAndValidateServerEntryList(encodedServerEntryList string) (serverEntries []*ServerEntry, err error) {
 	serverEntries = make([]*ServerEntry, 0)
 	for _, encodedServerEntry := range strings.Split(encodedServerEntryList, "\n") {
+		// TODO: skip this entry and continue if can't decode?
 		serverEntry, err := DecodeServerEntry(encodedServerEntry)
 		if err != nil {
 			return nil, ContextError(err)
 		}
+		if ValidateServerEntry(serverEntry) != nil {
+			// Skip this entry and continue with the next one
+			continue
+		}
 		serverEntries = append(serverEntries, serverEntry)
 	}
 	return serverEntries, nil

psiphon/serverEntry_test.go

@@ -0,0 +1,74 @@
+/*
+ * Copyright (c) 2015, Psiphon Inc.
+ * All rights reserved.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+package psiphon
+
+import (
+	"encoding/hex"
+	"testing"
+)
+
+const (
+	_VALID_NORMAL_SERVER_ENTRY                    = `192.168.0.1 80 <webServerSecret> <webServerCertificate> {"ipAddress":"192.168.0.1","webServerPort":"80","webServerSecret":"<webServerSecret>","webServerCertificate":"<webServerCertificate>","sshPort":22,"sshUsername":"<sshUsername>","sshPassword":"<sshPassword>","sshHostKey":"<sshHostKey>","sshObfuscatedPort":443,"sshObfuscatedKey":"<sshObfuscatedKey>","capabilities":["handshake","SSH","OSSH","VPN"],"region":"CA","meekServerPort":8080,"meekCookieEncryptionPublicKey":"<meekCookieEncryptionPublicKey>","meekObfuscatedKey":"<meekObfuscatedKey>","meekFrontingDomain":"<meekFrontingDomain>","meekFrontingHost":"<meekFrontingHost>"}`
+	_VALID_BLANK_LEGACY_SERVER_ENTRY              = `    {"ipAddress":"192.168.0.1","webServerPort":"80","webServerSecret":"<webServerSecret>","webServerCertificate":"<webServerCertificate>","sshPort":22,"sshUsername":"<sshUsername>","sshPassword":"<sshPassword>","sshHostKey":"<sshHostKey>","sshObfuscatedPort":443,"sshObfuscatedKey":"<sshObfuscatedKey>","capabilities":["handshake","SSH","OSSH","VPN"],"region":"CA","meekServerPort":8080,"meekCookieEncryptionPublicKey":"<meekCookieEncryptionPublicKey>","meekObfuscatedKey":"<meekObfuscatedKey>","meekFrontingDomain":"<meekFrontingDomain>","meekFrontingHost":"<meekFrontingHost>"}`
+	_INVALID_WINDOWS_REGISTRY_LEGACY_SERVER_ENTRY = `192.168.0.1 80 <webServerSecret> <webServerCertificate> {"sshPort":22,"sshUsername":"<sshUsername>","sshPassword":"<sshPassword>","sshHostKey":"<sshHostKey>","sshObfuscatedPort":443,"sshObfuscatedKey":"<sshObfuscatedKey>","capabilities":["handshake","SSH","OSSH","VPN"],"region":"CA","meekServerPort":8080,"meekCookieEncryptionPublicKey":"<meekCookieEncryptionPublicKey>","meekObfuscatedKey":"<meekObfuscatedKey>","meekFrontingDomain":"<meekFrontingDomain>","meekFrontingHost":"<meekFrontingHost>"}`
+	_INVALID_MALFORMED_IP_ADDRESS_SERVER_ENTRY    = `192.168.0.1 80 <webServerSecret> <webServerCertificate> {"ipAddress":"192.168.0.","webServerPort":"80","webServerSecret":"<webServerSecret>","webServerCertificate":"<webServerCertificate>","sshPort":22,"sshUsername":"<sshUsername>","sshPassword":"<sshPassword>","sshHostKey":"<sshHostKey>","sshObfuscatedPort":443,"sshObfuscatedKey":"<sshObfuscatedKey>","capabilities":["handshake","SSH","OSSH","VPN"],"region":"CA","meekServerPort":8080,"meekCookieEncryptionPublicKey":"<meekCookieEncryptionPublicKey>","meekObfuscatedKey":"<meekObfuscatedKey>","meekFrontingDomain":"<meekFrontingDomain>","meekFrontingHost":"<meekFrontingHost>"}`
+	_EXPECTED_IP_ADDRESS                          = `192.168.0.1`
+)
+
+// DecodeAndValidateServerEntryList should return 2 valid decoded entries from the input list of 4
+func TestDecodeAndValidateServerEntryList(t *testing.T) {
+
+	testEncodedServerEntryList := hex.EncodeToString([]byte(_VALID_NORMAL_SERVER_ENTRY)) + "\n" +
+		hex.EncodeToString([]byte(_VALID_BLANK_LEGACY_SERVER_ENTRY)) + "\n" +
+		hex.EncodeToString([]byte(_INVALID_WINDOWS_REGISTRY_LEGACY_SERVER_ENTRY)) + "\n" +
+		hex.EncodeToString([]byte(_INVALID_MALFORMED_IP_ADDRESS_SERVER_ENTRY))
+
+	serverEntries, err := DecodeAndValidateServerEntryList(testEncodedServerEntryList)
+	if err != nil {
+		t.Error(err.Error())
+		t.FailNow()
+	}
+	if len(serverEntries) != 2 {
+		t.Error("unexpected number of valid server entries")
+	}
+	for _, serverEntry := range serverEntries {
+		if serverEntry.IpAddress != _EXPECTED_IP_ADDRESS {
+			t.Error("unexpected IP address in decoded server entry: %s", serverEntry.IpAddress)
+		}
+	}
+}
+
+// Directly call DecodeServerEntry and ValidateServerEntry with invalid inputs
+func TestInvalidServerEntries(t *testing.T) {
+
+	testCases := [2]string{_INVALID_WINDOWS_REGISTRY_LEGACY_SERVER_ENTRY, _INVALID_MALFORMED_IP_ADDRESS_SERVER_ENTRY}
+
+	for _, testCase := range testCases {
+		encodedServerEntry := hex.EncodeToString([]byte(testCase))
+		serverEntry, err := DecodeServerEntry(encodedServerEntry)
+		if err != nil {
+			t.Error(err.Error())
+		}
+		err = ValidateServerEntry(serverEntry)
+		if err == nil {
+			t.Error("server entry should not validate: %s", testCase)
+		}
+	}
+}