pppoe: fix protocol field length variation

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

Ticket: 4810

diff --git a/src/decode-pppoe.c b/src/decode-pppoe.c
index 757a276211cfc731361926f914973151f95d1d23..1030552a8599a84b7e48a41bbb58d21971b2268e 100644 (file)
--- a/src/decode-pppoe.c
+++ b/src/decode-pppoe.c
@@ -38,7 +38,6 @@
 #include "decode-ppp.h"
 #include "decode-pppoe.h"
 #include "decode-events.h"
-
 #include "flow.h"
 
 #include "util-validate.h"
@@ -135,7 +134,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;
     }
@@ -146,12 +145,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:
@@ -185,46 +203,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);
+                if (IPV4_GET_RAW_VER((IPV4Hdr *)(pkt + pppoesh_len)) == 4) {
+                    DecodeIPV4(tv, dtv, p, pkt + pppoesh_len, len - pppoesh_len);
                 }
                 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);
+                DecodeIPV4(tv, dtv, p, pkt + pppoesh_len, len - pppoesh_len);
                 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);
+                DecodeIPV6(tv, dtv, p, pkt + pppoesh_len, len - pppoesh_len);
                 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;
         }
@@ -447,6 +464,120 @@ static int DecodePPPOEtest06 (void)
     }
     PASS;
 }
+
+/** 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));
+
+    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));
+
+    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));
+
+    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));
+
+    FAIL_IF(ENGINE_ISSET_EVENT(p, PPP_WRONG_TYPE));
+    SCFree(p);
+    PASS;
+}
 #endif /* UNITTESTS */
 
 
@@ -464,6 +595,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)