Fix the unsafe usage of strncpy in portsorch.cpp (#2110)

stephenxs · web-flow · commit bf4cd4adc066 · 2022-01-18T22:16:46.000+02:00
Originally, strncpy is used in the following way:
strncpy(attr.value.chardata, src_string, sizeof(attr.value.chardata));
where attr.value.chardata is a char array.

However, this is not safe in case strlen(src_string) &gt;= sizeof(attr.value.chardata) because there will no space in attr.value.chardata to store the terminating character.
It will leave the string attr.value.chardata open, the receiver of attr cannot determine the end of the string and suffer buffer overflow.

According to SAI API definition, the actually length of SAI_HOSTIF_ATTR_NAME should be SAI_HOSTIF_NAME_SIZE - 1 which is less than sizeof(attr.value.chardata)`. So a safe way to do it should be:

strncpy(attr.value.chardata, src_string, SAI_HOSTIF_NAME_SIZE);
attr.value.chardata[SAI_HOSTIF_NAME_SIZE - 1] = '\0'

Signed-off-by: Stephen Sun &lt;stephens@nvidia.com&gt;
diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp
@@ -2172,7 +2172,12 @@ bool PortsOrch::createVlanHostIntf(Port& vl, string hostif_name)
     attrs.push_back(attr);
 
     attr.id = SAI_HOSTIF_ATTR_NAME;
-    strncpy(attr.value.chardata, hostif_name.c_str(), sizeof(attr.value.chardata));
+    if (hostif_name.length() >= SAI_HOSTIF_NAME_SIZE)
+    {
+        SWSS_LOG_WARN("Host interface name %s is too long and will be truncated to %d bytes", hostif_name.c_str(), SAI_HOSTIF_NAME_SIZE - 1);
+    }
+    strncpy(attr.value.chardata, hostif_name.c_str(), SAI_HOSTIF_NAME_SIZE);
+    attr.value.chardata[SAI_HOSTIF_NAME_SIZE - 1] = '\0';
     attrs.push_back(attr);
 
     sai_status_t status = sai_hostif_api->create_hostif(&vl.m_vlan_info.host_intf_id, gSwitchId, (uint32_t)attrs.size(), attrs.data());
@@ -4186,6 +4191,11 @@ bool PortsOrch::addHostIntfs(Port &port, string alias, sai_object_id_t &host_int
 
     attr.id = SAI_HOSTIF_ATTR_NAME;
     strncpy((char *)&attr.value.chardata, alias.c_str(), SAI_HOSTIF_NAME_SIZE);
+    if (alias.length() >= SAI_HOSTIF_NAME_SIZE)
+    {
+        SWSS_LOG_WARN("Host interface name %s is too long and will be truncated to %d bytes", alias.c_str(), SAI_HOSTIF_NAME_SIZE - 1);
+    }
+    attr.value.chardata[SAI_HOSTIF_NAME_SIZE - 1] = '\0';
     attrs.push_back(attr);
 
     sai_status_t status = sai_hostif_api->create_hostif(&host_intfs_id, gSwitchId, (uint32_t)attrs.size(), attrs.data());
