Updated IPP Opcode/Attribute validation

We no longer validate specific attribute groupings within operations.
Instead we validate at the per-attribute level only, confirming the same
information as before, and leaving any higher-logic validating to
CUPS.

Bug: chromium:945409
Test: manually confirmed valid attribute/opcode set
Change-Id: I8c2541a8b7a1957239c2468526b75b2a77450646
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1704696Reviewed-by: Sean Kau <skau@chromium.org>
Commit-Queue: Luum Habtemariam <luum@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682451}

chrome/services/cups_proxy/ipp_attribute_validator.h

@@ -13,20 +13,29 @@
 
 namespace cups_proxy {
 
+enum ValidateAttributeResult {
+  kSuccess = 0,
+
+  // Unknown attribute name.
+  kUnknownAttribute,
+  kFatalError,
+  kMaxValue,
+};
+
 // Validates an attribute |name| of type |type| in operation |ipp_oper_id|.
 // |values_count| represents number of values in the attribute. The following
 // constraints are enforced:
 // - only operations from predefined set are accepted
 // - only attributes from predefined set are accepted
-// - an attribute must be part of given operation (request or response)
-// - a type of the attribute must match the specification
+// - serialization type of the attribute must match the specification
 // - a single-value attribute cannot have more than one value
 // - a set-of-values attribute cannot be empty
-// Returns false <=> at least one of the constraints has been violated.
-bool ValidateAttribute(ipp_op_t ipp_oper_id,
-                       const std::string& name,
-                       cups_ipp_parser::mojom::ValueType type,
-                       size_t values_count);
+// Returns kFatalError <=> at least one of the constraints has been violated.
+ValidateAttributeResult ValidateAttribute(
+    ipp_op_t ipp_oper_id,
+    const std::string& name,
+    cups_ipp_parser::mojom::ValueType type,
+    size_t values_count);
 
 }  // namespace cups_proxy
 

chrome/services/cups_proxy/ipp_validator.cc

@@ -15,6 +15,7 @@
 
 #include "base/containers/span.h"
 #include "base/strings/strcat.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "chrome/services/cups_proxy/ipp_attribute_validator.h"
@@ -121,6 +122,7 @@ base::Optional<HttpRequestLine> IppValidator::ValidateHttpRequestLine(
 
 base::Optional<std::vector<ipp_converter::HttpHeader>>
 IppValidator::ValidateHttpHeaders(
+    const size_t http_content_length,
     const base::flat_map<std::string, std::string>& headers) {
   // Sane, character-set checks.
   for (const auto& header : headers) {
@@ -130,9 +132,20 @@ IppValidator::ValidateHttpHeaders(
     }
   }
 
-  return std::vector<ipp_converter::HttpHeader>(headers.begin(), headers.end());
+  std::vector<ipp_converter::HttpHeader> ret(headers.begin(), headers.end());
+
+  // Update the ContentLength.
+  base::EraseIf(ret, [](const ipp_converter::HttpHeader& header) {
+    return header.first == "Content-Length";
+  });
+  ret.push_back({"Content-Length", base::NumberToString(http_content_length)});
+
+  return ret;
 }
 
+// Note: Since its possible to have valid IPP attributes that our
+// ipp_attribute_validator.cc is unaware of, we drop unknown attributes, rather
+// than fail the request.
 ipp_t* IppValidator::ValidateIppMessage(
     cups_ipp_parser::mojom::IppMessagePtr ipp_message) {
   printing::ScopedIppPtr ipp = printing::WrapIpp(ippNew());
@@ -161,10 +174,17 @@ ipp_t* IppValidator::ValidateIppMessage(
       return nullptr;
     }
 
-    if (!ValidateAttribute(ipp_oper_id, attribute->name, attribute->type,
-                           num_values)) {
+    auto ret = ValidateAttribute(ipp_oper_id, attribute->name, attribute->type,
+                                 num_values);
+    if (ret == ValidateAttributeResult::kFatalError) {
       return nullptr;
     }
+    if (ret == ValidateAttributeResult::kUnknownAttribute) {
+      // We drop unknown attributes.
+      DVLOG(1) << "CupsProxy validation: dropping unknown attribute "
+               << attribute->name;
+      continue;
+    }
 
     switch (attribute->type) {
       case cups_ipp_parser::mojom::ValueType::BOOLEAN: {
@@ -275,19 +295,6 @@ base::Optional<IppRequest> IppValidator::ValidateIppRequest(
     return base::nullopt;
   }
 
-  // Build request line.
-  auto request_line = ValidateHttpRequestLine(
-      to_validate->method, to_validate->endpoint, to_validate->http_version);
-  if (!request_line.has_value()) {
-    return base::nullopt;
-  }
-
-  // Build headers.
-  auto headers = ValidateHttpHeaders(to_validate->headers);
-  if (!headers.has_value()) {
-    return base::nullopt;
-  }
-
   // Build ipp message.
   // Note: Moving ipp here, to_validate->ipp no longer valid below.
   printing::ScopedIppPtr ipp =
@@ -302,6 +309,22 @@ base::Optional<IppRequest> IppValidator::ValidateIppRequest(
     return base::nullopt;
   }
 
+  // Build request line.
+  auto request_line = ValidateHttpRequestLine(
+      to_validate->method, to_validate->endpoint, to_validate->http_version);
+  if (!request_line.has_value()) {
+    return base::nullopt;
+  }
+
+  // Build headers; must happen after ipp message/data since it requires the
+  // ContentLength.
+  const size_t http_content_length =
+      ippLength(ipp.get()) + to_validate->data.size();
+  auto headers = ValidateHttpHeaders(http_content_length, to_validate->headers);
+  if (!headers.has_value()) {
+    return base::nullopt;
+  }
+
   // Marshall request
   IppRequest ret;
   ret.request_line = std::move(*request_line);

chrome/services/cups_proxy/ipp_validator.h

@@ -42,6 +42,7 @@ class IppValidator {
       base::StringPiece http_version);
 
   base::Optional<std::vector<ipp_converter::HttpHeader>> ValidateHttpHeaders(
+      const size_t http_content_length,
       const base::flat_map<std::string, std::string>& headers);
 
   ipp_t* ValidateIppMessage(cups_ipp_parser::mojom::IppMessagePtr ipp_message);

chrome/services/cups_proxy/ipp_validator_unittest.cc

@@ -176,6 +176,26 @@ TEST_F(IppValidatorTest, MissingHeaderValue) {
   EXPECT_TRUE(RunValidateIppRequest(request));
 }
 
+// Test that we drop unknown attributes and succeed the request.
+TEST_F(IppValidatorTest, UnknownAttribute) {
+  auto request = GetBasicIppRequest();
+
+  // Add fake attribute.
+  std::string fake_attr_name = "fake-attribute-name";
+  IppAttributePtr fake_attr =
+      BuildAttributePtr(fake_attr_name, IPP_TAG_OPERATION, IPP_TAG_TEXT);
+  fake_attr->type = ValueType::STRING;
+  fake_attr->value->set_strings({"fake_attribute_value"});
+  request->ipp->attributes.push_back(std::move(fake_attr));
+
+  auto result = RunValidateIppRequest(request);
+  ASSERT_TRUE(result);
+
+  // Ensure resulting validated IPP request doesn't contain fake_attr_name.
+  ipp_t* ipp = result->ipp.get();
+  EXPECT_FALSE(ippFindAttribute(ipp, fake_attr_name.c_str(), IPP_TAG_TEXT));
+}
+
 // TODO(crbug.com/945409): Test IPP validation.
 
 }  // namespace