Merge pull request #417 from rod-hynes/master

Fix meek race condition; add SessionID config param

MobileLibrary/iOS/PsiphonTunnel/PsiphonTunnel/PsiphonTunnel.h

@@ -176,8 +176,9 @@ typedef NS_ENUM(NSInteger, PsiphonConnectionState)
 - (void)onConnectionStateChangedFrom:(PsiphonConnectionState)oldState to:(PsiphonConnectionState)newState;
 
 /*!
- Called to indicate that tunnel-core is exiting imminently (usually do to
+ Called to indicate that tunnel-core is exiting imminently (usually due to
  a `stop()` call, but could be due to an unexpected error).
+ onExiting may be called before or after `stop()` returns.
  Swift: @code func onExiting() @endcode
  */
 - (void)onExiting;
@@ -301,7 +302,7 @@ Swift: @code func onInternetReachabilityChanged(_ currentReachability: Reachabil
 - (BOOL)start:(BOOL)ifNeeded;
 
 /*!
- Stop the tunnel (regardless of its current connection state). Returns before full stop is complete -- `TunneledAppDelegate::onExiting` is called when complete.
+ Stop the tunnel (regardless of its current connection state).
  Swift: @code func stop() @endcode
  */
 - (void)stop;

MobileLibrary/iOS/PsiphonTunnel/PsiphonTunnel/PsiphonTunnel.m

@@ -39,6 +39,8 @@
 
 @property (weak) id <TunneledAppDelegate> tunneledAppDelegate;
 
+@property (atomic, strong) NSString *sessionID;
+
 @end
 
 @implementation PsiphonTunnel {
@@ -59,8 +61,6 @@
     // DNS
     NSString *primaryGoogleDNS;
     NSString *secondaryGoogleDNS;
-
-    
     _Atomic BOOL useInitialDNS; // initialDNSCache validity flag.
     NSArray<NSString *> *initialDNSCache;  // This cache becomes void if internetReachabilityChanged is called.
 }
@@ -118,6 +118,15 @@
 
 // See comment in header
 - (BOOL)start:(BOOL)ifNeeded {
+
+    // Set a new session ID, as this is a user-initiated session start.
+    NSString *sessionID = [self generateSessionID];
+    if (sessionID == nil) {
+        // generateSessionID logs error message
+        return FALSE;
+    }
+    self.sessionID = sessionID;
+
     if (ifNeeded) {
         return [self startIfNeeded];
     }
@@ -127,10 +136,14 @@
 
 /*!
  Start the tunnel. If the tunnel is already started it will be stopped first.
+ Assumes self.sessionID has been initialized -- i.e., assumes that
+ start:(BOOL)ifNeeded has been called at least once.
  */
 - (BOOL)start {
     @synchronized (PsiphonTunnel.self) {
+
         [self stop];
+
         [self logMessage:@"Starting Psiphon library"];
 
         // Must always use IPv6Synthesizer for iOS
@@ -525,6 +538,8 @@
     config[@"UpgradeDownloadClientVersionHeader"] = nil;
     config[@"UpgradeDownloadFilename"] = nil;
 
+    config[@"SessionID"] = self.sessionID;
+
     NSString *finalConfigStr = [[[SBJson4Writer alloc] init] stringWithObject:config];
     
     if (finalConfigStr == nil) {
@@ -919,7 +934,7 @@
     _state = malloc(sizeof(struct __res_state));
 
     if (res_ninit(_state) < 0) {
-        NSLog(@"res_ninit failed.");
+        [self logMessage:@"getDNSServers: res_ninit failed."];
         free(_state);
         return nil;
     }
@@ -943,7 +958,7 @@
             if (EXIT_SUCCESS == ret_code) {
                 [serverList addObject:[NSString stringWithUTF8String:hostBuf]];
             } else {
-                NSLog(@"getnameinfo failed. Retcode: %d", ret_code);
+                [self logMessage:[NSString stringWithFormat: @"getDNSServers: getnameinfo failed: %d", ret_code]];
             }
         }
     }
@@ -1119,4 +1134,22 @@
     return @"US";
 }
 
+/*!
+ generateSessionID generates a session ID suitable for use with the Psiphon API.
+ */
+- (NSString *)generateSessionID {
+    const int sessionIDLen = 16;
+    uint8_t sessionID[sessionIDLen];
+    int result = SecRandomCopyBytes(kSecRandomDefault, sessionIDLen, sessionID);
+    if (result != errSecSuccess) {
+        [self logMessage:[NSString stringWithFormat: @"Error generating session ID: %d", result]];
+        return nil;
+    }
+    NSMutableString *hexEncodedSessionID = [NSMutableString stringWithCapacity:(sessionIDLen*2)];
+    for (int i = 0; i < sessionIDLen; i++) {
+        [hexEncodedSessionID appendFormat:@"%02x", sessionID[i]];
+    }
+    return hexEncodedSessionID;
+}
+
 @end

psiphon/config.go

@@ -27,7 +27,9 @@ import (
 	"net/http"
 	"os"
 	"strconv"
+	"strings"
 	"time"
+	"unicode"
 
 	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/common"
 	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/common/protocol"
@@ -491,6 +493,15 @@ type Config struct {
 
 	// IgnoreHandshakeStatsRegexps skips compiling and using stats regexes.
 	IgnoreHandshakeStatsRegexps bool
+
+	// SessionID specifies a client session ID to use in the Psiphon API. The session
+	// ID should be a randomly generated value that is used only for a single session,
+	// which is defined as the period between a user starting a Psiphon client and
+	// stopping the client.
+	// A session ID must be 32 hex digits (lower case). When blank, a random session
+	// ID is automatically generated. Supply a session ID when a single client session
+	// will cross multiple Controller instances.
+	SessionID string
 }
 
 // DownloadURL specifies a URL for downloading resources along with parameters
@@ -738,6 +749,22 @@ func LoadConfig(configJson []byte) (*Config, error) {
 		config.EstablishTunnelPausePeriodSeconds = &defaultEstablishTunnelPausePeriodSeconds
 	}
 
+	if config.SessionID == "" {
+		sessionID, err := MakeSessionId()
+		if err != nil {
+			return nil, common.ContextError(err)
+		}
+		config.SessionID = sessionID
+	}
+
+	// SessionID must be PSIPHON_API_CLIENT_SESSION_ID_LENGTH lowercase hex-encoded bytes
+	if len(config.SessionID) != 2*protocol.PSIPHON_API_CLIENT_SESSION_ID_LENGTH ||
+		-1 != strings.IndexFunc(config.SessionID, func(c rune) bool {
+			return !unicode.Is(unicode.ASCII_Hex_Digit, c) || unicode.IsUpper(c)
+		}) {
+		return nil, common.ContextError(errors.New("invalid SessionID"))
+	}
+
 	return &config, nil
 }
 

psiphon/controller.go

@@ -89,13 +89,9 @@ func NewController(config *Config) (controller *Controller, err error) {
 	// Needed by regen, at least
 	rand.Seed(int64(time.Now().Nanosecond()))
 
-	// Generate a session ID for the Psiphon server API. This session ID is
-	// used across all tunnels established by the controller.
-	sessionId, err := MakeSessionId()
-	if err != nil {
-		return nil, common.ContextError(err)
-	}
-	NoticeSessionId(sessionId)
+	// The session ID for the Psiphon server API is used across all
+	// tunnels established by the controller.
+	NoticeSessionId(config.SessionID)
 
 	// untunneledPendingConns may be used to interrupt the fetch remote server list
 	// request and other untunneled connection establishments. BindToDevice may be
@@ -117,7 +113,7 @@ func NewController(config *Config) (controller *Controller, err error) {
 
 	controller = &Controller{
 		config:    config,
-		sessionId: sessionId,
+		sessionId: config.SessionID,
 		// componentFailureSignal receives a signal from a component (including socks and
 		// http local proxies) if they unexpectedly fail. Senders should not block.
 		// Buffer allows at least one stop signal to be sent before there is a receiver.

psiphon/meekConn.go

@@ -600,6 +600,40 @@ func (meek *MeekConn) relay() {
 	}
 }
 
+// readCloseSignaller is an io.ReadCloser wrapper for an io.Reader
+// that is passed, as the request body, to http.Transport.RoundTrip.
+// readCloseSignaller adds the AwaitClosed call, which is used
+// to schedule recycling the buffer underlying the reader only after
+// RoundTrip has called Close and will no longer use the buffer.
+// See: https://golang.org/pkg/net/http/#RoundTripper
+type readCloseSignaller struct {
+	reader io.Reader
+	closed chan struct{}
+}
+
+func NewReadCloseSignaller(reader io.Reader) *readCloseSignaller {
+	return &readCloseSignaller{
+		reader: reader,
+		closed: make(chan struct{}, 1),
+	}
+}
+
+func (r *readCloseSignaller) Read(p []byte) (int, error) {
+	return r.reader.Read(p)
+}
+
+func (r *readCloseSignaller) Close() error {
+	select {
+	case r.closed <- *new(struct{}):
+	default:
+	}
+	return nil
+}
+
+func (r *readCloseSignaller) AwaitClosed() {
+	<-r.closed
+}
+
 // roundTrip configures and makes the actual HTTP POST request
 func (meek *MeekConn) roundTrip(sendBuffer *bytes.Buffer) (int64, error) {
 
@@ -654,13 +688,15 @@ func (meek *MeekConn) roundTrip(sendBuffer *bytes.Buffer) (int64, error) {
 		// Omit the request payload when retrying after receiving a
 		// partial server response.
 
-		var payloadReader io.Reader
+		var signaller *readCloseSignaller
+		var requestBody io.ReadCloser
 		if !serverAcknowledgedRequestPayload && sendBuffer != nil {
-			payloadReader = bytes.NewReader(sendBuffer.Bytes())
+			signaller = NewReadCloseSignaller(bytes.NewReader(sendBuffer.Bytes()))
+			requestBody = signaller
 		}
 
 		var request *http.Request
-		request, err := http.NewRequest("POST", meek.url.String(), payloadReader)
+		request, err := http.NewRequest("POST", meek.url.String(), requestBody)
 		if err != nil {
 			// Don't retry when can't initialize a Request
 			return 0, common.ContextError(err)
@@ -684,6 +720,11 @@ func (meek *MeekConn) roundTrip(sendBuffer *bytes.Buffer) (int64, error) {
 			request.Header.Set("Range", fmt.Sprintf("bytes=%d-", receivedPayloadSize))
 		}
 
+		// sendBuffer will be replaced once the data is no longer needed,
+		// when RoundTrip calls Close on the Body; this allows meekConn.Write()
+		// to unblock and start buffering data for the next roung trip while
+		// still reading the current round trip response.
+
 		response, err := meek.transport.RoundTrip(request)
 		if err != nil {
 			select {
@@ -719,11 +760,12 @@ func (meek *MeekConn) roundTrip(sendBuffer *bytes.Buffer) (int64, error) {
 			// must have received the request payload.
 			serverAcknowledgedRequestPayload = true
 
-			// sendBuffer can now be replaced, as the data is no longer
-			// needed; this allows meekConn.Write() to unblock and start
-			// buffering data for the next roung trip while still reading
-			// the current round trip response.
-			if sendBuffer != nil {
+			// sendBuffer is now no longer required for retries, ane the
+			// buffer may be replaced; this allows meekConn.Write() to unblock
+			// and start buffering data for the next roung trip while still
+			// reading the current round trip response.
+			if signaller != nil {
+				signaller.AwaitClosed()
 				sendBuffer.Truncate(0)
 				meek.replaceSendBuffer(sendBuffer)
 				sendBuffer = nil

psiphon/server/api.go

@@ -858,6 +858,7 @@ func isServerSecret(support *SupportServices, value string) bool {
 }
 
 func isHexDigits(_ *SupportServices, value string) bool {
+	// Allows both uppercase in addition to lowercase, for legacy support.
 	return -1 == strings.IndexFunc(value, func(c rune) bool {
 		return !unicode.Is(unicode.ASCII_Hex_Digit, c)
 	})