Quote input strings before constructing a command line (sonic-net#1098)

* Quote input strings before constructing a command line * Fix an input injection with embedded command * Use dash double quotation instead of bash $'string' style quotation, because system() and popen() will use `sh` internally and `sh` is symlink of `dash` on Debian
lguohan · Oct 30, 2019 · d4ccdc3 · d4ccdc3
1 parent 5516ec4
commit d4ccdc3
Show file tree

Hide file tree

Showing 11 changed files with 158 additions and 103 deletions.
diff --git a/cfgmgr/intfmgr.cpp b/cfgmgr/intfmgr.cpp
@@ -41,14 +41,14 @@ void IntfMgr::setIntfIp(const string &alias, const string &opCmd,
     if (ipPrefix.isV4())
     {
         (prefixLen < 31) ?
-        (cmd << IP_CMD << " address " << opCmd << " " << ipPrefixStr << " broadcast " << broadcastIpStr <<" dev " << alias) :
-        (cmd << IP_CMD << " address " << opCmd << " " << ipPrefixStr << " dev " << alias);
+        (cmd << IP_CMD << " address " << shellquote(opCmd) << " " << shellquote(ipPrefixStr) << " broadcast " << shellquote(broadcastIpStr) <<" dev " << shellquote(alias)) :
+        (cmd << IP_CMD << " address " << shellquote(opCmd) << " " << shellquote(ipPrefixStr) << " dev " << shellquote(alias));
     }
     else
     {
         (prefixLen < 127) ?
-        (cmd << IP_CMD << " -6 address " << opCmd << " " << ipPrefixStr << " broadcast " << broadcastIpStr << " dev " << alias) :
-        (cmd << IP_CMD << " -6 address " << opCmd << " " << ipPrefixStr << " dev " << alias);
+        (cmd << IP_CMD << " -6 address " << shellquote(opCmd) << " " << shellquote(ipPrefixStr) << " broadcast " << shellquote(broadcastIpStr) << " dev " << shellquote(alias)) :
+        (cmd << IP_CMD << " -6 address " << shellquote(opCmd) << " " << shellquote(ipPrefixStr) << " dev " << shellquote(alias));
     }
 
     int ret = swss::exec(cmd.str(), res);
@@ -65,11 +65,11 @@ void IntfMgr::setIntfVrf(const string &alias, const string vrfName)
 
     if (!vrfName.empty())
     {
-        cmd << IP_CMD << " link set " << alias << " master " << vrfName;
+        cmd << IP_CMD << " link set " << shellquote(alias) << " master " << shellquote(vrfName);
     }
     else
     {
-        cmd << IP_CMD << " link set " << alias << " nomaster";
+        cmd << IP_CMD << " link set " << shellquote(alias) << " nomaster";
     }
     EXEC_WITH_ERROR_THROW(cmd.str(), res);
 }

diff --git a/cfgmgr/portmgr.cpp b/cfgmgr/portmgr.cpp
@@ -25,7 +25,7 @@ bool PortMgr::setPortMtu(const string &alias, const string &mtu)
     string res;
 
     // ip link set dev <port_name> mtu <mtu>
-    cmd << IP_CMD << " link set dev " << alias << " mtu " << mtu;
+    cmd << IP_CMD << " link set dev " << shellquote(alias) << " mtu " << shellquote(mtu);
     EXEC_WITH_ERROR_THROW(cmd.str(), res);
 
     // Set the port MTU in application database to update both
@@ -44,7 +44,7 @@ bool PortMgr::setPortAdminStatus(const string &alias, const bool up)
     string res;
 
     // ip link set dev <port_name> [up|down]
-    cmd << IP_CMD << " link set dev " << alias << (up ? " up" : " down");
+    cmd << IP_CMD << " link set dev " << shellquote(alias) << (up ? " up" : " down");
     EXEC_WITH_ERROR_THROW(cmd.str(), res);
 
     vector<FieldValueTuple> fvs;

diff --git a/cfgmgr/shellcmd.h b/cfgmgr/shellcmd.h
@@ -1,6 +1,9 @@
 #ifndef __SHELLCMD__
 #define __SHELLCMD__
 
+#include <iomanip>
+#include <regex>
+
 #define IP_CMD               "/sbin/ip"
 #define BRIDGE_CMD           "/sbin/bridge"
 #define BRCTL_CMD            "/sbin/brctl"
@@ -18,4 +21,10 @@
     }                                           \
 })
 
