On 6/9/17, 7:54 PM, "[email protected] on behalf of Anand Kumar"
<[email protected] on behalf of [email protected]> wrote:
- Minimum valid fragment size is 400 bytes, any fragment smaller
is likely to be intentionally crafted (CVE-2000-0305).
- Validate maximum length of an Ip datagram
- Added counters to keep track of number of fragments for a given
Ip datagram.
Signed-off-by: Anand Kumar <[email protected]>
Acked-by: Alin Gabriel Serdean <[email protected]>
---
v1->v2: Update commmit message to add (CVE-2000-0305) vulnerability
---
datapath-windows/ovsext/Actions.c | 2 +-
datapath-windows/ovsext/IpFragment.c | 41
+++++++++++++++++++++++++-----------
datapath-windows/ovsext/IpFragment.h | 2 ++
3 files changed, 32 insertions(+), 13 deletions(-)
diff --git a/datapath-windows/ovsext/Actions.c
b/datapath-windows/ovsext/Actions.c
index c3f0362..4eeaab3 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -2030,7 +2030,7 @@ OvsDoExecuteActions(POVS_SWITCH_CONTEXT switchContext,
if (status != NDIS_STATUS_SUCCESS) {
/* Pending NBLs are consumed by Defragmentation. */
if (status != NDIS_STATUS_PENDING) {
- OVS_LOG_ERROR("CT Action failed");
+ OVS_LOG_ERROR("CT Action failed status = %lu", status);
dropReason = L"OVS-conntrack action failed";
} else {
/* We added a new pending NBL to be consumed later.
diff --git a/datapath-windows/ovsext/IpFragment.c
b/datapath-windows/ovsext/IpFragment.c
index 0874cb9..e601a15 100644
--- a/datapath-windows/ovsext/IpFragment.c
+++ b/datapath-windows/ovsext/IpFragment.c
@@ -25,6 +25,10 @@
#undef OVS_DBG_MOD
#endif
#define OVS_DBG_MOD OVS_DBG_IPFRAG
+/* Based on MIN_FRAGMENT_SIZE.*/
+#define MAX_FRAGMENTS 164
+#define MIN_FRAGMENT_SIZE 400
I don’t plan a full review; just one comment.
It is almost certainly an exploit attempt and can be treated as such in all
cases, but this
default is something like 20 years old, so it super conservatively small; using
a 400 minimum
for non-last fragments will do most of what is needed though.
+#define MAX_IPDATAGRAM_SIZE 65535
/* Function declarations */
static VOID OvsIpFragmentEntryCleaner(PVOID data);
@@ -146,8 +150,9 @@ OvsIpv4Reassemble(POVS_SWITCH_CONTEXT switchContext,
IPHdr *ipHdr, *newIpHdr;
CHAR *ethBuf[sizeof(EthHdr)];
CHAR *packetBuf;
- UINT16 ipHdrLen, packetLen, packetHeader;
+ UINT16 ipHdrLen, packetHeader;
POVS_FRAGMENT_LIST head = NULL;
+ UINT32 packetLen;
curNb = NET_BUFFER_LIST_FIRST_NB(*curNbl);
ASSERT(NET_BUFFER_NEXT_NB(curNb) == NULL);
@@ -161,7 +166,10 @@ OvsIpv4Reassemble(POVS_SWITCH_CONTEXT switchContext,
if (ipHdr == NULL) {
return NDIS_STATUS_INVALID_PACKET;
}
- ipHdrLen = (UINT16)(ipHdr->ihl * 4);
+ ipHdrLen = ipHdr->ihl * 4;
+ if (ipHdrLen + entry->totalLen > MAX_IPDATAGRAM_SIZE) {
+ return NDIS_STATUS_INVALID_LENGTH;
+ }
packetLen = ETH_HEADER_LENGTH + ipHdrLen + entry->totalLen;
packetBuf = (CHAR*)OvsAllocateMemoryWithTag(packetLen,
OVS_IPFRAG_POOL_TAG);
@@ -185,7 +193,10 @@ OvsIpv4Reassemble(POVS_SWITCH_CONTEXT switchContext,
packetHeader = ETH_HEADER_LENGTH + ipHdrLen;
head = entry->head;
while (head) {
- ASSERT((packetHeader + head->offset) <= packetLen);
+ if ((UINT32)(packetHeader + head->offset) > packetLen) {
+ status = NDIS_STATUS_INVALID_DATA;
+ goto cleanup;
+ }
NdisMoveMemory(packetBuf + packetHeader + head->offset,
head->pbuff, head->len);
head = head->next;
@@ -197,10 +208,6 @@ OvsIpv4Reassemble(POVS_SWITCH_CONTEXT switchContext,
status = NDIS_STATUS_RESOURCES;
}
- OvsFreeMemoryWithTag(packetBuf, OVS_IPFRAG_POOL_TAG);
- /* Timeout the entry so that clean up thread deletes it .*/
- entry->expiration -= IPFRAG_ENTRY_TIMEOUT;
-
/* Complete the fragment NBL */
ctx = (POVS_BUFFER_CONTEXT)NET_BUFFER_LIST_CONTEXT_DATA_START(*curNbl);
if (ctx->flags & OVS_BUFFER_NEED_COMPLETE) {
@@ -214,6 +221,9 @@ OvsIpv4Reassemble(POVS_SWITCH_CONTEXT switchContext,
ctx = (POVS_BUFFER_CONTEXT)NET_BUFFER_LIST_CONTEXT_DATA_START(*newNbl);
ctx->mru = entry->mru;
*curNbl = *newNbl;
+cleanup:
+ OvsFreeMemoryWithTag(packetBuf, OVS_IPFRAG_POOL_TAG);
+ entry->markedForDelete = TRUE;
return status;
}
/*
@@ -259,12 +269,15 @@ OvsProcessIpv4Fragment(POVS_SWITCH_CONTEXT
switchContext,
if (ipHdr == NULL) {
return NDIS_STATUS_INVALID_PACKET;
}
- ipHdrLen = (UINT16)(ipHdr->ihl * 4);
+ ipHdrLen = ipHdr->ihl * 4;
payloadLen = ntohs(ipHdr->tot_len) - ipHdrLen;
offset = ntohs(ipHdr->frag_off) & IP_OFFSET;
offset <<= 3;
flags = ntohs(ipHdr->frag_off) & IP_MF;
-
+ /* Only the last fragment can be of smaller size.*/
+ if (flags && ntohs(ipHdr->tot_len) < MIN_FRAGMENT_SIZE) {
+ return NDIS_STATUS_INVALID_LENGTH;
+ }
/*Copy fragment specific fields. */
fragKey.protocol = ipHdr->protocol;
fragKey.id = ipHdr->id;
@@ -318,6 +331,7 @@ OvsProcessIpv4Fragment(POVS_SWITCH_CONTEXT
switchContext,
entry->mru = ETH_HEADER_LENGTH + ipHdrLen + payloadLen;
entry->recvdLen += fragStorage->len;
entry->head = entry->tail = fragStorage;
+ entry->numFragments = 1;
if (!flags) {
entry->totalLen = offset + payloadLen;
}
@@ -337,8 +351,9 @@ OvsProcessIpv4Fragment(POVS_SWITCH_CONTEXT
switchContext,
/* Acquire the entry lock. */
NdisAcquireSpinLock(&(entry->lockObj));
NdisGetCurrentSystemTime((LARGE_INTEGER *)¤tTime);
- if (currentTime > entry->expiration) {
- /* Expired entry. */
+ if (currentTime > entry->expiration || entry->numFragments ==
MAX_FRAGMENTS) {
+ /* Mark the entry for delete. */
+ entry->markedForDelete = TRUE;
goto fragment_error;
}
POVS_FRAGMENT_LIST next = entry->head;
@@ -393,6 +408,7 @@ found:
/*Update Maximum recieved Unit */
entry->mru = entry->mru > (ETH_HEADER_LENGTH + ipHdrLen +
payloadLen) ?
entry->mru : (ETH_HEADER_LENGTH + ipHdrLen + payloadLen);
+ entry->numFragments++;
if (!flags) {
entry->totalLen = offset + payloadLen;
}
@@ -404,6 +420,7 @@ found:
return status;
}
fragment_error:
+ status = NDIS_STATUS_INVALID_PACKET;
/* Release the entry lock. */
NdisReleaseSpinLock(&(entry->lockObj));
payload_copy_error:
@@ -460,7 +477,7 @@ static VOID
OvsIpFragmentEntryDelete(POVS_IPFRAG_ENTRY entry, BOOLEAN checkExpiry)
{
NdisAcquireSpinLock(&(entry->lockObj));
- if (checkExpiry) {
+ if (!entry->markedForDelete && checkExpiry) {
UINT64 currentTime;
NdisGetCurrentSystemTime((LARGE_INTEGER *)¤tTime);
if (entry->expiration > currentTime) {
diff --git a/datapath-windows/ovsext/IpFragment.h
b/datapath-windows/ovsext/IpFragment.h
index e650399..cd5b960 100644
--- a/datapath-windows/ovsext/IpFragment.h
+++ b/datapath-windows/ovsext/IpFragment.h
@@ -37,6 +37,8 @@ typedef struct _OVS_IPFRAG_KEY {
typedef struct _OVS_IPFRAG_ENTRY {
NDIS_SPIN_LOCK lockObj; /* To access the entry. */
+ BOOLEAN markedForDelete;
+ UINT8 numFragments;
UINT16 totalLen;
UINT16 recvdLen;
UINT16 mru;
--
2.9.3.windows.1
_______________________________________________
dev mailing list
[email protected]
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=0LGiq-mkmw2jTPk_LrWeOvPcXDuO92oANfpzdxui6gM&s=Fj9i3liY5YmbLmFVY63-uMv2Vjwj11rXFfn8cXSU2Yw&e=
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
