Modified: trunk/Source/WebKit2/ChangeLog (201380 => 201381)
--- trunk/Source/WebKit2/ChangeLog 2016-05-25 13:03:24 UTC (rev 201380)
+++ trunk/Source/WebKit2/ChangeLog 2016-05-25 13:18:26 UTC (rev 201381)
@@ -1,3 +1,32 @@
+2016-05-25 Fujii Hironori <[email protected]>
+
+ [Unix] Potential buffer overrun of m_fileDescriptors in readBytesFromSocket of ConnectionUnix.cpp
+ https://bugs.webkit.org/show_bug.cgi?id=158058
+
+ Reviewed by Carlos Garcia Campos.
+
+ Memcpy does not check the boundary of m_fileDescriptors in
+ readBytesFromSocket of ConnectionUnix.cpp. This is not a problem
+ in normal cases, but in the case when Web process is hijacked and
+ malicious IPC packets were sent. WTF::Vector already has two
+ members m_capacity and m_size. There is no need to have a
+ separate member m_fileDescriptorsSize to remember the number of
+ remaining data.
+
+ * Platform/IPC/Connection.h: Remove members m_readBufferSize and
+ m_fileDescriptorsSize.
+ * Platform/IPC/unix/ConnectionUnix.cpp:
+ (IPC::Connection::platformInitialize): Removed initialization of
+ m_readBufferSize and m_fileDescriptorsSize. Reserve initial
+ capacity for m_readBuffer and m_fileDescriptors.
+ (IPC::Connection::processMessage): Replace m_readBufferSize and
+ m_fileDescriptorsSize with m_readBuffer.size() and
+ m_fileDescriptors.size(). Use Vector::shrink() to reset the
+ number of remaining data in the buffers.
+ (IPC::readBytesFromSocket) : Change argument types to WTF::Vector
+ instead of pointers and sizes.
+ (IPC::Connection::readyReadHandler): Call new readBytesFromSocket
+
2016-05-25 Chris Dumez <[email protected]>
Update constructRevalidationRequest() to stop returning a unique_ptr<ResourceRequest>
Modified: trunk/Source/WebKit2/Platform/IPC/Connection.h (201380 => 201381)
--- trunk/Source/WebKit2/Platform/IPC/Connection.h 2016-05-25 13:03:24 UTC (rev 201380)
+++ trunk/Source/WebKit2/Platform/IPC/Connection.h 2016-05-25 13:18:26 UTC (rev 201381)
@@ -325,9 +325,7 @@
bool processMessage();
Vector<uint8_t> m_readBuffer;
- size_t m_readBufferSize;
Vector<int> m_fileDescriptors;
- size_t m_fileDescriptorsSize;
int m_socketDescriptor;
#if PLATFORM(GTK)
GSocketMonitor m_socketMonitor;
Modified: trunk/Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp (201380 => 201381)
--- trunk/Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp 2016-05-25 13:03:24 UTC (rev 201380)
+++ trunk/Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp 2016-05-25 13:18:26 UTC (rev 201381)
@@ -132,10 +132,8 @@
void Connection::platformInitialize(Identifier identifier)
{
m_socketDescriptor = identifier;
- m_readBuffer.resize(messageMaxSize);
- m_readBufferSize = 0;
- m_fileDescriptors.resize(attachmentMaxAmount);
- m_fileDescriptorsSize = 0;
+ m_readBuffer.reserveInitialCapacity(messageMaxSize);
+ m_fileDescriptors.reserveInitialCapacity(attachmentMaxAmount);
}
void Connection::platformInvalidate()
@@ -161,7 +159,7 @@
bool Connection::processMessage()
{
- if (m_readBufferSize < sizeof(MessageInfo))
+ if (m_readBuffer.size() < sizeof(MessageInfo))
return false;
uint8_t* messageData = m_readBuffer.data();
@@ -170,7 +168,7 @@
messageData += sizeof(messageInfo);
size_t messageLength = sizeof(MessageInfo) + messageInfo.attachmentCount() * sizeof(AttachmentInfo) + (messageInfo.isMessageBodyIsOutOfLine() ? 0 : messageInfo.bodySize());
- if (m_readBufferSize < messageLength)
+ if (m_readBuffer.size() < messageLength)
return false;
size_t attachmentFileDescriptorCount = 0;
@@ -251,25 +249,25 @@
processIncomingMessage(WTFMove(decoder));
- if (m_readBufferSize > messageLength) {
- memmove(m_readBuffer.data(), m_readBuffer.data() + messageLength, m_readBufferSize - messageLength);
- m_readBufferSize -= messageLength;
+ if (m_readBuffer.size() > messageLength) {
+ memmove(m_readBuffer.data(), m_readBuffer.data() + messageLength, m_readBuffer.size() - messageLength);
+ m_readBuffer.shrink(m_readBuffer.size() - messageLength);
} else
- m_readBufferSize = 0;
+ m_readBuffer.shrink(0);
if (attachmentFileDescriptorCount) {
- if (m_fileDescriptorsSize > attachmentFileDescriptorCount) {
- memmove(m_fileDescriptors.data(), m_fileDescriptors.data() + attachmentFileDescriptorCount, (m_fileDescriptorsSize - attachmentFileDescriptorCount) * sizeof(int));
- m_fileDescriptorsSize -= attachmentFileDescriptorCount;
+ if (m_fileDescriptors.size() > attachmentFileDescriptorCount) {
+ memmove(m_fileDescriptors.data(), m_fileDescriptors.data() + attachmentFileDescriptorCount, (m_fileDescriptors.size() - attachmentFileDescriptorCount) * sizeof(int));
+ m_fileDescriptors.shrink(m_fileDescriptors.size() - attachmentFileDescriptorCount);
} else
- m_fileDescriptorsSize = 0;
+ m_fileDescriptors.shrink(0);
}
return true;
}
-static ssize_t readBytesFromSocket(int socketDescriptor, uint8_t* buffer, int count, int* fileDescriptors, size_t* fileDescriptorsCount)
+static ssize_t readBytesFromSocket(int socketDescriptor, Vector<uint8_t>& buffer, Vector<int>& fileDescriptors)
{
struct msghdr message;
memset(&message, 0, sizeof(message));
@@ -282,8 +280,10 @@
memset(attachmentDescriptorBuffer.get(), 0, sizeof(char) * message.msg_controllen);
message.msg_control = attachmentDescriptorBuffer.get();
- iov[0].iov_base = buffer;
- iov[0].iov_len = count;
+ size_t previousBufferSize = buffer.size();
+ buffer.grow(buffer.capacity());
+ iov[0].iov_base = buffer.data() + previousBufferSize;
+ iov[0].iov_len = buffer.size() - previousBufferSize;
message.msg_iov = iov;
message.msg_iovlen = 1;
@@ -295,33 +295,31 @@
if (errno == EINTR)
continue;
+ buffer.shrink(previousBufferSize);
return -1;
}
- bool found = false;
struct cmsghdr* controlMessage;
for (controlMessage = CMSG_FIRSTHDR(&message); controlMessage; controlMessage = CMSG_NXTHDR(&message, controlMessage)) {
if (controlMessage->cmsg_level == SOL_SOCKET && controlMessage->cmsg_type == SCM_RIGHTS) {
- *fileDescriptorsCount = (controlMessage->cmsg_len - CMSG_LEN(0)) / sizeof(int);
- memcpy(fileDescriptors, CMSG_DATA(controlMessage), sizeof(int) * *fileDescriptorsCount);
+ size_t previousFileDescriptorsSize = fileDescriptors.size();
+ size_t fileDescriptorsCount = (controlMessage->cmsg_len - CMSG_LEN(0)) / sizeof(int);
+ fileDescriptors.grow(fileDescriptors.size() + fileDescriptorsCount);
+ memcpy(fileDescriptors.data() + previousFileDescriptorsSize, CMSG_DATA(controlMessage), sizeof(int) * fileDescriptorsCount);
- for (size_t i = 0; i < *fileDescriptorsCount; ++i) {
- while (fcntl(fileDescriptors[i], F_SETFD, FD_CLOEXEC) == -1) {
+ for (size_t i = 0; i < fileDescriptorsCount; ++i) {
+ while (fcntl(fileDescriptors[previousFileDescriptorsSize + i], F_SETFD, FD_CLOEXEC) == -1) {
if (errno != EINTR) {
ASSERT_NOT_REACHED();
break;
}
}
}
-
- found = true;
break;
}
}
- if (!found)
- *fileDescriptorsCount = 0;
-
+ buffer.shrink(previousBufferSize + bytesRead);
return bytesRead;
}
@@ -331,10 +329,7 @@
void Connection::readyReadHandler()
{
while (true) {
- size_t fileDescriptorsCount = 0;
- size_t bytesToRead = m_readBuffer.size() - m_readBufferSize;
- ssize_t bytesRead = readBytesFromSocket(m_socketDescriptor, m_readBuffer.data() + m_readBufferSize, bytesToRead,
- m_fileDescriptors.data() + m_fileDescriptorsSize, &fileDescriptorsCount);
+ ssize_t bytesRead = readBytesFromSocket(m_socketDescriptor, m_readBuffer, m_fileDescriptors);
if (bytesRead < 0) {
// EINTR was already handled by readBytesFromSocket.
@@ -348,9 +343,6 @@
return;
}
- m_readBufferSize += bytesRead;
- m_fileDescriptorsSize += fileDescriptorsCount;
-
if (!bytesRead) {
connectionDidClose();
return;
