This is an automated email from the ASF dual-hosted git repository. awong pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 4ac1d9f4b4fd477e79859d81a58e2dacbe979b2d Author: Andrew Wong <[email protected]> AuthorDate: Tue Mar 19 23:41:16 2019 -0700 tserver: sanitize write op types We previously wouldn't make sure that Write requests actually contained write operations (i.e. INSERT, UPDATE, UPSERT, DELETE). The result is that a malicious user could send a bad write request to crash a tablet server. This patch addresses this by adding different decoder modes (e.g. for writes, for split rows), and using the appropriate decoding mode for writes. Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5 Reviewed-on: http://gerrit.cloudera.org:8080/12815 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo <[email protected]> --- src/kudu/common/row_operations-test.cc | 16 +++++-- src/kudu/common/row_operations.cc | 85 +++++++++++++++++++++++----------- src/kudu/common/row_operations.h | 24 +++++++--- src/kudu/master/catalog_manager.cc | 6 +-- src/kudu/tablet/tablet.cc | 2 +- src/kudu/tools/tool_action_common.cc | 2 +- src/kudu/tserver/tablet_server-test.cc | 46 ++++++++++++++++++ 7 files changed, 138 insertions(+), 43 deletions(-) diff --git a/src/kudu/common/row_operations-test.cc b/src/kudu/common/row_operations-test.cc index 464ee91..76c869f 100644 --- a/src/kudu/common/row_operations-test.cc +++ b/src/kudu/common/row_operations-test.cc @@ -15,10 +15,12 @@ // specific language governing permissions and limitations // under the License. +#include "kudu/common/row_operations.h" + #include <cstdint> #include <cstdlib> -#include <ostream> #include <memory> +#include <ostream> #include <string> #include <vector> @@ -27,7 +29,6 @@ #include "kudu/common/common.pb.h" #include "kudu/common/partial_row.h" -#include "kudu/common/row_operations.h" #include "kudu/common/schema.h" #include "kudu/common/types.h" #include "kudu/common/wire_protocol.pb.h" @@ -100,8 +101,13 @@ void RowOperationsTest::CheckDecodeDoesntCrash(const Schema& client_schema, const RowOperationsPB& pb) { arena_.Reset(); RowOperationsPBDecoder decoder(&pb, &client_schema, &server_schema, &arena_); + // Decoding the operations, regardless of the mode, should not result in a + // crash. vector<DecodedRowOperation> ops; - Status s = decoder.DecodeOperations(&ops); + Status s = decoder.DecodeOperations<DecoderMode::WRITE_OPS>(&ops); + if (!s.ok()) { + s = decoder.DecodeOperations<DecoderMode::SPLIT_ROWS>(&ops); + } if (s.ok() && !ops.empty()) { // If we got an OK result, then we should be able to stringify without // crashing. This ensures that any indirect data (eg strings) gets @@ -357,7 +363,7 @@ string TestProjection(RowOperationsPB::Type type, Arena arena(1024); vector<DecodedRowOperation> ops; RowOperationsPBDecoder dec(&pb, client_row.schema(), &server_schema, &arena); - Status s = dec.DecodeOperations(&ops); + Status s = dec.DecodeOperations<DecoderMode::WRITE_OPS>(&ops); if (!s.ok()) { return "error: " + s.ToString(); @@ -675,7 +681,7 @@ TEST_F(RowOperationsTest, SplitKeyRoundTrip) { Schema schema = client_schema.CopyWithColumnIds(); RowOperationsPBDecoder decoder(&pb, &client_schema, &schema, nullptr); vector<DecodedRowOperation> ops; - ASSERT_OK(decoder.DecodeOperations(&ops)); + ASSERT_OK(decoder.DecodeOperations<DecoderMode::SPLIT_ROWS>(&ops)); ASSERT_EQ(1, ops.size()); const shared_ptr<KuduPartialRow>& row2 = ops[0].split_row; diff --git a/src/kudu/common/row_operations.cc b/src/kudu/common/row_operations.cc index 52ef509..226cb1e 100644 --- a/src/kudu/common/row_operations.cc +++ b/src/kudu/common/row_operations.cc @@ -557,14 +557,14 @@ Status RowOperationsPBDecoder::DecodeSplitRow(const ClientServerMapping& mapping return Status::OK(); } +template <DecoderMode mode> Status RowOperationsPBDecoder::DecodeOperations(vector<DecodedRowOperation>* ops) { - // TODO: there's a bug here, in that if a client passes some column - // in its schema that has been deleted on the server, it will fail - // even if the client never actually specified any values for it. - // For example, a DBA might do a thorough audit that no one is using - // some column anymore, and then drop the column, expecting it to be - // compatible, but all writes would start failing until clients - // refreshed their schema. + // TODO(todd): there's a bug here, in that if a client passes some column in + // its schema that has been deleted on the server, it will fail even if the + // client never actually specified any values for it. For example, a DBA + // might do a thorough audit that no one is using some column anymore, and + // then drop the column, expecting it to be compatible, but all writes would + // start failing until clients refreshed their schema. // See DISABLED_TestProjectUpdatesSubsetOfColumns CHECK(!client_schema_->has_column_ids()); DCHECK(tablet_schema_->has_column_ids()); @@ -586,29 +586,60 @@ Status RowOperationsPBDecoder::DecodeOperations(vector<DecodedRowOperation>* ops DecodedRowOperation op; op.type = type; - switch (type) { - case RowOperationsPB::UNKNOWN: - return Status::NotSupported("Unknown row operation type"); - case RowOperationsPB::INSERT: - case RowOperationsPB::UPSERT: - RETURN_NOT_OK(DecodeInsertOrUpsert(prototype_row_storage, mapping, &op)); - break; - case RowOperationsPB::UPDATE: - case RowOperationsPB::DELETE: - RETURN_NOT_OK(DecodeUpdateOrDelete(mapping, &op)); - break; - case RowOperationsPB::SPLIT_ROW: - case RowOperationsPB::RANGE_LOWER_BOUND: - case RowOperationsPB::RANGE_UPPER_BOUND: - case RowOperationsPB::EXCLUSIVE_RANGE_LOWER_BOUND: - case RowOperationsPB::INCLUSIVE_RANGE_UPPER_BOUND: - RETURN_NOT_OK(DecodeSplitRow(mapping, &op)); - break; - } - + RETURN_NOT_OK(DecodeOp<mode>(type, prototype_row_storage, mapping, &op)); ops->push_back(op); } return Status::OK(); } +template<> +Status RowOperationsPBDecoder::DecodeOp<DecoderMode::WRITE_OPS>( + RowOperationsPB::Type type, const uint8_t* prototype_row_storage, + const ClientServerMapping& mapping, DecodedRowOperation* op) { + switch (type) { + case RowOperationsPB::UNKNOWN: + return Status::NotSupported("Unknown row operation type"); + case RowOperationsPB::INSERT: + case RowOperationsPB::UPSERT: + RETURN_NOT_OK(DecodeInsertOrUpsert(prototype_row_storage, mapping, op)); + break; + case RowOperationsPB::UPDATE: + case RowOperationsPB::DELETE: + RETURN_NOT_OK(DecodeUpdateOrDelete(mapping, op)); + break; + default: + return Status::InvalidArgument(Substitute("Invalid write operation type $0", + RowOperationsPB_Type_Name(type))); + } + return Status::OK(); +} + +template<> +Status RowOperationsPBDecoder::DecodeOp<DecoderMode::SPLIT_ROWS>( + RowOperationsPB::Type type, const uint8_t* /*prototype_row_storage*/, + const ClientServerMapping& mapping, DecodedRowOperation* op) { + switch (type) { + case RowOperationsPB::UNKNOWN: + return Status::NotSupported("Unknown row operation type"); + case RowOperationsPB::SPLIT_ROW: + case RowOperationsPB::RANGE_LOWER_BOUND: + case RowOperationsPB::RANGE_UPPER_BOUND: + case RowOperationsPB::EXCLUSIVE_RANGE_LOWER_BOUND: + case RowOperationsPB::INCLUSIVE_RANGE_UPPER_BOUND: + RETURN_NOT_OK(DecodeSplitRow(mapping, op)); + break; + default: + return Status::InvalidArgument(Substitute("Invalid split row type $0", + RowOperationsPB_Type_Name(type))); + } + return Status::OK(); +} + +template +Status RowOperationsPBDecoder::DecodeOperations<DecoderMode::SPLIT_ROWS>( + vector<DecodedRowOperation>* ops); +template +Status RowOperationsPBDecoder::DecodeOperations<DecoderMode::WRITE_OPS>( + vector<DecodedRowOperation>* ops); + } // namespace kudu diff --git a/src/kudu/common/row_operations.h b/src/kudu/common/row_operations.h index cfd4128..9dfaf5f 100644 --- a/src/kudu/common/row_operations.h +++ b/src/kudu/common/row_operations.h @@ -14,8 +14,7 @@ // KIND, either express or implied. See the License for the // specific language governing permissions and limitations // under the License. -#ifndef KUDU_COMMON_ROW_OPERATIONS_H -#define KUDU_COMMON_ROW_OPERATIONS_H +#pragma once #include <cstdint> #include <memory> @@ -31,12 +30,11 @@ namespace kudu { class Arena; +class ClientServerMapping; class ColumnSchema; class KuduPartialRow; class Schema; -class ClientServerMapping; - class RowOperationsPBEncoder { public: explicit RowOperationsPBEncoder(RowOperationsPB* pb); @@ -73,6 +71,14 @@ struct DecodedRowOperation { std::string ToString(const Schema& schema) const; }; +enum DecoderMode { + // Decode range split rows. + SPLIT_ROWS, + + // Decode write operations. + WRITE_OPS, +}; + class RowOperationsPBDecoder { public: RowOperationsPBDecoder(const RowOperationsPB* pb, @@ -81,6 +87,7 @@ class RowOperationsPBDecoder { Arena* dst_arena); ~RowOperationsPBDecoder(); + template <DecoderMode mode> Status DecodeOperations(std::vector<DecodedRowOperation>* ops); private: @@ -106,6 +113,12 @@ class RowOperationsPBDecoder { Status DecodeSplitRow(const ClientServerMapping& mapping, DecodedRowOperation* op); + // Decode the next encoded operation of the given type and properties. + // Returns an error if the type isn't allowed by the decoder mode. + template <DecoderMode mode> + Status DecodeOp(RowOperationsPB::Type type, const uint8_t* prototype_row_storage, + const ClientServerMapping& mapping, DecodedRowOperation* op); + const RowOperationsPB* const pb_; const Schema* const client_schema_; const Schema* const tablet_schema_; @@ -115,8 +128,7 @@ class RowOperationsPBDecoder { const int tablet_row_size_; Slice src_; - DISALLOW_COPY_AND_ASSIGN(RowOperationsPBDecoder); }; + } // namespace kudu -#endif /* KUDU_COMMON_ROW_OPERATIONS_H */ diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc index 63747f4..46e16e4 100644 --- a/src/kudu/master/catalog_manager.cc +++ b/src/kudu/master/catalog_manager.cc @@ -1424,7 +1424,7 @@ Status CatalogManager::CreateTable(const CreateTableRequestPB* orig_req, RowOperationsPBDecoder decoder(req.mutable_split_rows_range_bounds(), &client_schema, &schema, nullptr); vector<DecodedRowOperation> ops; - RETURN_NOT_OK(decoder.DecodeOperations(&ops)); + RETURN_NOT_OK(decoder.DecodeOperations<DecoderMode::SPLIT_ROWS>(&ops)); for (int i = 0; i < ops.size(); i++) { const DecodedRowOperation& op = ops[i]; @@ -2035,12 +2035,12 @@ Status CatalogManager::ApplyAlterPartitioningSteps( if (step.type() == AlterTableRequestPB::ADD_RANGE_PARTITION) { RowOperationsPBDecoder decoder(&step.add_range_partition().range_bounds(), &client_schema, &schema, nullptr); - RETURN_NOT_OK(decoder.DecodeOperations(&ops)); + RETURN_NOT_OK(decoder.DecodeOperations<DecoderMode::SPLIT_ROWS>(&ops)); } else { CHECK_EQ(step.type(), AlterTableRequestPB::DROP_RANGE_PARTITION); RowOperationsPBDecoder decoder(&step.drop_range_partition().range_bounds(), &client_schema, &schema, nullptr); - RETURN_NOT_OK(decoder.DecodeOperations(&ops)); + RETURN_NOT_OK(decoder.DecodeOperations<DecoderMode::SPLIT_ROWS>(&ops)); } if (ops.size() != 2) { diff --git a/src/kudu/tablet/tablet.cc b/src/kudu/tablet/tablet.cc index 5a86c8f..01fb260 100644 --- a/src/kudu/tablet/tablet.cc +++ b/src/kudu/tablet/tablet.cc @@ -444,7 +444,7 @@ Status Tablet::DecodeWriteOperations(const Schema* client_schema, client_schema, schema(), tx_state->arena()); - RETURN_NOT_OK(dec.DecodeOperations(&ops)); + RETURN_NOT_OK(dec.DecodeOperations<DecoderMode::WRITE_OPS>(&ops)); TRACE_COUNTER_INCREMENT("num_ops", ops.size()); // Important to set the schema before the ops -- we need the diff --git a/src/kudu/tools/tool_action_common.cc b/src/kudu/tools/tool_action_common.cc index 7fdd41f..ae464d6 100644 --- a/src/kudu/tools/tool_action_common.cc +++ b/src/kudu/tools/tool_action_common.cc @@ -253,7 +253,7 @@ Status PrintDecodedWriteRequestPB(const string& indent, Arena arena(32 * 1024); RowOperationsPBDecoder dec(&write.row_operations(), &request_schema, &tablet_schema, &arena); vector<DecodedRowOperation> ops; - RETURN_NOT_OK(dec.DecodeOperations(&ops)); + RETURN_NOT_OK(dec.DecodeOperations<DecoderMode::WRITE_OPS>(&ops)); cout << indent << "Tablet: " << write.tablet_id() << endl; cout << indent << "RequestId: " diff --git a/src/kudu/tserver/tablet_server-test.cc b/src/kudu/tserver/tablet_server-test.cc index 82aed13..b59e55e 100644 --- a/src/kudu/tserver/tablet_server-test.cc +++ b/src/kudu/tserver/tablet_server-test.cc @@ -1086,6 +1086,52 @@ TEST_F(TabletServerTest, TestInsertAndMutate) { ASSERT_GE(now_after.value(), now_before.value()); } +// Try sending write requests that do not contain write operations. Make sure +// we get an error that makes sense. +TEST_F(TabletServerTest, TestInvalidWriteRequest_WrongOpType) { + const vector<RowOperationsPB::Type> wrong_op_types = { + RowOperationsPB::SPLIT_ROW, + RowOperationsPB::RANGE_LOWER_BOUND, + RowOperationsPB::RANGE_UPPER_BOUND, + RowOperationsPB::EXCLUSIVE_RANGE_LOWER_BOUND, + RowOperationsPB::INCLUSIVE_RANGE_UPPER_BOUND, + }; + const auto send_bad_write = [&] (RowOperationsPB::Type op_type) { + WriteRequestPB req; + req.set_tablet_id(kTabletId); + WriteResponsePB resp; + RpcController controller; + + CHECK_OK(SchemaToPB(schema_, req.mutable_schema())); + RowOperationsPB* data = req.mutable_row_operations(); + AddTestRowToPB(op_type, schema_, 1234, 5678, "foo", data); + SCOPED_TRACE(SecureDebugString(req)); + CHECK_OK(proxy_->Write(req, &resp, &controller)); + return resp; + }; + // Send a bunch of op types that are inappropriate for write requests. + for (const auto& op_type : wrong_op_types) { + WriteResponsePB resp = send_bad_write(op_type); + SCOPED_TRACE(SecureDebugString(resp)); + ASSERT_TRUE(resp.has_error()); + ASSERT_EQ(TabletServerErrorPB::MISMATCHED_SCHEMA, resp.error().code()); + ASSERT_EQ(AppStatusPB::INVALID_ARGUMENT, resp.error().status().code()); + ASSERT_STR_CONTAINS(resp.error().status().message(), + "Invalid write operation type"); + } + { + // Do the same for UNKNOWN, which is an unexpected operation type in all + // cases, and thus results in a different error message. + WriteResponsePB resp = send_bad_write(RowOperationsPB::UNKNOWN); + SCOPED_TRACE(SecureDebugString(resp)); + ASSERT_TRUE(resp.has_error()); + ASSERT_EQ(TabletServerErrorPB::MISMATCHED_SCHEMA, resp.error().code()); + ASSERT_EQ(AppStatusPB::NOT_SUPPORTED, resp.error().status().code()); + ASSERT_STR_CONTAINS(resp.error().status().message(), + "Unknown row operation type"); + } +} + // Test that passing a schema with fields not present in the tablet schema // throws an exception. TEST_F(TabletServerTest, TestInvalidWriteRequest_BadSchema) {
