pppoe: fix protocol field length variation

Detect when protocol field is not a 16 bit field.
Added tests to prove logic

Ticket: 4810
(cherry picked from commit 6bf2117056e8c2e9448a02d2198384935b1d5b70)

diff --git a/src/decode-pppoe.c b/src/decode-pppoe.c
index 8887a30dba04ec43ac19035ae213d9b712c6c0a9..19c46edb02732849fa55c135b146c85c323b851e 100644 (file)
--- a/src/decode-pppoe.c
+++ b/src/decode-pppoe.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 2007-2013 Open Information Security Foundation
+/* Copyright (C) 2007-2021 Open Information Security Foundation
  *
  * You can copy, redistribute or modify this Program under the terms of
  * the GNU General Public License version 2 as published by the Free
@@ -38,7 +38,6 @@
 #include "decode-ppp.h"
 #include "decode-pppoe.h"
 #include "decode-events.h"
-
 #include "flow.h"
 
 #include "util-unittest.h"
@@ -132,7 +131,7 @@ int DecodePPPOESession(ThreadVars *tv, DecodeThreadVars *dtv, Packet *p,
 {
     StatsIncr(tv, dtv->counter_pppoe);
 
-    if (len < PPPOE_SESSION_HEADER_LEN) {
+    if (len < PPPOE_SESSION_HEADER_MIN_LEN) {
         ENGINE_SET_INVALID_EVENT(p, PPPOE_PKT_TOO_SMALL);
         return TM_ECODE_FAILED;
     }
@@ -145,12 +144,31 @@ int DecodePPPOESession(ThreadVars *tv, DecodeThreadVars *dtv, Packet *p,
            PPPOE_SESSION_GET_VERSION(p->pppoesh),  PPPOE_SESSION_GET_TYPE(p->pppoesh),  p->pppoesh->pppoe_code,  SCNtohs(p->pppoesh->session_id),  SCNtohs(p->pppoesh->pppoe_length));
 
     /* can't use DecodePPP() here because we only get a single 2-byte word to indicate protocol instead of the full PPP header */
