From 34b3be419c461dfbb0be8529a1dbf32bc20598a5 Mon Sep 17 00:00:00 2001 From: Mike Hommey Date: Tue, 25 Nov 2014 08:46:59 +0900 Subject: [PATCH] Bug 1102022 - Increase the SOCKS I/O buffer size to avoid buffer overflows. r=mcmanus This also adds static checks that buffer overflows do not sneak in again in the future. Interestingly, this also makes (at least) GCC generate more efficient code. For example, before, writing to the buffer in WriteV5AuthRequest would look like this: mov 0x38(%rbx),%eax mov 0x28(%rbx),%rcx movb $0x5,(%rcx,%rax,1) mov 0x38(%rbx),%eax inc %eax mov %eax,0x38(%rbx) mov 0x28(%rbx),%rcx movb $0x1,(%rcx,%rax,1) mov 0x38(%rbx),%eax inc %eax mov %eax,0x38(%rbx) mov 0x28(%rbx),%rcx movb $0x0,(%rcx,%rax,1) incl 0x38(%rbx) Now it looks like this: mov 0x28(%rbx),%rax movb $0x5,(%rax) movb $0x1,0x1(%rax) movb $0x0,0x2(%rax) movl $0x3,0x38(%rbx) --- netwerk/socket/nsSOCKSIOLayer.cpp | 251 ++++++++++++++++++------------ 1 file changed, 151 insertions(+), 100 deletions(-) diff --git a/netwerk/socket/nsSOCKSIOLayer.cpp b/netwerk/socket/nsSOCKSIOLayer.cpp index 47105d617af4..63703edc5877 100644 --- a/netwerk/socket/nsSOCKSIOLayer.cpp +++ b/netwerk/socket/nsSOCKSIOLayer.cpp @@ -57,9 +57,9 @@ class nsSOCKSSocketInfo : public nsISOCKSSocketInfo SOCKS_FAILED }; - // A buffer of 262 bytes should be enough for any request and response + // A buffer of 265 bytes should be enough for any request and response // in case of SOCKS4 as well as SOCKS5 - static const uint32_t BUFFER_SIZE = 262; + static const uint32_t BUFFER_SIZE = 265; static const uint32_t MAX_HOSTNAME_LEN = 255; public: @@ -99,13 +99,6 @@ private: PRStatus ReadV5ConnectResponseTop(); PRStatus ReadV5ConnectResponseBottom(); - void WriteUint8(uint8_t d); - void WriteUint16(uint16_t d); - void WriteUint32(uint32_t d); - void WriteNetAddr(const NetAddr *addr); - void WriteNetPort(const NetAddr *addr); - void WriteString(const nsACString &str); - uint8_t ReadUint8(); uint16_t ReadUint16(); uint32_t ReadUint32(); @@ -167,6 +160,115 @@ nsSOCKSSocketInfo::nsSOCKSSocketInfo() mDestinationAddr.inet.port = htons(0); } +/* Helper template class to statically check that writes to a fixed-size + * buffer are not going to overflow. + * + * Example usage: + * uint8_t real_buf[TOTAL_SIZE]; + * Buffer buf(&real_buf); + * auto buf2 = buf.WriteUint16(1); + * auto buf3 = buf2.WriteUint8(2); + * + * It is possible to chain them, to limit the number of (error-prone) + * intermediate variables: + * auto buf = Buffer(&real_buf) + * .WriteUint16(1) + * .WriteUint8(2); + * + * Debug builds assert when intermediate variables are reused: + * Buffer buf(&real_buf); + * auto buf2 = buf.WriteUint16(1); + * auto buf3 = buf.WriteUint8(2); // Asserts + * + * Strings can be written, given an explicit maximum length. + * buf.WriteString(str); + * + * The Written() method returns how many bytes have been written so far: + * Buffer buf(&real_buf); + * auto buf2 = buf.WriteUint16(1); + * auto buf3 = buf2.WriteUint8(2); + * buf3.Written(); // returns 3. + */ +template +class Buffer +{ +public: + Buffer() : mBuf(nullptr), mLength(0) {} + + explicit Buffer(uint8_t* aBuf, size_t aLength=0) + : mBuf(aBuf), mLength(aLength) {} + + template + Buffer(const Buffer& aBuf) : mBuf(aBuf.mBuf), mLength(aBuf.mLength) { + static_assert(Size2 > Size, "Cannot cast buffer"); + } + + Buffer WriteUint8(uint8_t aValue) { + return Write(aValue); + } + + Buffer WriteUint16(uint16_t aValue) { + return Write(aValue); + } + + Buffer WriteUint32(uint32_t aValue) { + return Write(aValue); + } + + Buffer WriteNetPort(const NetAddr* aAddr) { + return WriteUint16(aAddr->inet.port); + } + + Buffer WriteNetAddr(const NetAddr* aAddr) { + if (aAddr->raw.family == AF_INET) { + return Write(aAddr->inet.ip); + } else if (aAddr->raw.family == AF_INET6) { + return Write(aAddr->inet6.ip.u8); + } + NS_NOTREACHED("Unknown address family"); + return *this; + } + + template + Buffer WriteString(const nsACString& aStr) { + if (aStr.Length() > MaxLength) { + return Buffer(nullptr); + } + return WritePtr(aStr.Data(), aStr.Length()); + } + + size_t Written() { + MOZ_ASSERT(mBuf); + return mLength; + } + + operator bool() { return !!mBuf; } +private: + template + friend class Buffer; + + template + Buffer Write(T& aValue) { + return WritePtr(&aValue, sizeof(T)); + } + + template + Buffer WritePtr(const T* aValue, size_t aCopyLength) { + static_assert(Size >= Length, "Cannot write that much"); + MOZ_ASSERT(aCopyLength <= Length); + MOZ_ASSERT(mBuf); + memcpy(mBuf, aValue, aCopyLength); + Buffer result(mBuf + aCopyLength, mLength + aCopyLength); + mBuf = nullptr; + mLength = 0; + return result; + } + + uint8_t* mBuf; + size_t mLength; +}; + + void nsSOCKSSocketInfo::Init(int32_t version, int32_t family, const char *proxyHost, int32_t proxyPort, const char *host, uint32_t flags) { @@ -454,33 +556,39 @@ nsSOCKSSocketInfo::WriteV4ConnectRequest() proxy_resolve? "yes" : "no")); // Send a SOCKS 4 connect request. - WriteUint8(0x04); // version -- 4 - WriteUint8(0x01); // command -- connect - WriteNetPort(addr); + auto buf = Buffer(mData) + .WriteUint8(0x04) // version -- 4 + .WriteUint8(0x01) // command -- connect + .WriteNetPort(addr); + + // We don't have anything more to write after the if, so we can + // use a buffer with no further writes allowed. + Buffer<0> buf3; if (proxy_resolve) { // Add the full name, null-terminated, to the request // according to SOCKS 4a. A fake IP address, with the first // four bytes set to 0 and the last byte set to something other // than 0, is used to notify the proxy that this is a SOCKS 4a // request. This request type works for Tor and perhaps others. - WriteUint32(htonl(0x00000001)); // Fake IP - WriteUint8(0x00); // Send an emtpy username - if (mDestinationHost.Length() > MAX_HOSTNAME_LEN) { + auto buf2 = buf.WriteUint32(htonl(0x00000001)) // Fake IP + .WriteUint8(0x00) // Send an emtpy username + .WriteString(mDestinationHost); // Hostname + if (!buf2) { LOGERROR(("socks4: destination host name is too long!")); HandshakeFinished(PR_BAD_ADDRESS_ERROR); return PR_FAILURE; } - WriteString(mDestinationHost); // Hostname - WriteUint8(0x00); + buf3 = buf2.WriteUint8(0x00); } else if (addr->raw.family == AF_INET) { - WriteNetAddr(addr); // Add the IPv4 address - WriteUint8(0x00); // Send an emtpy username - } else if (addr->raw.family == AF_INET6) { - LOGERROR(("socks: SOCKS 4 can't handle IPv6 addresses!")); + buf3 = buf.WriteNetAddr(addr) // Add the IPv4 address + .WriteUint8(0x00); // Send an emtpy username + } else { + LOGERROR(("socks: SOCKS 4 can only handle IPv4 addresses!")); HandshakeFinished(PR_BAD_ADDRESS_ERROR); return PR_FAILURE; } + mDataLength = buf3.Written(); return PR_SUCCESS; } @@ -521,9 +629,11 @@ nsSOCKSSocketInfo::WriteV5AuthRequest() // Send an initial SOCKS 5 greeting LOGDEBUG(("socks5: sending auth methods")); - WriteUint8(0x05); // version -- 5 - WriteUint8(0x01); // # auth methods -- 1 - WriteUint8(0x00); // we don't support authentication + mDataLength = Buffer(mData) + .WriteUint8(0x05) // version -- 5 + .WriteUint8(0x01) // # auth methods -- 1 + .WriteUint8(0x00) // we don't support authentication + .Written(); return PR_SUCCESS; } @@ -569,36 +679,41 @@ nsSOCKSSocketInfo::WriteV5ConnectRequest() mDataLength = 0; mState = SOCKS5_WRITE_CONNECT_REQUEST; - WriteUint8(0x05); // version -- 5 - WriteUint8(0x01); // command -- connect - WriteUint8(0x00); // reserved + auto buf = Buffer(mData) + .WriteUint8(0x05) // version -- 5 + .WriteUint8(0x01) // command -- connect + .WriteUint8(0x00); // reserved + // We're writing a net port after the if, so we need a buffer allowing + // to write that much. + Buffer buf2; // Add the address to the SOCKS 5 request. SOCKS 5 supports several // address types, so we pick the one that works best for us. if (proxy_resolve) { // Add the host name. Only a single byte is used to store the length, // so we must prevent long names from being used. - if (mDestinationHost.Length() > MAX_HOSTNAME_LEN) { + buf2 = buf.WriteUint8(0x03) // addr type -- domainname + .WriteUint8(mDestinationHost.Length()) // name length + .WriteString(mDestinationHost); // Hostname + if (!buf2) { LOGERROR(("socks5: destination host name is too long!")); HandshakeFinished(PR_BAD_ADDRESS_ERROR); return PR_FAILURE; } - WriteUint8(0x03); // addr type -- domainname - WriteUint8(mDestinationHost.Length()); // name length - WriteString(mDestinationHost); } else if (addr->raw.family == AF_INET) { - WriteUint8(0x01); // addr type -- IPv4 - WriteNetAddr(addr); + buf2 = buf.WriteUint8(0x01) // addr type -- IPv4 + .WriteNetAddr(addr); } else if (addr->raw.family == AF_INET6) { - WriteUint8(0x04); // addr type -- IPv6 - WriteNetAddr(addr); + buf2 = buf.WriteUint8(0x04) // addr type -- IPv6 + .WriteNetAddr(addr); } else { LOGERROR(("socks5: destination address of unknown type!")); HandshakeFinished(PR_BAD_ADDRESS_ERROR); return PR_FAILURE; } - WriteNetPort(addr); // port + auto buf3 = buf2.WriteNetPort(addr); // port + mDataLength = buf3.Written(); return PR_SUCCESS; } @@ -852,70 +967,6 @@ nsSOCKSSocketInfo::GetPollFlags() const return 0; } -inline void -nsSOCKSSocketInfo::WriteUint8(uint8_t v) -{ - NS_ABORT_IF_FALSE(mDataLength + sizeof(v) <= BUFFER_SIZE, - "Can't write that much data!"); - mData[mDataLength] = v; - mDataLength += sizeof(v); -} - -inline void -nsSOCKSSocketInfo::WriteUint16(uint16_t v) -{ - NS_ABORT_IF_FALSE(mDataLength + sizeof(v) <= BUFFER_SIZE, - "Can't write that much data!"); - memcpy(mData + mDataLength, &v, sizeof(v)); - mDataLength += sizeof(v); -} - -inline void -nsSOCKSSocketInfo::WriteUint32(uint32_t v) -{ - NS_ABORT_IF_FALSE(mDataLength + sizeof(v) <= BUFFER_SIZE, - "Can't write that much data!"); - memcpy(mData + mDataLength, &v, sizeof(v)); - mDataLength += sizeof(v); -} - -void -nsSOCKSSocketInfo::WriteNetAddr(const NetAddr *addr) -{ - const char *ip = nullptr; - uint32_t len = 0; - - if (addr->raw.family == AF_INET) { - ip = (const char*)&addr->inet.ip; - len = sizeof(addr->inet.ip); - } else if (addr->raw.family == AF_INET6) { - ip = (const char*)addr->inet6.ip.u8; - len = sizeof(addr->inet6.ip.u8); - } - - NS_ABORT_IF_FALSE(ip != nullptr, "Unknown address"); - NS_ABORT_IF_FALSE(mDataLength + len <= BUFFER_SIZE, - "Can't write that much data!"); - - memcpy(mData + mDataLength, ip, len); - mDataLength += len; -} - -void -nsSOCKSSocketInfo::WriteNetPort(const NetAddr *addr) -{ - WriteUint16(addr->inet.port); -} - -void -nsSOCKSSocketInfo::WriteString(const nsACString &str) -{ - NS_ABORT_IF_FALSE(mDataLength + str.Length() <= BUFFER_SIZE, - "Can't write that much data!"); - memcpy(mData + mDataLength, str.Data(), str.Length()); - mDataLength += str.Length(); -} - inline uint8_t nsSOCKSSocketInfo::ReadUint8() {