Skip to content

Commit de8b3c7

Browse files
pimterrypimterry
committed
Separate UDP & TCP session keys to fix VPN bug
Previously, sessions were tracked in one big session table, keyed by source/destionation ip/port pairs. This sounds great, until you release that actually UDP & TCP ports are independent, and so it's totally possible to create a new TCP connection on the same ports that are being used for an existing UDP connection. Uh oh. In practice, this meant that SYN packets for TCP connections on existing UDP ports would attempt to re-ack, rejecting the SYN as a duplicate for an existing TCP connection. This would then fail, because the session didn't actually have existing TCP data required to do this. Oops. We now track them separately, which should avoid that situation entirely.
1 parent 6919196 commit de8b3c7

File tree

4 files changed

+48
-26
lines changed

4 files changed

+48
-26
lines changed

‎app/src/main/java/tech/httptoolkit/android/vpn/Session.java‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@ public class Session {
4040
private final String TAG = "Session";
4141

4242
private AbstractSelectableChannel channel;
43+
44+
private final SessionProtocol protocol;
4345

4446
private final int destIp;
4547
private final int destPort;
@@ -107,6 +109,7 @@ public class Session {
107109
private final ICloseSession sessionCloser;
108110

109111
Session(
112+
SessionProtocol protocol,
110113
int sourceIp,
111114
int sourcePort,
112115
int destinationIp,
@@ -116,6 +119,7 @@ public class Session {
116119
receivingStream = new ByteArrayOutputStream();
117120
sendingStream = new ByteArrayOutputStream();
118121

122+
this.protocol = protocol;
119123
this.sourceIp = sourceIp;
120124
this.sourcePort = sourcePort;
121125
this.destIp = destinationIp;
@@ -193,6 +197,10 @@ public boolean hasDataToSend(){
193197
return sendingStream.size() > 0;
194198
}
195199

200+
public SessionProtocol getProtocol() {
201+
return this.protocol;
202+
}
203+
196204
public int getDestIp() {
197205
return destIp;
198206
}
@@ -363,12 +371,14 @@ public void closeSession() {
363371
}
364372

365373
public String getSessionKey() {
366-
return Session.getSessionKey(this.destIp, this.destPort, this.sourceIp, this.sourcePort);
374+
return Session.getSessionKey(this.protocol, this.destIp, this.destPort, this.sourceIp, this.sourcePort);
367375
}
368376

369-
public static String getSessionKey(int destIp, int destPort, int sourceIp, int sourcePort) {
370-
return PacketUtil.intToIPAddress(sourceIp) + ":" + sourcePort + "->" +
371-
PacketUtil.intToIPAddress(destIp) + ":" + destPort;
377+
public static String getSessionKey(SessionProtocol protocol, int destIp, int destPort, int sourceIp, int sourcePort) {
378+
return protocol.name() + "|" +
379+
PacketUtil.intToIPAddress(sourceIp) + ":" + sourcePort +
380+
"->" +
381+
PacketUtil.intToIPAddress(destIp) + ":" + destPort;
372382
}
373383

374384
public String toString() {

‎app/src/main/java/tech/httptoolkit/android/vpn/SessionHandler.java‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,11 @@ private void handleUDPPacket(ByteBuffer clientPacketData, IPv4Header ipHeader) t
9898
UDPHeader udpheader = UDPPacketFactory.createUDPHeader(clientPacketData);
9999

100100
Session session = manager.getSession(
101-
ipHeader.getDestinationIP(), udpheader.getDestinationPort(),
102-
ipHeader.getSourceIP(), udpheader.getSourcePort()
101+
SessionProtocol.UDP,
102+
ipHeader.getDestinationIP(),
103+
udpheader.getDestinationPort(),
104+
ipHeader.getSourceIP(),
105+
udpheader.getSourcePort()
103106
);
104107

105108
boolean newSession = session == null;
@@ -140,7 +143,7 @@ private void handleTCPPacket(ByteBuffer clientPacketData, IPv4Header ipHeader) t
140143
// 3-way handshake + create new session
141144
replySynAck(ipHeader,tcpheader);
142145
} else if(tcpheader.isACK()) {
143-
String key = Session.getSessionKey(destinationIP, destinationPort, sourceIP, sourcePort);
146+
String key = Session.getSessionKey(SessionProtocol.TCP, destinationIP, destinationPort, sourceIP, sourcePort);
144147
Session session = manager.getSessionByKey(key);
145148

146149
if (session == null) {
@@ -176,7 +179,7 @@ private void handleTCPPacket(ByteBuffer clientPacketData, IPv4Header ipHeader) t
176179
sendFinAck(ipHeader, tcpheader, session);
177180
} else if (session.isAckedToFin() && !tcpheader.isFIN()) {
178181
//the last ACK from client after FIN-ACK flag was sent
179-
manager.closeSession(destinationIP, destinationPort, sourceIP, sourcePort);
182+
manager.closeSession(SessionProtocol.TCP, destinationIP, destinationPort, sourceIP, sourcePort);
180183
Log.d(TAG, "got last ACK after FIN, session is now closed.");
181184
}
182185
}
@@ -190,7 +193,7 @@ private void handleTCPPacket(ByteBuffer clientPacketData, IPv4Header ipHeader) t
190193
Log.d(TAG, "FIN from vpn client, will ack it.");
191194
ackFinAck(ipHeader, tcpheader, session);
192195
} else if (tcpheader.isRST()) {
193-
resetConnection(ipHeader, tcpheader);
196+
resetTCPConnection(ipHeader, tcpheader);
194197
}
195198

196199
if (!session.isAbortingConnection()) {
@@ -199,14 +202,14 @@ private void handleTCPPacket(ByteBuffer clientPacketData, IPv4Header ipHeader) t
199202
}
200203
} else if(tcpheader.isFIN()){
201204
//case client sent FIN without ACK
202-
Session session = manager.getSession(destinationIP, destinationPort, sourceIP, sourcePort);
205+
Session session = manager.getSession(SessionProtocol.TCP, destinationIP, destinationPort, sourceIP, sourcePort);
203206
if(session == null)
204207
ackFinAck(ipHeader, tcpheader, null);
205208
else
206209
manager.keepSessionAlive(session);
207210

208211
} else if(tcpheader.isRST()){
209-
resetConnection(ipHeader, tcpheader);
212+
resetTCPConnection(ipHeader, tcpheader);
210213
} else {
211214
Log.d(TAG,"unknown TCP flag");
212215
String str1 = PacketUtil.getOutput(ipHeader, tcpheader, clientPacketData.array());
@@ -364,8 +367,9 @@ private void acceptAck(TCPHeader tcpHeader, Session session){
364367
* @param ip IP
365368
* @param tcp TCP
366369
*/
367-
private void resetConnection(IPv4Header ip, TCPHeader tcp){
370+
private void resetTCPConnection(IPv4Header ip, TCPHeader tcp){
368371
Session session = manager.getSession(
372+
SessionProtocol.TCP,
369373
ip.getDestinationIP(), tcp.getDestinationPort(),
370374
ip.getSourceIP(), tcp.getSourcePort()
371375
);

‎app/src/main/java/tech/httptoolkit/android/vpn/SessionManager.java‎

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -26,16 +26,13 @@
2626
import tech.httptoolkit.android.TagKt;
2727
import tech.httptoolkit.android.vpn.socket.DataConst;
2828
import tech.httptoolkit.android.vpn.socket.ICloseSession;
29-
import tech.httptoolkit.android.vpn.socket.SocketNIODataService;
3029
import tech.httptoolkit.android.vpn.socket.SocketProtector;
3130
import tech.httptoolkit.android.vpn.util.PacketUtil;
3231

3332
import java.io.IOException;
3433
import java.net.InetSocketAddress;
3534
import java.net.SocketAddress;
36-
import java.net.SocketException;
3735
import java.nio.ByteBuffer;
38-
import java.nio.channels.ClosedChannelException;
3936
import java.nio.channels.DatagramChannel;
4037
import java.nio.channels.SocketChannel;
4138
import java.nio.channels.spi.AbstractSelectableChannel;
@@ -59,8 +56,13 @@ public class SessionManager implements ICloseSession {
5956
*/
6057
public void keepSessionAlive(Session session) {
6158
if(session != null){
62-
String key = Session.getSessionKey(session.getDestIp(), session.getDestPort(),
63-
session.getSourceIp(), session.getSourcePort());
59+
String key = Session.getSessionKey(
60+
session.getProtocol(),
61+
session.getDestIp(),
62+
session.getDestPort(),
63+
session.getSourceIp(),
64+
session.getSourcePort()
65+
);
6466
table.put(key, session);
6567
}
6668
}
@@ -77,8 +79,8 @@ public int addClientData(ByteBuffer buffer, Session session) {
7779
return session.setSendingData(buffer);
7880
}
7981

80-
public Session getSession(int ip, int port, int srcIp, int srcPort) {
81-
String key = Session.getSessionKey(ip, port, srcIp, srcPort);
82+
public Session getSession(SessionProtocol protocol, int ip, int port, int srcIp, int srcPort) {
83+
String key = Session.getSessionKey(protocol, ip, port, srcIp, srcPort);
8284

8385
return getSessionByKey(key);
8486
}
@@ -99,8 +101,8 @@ public Session getSessionByKey(String key) {
99101
* @param srcIp Source IP Address
100102
* @param srcPort Source Port
101103
*/
102-
public void closeSession(int ip, int port, int srcIp, int srcPort){
103-
String key = Session.getSessionKey(ip, port, srcIp, srcPort);
104+
public void closeSession(SessionProtocol protocol, int ip, int port, int srcIp, int srcPort){
105+
String key = Session.getSessionKey(protocol, ip, port, srcIp, srcPort);
104106
Session session = table.remove(key);
105107

106108
if(session != null){
@@ -117,21 +119,21 @@ public void closeSession(int ip, int port, int srcIp, int srcPort){
117119
}
118120

119121
public void closeSession(@NonNull Session session){
120-
closeSession(session.getDestIp(),
122+
closeSession(session.getProtocol(), session.getDestIp(),
121123
session.getDestPort(), session.getSourceIp(),
122124
session.getSourcePort());
123125
}
124126

125127
@NotNull
126128
public Session createNewUDPSession(int ip, int port, int srcIp, int srcPort) throws IOException {
127-
String keys = Session.getSessionKey(ip, port, srcIp, srcPort);
129+
String keys = Session.getSessionKey(SessionProtocol.UDP, ip, port, srcIp, srcPort);
128130

129131
// For TCP, we freak out if you try to create an already existing session.
130132
// With UDP though, it's totally fine:
131133
Session existingSession = table.get(keys);
132134
if (existingSession != null) return existingSession;
133135

134-
Session session = new Session(srcIp, srcPort, ip, port, this);
136+
Session session = new Session(SessionProtocol.UDP, srcIp, srcPort, ip, port, this);
135137

136138
DatagramChannel channel;
137139

@@ -160,7 +162,7 @@ public Session createNewUDPSession(int ip, int port, int srcIp, int srcPort) thr
160162

161163
@NotNull
162164
public Session createNewTCPSession(int ip, int port, int srcIp, int srcPort) throws IOException {
163-
String key = Session.getSessionKey(ip, port, srcIp, srcPort);
165+
String key = Session.getSessionKey(SessionProtocol.TCP, ip, port, srcIp, srcPort);
164166

165167
Session existingSession = table.get(key);
166168

@@ -169,7 +171,7 @@ public Session createNewTCPSession(int ip, int port, int srcIp, int srcPort) thr
169171
// We return the initialized session, which will be reacked to indicate rejection.
170172
if (existingSession != null) return existingSession;
171173

172-
Session session = new Session(srcIp, srcPort, ip, port, this);
174+
Session session = new Session(SessionProtocol.TCP, srcIp, srcPort, ip, port, this);
173175

174176
SocketChannel channel;
175177
channel = SocketChannel.open();

‎app/src/main/java/tech/httptoolkit/android/vpn/SessionProtocol.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
package tech.httptoolkit.android.vpn;
2+
3+
public enum SessionProtocol {
4+
TCP,
5+
UDP
6+
}

0 commit comments

Comments
 (0)