Merge pull request #480 from CanonicalLtd/fix-long-uid-gid-failure

client/mount: Better uid/gid validation to account for larger values
canonical · Nov 7, 2018 · ba67cda · ba67cda
2 parents 2c89d67 + 973369e
commit ba67cda
Show file tree

Hide file tree

Showing 2 changed files with 85 additions and 4 deletions.
diff --git a/src/client/cmd/mount.cpp b/src/client/cmd/mount.cpp
@@ -37,6 +37,17 @@ using RpcMethod = mp::Rpc::Stub;
 namespace
 {
 constexpr auto category = "mount cmd";
+
+auto convert_id_for(const QString& id_string)
+{
+    bool ok;
+
+    auto id = id_string.toUInt(&ok);
+    if (!ok)
+        throw std::runtime_error(id_string.toStdString() + " is an invalid id");
+
+    return id;
+}
 } // namespace
 
 mp::ReturnCode cmd::Mount::run(mp::ArgParser* parser)
@@ -161,7 +172,7 @@ mp::ParseCode cmd::Mount::parse_args(mp::ArgParser* parser)
         }
     }
 
-    QRegExp map_matcher("^([0-9]{1,5}[:][0-9]{1,5})$");
+    QRegExp map_matcher("^([0-9]+[:][0-9]+)$");
 
     if (parser->isSet(uid_map))
     {
@@ -177,7 +188,18 @@ mp::ParseCode cmd::Mount::parse_args(mp::ArgParser* parser)
 
             auto parsed_map = map.split(":");
 
-            (*request.mutable_mount_maps()->mutable_uid_map())[parsed_map.at(0).toInt()] = parsed_map.at(1).toInt();
+            try
+            {
+                auto host_uid = convert_id_for(parsed_map.at(0));
+                auto instance_uid = convert_id_for(parsed_map.at(1));
+
+                (*request.mutable_mount_maps()->mutable_uid_map())[host_uid] = instance_uid;
+            }
+            catch (const std::exception& e)
+            {
+                cerr << e.what() << "\n";
+                return ParseCode::CommandLineError;
+            }
         }
     }
 
@@ -195,7 +217,18 @@ mp::ParseCode cmd::Mount::parse_args(mp::ArgParser* parser)
 
             auto parsed_map = map.split(":");
 
-            (*request.mutable_mount_maps()->mutable_gid_map())[parsed_map.at(0).toInt()] = parsed_map.at(1).toInt();
+            try
+            {
+                auto host_gid = convert_id_for(parsed_map.at(0));
+                auto instance_gid = convert_id_for(parsed_map.at(1));
+
+                (*request.mutable_mount_maps()->mutable_gid_map())[host_gid] = instance_gid;
+            }
+            catch (const std::exception& e)
+            {
+                cerr << e.what() << "\n";
+                return ParseCode::CommandLineError;
+            }
         }
     }
 

diff --git a/tests/test_cli_client.cpp b/tests/test_cli_client.cpp
@@ -331,7 +331,7 @@ TEST_F(Client, mount_cmd_good_absolute_source_path)
     EXPECT_THAT(send_command({"mount", mpt::test_data_path().toStdString(), "test-vm:test"}), Eq(mp::ReturnCode::Ok));
 }
 
-TEST_F(Client, mount_cmd_good_relative_soure_path)
+TEST_F(Client, mount_cmd_good_relative_source_path)
 {
     EXPECT_THAT(send_command({"mount", "..", "test-vm:test"}), Eq(mp::ReturnCode::Ok));
 }
@@ -342,6 +342,54 @@ TEST_F(Client, mount_cmd_fails_invalid_source_path)
                 Eq(mp::ReturnCode::CommandLineError));
 }
 
+TEST_F(Client, mount_cmd_good_valid_uid_map)
+{
+    EXPECT_THAT(send_command({"mount", mpt::test_data_path().toStdString(), "-u", "1000:501", "test-vm:test"}),
+                Eq(mp::ReturnCode::Ok));
+}
+
+TEST_F(Client, mount_cmd_good_valid_large_uid_map)
+{
+    EXPECT_THAT(send_command({"mount", mpt::test_data_path().toStdString(), "-u", "218038053:0", "test-vm:test"}),
+                Eq(mp::ReturnCode::Ok));
+}
+
+TEST_F(Client, mount_cmd_fails_invalid_string_uid_map)
+{
+    EXPECT_THAT(send_command({"mount", mpt::test_data_path().toStdString(), "-u", "foo:bar", "test-vm:test"}),
+                Eq(mp::ReturnCode::CommandLineError));
+}
+
+TEST_F(Client, mount_cmd_fails_invalid_host_int_uid_map)
+{
+    EXPECT_THAT(send_command({"mount", mpt::test_data_path().toStdString(), "-u", "5000000000:0", "test-vm:test"}),
+                Eq(mp::ReturnCode::CommandLineError));
+}
+
+TEST_F(Client, mount_cmd_good_valid_gid_map)
+{
+    EXPECT_THAT(send_command({"mount", mpt::test_data_path().toStdString(), "-g", "1000:501", "test-vm:test"}),
+                Eq(mp::ReturnCode::Ok));
+}
+
+TEST_F(Client, mount_cmd_good_valid_large_gid_map)
+{
+    EXPECT_THAT(send_command({"mount", mpt::test_data_path().toStdString(), "-g", "218038053:0", "test-vm:test"}),
+                Eq(mp::ReturnCode::Ok));
+}
+
+TEST_F(Client, mount_cmd_fails_invalid_string_gid_map)
+{
+    EXPECT_THAT(send_command({"mount", mpt::test_data_path().toStdString(), "-g", "foo:bar", "test-vm:test"}),
+                Eq(mp::ReturnCode::CommandLineError));
+}
+
+TEST_F(Client, mount_cmd_fails_invalid_host_int_gid_map)
+{
+    EXPECT_THAT(send_command({"mount", mpt::test_data_path().toStdString(), "-g", "5000000000:0", "test-vm:test"}),
+                Eq(mp::ReturnCode::CommandLineError));
+}
+
 // recover cli tests
 TEST_F(Client, recover_cmd_fails_no_args)
 {