-
     if (SCNtohs(p->pppoesh->pppoe_length) > 0) {
         /* decode contained PPP packet */
 
-        switch (SCNtohs(p->pppoesh->protocol))
-        {
+        uint8_t pppoesh_len;
+        uint16_t ppp_protocol = SCNtohs(p->pppoesh->protocol);
+
+        /* According to RFC1661-2, if the least significant bit of the most significant octet is
+         * set, we're dealing with a single-octet protocol field */
+        if (ppp_protocol & 0x0100) {
+            /* Single-octet variant */
+            ppp_protocol >>= 8;
+            pppoesh_len = PPPOE_SESSION_HEADER_MIN_LEN;
+        } else {
+            /* Double-octet variant; increase the length of the session header accordingly */
+            pppoesh_len = PPPOE_SESSION_HEADER_MIN_LEN + 1;
+
+            if (len < pppoesh_len) {
+                ENGINE_SET_INVALID_EVENT(p, PPPOE_PKT_TOO_SMALL);
+                return TM_ECODE_FAILED;
+            }
+        }
+
+        SCLogDebug("Protocol %" PRIu16 " len %" PRIu8 "", ppp_protocol, pppoesh_len);
+
+        switch (ppp_protocol) {
             case PPP_VJ_COMP:
             case PPP_IPX:
             case PPP_OSI:
@@ -184,46 +202,45 @@ int DecodePPPOESession(ThreadVars *tv, DecodeThreadVars *dtv, Packet *p,
 
             case PPP_VJ_UCOMP:
 
-                if(len < (PPPOE_SESSION_HEADER_LEN + IPV4_HEADER_LEN))    {
+                if (len - pppoesh_len < IPV4_HEADER_LEN) {
                     ENGINE_SET_INVALID_EVENT(p, PPPVJU_PKT_TOO_SMALL);
                     return TM_ECODE_OK;
                 }
-                if (unlikely(len > PPPOE_SESSION_HEADER_LEN + USHRT_MAX)) {
+                if (unlikely(len - pppoesh_len > USHRT_MAX)) {
                     return TM_ECODE_FAILED;
                 }
 
-                if(IPV4_GET_RAW_VER((IPV4Hdr *)(pkt + PPPOE_SESSION_HEADER_LEN)) == 4) {
-                    DecodeIPV4(tv, dtv, p, pkt + PPPOE_SESSION_HEADER_LEN, len - PPPOE_SESSION_HEADER_LEN, pq );
+                if (IPV4_GET_RAW_VER((IPV4Hdr *)(pkt + pppoesh_len)) == 4) {
+                    DecodeIPV4(tv, dtv, p, pkt + pppoesh_len, len - pppoesh_len, pq);
                 }
                 break;
 
             case PPP_IP:
-                if(len < (PPPOE_SESSION_HEADER_LEN + IPV4_HEADER_LEN))    {
+                if (len - pppoesh_len < IPV4_HEADER_LEN) {
                     ENGINE_SET_INVALID_EVENT(p, PPPIPV4_PKT_TOO_SMALL);
                     return TM_ECODE_OK;
                 }
-                if (unlikely(len > PPPOE_SESSION_HEADER_LEN + USHRT_MAX)) {
+                if (unlikely(len - pppoesh_len > USHRT_MAX)) {
                     return TM_ECODE_FAILED;
                 }
-
-                DecodeIPV4(tv, dtv, p, pkt + PPPOE_SESSION_HEADER_LEN, len - PPPOE_SESSION_HEADER_LEN, pq );
+                DecodeIPV4(tv, dtv, p, pkt + pppoesh_len, len - pppoesh_len, pq);
                 break;
 
             /* PPP IPv6 was not tested */
             case PPP_IPV6:
-                if(len < (PPPOE_SESSION_HEADER_LEN + IPV6_HEADER_LEN))    {
+                if (len - pppoesh_len < IPV6_HEADER_LEN) {
                     ENGINE_SET_INVALID_EVENT(p, PPPIPV6_PKT_TOO_SMALL);
                     return TM_ECODE_OK;
                 }
-                if (unlikely(len > PPPOE_SESSION_HEADER_LEN + USHRT_MAX)) {
+                if (unlikely(len - pppoesh_len > USHRT_MAX)) {
                     return TM_ECODE_FAILED;
                 }
 
-                DecodeIPV6(tv, dtv, p, pkt + PPPOE_SESSION_HEADER_LEN, len - PPPOE_SESSION_HEADER_LEN, pq );
+                DecodeIPV6(tv, dtv, p, pkt + pppoesh_len, len - pppoesh_len, pq);
                 break;
 
             default:
-                SCLogDebug("unknown PPP protocol: %" PRIx32 "",SCNtohs(p->pppoesh->protocol));
+                SCLogDebug("unknown PPP protocol: %" PRIx32 "", ppp_protocol);
                 ENGINE_SET_INVALID_EVENT(p, PPP_WRONG_TYPE);
                 return TM_ECODE_OK;
         }
@@ -446,6 +463,120 @@ static int DecodePPPOEtest06 (void)
 
     return 1;
 }
+
+/** DecodePPPOEtest07
+ *  \brief Valid PPPOE packet with 8 bit protocol field - check the valid  ICMP type is accepted
+ *  \retval 1 Expected test value
+ */
+static int DecodePPPOEtest07(void)
+{
+
+    uint8_t raw_pppoe[] = { 0x11, 0x00, 0x00, 0x2d, 0x00, 0x1c, 0x21, 0x45, 0x00, 0x00, 0x1d, 0x97,
+        0xc3, 0x00, 0x00, 0x40, 0x01, 0x47, 0x0f, 0x0a, 0x64, 0x00, 0x00, 0xc0, 0xa8, 0xd1, 0x01,
+        0x08, 0x00, 0xd4, 0x4c, 0x1f, 0x32, 0x04, 0x81, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+        0x00, 0x00, 0x00, 0x00 };
+
+    Packet *p = PacketGetFromAlloc();
+    if (unlikely(p == NULL))
+        FAIL;
+    ThreadVars tv;
+    DecodeThreadVars dtv;
+
+    memset(&tv, 0, sizeof(ThreadVars));
+    memset(&dtv, 0, sizeof(DecodeThreadVars));
+
+    DecodePPPOESession(&tv, &dtv, p, raw_pppoe, sizeof(raw_pppoe), NULL);
+
+    FAIL_IF(ENGINE_ISSET_EVENT(p, PPP_WRONG_TYPE));
+    SCFree(p);
+    PASS;
+}
+
+/** DecodePPPOEtest08
+ *  \brief Valid PPPOE packet with 8 bit protocol field - check the valid HTTP type is accepted
+ *  \retval 1 Expected test value
+ */
+static int DecodePPPOEtest08(void)
+{
+
+    uint8_t raw_pppoe[] = { 0x11, 0x00, 0x00, 0x2d, 0x00, 0x3d, 0x21, 0x45, 0x00, 0x00, 0x3c, 0x00,
+        0x00, 0x40, 0x00, 0x40, 0x06, 0xed, 0xda, 0x0a, 0x64, 0x00, 0x00, 0x8e, 0xfa, 0xb3, 0x83,
+        0xde, 0xb5, 0x00, 0x50, 0xd4, 0xbd, 0x76, 0x54, 0x00, 0x00, 0x00, 0x00, 0xa0, 0x02, 0xfe,
+        0xcc, 0x74, 0x2f, 0x00, 0x00, 0x02, 0x04, 0x05, 0xac, 0x01, 0x03, 0x03, 0x07, 0x04, 0x02,
+        0x08, 0x0a, 0xcb, 0xae, 0x92, 0x63, 0x00, 0x00, 0x00, 0x00 };
+
+    Packet *p = PacketGetFromAlloc();
+    if (unlikely(p == NULL))
+        FAIL;
+    ThreadVars tv;
+    DecodeThreadVars dtv;
+
+    memset(&tv, 0, sizeof(ThreadVars));
+    memset(&dtv, 0, sizeof(DecodeThreadVars));
+
+    DecodePPPOESession(&tv, &dtv, p, raw_pppoe, sizeof(raw_pppoe), NULL);
+
+    FAIL_IF(ENGINE_ISSET_EVENT(p, PPP_WRONG_TYPE));
+    SCFree(p);
+    PASS;
+}
+
+/** DecodePPPOEtest09
+ *  \brief Valid PPPOE packet with 16 bit protocol field - check the valid  ICMP type is accepted
+ *  \retval 1 Expected test value
+ */
+static int DecodePPPOEtest09(void)
+{
+
+    uint8_t raw_pppoe[] = { 0x11, 0x00, 0x00, 0x2d, 0x00, 0x1c, 0x00, 0x21, 0x45, 0x00, 0x00, 0x1d,
+        0x97, 0xc3, 0x00, 0x00, 0x40, 0x01, 0x47, 0x0f, 0x0a, 0x64, 0x00, 0x00, 0xc0, 0xa8, 0xd1,
+        0x01, 0x08, 0x00, 0xd4, 0x4c, 0x1f, 0x32, 0x04, 0x81, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+        0x00, 0x00, 0x00, 0x00, 0x00 };
+
+    Packet *p = PacketGetFromAlloc();
+    if (unlikely(p == NULL))
+        FAIL;
+    ThreadVars tv;
+    DecodeThreadVars dtv;
+
+    memset(&tv, 0, sizeof(ThreadVars));
+    memset(&dtv, 0, sizeof(DecodeThreadVars));
+
+    DecodePPPOESession(&tv, &dtv, p, raw_pppoe, sizeof(raw_pppoe), NULL);
+
+    FAIL_IF(ENGINE_ISSET_EVENT(p, PPP_WRONG_TYPE));
+    SCFree(p);
+    PASS;
+}
+
+/** DecodePPPOEtest10
+ *  \brief Valid PPPOE packet with 16 bit protocol field - check the valid HTTP type is accepted
+ *  \retval 1 Expected test value
+ */
+static int DecodePPPOEtest10(void)
+{
+
+    uint8_t raw_pppoe[] = { 0x11, 0x00, 0x00, 0x2d, 0x00, 0x3d, 0x00, 0x21, 0x45, 0x00, 0x00, 0x3c,
+        0x00, 0x00, 0x40, 0x00, 0x40, 0x06, 0xed, 0xda, 0x0a, 0x64, 0x00, 0x00, 0x8e, 0xfa, 0xb3,
+        0x83, 0xde, 0xb5, 0x00, 0x50, 0xd4, 0xbd, 0x76, 0x54, 0x00, 0x00, 0x00, 0x00, 0xa0, 0x02,
+        0xfe, 0xcc, 0x74, 0x2f, 0x00, 0x00, 0x02, 0x04, 0x05, 0xac, 0x01, 0x03, 0x03, 0x07, 0x04,
+        0x02, 0x08, 0x0a, 0xcb, 0xae, 0x92, 0x63, 0x00, 0x00, 0x00, 0x00 };
+
+    Packet *p = PacketGetFromAlloc();
+    if (unlikely(p == NULL))
+        FAIL;
+    ThreadVars tv;
+    DecodeThreadVars dtv;
+
+    memset(&tv, 0, sizeof(ThreadVars));
+    memset(&dtv, 0, sizeof(DecodeThreadVars));
+
+    DecodePPPOESession(&tv, &dtv, p, raw_pppoe, sizeof(raw_pppoe), NULL);
+
+    FAIL_IF(ENGINE_ISSET_EVENT(p, PPP_WRONG_TYPE));
+    SCFree(p);
+    PASS;
+}
 #endif /* UNITTESTS */
 
 
@@ -463,6 +594,10 @@ void DecodePPPOERegisterTests(void)
     UtRegisterTest("DecodePPPOEtest04", DecodePPPOEtest04);
     UtRegisterTest("DecodePPPOEtest05", DecodePPPOEtest05);
     UtRegisterTest("DecodePPPOEtest06", DecodePPPOEtest06);
+    UtRegisterTest("DecodePPPOEtest07", DecodePPPOEtest07);
+    UtRegisterTest("DecodePPPOEtest08", DecodePPPOEtest08);
+    UtRegisterTest("DecodePPPOEtest09", DecodePPPOEtest09);
+    UtRegisterTest("DecodePPPOEtest10", DecodePPPOEtest10);
 #endif /* UNITTESTS */
 }
 

diff --git a/src/decode-pppoe.h b/src/decode-pppoe.h
index 6aecf74151995cad5c3391db09ddc09434c72626..44550ad1122e5a3a119b0173811a67a7bf3b3531 100644 (file)
--- a/src/decode-pppoe.h
+++ b/src/decode-pppoe.h
@@ -27,7 +27,8 @@
 #include "decode.h"
 #include "threadvars.h"
 
-#define PPPOE_SESSION_HEADER_LEN 8
+// Session header length minus the protocol field
+#define PPPOE_SESSION_HEADER_MIN_LEN     7
 #define PPPOE_DISCOVERY_HEADER_MIN_LEN 6
 #define PPPOE_SESSION_GET_VERSION(hdr) ((hdr)->pppoe_version_type & 0xF0) >> 4
 #define PPPOE_SESSION_GET_TYPE(hdr) ((hdr)->pppoe_version_type & 0x0F)