776682 - Signaling code - strcpy and strncpy should be replaced with safe equivalents.

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Ethan Hugg [:ehugg]]

Signaling code has several instances of strcpy, some of which are probably dangerous.  I propose replacing them with sstrncpy(), the version from CPR which will not overwrite and guarantees a NULL at the end.   Also we should incorporate PR_ASSERT into the SIPCC code and start asserting at times where truncating is not desired for sstrncpy or where memcpy would overwrite its bounds.

Comment 1

[Ethan Hugg [:ehugg]]

Attachment: Signaling code needs strcpy calls replaced and asserts added before some sstrncpy and memcpy calls

Comment 2

[Ethan Hugg [:ehugg]]

Attachment: Signaling code needs strcpy calls replaced and asserts added before some sstrncpy and memcpy calls

Comment 3

[Ethan Hugg [:ehugg]]

Attachment: Signaling code needs strcpy calls replaced and asserts added before some sstrncpy and memcpy calls

Comment 4

[Ethan Hugg [:ehugg]]

This patch is getting big so I decided to stop here.

I replaced all instances of strcpy and strncpy with sstrncpy which is the version used in most of SIPCC.  It guarantees a NULL at the end.  The exceptions are where strcpy/strncpy is used inside another string utility.  Some of the changes are in code that I think we do not use, but I changed them anyway to make it easy to see if new changes conform.
In ccapi.c if the peer-supplied SDP is too large we do not assert, but log an error and return similarly to the other error cases in those functions.
I made PR_ASSERT usable in SIPCC code and added it a couple places, like to checks of PC_HANDLE_SIZE
I replaced 'fname' with __FUNCTION__, ccapi.c and VcmSIPCCBinding.cpp

Things we could continue with by making this patch bigger or in another patch:

We could do the same for strcat/strncat as there is a sstrncat used in SIPCC
We could remove more or all cases of 'fname' usage.
We could consolidate the identical code in CPR that does sstrncpy and a few other string functions.  Now that CPR does only linux/darwin/win32 this kind of thing could be done in a bunch of places.

Comment 5

[enda mannion (:emannion)]

Comment on attachment 645848 [details] [diff] [review]
Signaling code needs strcpy calls replaced and asserts added before some sstrncpy and memcpy calls

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

I added a token comment.

::: media/webrtc/signaling/src/sipcc/core/common/config_parser.c
@@ +572,5 @@
>          if (i > MAX_CFG_VERSION_STAMP_LEN) {
>              CONFIG_ERROR(CFG_F_PREFIX "config version %d, bigger than allocated space %d\n", "config_parser_element", i, MAX_CFG_VERSION_STAMP_LEN);
>              i = MAX_CFG_VERSION_STAMP_LEN;
>          }
> +        sstrncpy(g_cfg_version_stamp, "1284570837-bbc096ed-7392-427d-9694-5ce49d5c3acb", i);

My guess is that we do not use this g_cfg_version_stamp variable and that it can be removed altogether.

Comment 6

[Ethan Hugg [:ehugg]]

Re-scoping this bug to just be strcpy and strncpy.  Making new bugs for strcat, strncat, _snprintf, strtok and strtok_r

Comment 7

[Ethan Hugg [:ehugg]]

Attachment: replace instances of strcpy with sstrncpy

Comment 8

[Ethan Hugg [:ehugg]]

Comment on attachment 658282 [details] [diff] [review]
replace instances of strcpy with sstrncpy


This patch removes all instances of strcpy with the exception of a few in other string utils.

I also removed cpr_strcpy and an unused function platGetDefaultGW

I will follow with another for strncpy which will be mostly straight replacement with sstrncpy.

Comment 9

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

Comment on attachment 658282 [details] [diff] [review]
replace instances of strcpy with sstrncpy

media/webrtc/signaling/src/sipcc/core/common/config_api.c

      strncpy(buf, "**********", MAX_CONFIG_VAL_PRINT_LEN);

That should be sstrncpy(......, len);

media/webrtc/signaling/src/sipcc/core/sipstack/ccsip_core.c

-    strcpy(dupCCB->sipCallID, origCCB->sipCallID);
+    sstrncpy(dupCCB->sipCallID, origCCB->sipCallID, MAX_SIP_CALL_ID);

