RDKCOM-5108 RDKBDEV-2972: use sysctl_iface_set() to write to /proc files #14
Conversation
|
Internal ticket for tracking https://ccp.sys.comcast.net/browse/RDKCOM-5108 |
pradeeptakdas
changed the title
pradeeptakdas
force-pushed
the
aacbef
branch
from
916b3d4 to
d87dc2f
Compare
|
Based on @guto86 input I have requested Security team to review and approve the changes |
|
I added 2 comments to the code that are security-related. Both would be easy to address with simple additional checks. |
Reason for change: This function enhances code reusability, promotes safety through buffer overflow prevention and error handling. Test Procedure: Sanity. Risks: None. Signed-off-by: Andre McCurdy <[email protected]>
Fixed the compilation issue.
| int fd; | ||
|
|
||
| if (ifname) { | ||
| snprintf(buf, sizeof(buf), path, ifname); |
There was a problem hiding this comment.
This code silently truncates the filename if the combined length of path and ifname exceed the size of buf.
This scenario could be detected easily by checking the return value of snprintf. If it is 127 (or greater) the result exactly fills buf or else exceeded it and was truncated.
The safest thing to do in this scenario is log an error message and return -1.
Since interface names can be manipulated through TR-181 parameters, this represents an attack surface, so we should defend against a truncated result, even if we expect it to be rare in practice.
| @@ -1508,13 +1539,13 @@ static INT EthLink_Hal_BridgeConfigBcm(WAN_MODE_BRIDGECFG *pCfg) | |||
| v_secure_system("ip link set %s name %s", pCfg->wanPhyName,ETHWAN_DOCSIS_INF_NAME); | |||
| v_secure_system("brctl addbr %s", pCfg->wanPhyName); | |||
| v_secure_system("brctl addif %s %s", pCfg->wanPhyName,pCfg->ethwan_ifname); | |||
| sysctl_iface_set("/proc/sys/net/ipv6/conf/%s/autoconf", pCfg->ethwan_ifname, "0"); | |||
| sysctl_iface_set("/proc/sys/net/ipv6/conf/%s/disable_ipv6", pCfg->ethwan_ifname, "1"); | |||
There was a problem hiding this comment.
Just an observation, not a bug, ...
Notice that pCfg->ethwan_ifname is a variable that presumably is connected to a TR-181 parameter.
If a hacker somehow changes the interface name to "../../../../../../../any/path/here/filename" ... they can force writing "0" or "1" to an arbitrary file.
This is a rare scenario, but I notice there is no input validation on the interface name. Simply checking for "/" in the interface name might be a sufficient check, since this should never occur with a valid interface name.
|
Let's abandon the change. |
Reason for change:
This function enhances code reusability, promotes safety through buffer overflow prevention and error handling.
Test Procedure: Sanity.
Risks: None.
Signed-off-by: Andre McCurdy [email protected]