Use SQLITE_CONSTRAINT when required constraint does not exist (#5422)

Summary:
Pull Request resolved: https://github.com/facebook/osquery/pull/5422

We were just de-prioritizing type of queries not constraining required columns. However, when the query is just useless without specific constraint, sqlite suggestion is to return SQLITE_CONSTRAINT status.

Reviewed By: marekcirkos

Differential Revision: D13964562

fbshipit-source-id: ee0e5f8baf9abbf83c34f7a39d2b5bd705cbac6d

osquery/sql/tests/sqlite_util_tests.cpp

@@ -192,14 +192,37 @@ TEST_F(SQLiteUtilTests, test_get_query_columns) {
   ASSERT_FALSE(status.ok());
 }
 
-TEST_F(SQLiteUtilTests, test_get_query_tables) {
+TEST_F(SQLiteUtilTests, test_get_query_tables_failed) {
   std::string query =
       "SELECT * FROM time, osquery_info, (SELECT * FROM file) ff GROUP BY pid";
   std::vector<std::string> tables;
   auto status = getQueryTables(query, tables);
   EXPECT_TRUE(status.ok());
 
-  std::vector<std::string> expected = {"file", "time", "osquery_info"};
+  std::vector<std::string> expected = {};
+  EXPECT_EQ(expected, tables);
+}
+
+TEST_F(SQLiteUtilTests, test_get_query_tables) {
+  std::string query =
+      "SELECT * FROM time, osquery_info, (SELECT * FROM users) ff GROUP BY pid";
+  std::vector<std::string> tables;
+  auto status = getQueryTables(query, tables);
+  EXPECT_TRUE(status.ok());
+
+  std::vector<std::string> expected = {"time", "osquery_info", "users"};
+  EXPECT_EQ(expected, tables);
+}
+
+TEST_F(SQLiteUtilTests, test_get_query_tables_required) {
+  std::string query =
+      "SELECT * FROM time, osquery_info, (SELECT * FROM file where path = "
+      "'osquery') ff GROUP BY pid";
+  std::vector<std::string> tables;
+  auto status = getQueryTables(query, tables);
+  EXPECT_TRUE(status.ok());
+
+  std::vector<std::string> expected = {"time", "osquery_info", "file"};
   EXPECT_EQ(expected, tables);
 }
 
@@ -218,85 +241,119 @@ TEST_F(SQLiteUtilTests, test_query_planner) {
   TableColumns columns;
 
   std::string query = "select path, path from file";
-  getQueryColumnsInternal(query, columns, dbc);
+  EXPECT_FALSE(getQueryColumnsInternal(query, columns, dbc).ok());
+
+  query = "select path, path from file where path in ('osquery', 'noquery')";
+  EXPECT_TRUE(getQueryColumnsInternal(query, columns, dbc).ok());
   EXPECT_EQ(getTypes(columns), TypeList({TEXT_TYPE, TEXT_TYPE}));
 
-  query = "select path, seconds from file, time";
-  getQueryColumnsInternal(query, columns, dbc);
+  query = "select path, seconds from file, time where path LIKE 'osquery'";
+  EXPECT_TRUE(getQueryColumnsInternal(query, columns, dbc).ok());
   EXPECT_EQ(getTypes(columns), TypeList({TEXT_TYPE, INTEGER_TYPE}));
 
   query = "select path || path from file";
-  getQueryColumnsInternal(query, columns, dbc);
+  EXPECT_FALSE(getQueryColumnsInternal(query, columns, dbc).ok());
+
+  query = "select path || path from file where path = 'osquery'";
+  EXPECT_TRUE(getQueryColumnsInternal(query, columns, dbc).ok());
   EXPECT_EQ(getTypes(columns), TypeList({TEXT_TYPE}));
 
-  query = "select seconds, path || path from file, time";
-  getQueryColumnsInternal(query, columns, dbc);
+  query = "select seconds, path || path from file, time ";
+  EXPECT_FALSE(getQueryColumnsInternal(query, columns, dbc).ok());
+
+  query =
+      "select seconds, path || path from file, time where path in ('osquery')";
+  EXPECT_TRUE(getQueryColumnsInternal(query, columns, dbc).ok());
   EXPECT_EQ(getTypes(columns), TypeList({INTEGER_TYPE, TEXT_TYPE}));
 
   query = "select seconds, seconds from time";
-  getQueryColumnsInternal(query, columns, dbc);
+  EXPECT_TRUE(getQueryColumnsInternal(query, columns, dbc).ok());
   EXPECT_EQ(getTypes(columns), TypeList({INTEGER_TYPE, INTEGER_TYPE}));
 
   query = "select count(*) from time";
-  getQueryColumnsInternal(query, columns, dbc);
+  EXPECT_TRUE(getQueryColumnsInternal(query, columns, dbc).ok());
   EXPECT_EQ(getTypes(columns), TypeList({BIGINT_TYPE}));
 
   query = "select count(*), count(seconds), seconds from time";
-  getQueryColumnsInternal(query, columns, dbc);
+  EXPECT_TRUE(getQueryColumnsInternal(query, columns, dbc).ok());
   EXPECT_EQ(getTypes(columns),
             TypeList({BIGINT_TYPE, BIGINT_TYPE, INTEGER_TYPE}));
 
   query = "select 1, 'path', path from file";
-  getQueryColumnsInternal(query, columns, dbc);
+  EXPECT_FALSE(getQueryColumnsInternal(query, columns, dbc).ok());
+
+  query = "select 1, 'path', path from file where path = 'os'";
+  EXPECT_TRUE(getQueryColumnsInternal(query, columns, dbc).ok());
   EXPECT_EQ(getTypes(columns), TypeList({INTEGER_TYPE, TEXT_TYPE, TEXT_TYPE}));
 
   query = "select weekday, day, count(*), seconds from time";
-  getQueryColumnsInternal(query, columns, dbc);
+  EXPECT_TRUE(getQueryColumnsInternal(query, columns, dbc).ok());
   EXPECT_EQ(getTypes(columns),
             TypeList({TEXT_TYPE, INTEGER_TYPE, BIGINT_TYPE, INTEGER_TYPE}));
 
   query = "select seconds + 1 from time";
-  getQueryColumnsInternal(query, columns, dbc);
+  EXPECT_TRUE(getQueryColumnsInternal(query, columns, dbc).ok());
   EXPECT_EQ(getTypes(columns), TypeList({BIGINT_TYPE}));
 
   query = "select seconds * seconds from time";
-  getQueryColumnsInternal(query, columns, dbc);
+  EXPECT_TRUE(getQueryColumnsInternal(query, columns, dbc).ok());
   EXPECT_EQ(getTypes(columns), TypeList({BIGINT_TYPE}));
 
   query = "select seconds > 1, seconds, count(seconds) from time";
-  getQueryColumnsInternal(query, columns, dbc);
+  EXPECT_TRUE(getQueryColumnsInternal(query, columns, dbc).ok());
   EXPECT_EQ(getTypes(columns),
             TypeList({INTEGER_TYPE, INTEGER_TYPE, BIGINT_TYPE}));
 
   query =
       "select f1.*, seconds, f2.directory from (select path || path from file) "
       "f1, file as f2, time";
-  getQueryColumnsInternal(query, columns, dbc);
+  EXPECT_FALSE(getQueryColumnsInternal(query, columns, dbc).ok());
+
+  query =
+      "select f1.*, seconds, f2.directory from (select path || path from file) "
+      "f1, file as f2, time where path in ('query', 'query')";
+  EXPECT_FALSE(getQueryColumnsInternal(query, columns, dbc).ok());
+
+  query =
+      "select f1.*, seconds, f2.directory from (select path || path from file "
+      "where path = 'query') "
+      "f1, file as f2, time where path in ('query', 'query')";
+  EXPECT_TRUE(getQueryColumnsInternal(query, columns, dbc).ok());
   EXPECT_EQ(getTypes(columns), TypeList({TEXT_TYPE, INTEGER_TYPE, TEXT_TYPE}));
 
   query = "select CAST(seconds AS INTEGER) FROM time";
-  getQueryColumnsInternal(query, columns, dbc);
+  EXPECT_TRUE(getQueryColumnsInternal(query, columns, dbc).ok());
   EXPECT_EQ(getTypes(columns), TypeList({BIGINT_TYPE}));
 
   query = "select CAST(seconds AS TEXT) FROM time";
-  getQueryColumnsInternal(query, columns, dbc);
+  EXPECT_TRUE(getQueryColumnsInternal(query, columns, dbc).ok());
   EXPECT_EQ(getTypes(columns), TypeList({TEXT_TYPE}));
 
   query = "select CAST(seconds AS REAL) FROM time";
-  getQueryColumnsInternal(query, columns, dbc);
+  EXPECT_TRUE(getQueryColumnsInternal(query, columns, dbc).ok());
   EXPECT_EQ(getTypes(columns), TypeList({DOUBLE_TYPE}));
 
   query = "select CAST(seconds AS BOOLEAN) FROM time";
-  getQueryColumnsInternal(query, columns, dbc);
+  EXPECT_TRUE(getQueryColumnsInternal(query, columns, dbc).ok());
   EXPECT_EQ(getTypes(columns), TypeList({UNKNOWN_TYPE}));
 
   query = "select CAST(seconds AS DATETIME) FROM time";
-  getQueryColumnsInternal(query, columns, dbc);
+  EXPECT_TRUE(getQueryColumnsInternal(query, columns, dbc).ok());
   EXPECT_EQ(getTypes(columns), TypeList({UNKNOWN_TYPE}));
 
   query = "select CAST(seconds AS BLOB) FROM time";
-  getQueryColumnsInternal(query, columns, dbc);
+  EXPECT_TRUE(getQueryColumnsInternal(query, columns, dbc).ok());
   EXPECT_EQ(getTypes(columns), TypeList({BLOB_TYPE}));
+
+  query = "select url, round_trip_time, response_code from curl";
+  EXPECT_FALSE(getQueryColumnsInternal(query, columns, dbc).ok());
+
+  query =
+      "select url, round_trip_time, response_code from curl where url = "
+      "'https://github.com/facebook/osquery'";
+  EXPECT_TRUE(getQueryColumnsInternal(query, columns, dbc).ok());
+  EXPECT_EQ(getTypes(columns),
+            TypeList({TEXT_TYPE, BIGINT_TYPE, INTEGER_TYPE}));
 }
 
 using TypeMap = std::map<std::string, ColumnType>;

osquery/sql/virtual_table.cpp

@@ -759,10 +759,9 @@ static int xBestIndex(sqlite3_vtab* tab, sqlite3_index_info* pIdxInfo) {
   // Check the table for a required column.
   for (const auto& column : columns) {
     auto& options = std::get<2>(column);
-    if (options & ColumnOptions::REQUIRED && !required_satisfied) {
+    if ((options & ColumnOptions::REQUIRED) && !required_satisfied) {
       // A column is marked required, but no constraint satisfies.
-      cost += 1e10;
-      break;
+      return SQLITE_CONSTRAINT;
     }
   }
 

tests/integration/tables/authenticode.cpp

@@ -24,7 +24,7 @@ class authenticode : public testing::Test {
 
 TEST_F(authenticode, test_sanity) {
   // 1. Query data
-  auto const data = execute_query("select * from authenticode");
+  auto const data = execute_query("select * from authenticode where path = ''");
   // 2. Check size before validation
   // ASSERT_GE(data.size(), 0ul);
   // ASSERT_EQ(data.size(), 1ul);

tests/integration/tables/curl.cpp

@@ -24,7 +24,7 @@ class curl : public testing::Test {
 
 TEST_F(curl, test_sanity) {
   // 1. Query data
-  auto const data = execute_query("select * from curl");
+  auto const data = execute_query("select * from curl where url = ''");
   // 2. Check size before validation
   // ASSERT_GE(data.size(), 0ul);
   // ASSERT_EQ(data.size(), 1ul);

tests/integration/tables/curl_certificate.cpp

@@ -24,7 +24,8 @@ class curlCertificate : public testing::Test {
 
 TEST_F(curlCertificate, test_sanity) {
   // 1. Query data
-  auto const data = execute_query("select * from curl_certificate");
+  auto const data =
+      execute_query("select * from curl_certificate where hostname = ''");
   // 2. Check size before validation
   // ASSERT_GE(data.size(), 0ul);
   // ASSERT_EQ(data.size(), 1ul);

tests/integration/tables/device_file.cpp

@@ -24,7 +24,8 @@ class deviceFile : public testing::Test {
 
 TEST_F(deviceFile, test_sanity) {
   // 1. Query data
-  auto const data = execute_query("select * from device_file");
+  auto const data = execute_query(
+      "select * from device_file where device = '' and partition = ''");
   // 2. Check size before validation
   // ASSERT_GE(data.size(), 0ul);
   // ASSERT_EQ(data.size(), 1ul);

tests/integration/tables/device_hash.cpp

@@ -24,7 +24,9 @@ class deviceHash : public testing::Test {
 
 TEST_F(deviceHash, test_sanity) {
   // 1. Query data
-  auto const data = execute_query("select * from device_hash");
+  auto const data = execute_query(
+      "select * from device_hash where device = '' and partition = '' and "
+      "inode = ''");
   // 2. Check size before validation
   // ASSERT_GE(data.size(), 0ul);
   // ASSERT_EQ(data.size(), 1ul);

tests/integration/tables/device_partitions.cpp

@@ -24,7 +24,8 @@ class devicePartitions : public testing::Test {
 
 TEST_F(devicePartitions, test_sanity) {
   // 1. Query data
-  auto const data = execute_query("select * from device_partitions");
+  auto const data =
+      execute_query("select * from device_partitions where device = ''");
   // 2. Check size before validation
   // ASSERT_GE(data.size(), 0ul);
   // ASSERT_EQ(data.size(), 1ul);

tests/integration/tables/docker_container_processes.cpp

@@ -24,7 +24,8 @@ class dockerContainerProcesses : public testing::Test {
 
 TEST_F(dockerContainerProcesses, test_sanity) {
   // 1. Query data
-  auto const data = execute_query("select * from docker_container_processes");
+  auto const data =
+      execute_query("select * from docker_container_processes where id = ''");
   // 2. Check size before validation
   // ASSERT_GE(data.size(), 0ul);
   // ASSERT_EQ(data.size(), 1ul);

tests/integration/tables/docker_container_stats.cpp

@@ -24,7 +24,8 @@ class dockerContainerStats : public testing::Test {
 
 TEST_F(dockerContainerStats, test_sanity) {
   // 1. Query data
-  auto const data = execute_query("select * from docker_container_stats");
+  auto const data =
+      execute_query("select * from docker_container_stats where id = ''");
   // 2. Check size before validation
   // ASSERT_GE(data.size(), 0ul);
   // ASSERT_EQ(data.size(), 1ul);

tests/integration/tables/elf_dynamic.cpp

@@ -24,7 +24,7 @@ class elfDynamic : public testing::Test {
 
 TEST_F(elfDynamic, test_sanity) {
   // 1. Query data
-  auto const data = execute_query("select * from elf_dynamic");
+  auto const data = execute_query("select * from elf_dynamic where path = ''");
   // 2. Check size before validation
   // ASSERT_GE(data.size(), 0ul);
   // ASSERT_EQ(data.size(), 1ul);

tests/integration/tables/elf_info.cpp

@@ -24,7 +24,7 @@ class elfInfo : public testing::Test {
 
 TEST_F(elfInfo, test_sanity) {
   // 1. Query data
-  auto const data = execute_query("select * from elf_info");
+  auto const data = execute_query("select * from elf_info where path = ''");
   // 2. Check size before validation
   // ASSERT_GE(data.size(), 0ul);
   // ASSERT_EQ(data.size(), 1ul);

tests/integration/tables/elf_sections.cpp

@@ -24,7 +24,7 @@ class elfSections : public testing::Test {
 
 TEST_F(elfSections, test_sanity) {
   // 1. Query data
-  auto const data = execute_query("select * from elf_sections");
+  auto const data = execute_query("select * from elf_sections where path = ''");
   // 2. Check size before validation
   // ASSERT_GE(data.size(), 0ul);
   // ASSERT_EQ(data.size(), 1ul);

tests/integration/tables/elf_segments.cpp

@@ -24,7 +24,7 @@ class elfSegments : public testing::Test {
 
 TEST_F(elfSegments, test_sanity) {
   // 1. Query data
-  auto const data = execute_query("select * from elf_segments");
+  auto const data = execute_query("select * from elf_segments where path = ''");
   // 2. Check size before validation
   // ASSERT_GE(data.size(), 0ul);
   // ASSERT_EQ(data.size(), 1ul);

tests/integration/tables/elf_symbols.cpp

@@ -24,7 +24,7 @@ class elfSymbols : public testing::Test {
 
 TEST_F(elfSymbols, test_sanity) {
   // 1. Query data
-  auto const data = execute_query("select * from elf_symbols");
+  auto const data = execute_query("select * from elf_symbols where path = ''");
   // 2. Check size before validation
   // ASSERT_GE(data.size(), 0ul);
   // ASSERT_EQ(data.size(), 1ul);

tests/integration/tables/extended_attributes.cpp

@@ -24,7 +24,8 @@ class extendedAttributes : public testing::Test {
 
 TEST_F(extendedAttributes, test_sanity) {
   // 1. Query data
-  auto const data = execute_query("select * from extended_attributes");
+  auto const data =
+      execute_query("select * from extended_attributes where path = ''");
   // 2. Check size before validation
   // ASSERT_GE(data.size(), 0ul);
   // ASSERT_EQ(data.size(), 1ul);

tests/integration/tables/magic.cpp

@@ -24,7 +24,7 @@ class magic : public testing::Test {
 
 TEST_F(magic, test_sanity) {
   // 1. Query data
-  auto const data = execute_query("select * from magic");
+  auto const data = execute_query("select * from magic where path = ''");
   // 2. Check size before validation
   // ASSERT_GE(data.size(), 0ul);
   // ASSERT_EQ(data.size(), 1ul);

tests/integration/tables/package_bom.cpp

@@ -24,7 +24,7 @@ class packageBom : public testing::Test {
 
 TEST_F(packageBom, test_sanity) {
   // 1. Query data
-  auto const data = execute_query("select * from package_bom");
+  auto const data = execute_query("select * from package_bom where path = ''");
   // 2. Check size before validation
   // ASSERT_GE(data.size(), 0ul);
   // ASSERT_EQ(data.size(), 1ul);

tests/integration/tables/plist.cpp

@@ -24,7 +24,7 @@ class plist : public testing::Test {
 
 TEST_F(plist, test_sanity) {
   // 1. Query data
-  auto const data = execute_query("select * from plist");
+  auto const data = execute_query("select * from plist where path = ''");
   // 2. Check size before validation
   // ASSERT_GE(data.size(), 0ul);
   // ASSERT_EQ(data.size(), 1ul);

tests/integration/tables/signature.cpp

@@ -24,7 +24,7 @@ class signature : public testing::Test {
 
 TEST_F(signature, test_sanity) {
   // 1. Query data
-  auto const data = execute_query("select * from signature");
+  auto const data = execute_query("select * from signature where path = ''");
   // 2. Check size before validation
   // ASSERT_GE(data.size(), 0ul);
   // ASSERT_EQ(data.size(), 1ul);

tests/integration/tables/yara.cpp

@@ -24,7 +24,7 @@ class yara : public testing::Test {
 
 TEST_F(yara, test_sanity) {
   // 1. Query data
-  auto const data = execute_query("select * from yara");
+  auto const data = execute_query("select * from yara where path = ''");
   // 2. Check size before validation
   // ASSERT_GE(data.size(), 0ul);
   // ASSERT_EQ(data.size(), 1ul);