785109 - Signaling code - strcat and strncat should be replaced with safe equivalents

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Ethan Hugg [:ehugg]]

sstrncat is a safe version that is used throughout most of the signaling code.

Comment 1

[Ethan Hugg [:ehugg]]

Attachment: strcat replaced with sstrncat in signaling code

Comment 2

[Ethan Hugg [:ehugg]]

Comment on attachment 659298 [details] [diff] [review]
strcat replaced with sstrncat in signaling code


This removes the few places where strcat was used and simplifies some of the code around it.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 659298 [details] [diff] [review]
strcat replaced with sstrncat in signaling code

                 if (offset) {
                     sstrncpy(new_buf, hdr_start, offset - hdr_start);
                     new_buf[offset - hdr_start] = '\0';
                 } else {
                     sstrncpy(new_buf, hdr_start, new_buf_len);
                 }
-                strcat(new_buf, ";");
-                strcat(new_buf, VIA_RECEIVED);
-                strcat(new_buf, "=");
-                strcat(new_buf, dotted_ip);
+                sstrncat(new_buf, ";", new_buf_len - strlen(new_buf));
+                sstrncat(new_buf, VIA_RECEIVED, new_buf_len - strlen(new_buf));
+                sstrncat(new_buf, "=", new_buf_len - strlen(new_buf));
+                sstrncat(new_buf, dotted_ip, new_buf_len - strlen(new_buf));
                 if (offset) {
-                    strcat(new_buf, offset);
+                    sstrncat(new_buf, offset, new_buf_len - strlen(new_buf));
                 }

How about:

if (offset) {
  snprintf(new_buf,new_buf_len,"%*s;%s=%s%s",offset - hdr_start,hdr_start,VIA_RECEIVED,dotted_ip,offset);
} else {
  snprintf(new_buf,new_buf_len,"%s;%s=%s",hdr_start,VIA_RECEIVED,dotted_ip);
}

Comment 4

[Ethan Hugg [:ehugg]]

