From 002e269071528cf1e5634141200f50aaeeac9b0e Mon Sep 17 00:00:00 2001 From: Tomek Mrugalski Date: Tue, 18 Apr 2017 20:37:11 +0200 Subject: [PATCH] [5187] Kea is less strict with filename argument to config-write --- doc/guide/ctrl-channel.xml | 7 ++-- src/bin/dhcp4/ctrl_dhcp4_srv.cc | 25 ++----------- .../dhcp4/tests/ctrl_dhcp4_srv_unittest.cc | 36 ------------------- src/bin/dhcp6/ctrl_dhcp6_srv.cc | 25 ++----------- .../dhcp6/tests/ctrl_dhcp6_srv_unittest.cc | 36 ------------------- 5 files changed, 9 insertions(+), 120 deletions(-) diff --git a/doc/guide/ctrl-channel.xml b/doc/guide/ctrl-channel.xml index c73a0c0a9d..6bf1a74ef8 100644 --- a/doc/guide/ctrl-channel.xml +++ b/doc/guide/ctrl-channel.xml @@ -363,10 +363,9 @@ $ curl -X POST -H "Content-Type: application/json" -d '{ "command": "config-get" to write its current configuration to a file on disk. It takes one optional argument called filename that specifies the name of the file to write configuration to. If not specified, the - name used when starting Kea (passed as -c argument) will be used. Note - that the filename specified must not contain .. or backslashes. Kea - should be able to write its files only in the directory it is running - and any attempts to step out of that directory will be rejected. + name used when starting Kea (passed as -c argument) will be + used. If relative path is specified, Kea will write its files + only in the directory it is running. An example command invocation looks like this: diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc index 9fc5a241e5..e4708273d1 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc @@ -246,30 +246,11 @@ ControlledDhcpv4Srv::commandConfigWriteHandler(const string&, if (filename.empty()) { // filename parameter was not specified, so let's use whatever we remember filename = getConfigFile(); - if (filename.empty()) { - return (createAnswer(CONTROL_RESULT_ERROR, "Unable to determine filename." - "Please specify filename explicitly.")); - } } - // Now do the sanity checks on the filename - if (filename.find("..") != string::npos) { - // Trying to escape the directory.. nope. - return (createAnswer(CONTROL_RESULT_ERROR, - "Using '..' in filename is not allowed.")); - } - - if (filename.find("\\") != string::npos) { - // Trying to inject escapes (possibly to inject quotes and something - // nasty afterward) - return (createAnswer(CONTROL_RESULT_ERROR, - "Using \\ in filename is not allowed.")); - } - - if (filename[0] == '/') { - // Absolute paths are not allowed. - return (createAnswer(CONTROL_RESULT_ERROR, - "Absolute path in filename is not allowed.")); + if (filename.empty()) { + return (createAnswer(CONTROL_RESULT_ERROR, "Unable to determine filename." + "Please specify filename explicitly.")); } // Ok, it's time to write the file. diff --git a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc index 49a912ae78..5af295b469 100644 --- a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc @@ -965,42 +965,6 @@ TEST_F(CtrlChannelDhcpv4SrvTest, writeConfigFilename) { ::remove("test2.json"); } -// Tests if config-write rejects invalid filename (a one that tries to escape -// the current directory). -TEST_F(CtrlChannelDhcpv4SrvTest, writeConfigInvalidJailEscape) { - createUnixChannelServer(); - std::string response; - - sendUnixCommand("{ \"command\": \"config-write\", \"arguments\": " - "{ \"filename\": \"../test3.json\" } }", response); - checkConfigWrite(response, CONTROL_RESULT_ERROR, - "Using '..' in filename is not allowed."); -} - -// Tests if config-write rejects invalid filename (absolute paths are not allowed) -TEST_F(CtrlChannelDhcpv4SrvTest, writeConfigInvalidAbsPath) { - createUnixChannelServer(); - std::string response; - - sendUnixCommand("{ \"command\": \"config-write\", \"arguments\": " - "{ \"filename\": \"/tmp/test4.json\" } }", response); - checkConfigWrite(response, CONTROL_RESULT_ERROR, - "Absolute path in filename is not allowed."); -} - -// Tests if config-write rejects invalid filename (one with backslashes, which may -// lead to some other tricks) -TEST_F(CtrlChannelDhcpv4SrvTest, writeConfigInvalidEscape) { - createUnixChannelServer(); - std::string response; - - // This will be converted to foo(single backslash)test5.json - sendUnixCommand("{ \"command\": \"config-write\", \"arguments\": " - "{ \"filename\": \"foo\\\\test5.json\" } }", response); - checkConfigWrite(response, CONTROL_RESULT_ERROR, - "Using \\ in filename is not allowed."); -} - // Tests if config-reload attempts to reload a file and reports that the // file is missing. TEST_F(CtrlChannelDhcpv4SrvTest, configReloadMissingFile) { diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.cc b/src/bin/dhcp6/ctrl_dhcp6_srv.cc index 8fceb3324c..4648d4bde9 100644 --- a/src/bin/dhcp6/ctrl_dhcp6_srv.cc +++ b/src/bin/dhcp6/ctrl_dhcp6_srv.cc @@ -249,30 +249,11 @@ ControlledDhcpv6Srv::commandConfigWriteHandler(const string&, ConstElementPtr ar // filename parameter was not specified, so let's use whatever we remember // from the command-line filename = getConfigFile(); - if (filename.empty()) { - return (createAnswer(CONTROL_RESULT_ERROR, "Unable to determine filename." - "Please specify filename explicitly.")); - } } - // Now do the sanity checks on the filename - if (filename.find("..") != string::npos) { - // Trying to escape the directory with .. nope. - return (createAnswer(CONTROL_RESULT_ERROR, - "Using '..' in filename is not allowed.")); - } - - if (filename.find("\\") != string::npos) { - // Trying to inject escapes (possibly to inject quotes and something - // nasty afterward) - return (createAnswer(CONTROL_RESULT_ERROR, - "Using \\ in filename is not allowed.")); - } - - if (filename[0] == '/') { - // Absolute paths are not allowed. - return (createAnswer(CONTROL_RESULT_ERROR, - "Absolute path in filename is not allowed.")); + if (filename.empty()) { + return (createAnswer(CONTROL_RESULT_ERROR, "Unable to determine filename." + "Please specify filename explicitly.")); } // Ok, it's time to write the file. diff --git a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc index ceebdab9e3..f325ab3e6b 100644 --- a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc @@ -991,42 +991,6 @@ TEST_F(CtrlChannelDhcpv6SrvTest, configWriteFilename) { ::remove("test2.json"); } -// Tests if config-write rejects invalid filename (a one that tries to escape -// the current directory). -TEST_F(CtrlChannelDhcpv6SrvTest, configWriteInvalidJailEscape) { - createUnixChannelServer(); - std::string response; - - sendUnixCommand("{ \"command\": \"config-write\", \"arguments\": " - "{ \"filename\": \"../test3.json\" } }", response); - checkConfigWrite(response, CONTROL_RESULT_ERROR, - "Using '..' in filename is not allowed."); -} - -// Tests if config-write rejects invalid filename (absolute paths are not allowed) -TEST_F(CtrlChannelDhcpv6SrvTest, configWriteInvalidAbsPath) { - createUnixChannelServer(); - std::string response; - - sendUnixCommand("{ \"command\": \"config-write\", \"arguments\": " - "{ \"filename\": \"/tmp/test4.json\" } }", response); - checkConfigWrite(response, CONTROL_RESULT_ERROR, - "Absolute path in filename is not allowed."); -} - -// Tests if config-write rejects invalid filename (one with backslashes, which may -// lead to some other tricks) -TEST_F(CtrlChannelDhcpv6SrvTest, configWriteInvalidEscape) { - createUnixChannelServer(); - std::string response; - - // This will be converted to foo(single backslash)test5.json - sendUnixCommand("{ \"command\": \"config-write\", \"arguments\": " - "{ \"filename\": \"foo\\\\test5.json\" } }", response); - checkConfigWrite(response, CONTROL_RESULT_ERROR, - "Using \\ in filename is not allowed."); -} - // Tests if config-reload attempts to reload a file and reports that the // file is missing. TEST_F(CtrlChannelDhcpv6SrvTest, configReloadMissingFile) { -- 2.47.3