Address concerns from Guido Vranken fuzzers (#141)

* 002: Assert return value of snprintf/getnameinfo

* 003: Address OOB reads on decrypted data

* 004: Handle boundary prefixes in .partitioned()

* 005: Fix OOB read in matchesDestination()

* 006: Fix parsing in netname6()

* 007: Fix incorrect use of sizeof()

* 008: Add safety checks in MSSFix()

* 009: Fix bad usage of minilzo calls

* Add checks after RoutingTableEntryAddress4/6

TunnelKit/Sources/Core/RoutingTableEntry.m

@@ -30,6 +30,9 @@
 
 #import "RoutingTableEntry.h"
 
+#define ASSERT_PRINTF(r)        NSCAssert(r >= 0, @"*printf() failed")
+#define ASSERT_GETNAMEINFO(r)   NSCAssert(r == 0, @"getnameinfo() failed")
+
 // adapted from: https://github.com/jianpx/ios-cabin
 
 #define ROUNDUP(a) ((a) > 0 ? (1 + (((a) - 1) | (sizeof(uint32_t) - 1))) : sizeof(uint32_t))
@@ -80,6 +83,7 @@ static NSString *RoutingTableEntryName(struct sockaddr *sa, struct sockaddr *mas
     network = networkComps.firstObject;
     if (networkComps.count == 2) {
         prefix = [networkComps.lastObject integerValue];
+        NSAssert(prefix >= 0 && prefix <= 32, @"IPv4 prefix must lie in [0..32]");
     } else {
         prefix = 32;
     }
@@ -110,6 +114,7 @@ static NSString *RoutingTableEntryName(struct sockaddr *sa, struct sockaddr *mas
     network = networkComps.firstObject;
     if (networkComps.count == 2) {
         prefix = [networkComps.lastObject integerValue];
+        NSAssert(prefix >= 0 && prefix <= 128, @"IPv6 prefix must lie in [0..128]");
     } else {
         prefix = 128;
     }
@@ -200,6 +205,9 @@ static NSString *RoutingTableEntryName(struct sockaddr *sa, struct sockaddr *mas
     if (self.isIPv6) {
         NSData *networkAddress = RoutingTableEntryAddress6(self.network);
         NSData *destinationAddress = RoutingTableEntryAddress6(destination);
+        if (!networkAddress || !destinationAddress) {
+            return NO;
+        }
         
 //        NSLog(@"network:     %@ = %@", networkAddress, self.network);
 //        NSLog(@"destination: %@ = %@", destinationAddress, destination);
@@ -228,6 +236,9 @@ static NSString *RoutingTableEntryName(struct sockaddr *sa, struct sockaddr *mas
     else {
         const uint32_t networkAddress = RoutingTableEntryAddress4(self.network);
         const uint32_t destinationAddress = RoutingTableEntryAddress4(destination);
+        if ((networkAddress == UINT32_MAX) || (destinationAddress == UINT32_MAX)) {
+            return NO;
+        }
         const uint32_t networkMask = ~((1 << (32 - self.prefix)) - 1);
         
 //        NSLog(@"network:     %x = %@", networkAddress, self.network);
@@ -243,15 +254,23 @@ static NSString *RoutingTableEntryName(struct sockaddr *sa, struct sockaddr *mas
     NSMutableArray<RoutingTableEntry *> *segments = [[NSMutableArray alloc] init];
     const int halfPrefix = (int)(self.prefix + 1);
     if (self.isIPv6) {
+        if (self.prefix == 128) {
+            NSLog(@"Can't partition single IPv6");
+            return @[self, self];
+        }
+        
         struct in6_addr saddr1, saddr2;
         char addr[INET6_ADDRSTRLEN];
         NSData *addressData = RoutingTableEntryAddress6(self.network);
+        if (!addressData) {
+            return nil;
+        }
         memcpy(&saddr1, addressData.bytes, addressData.length);
         NSMutableData *addressData2 = [addressData mutableCopy];
-
+        
         uint8_t *addressBytes2 = (uint8_t *)addressData2.bytes;
-        const uint8_t mask2 = 1 << (8 - halfPrefix % 8);
+        const uint8_t mask2 = 1 << ((8 - halfPrefix % 8) % 8);
-        addressBytes2[halfPrefix / 8] |= mask2;
+        addressBytes2[(halfPrefix - 1) / 8] |= mask2;
 
         memcpy(&saddr2, addressData2.bytes, addressData2.length);
 
@@ -263,8 +282,16 @@ static NSString *RoutingTableEntryName(struct sockaddr *sa, struct sockaddr *mas
         [segments addObject:[[RoutingTableEntry alloc] initWithIPv6Network:network1 gateway:self.gateway networkInterface:self.networkInterface]];
         [segments addObject:[[RoutingTableEntry alloc] initWithIPv6Network:network2 gateway:self.gateway networkInterface:self.networkInterface]];
     } else {
+        if (self.prefix == 32) {
+            NSLog(@"Can't partition single IPv4");
+            return @[self, self];
+        }
+
         struct in_addr saddr1, saddr2;
         const uint32_t address = RoutingTableEntryAddress4(self.network);
+        if (address == UINT32_MAX) {
+            return nil;
+        }
         saddr1.s_addr = htonl(address);
         saddr2.s_addr = htonl(address | (1 << (32 - halfPrefix)));
 
@@ -294,7 +321,7 @@ static char *netname6(struct sockaddr_in6 *sa6, struct sockaddr *sam);
 static char *routename(uint32_t in);
 static char *routename6(struct sockaddr_in6 *sa6);
 static uint32_t forgemask(uint32_t a);
-static void domask(char *dst, uint32_t addr, uint32_t mask);
+static void domask(char *dst, size_t dstsize, uint32_t addr, uint32_t mask);
 static void trimdomain(char *cp);
 
 static inline uint32_t RoutingTableEntryAddress4(NSString *string)
@@ -376,7 +403,7 @@ char *routename(uint32_t in)
     
 #define C(x)    ((x) & 0xff)
     in = ntohl(in);
-    snprintf(line, sizeof(line), "%u.%u.%u.%u", C(in >> 24), C(in >> 16), C(in >> 8), C(in));
+    ASSERT_PRINTF(snprintf(line, sizeof(line), "%u.%u.%u.%u", C(in >> 24), C(in >> 16), C(in >> 8), C(in)));
 
     return (line);
 }
@@ -391,7 +418,7 @@ char *routename6(struct sockaddr_in6 *sa6)
     sa6_local.sin6_addr = sa6->sin6_addr;
     sa6_local.sin6_scope_id = sa6->sin6_scope_id;
     
-    getnameinfo((struct sockaddr *)&sa6_local, sa6_local.sin6_len, line, sizeof(line), NULL, 0, flag);
+    ASSERT_GETNAMEINFO(getnameinfo((struct sockaddr *)&sa6_local, sa6_local.sin6_len, line, sizeof(line), NULL, 0, flag));
     
     return line;
 }
@@ -426,37 +453,35 @@ char *netname(uint32_t in, uint32_t mask)
         switch (dmask) {
             case IN_CLASSA_NET:
                 if ((i & IN_CLASSA_HOST) == 0) {
-                    snprintf(line, sizeof(line), "%u", C(i >> 24));
+                    ASSERT_PRINTF(snprintf(line, sizeof(line), "%u", C(i >> 24)));
                     break;
                 }
                 /* FALLTHROUGH */
             case IN_CLASSB_NET:
                 if ((i & IN_CLASSB_HOST) == 0) {
-                    snprintf(line, sizeof(line), "%u.%u",
+                    ASSERT_PRINTF(snprintf(line, sizeof(line), "%u.%u", C(i >> 24), C(i >> 16)));
-                             C(i >> 24), C(i >> 16));
                     break;
                 }
                 /* FALLTHROUGH */
             case IN_CLASSC_NET:
                 if ((i & IN_CLASSC_HOST) == 0) {
-                    snprintf(line, sizeof(line), "%u.%u.%u",
+                    ASSERT_PRINTF(snprintf(line, sizeof(line), "%u.%u.%u", C(i >> 24), C(i >> 16), C(i >> 8)));
-                             C(i >> 24), C(i >> 16), C(i >> 8));
                     break;
                 }
                 /* FALLTHROUGH */
             default:
-                snprintf(line, sizeof(line), "%u.%u.%u.%u",
+                ASSERT_PRINTF(snprintf(line, sizeof(line), "%u.%u.%u.%u", C(i >> 24), C(i >> 16), C(i >> 8), C(i)));
-                         C(i >> 24), C(i >> 16), C(i >> 8), C(i));
                 break;
         }
     }
-    domask(line+strlen(line), i, omask);
+    domask(line + strlen(line), sizeof(line) - strlen(line), i, omask);
     return (line);
 }
 
 
 char *netname6(struct sockaddr_in6 *sa6, struct sockaddr *sam)
 {
+    char host[MAXHOSTNAMELEN];
     static char line[MAXHOSTNAMELEN + 10];
     u_char *lim;
     int masklen, illegal = 0, flag = NI_NUMERICHOST;
@@ -508,10 +533,12 @@ char *netname6(struct sockaddr_in6 *sa6, struct sockaddr *sam)
         return("default");
     }
     
-    getnameinfo((struct sockaddr *)sa6, sa6->sin6_len, line, sizeof(line), NULL, 0, flag);
+    ASSERT_GETNAMEINFO(getnameinfo((struct sockaddr *)sa6, sa6->sin6_len, host, sizeof(host), NULL, 0, flag));
     
     if (masklen > 0) {
-        sprintf(line, "%s/%u", line, masklen);
+        ASSERT_PRINTF(sprintf(line, "%s/%u", host, masklen));
+    } else {
+        ASSERT_PRINTF(sprintf(line, "%s", host));
     }
     
     return line;
@@ -530,7 +557,7 @@ uint32_t forgemask(uint32_t a)
     return (m);
 }
 
-void domask(char *dst, uint32_t addr, uint32_t mask)
+void domask(char *dst, size_t dstsize, uint32_t addr, uint32_t mask)
 {
     int b, i;
     
@@ -553,9 +580,9 @@ void domask(char *dst, uint32_t addr, uint32_t mask)
         }
     }
     if (i == -1) {
-        snprintf(dst, sizeof(dst), "&0x%x", mask);
+        ASSERT_PRINTF(snprintf(dst, dstsize, "&0x%x", mask));
     } else {
-        snprintf(dst, sizeof(dst), "/%d", 32-i);
+        ASSERT_PRINTF(snprintf(dst, dstsize, "/%d", 32-i));
     }
 }
 

TunnelKit/Sources/Extra/LZO/StandardLZO.m

@@ -63,7 +63,8 @@ static HEAP_ALLOC(wrkmem, LZO1X_1_MEM_COMPRESS);
 
 - (NSData *)compressedDataWithData:(NSData *)data error:(NSError * _Nullable __autoreleasing * _Nullable)error
 {
-    NSMutableData *dst = [[NSMutableData alloc] initWithLength:data.length];
+    const NSInteger dstBufferLength = data.length + data.length / 16 + 64 + 3;
+    NSMutableData *dst = [[NSMutableData alloc] initWithLength:dstBufferLength];
     lzo_uint dstLength;
     const int status = lzo1x_1_compress(data.bytes, data.length, dst.mutableBytes, &dstLength, wrkmem);
     if (status != LZO_E_OK) {
@@ -86,8 +87,8 @@ static HEAP_ALLOC(wrkmem, LZO1X_1_MEM_COMPRESS);
 
 - (NSData *)decompressedDataWithBytes:(const uint8_t *)bytes length:(NSInteger)length error:(NSError * _Nullable __autoreleasing * _Nullable)error
 {
-    lzo_uint dstLength;
+    lzo_uint dstLength = LZO1X_1_15_MEM_COMPRESS;
-    const int status = lzo1x_decompress(bytes, length, self.decompressedBuffer.mutableBytes, &dstLength, NULL);
+    const int status = lzo1x_decompress_safe(bytes, length, self.decompressedBuffer.mutableBytes, &dstLength, NULL);
     if (status != LZO_E_OK) {
         if (error) {
             *error = TunnelKitErrorWithCode(TunnelKitErrorCodeLZO);

TunnelKit/Sources/Protocols/OpenVPN/CryptoAEAD.m

@@ -311,8 +311,8 @@ static const NSInteger CryptoAEADTagLength = 16;
 {
     NSAssert(packet.length > 0, @"Decrypting an empty packet, how did it get this far?");
 
-    DATA_PATH_DECRYPT_INIT(packet.bytes)
+    DATA_PATH_DECRYPT_INIT(packet)
-    if (packet.length < headerLength) {
+    if (packet.length < headerLength + PacketIdLength) {
         return NO;
     }
     

TunnelKit/Sources/Protocols/OpenVPN/CryptoCBC.m

@@ -342,8 +342,8 @@ const NSInteger CryptoCBCMaxHMACLength = 100;
 {
     NSAssert(packet.length > 0, @"Decrypting an empty packet, how did it get this far?");
 
-    DATA_PATH_DECRYPT_INIT(packet.bytes)
+    DATA_PATH_DECRYPT_INIT(packet)
-    if (packet.length < headerLength) {
+    if (packet.length < headerLength + self.crypto.digestLength + self.crypto.cipherIVLength) {
         return NO;
     }
 

TunnelKit/Sources/Protocols/OpenVPN/DataPathCrypto.h

@@ -45,7 +45,8 @@ NS_ASSUME_NONNULL_BEGIN
         headerLength += PacketPeerIdLength; \
     }
 
-#define DATA_PATH_DECRYPT_INIT(ptr) \
+#define DATA_PATH_DECRYPT_INIT(packet) \
+    const uint8_t *ptr = packet.bytes; \
     PacketCode code; \
     PacketOpcodeGet(ptr, &code, NULL); \
     uint32_t peerId = PacketPeerIdDisabled; \
@@ -53,7 +54,10 @@ NS_ASSUME_NONNULL_BEGIN
     int headerLength = PacketOpcodeLength; \
     if (hasPeerId) { \
         headerLength += PacketPeerIdLength; \
-        peerId = PacketHeaderGetDataV2PeerId(packet.bytes); \
+        if (packet.length < headerLength) { \
+            return NO; \
+        } \
+        peerId = PacketHeaderGetDataV2PeerId(ptr); \
     }
 
 typedef void (^DataPathAssembleBlock)(uint8_t *packetDest, NSInteger *packetLengthOffset, NSData *payload);

TunnelKit/Sources/Protocols/OpenVPN/MSS.m

@@ -71,27 +71,54 @@ static inline void MSSUpdateSum(uint16_t* sum_ptr, uint16_t* val_ptr, uint16_t n
 
 void MSSFix(uint8_t *data, NSInteger data_len)
 {
-    ip_hdr_t *iph = (ip_hdr_t*)data;
+    /* XXX Prevent buffer overread */
-    if (iph->proto != PROTO_TCP)  return;
+    if (data_len < sizeof(ip_hdr_t)) {
-    uint32_t iph_size = iph->hdr_len*4;
+        return;
-    if (iph_size+sizeof(tcp_hdr_t) > data_len)  return;
+    }
+    ip_hdr_t *iph = (ip_hdr_t *)data;
+    if (iph->proto != PROTO_TCP) {
+        return;
+    }
+    uint32_t iph_size = iph->hdr_len * 4;
+    if (iph_size + sizeof(tcp_hdr_t) > data_len) {
+        return;
+    }
     
-    tcp_hdr_t *tcph = (tcp_hdr_t*)(data + iph_size);
+    tcp_hdr_t *tcph = (tcp_hdr_t *)(data + iph_size);
-    if (!(tcph->flags & FLAG_SYN))  return;
+    if (!(tcph->flags & FLAG_SYN)) {
+        return;
+    }
     uint8_t *opts = data + iph_size + sizeof(tcp_hdr_t);
 
-    uint32_t tcph_len = tcph->hdr_len*4, optlen = tcph_len-sizeof(tcp_hdr_t);
+    uint32_t tcph_len = tcph->hdr_len * 4, optlen = tcph_len-sizeof(tcp_hdr_t);
-    if (iph_size+sizeof(tcp_hdr_t)+optlen > data_len)  return;
+    if (iph_size + sizeof(tcp_hdr_t) + optlen > data_len) {
+        return;
+    }
 
     for (uint32_t i = 0; i < optlen;) {
-        tcp_opt_t *o = (tcp_opt_t*)&opts[i];
+        tcp_opt_t *o = (tcp_opt_t *)&opts[i];
-        if (o->opt == OPT_END)  return;
+
+        /* XXX Prevent buffer overread */
+        if ((void *)(o + sizeof(tcp_opt_t)) > (void *)(data + data_len)) {
+            return;
+        }
+
+        if (o->opt == OPT_END) {
+            return;
+        }
         if (o->opt == OPT_MSS) {
-            if (i+o->size > optlen)  return;
+            if (i + o->size > optlen) {
-            if (ntohs(o->mss) <= MSS_VAL)  return;
+                return;
+            }
+            if (ntohs(o->mss) <= MSS_VAL) {
+                return;
+            }
             MSSUpdateSum(&tcph->sum, &o->mss, MSS_VAL);
             return;
         }
-        i += (o->opt == OPT_NOP) ? 1 : o->size;
+
+        /* XXX Prevent infinite loop */
+        i += (o->opt == OPT_NOP) ? 1 : (o->size ? o->size : 1);
+//        i += (o->opt == OPT_NOP) ? 1 : o->size;
     }
 }

TunnelKitTests/Core/RoutingTests.swift

@@ -98,7 +98,9 @@ class RoutingTests: XCTestCase {
     
     func testPartitioning() {
         let v4 = RoutingTableEntry(iPv4Network: "192.168.1.0/24", gateway: nil, networkInterface: "en0")
+        let v4Boundary = RoutingTableEntry(iPv4Network: "192.168.1.0/31", gateway: nil, networkInterface: "en0")
         let v6 = RoutingTableEntry(iPv6Network: "abcd:efef:120::/46", gateway: nil, networkInterface: "en0")
+        let v6Boundary = RoutingTableEntry(iPv6Network: "abcd:efef:120::/127", gateway: nil, networkInterface: "en0")
         
         let v4parts = v4.partitioned()
         let v4parts1 = v4parts[0]
@@ -115,5 +117,21 @@ class RoutingTests: XCTestCase {
         XCTAssertEqual(v6parts1.prefix(), 47)
         XCTAssertEqual(v6parts2.network(), "abcd:efef:122::")
         XCTAssertEqual(v6parts2.prefix(), 47)
+
+        let v4BoundaryParts = v4Boundary.partitioned()
+        let v4BoundaryParts1 = v4BoundaryParts[0]
+        let v4BoundaryParts2 = v4BoundaryParts[1]
+        XCTAssertEqual(v4BoundaryParts1.network(), "192.168.1.0")
+        XCTAssertEqual(v4BoundaryParts1.prefix(), 32)
+        XCTAssertEqual(v4BoundaryParts2.network(), "192.168.1.1")
+        XCTAssertEqual(v4BoundaryParts2.prefix(), 32)
+
+        let v6BoundaryParts = v6Boundary.partitioned()
+        let v6BoundaryParts1 = v6BoundaryParts[0]
+        let v6BoundaryParts2 = v6BoundaryParts[1]
+        XCTAssertEqual(v6BoundaryParts1.network(), "abcd:efef:120::")
+        XCTAssertEqual(v6BoundaryParts1.prefix(), 128)
+        XCTAssertEqual(v6BoundaryParts2.network(), "abcd:efef:120::1")
+        XCTAssertEqual(v6BoundaryParts2.prefix(), 128)
     }
 }