sigcomp: Add buffer check to STATE-ACCESS

Two conditions were not checked, state_length == 0 && state_begin != 0
and the boundaries of the state buffer. The former is not a big deal,
but the second issue causes a buffer overrun (detected by ASAN).

The buffer size is supposed to be stored in the state buffer, that was
not the case for the initial two SIP SDP and Presence state buffers.
Fix a typo for presence_buf zero-ing while at it.

Bug: 9601
Change-Id: I41dde83185da60b670cca010ecc7b2a2aaaedeb9
Reviewed-on: https://code.wireshark.org/review/1529
Reviewed-by: Anders Broman <a.broman58@gmail.com>

diff --git a/epan/sigcomp-udvm.c b/epan/sigcomp-udvm.c
index bd14dab9002593d86b2c2323568d9e38b69090a3..2f5d1017641e663eefb54fc80edccd75b13b54b3 100644 (file)
--- a/epan/sigcomp-udvm.c
+++ b/epan/sigcomp-udvm.c
@@ -110,6 +110,7 @@ const value_string result_code_vals[] = {
        {14,    "Input bytes requested beyond end of message" },
        {15,    "Maximum number of UDVM cycles reached" },
        {16,    "UDVM stack underflow" },
+       {17,    "state_length is 0, but state_begin is non-zero" },
        { 255,  "This branch isn't coded yet" },
        { 0,    NULL }
 };

diff --git a/epan/sigcomp_state_hdlr.c b/epan/sigcomp_state_hdlr.c
index ce355dce45a92344ae608751b9256c68a06f4fa1..cdb8782eae59812328e6e393f33a5f6cde4a093c 100644 (file)
--- a/epan/sigcomp_state_hdlr.c
+++ b/epan/sigcomp_state_hdlr.c
@@ -634,6 +634,8 @@ sigcomp_init_udvm(void){
         * Debug        g_warning("Sigcomp init: Storing partial state =%s",partial_state_str);
         */
        memset(sip_sdp_buff, 0, 8);
+       sip_sdp_buff[0] = SIP_SDP_STATE_LENGTH >> 8;
+       sip_sdp_buff[1] = SIP_SDP_STATE_LENGTH & 0xff;
        i = 0;
        while ( i < SIP_SDP_STATE_LENGTH ){
                sip_sdp_buff[i+8] = sip_sdp_static_dictionaty_for_sigcomp[i];
@@ -654,7 +656,9 @@ sigcomp_init_udvm(void){
 
        partial_state_str = bytes_to_ep_str(presence_state_identifier, 6);
 
-       memset(sip_sdp_buff, 0, 8);
+       memset(presence_buff, 0, 8);
+       presence_buff[0] = PRESENCE_STATE_LENGTH >> 8;
+       presence_buff[1] = PRESENCE_STATE_LENGTH & 0xff;
        i = 0;
        while ( i < PRESENCE_STATE_LENGTH ){
                presence_buff[i+8] = presence_static_dictionary_for_sigcomp[i];
@@ -672,6 +676,7 @@ int udvm_state_access(tvbuff_t *tvb, proto_tree *tree,guint8 *buff,guint16 p_id_
        int                     result_code = 0;
        guint32         n;
        guint16         k;
+       guint16         buf_size_real;
        guint16         byte_copy_right;
        guint16         byte_copy_left;
        char            partial_state[STATE_BUFFER_SIZE]; /* Size is 6 - 20 */
@@ -730,10 +735,7 @@ int udvm_state_access(tvbuff_t *tvb, proto_tree *tree,guint8 *buff,guint16 p_id_
         * If k = byte_copy_right then set n := byte_copy_left, else set n := k
         *
         */
-       /*
-       if ( ( state_begin + state_length ) > sip_sdp_state_length )
-               return 3;
-               */
+
        /*
         * buff                                 = Where "state" will be stored
         * p_id_start                   = Partial state identifier start pos in the buffer(buff)
@@ -745,6 +747,8 @@ int udvm_state_access(tvbuff_t *tvb, proto_tree *tree,guint8 *buff,guint16 p_id_
         * FALSE                                = Indicates that state_* is in the stored state
         */
 
+       buf_size_real = (state_buff[0] << 8) | state_buff[1];
+
        /*
         * The value of
         * state_length MUST be taken from the returned item of state in the
@@ -752,9 +756,8 @@ int udvm_state_access(tvbuff_t *tvb, proto_tree *tree,guint8 *buff,guint16 p_id_
         *
         * The same is true of state_address, state_instruction.
         */
-       if ( *state_length == 0 ){
-               *state_length = state_buff[0] << 8;
-               *state_length = *state_length | state_buff[1];
+       if (*state_length == 0) {
+               *state_length = buf_size_real;
        }
        if ( *state_address == 0 ){
                *state_address = state_buff[2] << 8;
@@ -765,6 +768,22 @@ int udvm_state_access(tvbuff_t *tvb, proto_tree *tree,guint8 *buff,guint16 p_id_
                *state_instruction = *state_instruction | state_buff[5];
        }
 
+       /*
+        * Decompression failure occurs if bytes are copied from beyond the end of
+        * the state_value.
+        */
+       if ((state_begin + *state_length) > buf_size_real) {
+               return 3;
+       }
+
+       /*
+        * Note that decompression failure will always occur if the state_length
+        * operand is set to 0 but the state_begin operand is non-zero.
+        */
+       if (*state_length == 0 && state_begin != 0) {
+               return 17;
+       }
+
        n = state_begin + 8;
        k = *state_address;