+static inline std::string shellquote(const std::string& str)
+{
+    static const std::regex re("([$`\"\\\n])");
+    return "\"" + std::regex_replace(str, re, "\\$1") + "\"";
+}
+
 #endif /* __SHELLCMD__ */
diff --git a/cfgmgr/teammgr.cpp b/cfgmgr/teammgr.cpp
@@ -319,7 +319,7 @@ bool TeamMgr::setLagAdminStatus(const string &alias, const string &admin_status)
     string res;
 
     // ip link set dev <port_channel_name> [up|down]
-    cmd << IP_CMD << " link set dev " << alias << " " << admin_status;
+    cmd << IP_CMD << " link set dev " << shellquote(alias) << " " << shellquote(admin_status);
     EXEC_WITH_ERROR_THROW(cmd.str(), res);
 
     SWSS_LOG_NOTICE("Set port channel %s admin status to %s",
@@ -336,7 +336,7 @@ bool TeamMgr::setLagMtu(const string &alias, const string &mtu)
     string res;
 
     // ip link set dev <port_channel_name> mtu <mtu_value>
-    cmd << IP_CMD << " link set dev " << alias << " mtu " << mtu;
+    cmd << IP_CMD << " link set dev " << shellquote(alias) << " mtu " << shellquote(mtu);
     EXEC_WITH_ERROR_THROW(cmd.str(), res);
 
     vector<FieldValueTuple> fvs;
@@ -423,7 +423,7 @@ bool TeamMgr::removeLag(const string &alias)
     stringstream cmd;
     string res;
 
-    cmd << TEAMD_CMD << " -k -t " << alias;
+    cmd << TEAMD_CMD << " -k -t " << shellquote(alias);
     EXEC_WITH_ERROR_THROW(cmd.str(), res);
 
     SWSS_LOG_NOTICE("Stop port channel %s", alias.c_str());
@@ -451,8 +451,8 @@ task_process_status TeamMgr::addLagMember(const string &lag, const string &membe
     // Set admin down LAG member (required by teamd) and enslave it
     // ip link set dev <member> down;
     // teamdctl <port_channel_name> port add <member>;
-    cmd << IP_CMD << " link set dev " << member << " down; ";
-    cmd << TEAMDCTL_CMD << " " << lag << " port add " << member;
+    cmd << IP_CMD << " link set dev " << shellquote(member) << " down; ";
+    cmd << TEAMDCTL_CMD << " " << shellquote(lag) << " port add " << shellquote(member);
 
     if (exec(cmd.str(), res) != 0)
     {
@@ -505,7 +505,7 @@ task_process_status TeamMgr::addLagMember(const string &lag, const string &membe
 
     // ip link set dev <member> [up|down]
     cmd.str(string());
-    cmd << IP_CMD << " link set dev " << member << " " << admin_status;
+    cmd << IP_CMD << " link set dev " << shellquote(member) << " " << shellquote(admin_status);
     EXEC_WITH_ERROR_THROW(cmd.str(), res);
 
     fvs.clear();
@@ -550,8 +550,8 @@ bool TeamMgr::removeLagMember(const string &lag, const string &member)
 
     // ip link set dev <port_name> [up|down];
     // ip link set dev <port_name> mtu
-    cmd << IP_CMD << " link set dev " << member << " " << admin_status << "; ";
-    cmd << IP_CMD << " link set dev " << member << " mtu " << mtu;
+    cmd << IP_CMD << " link set dev " << shellquote(member) << " " << shellquote(admin_status) << "; ";
+    cmd << IP_CMD << " link set dev " << shellquote(member) << " mtu " << shellquote(mtu);
 
     EXEC_WITH_ERROR_THROW(cmd.str(), res);
     fvs.clear();

diff --git a/cfgmgr/vlanmgr.cpp b/cfgmgr/vlanmgr.cpp
@@ -134,11 +134,11 @@ bool VlanMgr::setHostVlanAdminState(int vlan_id, const string &admin_status)
 
     // The command should be generated as:
     // /sbin/ip link set Vlan{{vlan_id}} {{admin_status}}
-    const std::string cmds = std::string("")
-      + IP_CMD + " link set " + VLAN_PREFIX + std::to_string(vlan_id) + " " + admin_status;
+    ostringstream cmds;
+    cmds << IP_CMD " link set " VLAN_PREFIX + std::to_string(vlan_id) + " " << shellquote(admin_status);
 
     std::string res;
-    EXEC_WITH_ERROR_THROW(cmds, res);
+    EXEC_WITH_ERROR_THROW(cmds.str(), res);
 
     return true;
 }
@@ -177,14 +177,14 @@ bool VlanMgr::addHostVlanMember(int vlan_id, const string &port_alias, const str
     // /bin/bash -c "/sbin/ip link set {{port_alias}} master Bridge &&
     //               /sbin/bridge vlan del vid 1 dev {{ port_alias }} &&
     //               /sbin/bridge vlan add vid {{vlan_id}} dev {{port_alias}} {{tagging_mode}}"
-    const std::string cmds = std::string("")
-      + BASH_CMD + " -c \""
-      + IP_CMD + " link set " + port_alias + " master " + DOT1Q_BRIDGE_NAME + " && "
-      + BRIDGE_CMD + " vlan del vid " + DEFAULT_VLAN_ID + " dev " + port_alias + " && "
-      + BRIDGE_CMD + " vlan add vid " + std::to_string(vlan_id) + " dev " + port_alias + " " + tagging_cmd + "\"";
+    ostringstream cmds, inner;
+    inner << IP_CMD " link set " << shellquote(port_alias) << " master " DOT1Q_BRIDGE_NAME " && "
+      BRIDGE_CMD " vlan del vid " DEFAULT_VLAN_ID " dev " << shellquote(port_alias) << " && "
+      BRIDGE_CMD " vlan add vid " + std::to_string(vlan_id) + " dev " << shellquote(port_alias) << " " + tagging_cmd;
+    cmds << BASH_CMD " -c " << shellquote(inner.str());
 
     std::string res;
-    EXEC_WITH_ERROR_THROW(cmds, res);
+    EXEC_WITH_ERROR_THROW(cmds.str(), res);
 
     return true;
 }
@@ -202,17 +202,17 @@ bool VlanMgr::removeHostVlanMember(int vlan_id, const string &port_alias)
     //               else exit $ret; fi )'
 
     // When port is not member of any VLAN, it shall be detached from Dot1Q bridge!
-    const std::string cmds = std::string("")
-      + BASH_CMD + " -c \'"
-      + BRIDGE_CMD + " vlan del vid " + std::to_string(vlan_id) + " dev " + port_alias + " && ( "
-      + BRIDGE_CMD + " vlan show dev " + port_alias + " | "
-      + GREP_CMD + " -q None; ret=$?; if [ $ret -eq 0 ]; then "
-      + IP_CMD + " link set " + port_alias + " nomaster; "
-      + "elif [ $ret -eq 1 ]; then exit 0; "
-      + "else exit $ret; fi )\'";
+    ostringstream cmds, inner;
+    inner << BRIDGE_CMD " vlan del vid " + std::to_string(vlan_id) + " dev " << shellquote(port_alias) << " && ( "
+      BRIDGE_CMD " vlan show dev " << shellquote(port_alias) << " | "
+      GREP_CMD " -q None; ret=$?; if [ $ret -eq 0 ]; then "
+      IP_CMD " link set " << shellquote(port_alias) << " nomaster; "
+      "elif [ $ret -eq 1 ]; then exit 0; "
+      "else exit $ret; fi )";
+    cmds << BASH_CMD " -c " << shellquote(cmds.str());
 
     std::string res;
-    EXEC_WITH_ERROR_THROW(cmds, res);
+    EXEC_WITH_ERROR_THROW(cmds.str(), res);
 
     return true;
 }

diff --git a/cfgmgr/vrfmgr.cpp b/cfgmgr/vrfmgr.cpp
@@ -99,7 +99,7 @@ bool VrfMgr::delLink(const string& vrfName)
         return false;
     }
 
-    cmd << IP_CMD << " link del " << vrfName;
+    cmd << IP_CMD << " link del " << shellquote(vrfName);
     EXEC_WITH_ERROR_THROW(cmd.str(), res);
 
     recycleTable(m_vrfTableMap[vrfName]);
@@ -126,14 +126,14 @@ bool VrfMgr::setLink(const string& vrfName)
         return false;
     }
 
-    cmd << IP_CMD << " link add " << vrfName << " type vrf table " << table;
+    cmd << IP_CMD << " link add " << shellquote(vrfName) << " type vrf table " << table;
     EXEC_WITH_ERROR_THROW(cmd.str(), res);
 
     m_vrfTableMap.emplace(vrfName, table);
 
     cmd.str("");
     cmd.clear();
-    cmd << IP_CMD << " link set " << vrfName << " up";
+    cmd << IP_CMD << " link set " << shellquote(vrfName) << " up";
     EXEC_WITH_ERROR_THROW(cmd.str(), res);
 
     return true;