sizeof(dupCCB->sipCallID) instead of a constant where possible, especially if the buffer size is set far away from this usage.  That way if it ever gets changed to a different #define this will still be correct.

r+ with those nits addressed

Comment 10

[Ethan Hugg [:ehugg]]

Attachment: replace instances of strcpy with sstrncpy

Comment 11

[Ethan Hugg [:ehugg]]

Attachment: replace instances of strcpy with sstrncpy

Comment 12

[Ethan Hugg [:ehugg]]

Comment on attachment 658385 [details] [diff] [review]
replace instances of strcpy with sstrncpy


Pushed to Alder - http://hg.mozilla.org/projects/alder/rev/5dddb9b77d79

Comment 13

[Ethan Hugg [:ehugg]]

Attachment: replace instances of strncpy with sstrncpy

Comment 14

[Ethan Hugg [:ehugg]]

Comment on attachment 658645 [details] [diff] [review]
replace instances of strncpy with sstrncpy


No more calls to strncpy exist in signaling with this patch.

Also removed some unused code in sipcc/plat.  Signaling.gyp has changed so it'll need a 'touch configure.in'.

Comment 15

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

Comment on attachment 658645 [details] [diff] [review]
replace instances of strncpy with sstrncpy

media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_config.c
-    strncpy(g_dev_name, device_name, 64);
+    sstrncpy(g_dev_name, device_name, 64);

Can we change 64 to sizeof(something)?  

     if (apply_config == FALSE) {
         memset(g_cfg_version_stamp, 0, sizeof(g_cfg_version_stamp));
         i = strlen("1284570837-bbc096ed-7392-427d-9694-5ce49d5c3acb");
         if (i > MAX_CFG_VERSION_STAMP_LEN) {
             CONFIG_ERROR(CFG_F_PREFIX "config version %d, bigger than allocated space %d\n", "config_parser_element", i, MAX_CFG_VERSION_STAMP_LEN);
             i = MAX_CFG_VERSION_STAMP_LEN;
         }
-        strncpy(g_cfg_version_stamp, "1284570837-bbc096ed-7392-427d-9694-5ce49d5c3acb", i);
+        sstrncpy(g_cfg_version_stamp, "1284570837-bbc096ed-7392-427d-9694-5ce49d5c3acb", i);

The size stuff here is kinda odd.  Why isn't it just sizeof(g_cfg_version_stamp) instead of i?

     dcb->ice_ufrag = (char *)cpr_malloc(strlen(ufrag) + 1);
     if (!dcb->ice_ufrag)
     	return SM_RC_END;
 
-    strncpy(dcb->ice_ufrag, ufrag, strlen(ufrag) + 1);
+    sstrncpy(dcb->ice_ufrag, ufrag, strlen(ufrag) + 1);

Can't we use strdup() here?  (maybe not if it's using cpr_malloc())

                 join->from_tag = (char *) cpr_calloc(1, params-param_value + 1);
                 if (join->from_tag) {
-                    strncpy(join->from_tag, param_value, params-param_value);
+                    sstrncpy(join->from_tag, param_value, params-param_value);

Is this correct?  I'd think you'd end with with two 0 bytes at the end

                 join->to_tag = (char *) cpr_calloc(1, params-param_value + 1);
                 if (join->to_tag) {
-                    strncpy(join->to_tag, param_value, params-param_value);
+                    sstrncpy(join->to_tag, param_value, params-param_value);
                 }

Ditto.  With strncpy(), I can see the combination of calloc and the length mismatch making strncpy safe; with sstrncpy probably the length for the copy needs to be bumped.

r=jesup with those nits and issues addressed.

Comment 16

[Ethan Hugg [:ehugg]]

>media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_config.c
>-    strncpy(g_dev_name, device_name, 64);
>+    sstrncpy(g_dev_name, device_name, 64);
>
>Can we change 64 to sizeof(something)?  

Yes, I'll move the extern decl into a header so the size can be shared with ccapi_service.   I should probably try to get rid of these globals, but I'm not sure yet that I wouldn't break the SIP functionality.

I'll fix the other parts as suggested.

Comment 17

[Ethan Hugg [:ehugg]]

Attachment: replace instances of strncpy with sstrncpy

Comment 18

[Ethan Hugg [:ehugg]]

Comment on attachment 658896 [details] [diff] [review]
replace instances of strncpy with sstrncpy


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