Last Comment Bug 776682 - Signaling code - strcpy and strncpy should be replaced with safe equivalents.
: Signaling code - strcpy and strncpy should be replaced with safe equivalents.
Status: RESOLVED FIXED
[WebRTC], [blocking-webrtc+], [qa-]
:
Product: Core
Classification: Components
Component: WebRTC: Signaling (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Ethan Hugg [:ehugg]
: Jason Smith [:jsmith]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-23 13:21 PDT by Ethan Hugg [:ehugg]
Modified: 2012-10-10 18:19 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Signaling code needs strcpy calls replaced and asserts added before some sstrncpy and memcpy calls (68.71 KB, patch)
2012-07-23 15:16 PDT, Ethan Hugg [:ehugg]
no flags Details | Diff | Splinter Review
Signaling code needs strcpy calls replaced and asserts added before some sstrncpy and memcpy calls (129.33 KB, patch)
2012-07-25 11:14 PDT, Ethan Hugg [:ehugg]
no flags Details | Diff | Splinter Review
Signaling code needs strcpy calls replaced and asserts added before some sstrncpy and memcpy calls (129.33 KB, patch)
2012-07-25 12:11 PDT, Ethan Hugg [:ehugg]
no flags Details | Diff | Splinter Review
replace instances of strcpy with sstrncpy (31.01 KB, patch)
2012-09-04 15:52 PDT, Ethan Hugg [:ehugg]
rjesup: feedback+
Details | Diff | Splinter Review
replace instances of strcpy with sstrncpy (32.04 KB, patch)
2012-09-04 23:00 PDT, Ethan Hugg [:ehugg]
no flags Details | Diff | Splinter Review
replace instances of strcpy with sstrncpy (32.34 KB, patch)
2012-09-04 23:23 PDT, Ethan Hugg [:ehugg]
no flags Details | Diff | Splinter Review
replace instances of strncpy with sstrncpy (72.25 KB, patch)
2012-09-05 15:08 PDT, Ethan Hugg [:ehugg]
rjesup: review+
Details | Diff | Splinter Review
replace instances of strncpy with sstrncpy (76.69 KB, patch)
2012-09-06 08:53 PDT, Ethan Hugg [:ehugg]
no flags Details | Diff | Splinter Review

Description Ethan Hugg [:ehugg] 2012-07-23 13:21:33 PDT
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] 2012-07-23 15:16:36 PDT
Created attachment 645094 [details] [diff] [review]
Signaling code needs strcpy calls replaced and asserts added before some sstrncpy and memcpy calls
Comment 2 Ethan Hugg [:ehugg] 2012-07-25 11:14:14 PDT
Created attachment 645822 [details] [diff] [review]
Signaling code needs strcpy calls replaced and asserts added before some sstrncpy and memcpy calls
Comment 3 Ethan Hugg [:ehugg] 2012-07-25 12:11:58 PDT
Created attachment 645848 [details] [diff] [review]
Signaling code needs strcpy calls replaced and asserts added before some sstrncpy and memcpy calls
Comment 4 Ethan Hugg [:ehugg] 2012-07-25 12:19:21 PDT
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) 2012-07-25 15:42:43 PDT
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] 2012-08-23 09:24:21 PDT
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] 2012-09-04 15:52:30 PDT
Created attachment 658282 [details] [diff] [review]
replace instances of strcpy with sstrncpy
Comment 8 Ethan Hugg [:ehugg] 2012-09-04 16:04:24 PDT
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] 2012-09-04 22:13:51 PDT
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] 2012-09-04 23:00:33 PDT
Created attachment 658380 [details] [diff] [review]
replace instances of strcpy with sstrncpy
Comment 11 Ethan Hugg [:ehugg] 2012-09-04 23:23:17 PDT
Created attachment 658385 [details] [diff] [review]
replace instances of strcpy with sstrncpy
Comment 12 Ethan Hugg [:ehugg] 2012-09-04 23:37:06 PDT
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] 2012-09-05 15:08:30 PDT
Created attachment 658645 [details] [diff] [review]
replace instances of strncpy with sstrncpy
Comment 14 Ethan Hugg [:ehugg] 2012-09-05 15:14:29 PDT
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] 2012-09-05 21:17:26 PDT
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] 2012-09-06 07:56:53 PDT
>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] 2012-09-06 08:53:21 PDT
Created attachment 658896 [details] [diff] [review]
replace instances of strncpy with sstrncpy
Comment 18 Ethan Hugg [:ehugg] 2012-09-06 11:17:32 PDT
Comment on attachment 658896 [details] [diff] [review]
replace instances of strncpy with sstrncpy


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

Note You need to log in before you can comment on or make changes to this bug.