Skip to content

Commit d4ccdc3

Browse files
qiluo-msftqiluo-msft
authored
Quote input strings before constructing a command line (#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
1 parent 5516ec4 commit d4ccdc3

11 files changed

+158
-103
lines changed

‎cfgmgr/intfmgr.cpp

+6-6
Original file line numberDiff line numberDiff line change
@@ -41,14 +41,14 @@ void IntfMgr::setIntfIp(const string &alias, const string &opCmd,
4141
if (ipPrefix.isV4())
4242
{
4343
(prefixLen < 31) ?
44-
(cmd << IP_CMD << " address " << opCmd << " " << ipPrefixStr << " broadcast " << broadcastIpStr <<" dev " << alias) :
45-
(cmd << IP_CMD << " address " << opCmd << " " << ipPrefixStr << " dev " << alias);
44+
(cmd << IP_CMD << " address " << shellquote(opCmd) << " " << shellquote(ipPrefixStr) << " broadcast " << shellquote(broadcastIpStr) <<" dev " << shellquote(alias)) :
45+
(cmd << IP_CMD << " address " << shellquote(opCmd) << " " << shellquote(ipPrefixStr) << " dev " << shellquote(alias));
4646
}
4747
else
4848
{
4949
(prefixLen < 127) ?
50-
(cmd << IP_CMD << " -6 address " << opCmd << " " << ipPrefixStr << " broadcast " << broadcastIpStr << " dev " << alias) :
51-
(cmd << IP_CMD << " -6 address " << opCmd << " " << ipPrefixStr << " dev " << alias);
50+
(cmd << IP_CMD << " -6 address " << shellquote(opCmd) << " " << shellquote(ipPrefixStr) << " broadcast " << shellquote(broadcastIpStr) << " dev " << shellquote(alias)) :
51+
(cmd << IP_CMD << " -6 address " << shellquote(opCmd) << " " << shellquote(ipPrefixStr) << " dev " << shellquote(alias));
5252
}
5353

5454
int ret = swss::exec(cmd.str(), res);
@@ -65,11 +65,11 @@ void IntfMgr::setIntfVrf(const string &alias, const string vrfName)
6565

6666
if (!vrfName.empty())
6767
{
68-
cmd << IP_CMD << " link set " << alias << " master " << vrfName;
68+
cmd << IP_CMD << " link set " << shellquote(alias) << " master " << shellquote(vrfName);
6969
}
7070
else
7171
{
72-
cmd << IP_CMD << " link set " << alias << " nomaster";
72+
cmd << IP_CMD << " link set " << shellquote(alias) << " nomaster";
7373
}
7474
EXEC_WITH_ERROR_THROW(cmd.str(), res);
7575
}

‎cfgmgr/portmgr.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ bool PortMgr::setPortMtu(const string &alias, const string &mtu)
2525
string res;
2626

2727
// ip link set dev <port_name> mtu <mtu>
28-
cmd << IP_CMD << " link set dev " << alias << " mtu " << mtu;
28+
cmd << IP_CMD << " link set dev " << shellquote(alias) << " mtu " << shellquote(mtu);
2929
EXEC_WITH_ERROR_THROW(cmd.str(), res);
3030

3131
// Set the port MTU in application database to update both
@@ -44,7 +44,7 @@ bool PortMgr::setPortAdminStatus(const string &alias, const bool up)
4444
string res;
4545

4646
// ip link set dev <port_name> [up|down]
47-
cmd << IP_CMD << " link set dev " << alias << (up ? " up" : " down");
47+
cmd << IP_CMD << " link set dev " << shellquote(alias) << (up ? " up" : " down");
4848
EXEC_WITH_ERROR_THROW(cmd.str(), res);
4949

5050
vector<FieldValueTuple> fvs;

‎cfgmgr/shellcmd.h

+9
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
#ifndef __SHELLCMD__
22
#define __SHELLCMD__
33

4+
#include <iomanip>
5+
#include <regex>
6+
47
#define IP_CMD "/sbin/ip"
58
#define BRIDGE_CMD "/sbin/bridge"
69
#define BRCTL_CMD "/sbin/brctl"
@@ -18,4 +21,10 @@
1821
} \
1922
})
2023

24+
static inline std::string shellquote(const std::string& str)
25+
{
26+
static const std::regex re("([$`\"\\\n])");
27+
return "\"" + std::regex_replace(str, re, "\\$1") + "\"";
28+
}
29+
2130
#endif /* __SHELLCMD__ */

‎cfgmgr/teammgr.cpp