(In reply to Randell Jesup [:jesup] from comment #3)
> Comment on attachment 659298 [details] [diff] [review]
> strcat replaced with sstrncat in signaling code
> 
>                  if (offset) {
>                      sstrncpy(new_buf, hdr_start, offset - hdr_start);
>                      new_buf[offset - hdr_start] = '\0';
>                  } else {
>                      sstrncpy(new_buf, hdr_start, new_buf_len);
>                  }
> -                strcat(new_buf, ";");
> -                strcat(new_buf, VIA_RECEIVED);
> -                strcat(new_buf, "=");
> -                strcat(new_buf, dotted_ip);
> +                sstrncat(new_buf, ";", new_buf_len - strlen(new_buf));
> +                sstrncat(new_buf, VIA_RECEIVED, new_buf_len -
> strlen(new_buf));
> +                sstrncat(new_buf, "=", new_buf_len - strlen(new_buf));
> +                sstrncat(new_buf, dotted_ip, new_buf_len - strlen(new_buf));
>                  if (offset) {
> -                    strcat(new_buf, offset);
> +                    sstrncat(new_buf, offset, new_buf_len -
> strlen(new_buf));
>                  }
> 
> How about:
> 
> if (offset) {
>   snprintf(new_buf,new_buf_len,"%*s;%s=%s%s",offset -
> hdr_start,hdr_start,VIA_RECEIVED,dotted_ip,offset);
> } else {
>   snprintf(new_buf,new_buf_len,"%s;%s=%s",hdr_start,VIA_RECEIVED,dotted_ip);
> }

Yes, that will be much clearer, I'll put that in.

Comment 5

[Ethan Hugg [:ehugg]]

Attachment: strcat replaced with sstrncat in signaling code f=jesup

Comment 6

[Ethan Hugg [:ehugg]]

Attachment: strcat replaced with sstrncat in signaling code f=jesup

Comment 7

[Ethan Hugg [:ehugg]]

Comment on attachment 659589 [details] [diff] [review]
strcat replaced with sstrncat in signaling code f=jesup


Pushed to Alder - http://hg.mozilla.org/projects/alder/rev/1c3bc2d09889

Comment 8

[Ethan Hugg [:ehugg]]

Attachment: replace instances of strncat

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 662794 [details] [diff] [review]
replace instances of strncat

Review of attachment 662794 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/sipcc/core/sipstack/ccsip_core.c
@@ +2816,5 @@
>          line_t  previous_call_index = 0;
>  
>          memset(tempreplace, 0, MAX_SIP_URL_LENGTH);
>          sstrncpy(tempreplace, "Replaces=", sizeof(tempreplace));
> +        sstrncat(tempreplace, replaceshdr, (sizeof(tempreplace) - 9));

Would prefer sizeof(something) to '9'...
(sizeof("Replaces=") I assume)

@@ +6036,5 @@
>          char tempreplace[MAX_SIP_URL_LENGTH];
>  
>          memset(tempreplace, 0, MAX_SIP_URL_LENGTH);
>          sstrncpy(tempreplace, "Replaces=", sizeof(tempreplace));
> +        sstrncat(tempreplace, referto->sip_replaces_hdr, (sizeof(tempreplace) - 9));

ditto

@@ +9593,5 @@
>              sstrncpy(referto, "<sip:", MAX_SIP_URL_LENGTH);
>              len = semi - pTransferNumberString;
>          } else {
>              sstrncpy(referto, "sip:", MAX_SIP_URL_LENGTH);
> +            len = MAX_SIP_URL_LENGTH - 4; /* 4 = sip: */

sizeof("sip:")

::: media/webrtc/signaling/src/sipcc/core/sipstack/ccsip_pmh.c
@@ +5538,5 @@
>  
>      snprintf(joinhdr, MAX_SIP_HEADER_LENGTH, "%s", join->call_id);
>      left = (uint16_t) MAX_SIP_HEADER_LENGTH - strlen(join->call_id);
>      if (join->from_tag && left > 0) {
> +        sstrncat(joinhdr, ";from-tag=", left);

I note these didn't have a size change; were they wrong/dangerous before?

::: media/webrtc/signaling/src/sipcc/core/sipstack/ccsip_publish.c
@@ +434,5 @@
>       * Populate full RURI if it is not yet. Sometimes, applications may only provide user part.
>       */
>      if (pcb_p->full_ruri[0] == 0) {
>          sstrncpy(pcb_p->full_ruri, "sip:", MAX_SIP_URL_LENGTH);
> +        sstrncat(pcb_p->full_ruri, pcb_p->ruri, MAX_SIP_URL_LENGTH - 4);

sizeof("sip:")

::: media/webrtc/signaling/src/sipcc/core/sipstack/ccsip_subsmanager.c
@@ +4169,5 @@
>          sstrncpy(scbp->SubURI, "sip:", MAX_SIP_URL_LENGTH);
>  
>          /* Indialog requests should contain suburi only
>           */
> +        sstrncat(scbp->SubURI, scbp->SubURIOriginal, MAX_SIP_URL_LENGTH - 4);

sizeof("sip:")

::: media/webrtc/signaling/src/sipcc/core/sipstack/ccsip_task.c
@@ +241,5 @@
>                      SIP_PHONE_MODEL_NUMBER_LEN);
>          } else {
>              // Default to 7970
>              CCSIP_DEBUG_ERROR(SIP_F_PREFIX"unknown model,defaulting to model 7970: %s\n", fname, model);
> +            sstrncat(sipHdrUserAgent, CCSIP_SIP_7970_USER_AGENT,

This is just amusing...

Comment 10

[Ethan Hugg [:ehugg]]

>I note these didn't have a size change; were they wrong/dangerous before?

Yes, I found a few others besides the two in lsm.c which specified the whole buffer.  Also some of the ones that did have -1s, didn't have a guarantee that the last byte would be a null I think.


>This is just amusing...

Lots more code to get rid of as you can imagine.  We need to figure out what parts of the unused code we want to work and get some unittests for them, then we can start cutting.

I'll change the magic numbers to sizeof()s and update.

Comment 11

[Ethan Hugg [:ehugg]]

Attachment: replace instances of strncat

Comment 12

[Ethan Hugg [:ehugg]]

Comment on attachment 663591 [details] [diff] [review]
replace instances of strncat


Pushed to Alder - https://hg.mozilla.org/projects/alder/rev/3fe6a9d69f45