+8-8
Original file line numberDiff line numberDiff line change
@@ -319,7 +319,7 @@ bool TeamMgr::setLagAdminStatus(const string &alias, const string &admin_status)
319319
string res;
320320

321321
// ip link set dev <port_channel_name> [up|down]
322-
cmd << IP_CMD << " link set dev " << alias << " " << admin_status;
322+
cmd << IP_CMD << " link set dev " << shellquote(alias) << " " << shellquote(admin_status);
323323
EXEC_WITH_ERROR_THROW(cmd.str(), res);
324324

325325
SWSS_LOG_NOTICE("Set port channel %s admin status to %s",
@@ -336,7 +336,7 @@ bool TeamMgr::setLagMtu(const string &alias, const string &mtu)
336336
string res;
337337

338338
// ip link set dev <port_channel_name> mtu <mtu_value>
339-
cmd << IP_CMD << " link set dev " << alias << " mtu " << mtu;
339+
cmd << IP_CMD << " link set dev " << shellquote(alias) << " mtu " << shellquote(mtu);
340340
EXEC_WITH_ERROR_THROW(cmd.str(), res);
341341

342342
vector<FieldValueTuple> fvs;
@@ -423,7 +423,7 @@ bool TeamMgr::removeLag(const string &alias)
423423
stringstream cmd;
424424
string res;
425425

426-
cmd << TEAMD_CMD << " -k -t " << alias;
426+
cmd << TEAMD_CMD << " -k -t " << shellquote(alias);
427427
EXEC_WITH_ERROR_THROW(cmd.str(), res);
428428

429429
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
451451
// Set admin down LAG member (required by teamd) and enslave it
452452
// ip link set dev <member> down;
453453
// teamdctl <port_channel_name> port add <member>;
454-
cmd << IP_CMD << " link set dev " << member << " down; ";
455-
cmd << TEAMDCTL_CMD << " " << lag << " port add " << member;
454+
cmd << IP_CMD << " link set dev " << shellquote(member) << " down; ";
455+
cmd << TEAMDCTL_CMD << " " << shellquote(lag) << " port add " << shellquote(member);
456456

457457
if (exec(cmd.str(), res) != 0)
458458
{
@@ -505,7 +505,7 @@ task_process_status TeamMgr::addLagMember(const string &lag, const string &membe
505505

506506
// ip link set dev <member> [up|down]
507507
cmd.str(string());
508-
cmd << IP_CMD << " link set dev " << member << " " << admin_status;
508+
cmd << IP_CMD << " link set dev " << shellquote(member) << " " << shellquote(admin_status);
509509
EXEC_WITH_ERROR_THROW(cmd.str(), res);
510510

511511
fvs.clear();
@@ -550,8 +550,8 @@ bool TeamMgr::removeLagMember(const string &lag, const string &member)
550550

551551
// ip link set dev <port_name> [up|down];
552552
// ip link set dev <port_name> mtu
553-
cmd << IP_CMD << " link set dev " << member << " " << admin_status << "; ";
554-
cmd << IP_CMD << " link set dev " << member << " mtu " << mtu;
553+
cmd << IP_CMD << " link set dev " << shellquote(member) << " " << shellquote(admin_status) << "; ";
554+
cmd << IP_CMD << " link set dev " << shellquote(member) << " mtu " << shellquote(mtu);
555555

556556
EXEC_WITH_ERROR_THROW(cmd.str(), res);
557557
fvs.clear();

‎cfgmgr/vlanmgr.cpp

+18-18
Original file line numberDiff line numberDiff line change
@@ -134,11 +134,11 @@ bool VlanMgr::setHostVlanAdminState(int vlan_id, const string &admin_status)
134134

135135
// The command should be generated as:
136136
// /sbin/ip link set Vlan{{vlan_id}} {{admin_status}}
137-
const std::string cmds = std::string("")
138-
+ IP_CMD + " link set " + VLAN_PREFIX + std::to_string(vlan_id) + " " + admin_status;
137+
ostringstream cmds;
138+
cmds << IP_CMD " link set " VLAN_PREFIX + std::to_string(vlan_id) + " " << shellquote(admin_status);
139139

140140
std::string res;
141-
EXEC_WITH_ERROR_THROW(cmds, res);
141+
EXEC_WITH_ERROR_THROW(cmds.str(), res);
142142

143143
return true;
144144
}
@@ -177,14 +177,14 @@ bool VlanMgr::addHostVlanMember(int vlan_id, const string &port_alias, const str
177177
// /bin/bash -c "/sbin/ip link set {{port_alias}} master Bridge &&
178178
// /sbin/bridge vlan del vid 1 dev {{ port_alias }} &&
179179
// /sbin/bridge vlan add vid {{vlan_id}} dev {{port_alias}} {{tagging_mode}}"
180-
const std::string cmds = std::string("")
181-
+ BASH_CMD + " -c \""
182-
+ IP_CMD + " link set " + port_alias + " master " + DOT1Q_BRIDGE_NAME + " && "
183-
+ BRIDGE_CMD + " vlan del vid " + DEFAULT_VLAN_ID + " dev " + port_alias + " && "
184-
+ BRIDGE_CMD + " vlan add vid " + std::to_string(vlan_id) + " dev " + port_alias + " " + tagging_cmd + "\"";
180+
ostringstream cmds, inner;
181+
inner << IP_CMD " link set " << shellquote(port_alias) << " master " DOT1Q_BRIDGE_NAME " && "
182+
BRIDGE_CMD " vlan del vid " DEFAULT_VLAN_ID " dev " << shellquote(port_alias) << " && "
183+
BRIDGE_CMD " vlan add vid " + std::to_string(vlan_id) + " dev " << shellquote(port_alias) << " " + tagging_cmd;
184+
cmds << BASH_CMD " -c " << shellquote(inner.str());
185185

186186
std::string res;
187-
EXEC_WITH_ERROR_THROW(cmds, res);
187+
EXEC_WITH_ERROR_THROW(cmds.str(), res);
188188

189189
return true;
190190
}
@@ -202,17 +202,17 @@ bool VlanMgr::removeHostVlanMember(int vlan_id, const string &port_alias)
202202
// else exit $ret; fi )'
203203

204204
// When port is not member of any VLAN, it shall be detached from Dot1Q bridge!
205-
const std::string cmds = std::string("")
206-
+ BASH_CMD + " -c \'"
207-
+ BRIDGE_CMD + " vlan del vid " + std::to_string(vlan_id) + " dev " + port_alias + " && ( "
208-
+ BRIDGE_CMD + " vlan show dev " + port_alias + " | "
209-
+ GREP_CMD + " -q None; ret=$?; if [ $ret -eq 0 ]; then "
210-
+ IP_CMD + " link set " + port_alias + " nomaster; "
211-
+ "elif [ $ret -eq 1 ]; then exit 0; "
212-
+ "else exit $ret; fi )\'";
205+
ostringstream cmds, inner;
206+
inner << BRIDGE_CMD " vlan del vid " + std::to_string(vlan_id) + " dev " << shellquote(port_alias) << " && ( "
207+
BRIDGE_CMD " vlan show dev " << shellquote(port_alias) << " | "
208+
GREP_CMD " -q None; ret=$?; if [ $ret -eq 0 ]; then "
209+
IP_CMD " link set " << shellquote(port_alias) << " nomaster; "
210+
"elif [ $ret -eq 1 ]; then exit 0; "
211+
"else exit $ret; fi )";
212+
cmds << BASH_CMD " -c " << shellquote(cmds.str());
213213

214214
std::string res;
215-
EXEC_WITH_ERROR_THROW(cmds, res);
215+
EXEC_WITH_ERROR_THROW(cmds.str(), res);
216216

217217
return true;
218218
}

‎cfgmgr/vrfmgr.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ bool VrfMgr::delLink(const string& vrfName)
9999
return false;
100100
}
101101

102-
cmd << IP_CMD << " link del " << vrfName;
102+
cmd << IP_CMD << " link del " << shellquote(vrfName);
103103
EXEC_WITH_ERROR_THROW(cmd.str(), res);
104104

105105
recycleTable(m_vrfTableMap[vrfName]);
@@ -126,14 +126,14 @@ bool VrfMgr::setLink(const string& vrfName)
126126
return false;
127127
}
128128

129-
cmd << IP_CMD << " link add " << vrfName << " type vrf table " << table;
129+
cmd << IP_CMD << " link add " << shellquote(vrfName) << " type vrf table " << table;
130130
EXEC_WITH_ERROR_THROW(cmd.str(), res);
131131

132132
m_vrfTableMap.emplace(vrfName, table);
133133

134134
cmd.str("");
135135
cmd.clear();
136-
cmd << IP_CMD << " link set " << vrfName << " up";
136+
cmd << IP_CMD << " link set " << shellquote(vrfName) << " up";
137137
EXEC_WITH_ERROR_THROW(cmd.str(), res);
138138

139139
return true;

0 commit comments

Comments
 (0)