CupsIppParser method prototype change

ParseIpp mojo method now takes in a byte array, matching the expected
input of an arbitrary buffer representing an IPP request.

Implementation detail: Now correctly separate HTTP and IPP metadata and
hold in appropriate containers, i.e. HTTP in a string, IPP in a byte
array.

Bug: chromium:945409
Test: CupsIppParserServiceTests still pass.
Change-Id: I2d337c065ef62218c358a01623f2a2181fe6fc7f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1586338
Commit-Queue: Luum Habtemariam <luum@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Sean Kau <skau@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#655525}

chrome/services/cups_ipp_parser/BUILD.gn

@@ -27,7 +27,7 @@ source_set("lib") {
     "//services/service_manager/public/cpp",
   ]
 
-  # We stub the implementation if libCUPS is not present
+  # We stub the implementation if libCUPS is not present.
   if (enable_service) {
     configs += [ "//printing:cups" ]
     sources += [ "ipp_parser.cc" ]

chrome/services/cups_ipp_parser/cups_ipp_parser_service.h

@@ -11,6 +11,19 @@
 #include "services/service_manager/public/cpp/service_keepalive.h"
 #include "services/service_manager/public/mojom/service.mojom.h"
 
+// CupsIppParser Service Implementation.
+//
+// This service's sole purpose is parsing CUPS IPP printing requests. It accepts
+// arbitrary byte buffers as input and returns a fully-parsed IPP request,
+// mojom::IppRequest, as a result.
+//
+// Because this service doesn't know the origin of these requests, it
+// treats each request as potentially malicious. As such, this service runs
+// out-of-process, lessening the chance of exposing an exploit to the rest of
+// Chrome.
+//
+// Note: In practice, this service is used to support printing requests incoming
+// from ChromeOS.
 class CupsIppParserService : public service_manager::Service {
  public:
   explicit CupsIppParserService(service_manager::mojom::ServiceRequest request);

chrome/services/cups_ipp_parser/fake_ipp_parser.cc

@@ -15,10 +15,10 @@ IppParser::IppParser(
 
 IppParser::~IppParser() = default;
 
-void IppParser::ParseIpp(const std::string& to_parse,
+void IppParser::ParseIpp(const std::vector<uint8_t>& to_parse,
                          ParseIppCallback callback) {
   DVLOG(1) << "IppParser stubbed";
-  std::move(callback).Run(mojom::IppParserResult::FAILURE, nullptr);
+  std::move(callback).Run(nullptr);
 }
 
 }  // namespace chrome

chrome/services/cups_ipp_parser/ipp_parser.cc

@@ -23,7 +23,7 @@ using ipp_converter::kIppSentinel;
 // Log debugging error and send empty response, signalling error.
 void Fail(const std::string& error_log, IppParser::ParseIppCallback cb) {
   DVLOG(1) << "IPP Parser Error: " << error_log;
-  std::move(cb).Run(mojom::IppParserResult::FAILURE, nullptr);
+  std::move(cb).Run(nullptr);
   return;
 }
 
@@ -39,14 +39,14 @@ int LocateEndOfRequestLine(base::StringPiece request) {
 
 // Returns the starting index of the first HTTP header, -1 on failure.
 int LocateStartOfHeaders(base::StringPiece request) {
-  auto idx = request.find(kCarriage);
-  if (idx == base::StringPiece::npos) {
+  auto idx = LocateEndOfRequestLine(request);
+  if (idx < 0) {
     return -1;
   }
 
   // Advance to first header and check it exists
   idx += strlen(kCarriage);
-  return idx < request.size() ? idx : -1;
+  return idx < static_cast<int>(request.size()) ? idx : -1;
 }
 
 // Returns the starting index of the end-of-headers-delimiter, -1 on failure.
@@ -58,36 +58,51 @@ int LocateEndOfHeaders(base::StringPiece request) {
 
   // Back up to the start of the delimiter.
   // Note: The end-of-http-headers delimiter is 2 back-to-back carriage returns.
-  const size_t end_of_headers_delimiter_size = 2 * strlen(kCarriage);
+  const int end_of_headers_delimiter_size = 2 * strlen(kCarriage);
   return idx - end_of_headers_delimiter_size;
 }
 
-// Returns the starting index of the IPP message, -1 on failure.
-int LocateStartOfIppMessage(base::StringPiece request) {
-  return net::HttpUtil::LocateEndOfHeaders(request.data(), request.size());
-}
-
-// Return the starting index of the IPP data/payload(pdf),
-// Returns |request|.size() on empty IppData and -1 on failure.
-int LocateStartOfIppData(base::StringPiece request) {
-  int end_of_headers = LocateEndOfHeaders(request);
-  if (end_of_headers < 0) {
+// Return the starting index of the IPP data/payload (pdf).
+// Returns |ipp_metadata|.size() on empty IPP data and -1 on failure.
+int LocateStartOfIppData(base::span<const uint8_t> ipp_metadata) {
+  std::vector<uint8_t> sentinel_wrapper(
+      ipp_converter::ConvertToByteBuffer(kIppSentinel));
+  auto it = std::search(ipp_metadata.begin(), ipp_metadata.end(),
+                        sentinel_wrapper.begin(), sentinel_wrapper.end());
+  if (it == ipp_metadata.end()) {
     return -1;
   }
 
-  auto idx = request.find(kIppSentinel, end_of_headers);
-  if (idx == base::StringPiece::npos) {
-    return -1;
+  // Advance to the start of IPP data and check existence or end of request.
+  it += strlen(kIppSentinel);
+  return it <= ipp_metadata.end() ? std::distance(ipp_metadata.begin(), it)
+                                  : -1;
+}
+
+// Returns the starting index of the IPP metadata, -1 on failure.
+int LocateStartOfIppMetadata(base::span<const uint8_t> request) {
+  std::vector<char> char_buffer = ipp_converter::ConvertToCharBuffer(request);
+  return net::HttpUtil::LocateEndOfHeaders(char_buffer.data(),
+                                           char_buffer.size());
+}
+
+bool SplitRequestMetadata(base::span<const uint8_t> request,
+                          std::string* http_metadata,
+                          base::span<const uint8_t>* ipp_metadata) {
+  size_t start_of_ipp_metadata = LocateStartOfIppMetadata(request);
+  if (start_of_ipp_metadata < 0) {
+    return false;
   }
 
-  // Advance to start and check existence or end of request.
-  idx += strlen(kIppSentinel);
-  return idx <= request.size() ? idx : -1;
+  *http_metadata =
+      ipp_converter::ConvertToString(request.first(start_of_ipp_metadata));
+  *ipp_metadata = request.subspan(start_of_ipp_metadata);
+  return true;
 }
 
-base::Optional<std::vector<std::string>> ExtractRequestLine(
+base::Optional<std::vector<std::string>> ExtractHttpRequestLine(
     base::StringPiece request) {
-  int end_of_request_line = LocateEndOfRequestLine(request);
+  size_t end_of_request_line = LocateEndOfRequestLine(request);
   if (end_of_request_line < 0) {
     return base::nullopt;
   }
@@ -97,14 +112,14 @@ base::Optional<std::vector<std::string>> ExtractRequestLine(
   return ipp_converter::ParseRequestLine(request_line_slice);
 }
 
-base::Optional<std::vector<HttpHeader>> ExtractHeaders(
+base::Optional<std::vector<HttpHeader>> ExtractHttpHeaders(
     base::StringPiece request) {
-  int start_of_headers = LocateStartOfHeaders(request);
+  size_t start_of_headers = LocateStartOfHeaders(request);
   if (start_of_headers < 0) {
     return base::nullopt;
   }
 
-  int end_of_headers = LocateEndOfHeaders(request);
+  size_t end_of_headers = LocateEndOfHeaders(request);
   if (end_of_headers < 0) {
     return base::nullopt;
   }
@@ -114,32 +129,24 @@ base::Optional<std::vector<HttpHeader>> ExtractHeaders(
   return ipp_converter::ParseHeaders(headers_slice);
 }
 
-mojom::IppMessagePtr ExtractIppMessage(base::StringPiece request) {
-  int start_of_ipp_message = LocateStartOfIppMessage(request);
-  if (start_of_ipp_message < 0) {
-    return nullptr;
-  }
-
-  std::vector<uint8_t> ipp_slice =
-      ipp_converter::ConvertToByteBuffer(request.substr(start_of_ipp_message));
-  printing::ScopedIppPtr ipp = ipp_converter::ParseIppMessage(ipp_slice);
+mojom::IppMessagePtr ExtractIppMessage(base::span<const uint8_t> ipp_metadata) {
+  printing::ScopedIppPtr ipp = ipp_converter::ParseIppMessage(ipp_metadata);
   if (!ipp) {
     return nullptr;
   }
 
   return ipp_converter::ConvertIppToMojo(ipp.get());
 }
 
-// Parse IPP request's |ipp_data|
-base::Optional<std::vector<uint8_t>> ExtractIppData(base::StringPiece request) {
-  size_t start_of_ipp_data = LocateStartOfIppData(request);
+base::Optional<std::vector<uint8_t>> ExtractIppData(
+    base::span<const uint8_t> ipp_metadata) {
+  auto start_of_ipp_data = LocateStartOfIppData(ipp_metadata);
   if (start_of_ipp_data < 0) {
     return base::nullopt;
   }
 
-  // Subtlety: Correctly generates empty buffers for requests without ipp_data.
-  const base::StringPiece ipp_data_slice = request.substr(start_of_ipp_data);
-  return ipp_converter::ConvertToByteBuffer(ipp_data_slice);
+  ipp_metadata = ipp_metadata.subspan(start_of_ipp_data);
+  return std::vector<uint8_t>{ipp_metadata.begin(), ipp_metadata.end()};
 }
 
 }  // namespace
@@ -150,50 +157,54 @@ IppParser::IppParser(
 
 IppParser::~IppParser() = default;
 
-// Checks that |to_parse| is a correctly formatted IPP request, per RFC2910.
-// Calls |callback| with a fully parsed IPP request on success, empty on
-// failure.
-void IppParser::ParseIpp(const std::string& to_parse,
+void IppParser::ParseIpp(const std::vector<uint8_t>& to_parse,
                          ParseIppCallback callback) {
-  // Parse Request line
-  auto request_line = ExtractRequestLine(to_parse);
+  // Separate |to_parse| into http metadata (interpreted as ASCII chars), and
+  // ipp metadata (interpreted as arbitrary bytes).
+  std::string http_metadata;
+  base::span<const uint8_t> ipp_metadata;
+  if (!SplitRequestMetadata(to_parse, &http_metadata, &ipp_metadata)) {
+    return Fail("Failed to split HTTP and IPP metadata", std::move(callback));
+  }
+
+  // Parse Request line.
+  auto request_line = ExtractHttpRequestLine(http_metadata);
   if (!request_line) {
     return Fail("Failed to parse request line", std::move(callback));
   }
 
-  // Parse Headers
-  auto headers = ExtractHeaders(to_parse);
+  // Parse Headers.
+  auto headers = ExtractHttpHeaders(http_metadata);
   if (!headers) {
     return Fail("Failed to parse headers", std::move(callback));
   }
 
-  // Parse IPP message
-  auto ipp_message = ExtractIppMessage(to_parse);
+  // Parse IPP message.
+  auto ipp_message = ExtractIppMessage(ipp_metadata);
   if (!ipp_message) {
     return Fail("Failed to parse IPP message", std::move(callback));
   }
 
-  // Parse IPP data
-  auto ipp_data = ExtractIppData(to_parse);
+  // Parse IPP data.
+  auto ipp_data = ExtractIppData(ipp_metadata);
   if (!ipp_data) {
     return Fail("Failed to parse IPP data", std::move(callback));
   }
 
-  // Marshall response
+  // Marshall response.
   mojom::IppRequestPtr parsed_request = mojom::IppRequest::New();
 
-  std::vector<std::string> request_line_terms = request_line.value();
+  std::vector<std::string> request_line_terms = *request_line;
   parsed_request->method = request_line_terms[0];
   parsed_request->endpoint = request_line_terms[1];
   parsed_request->http_version = request_line_terms[2];
 
-  parsed_request->headers = std::move(headers.value());
+  parsed_request->headers = std::move(*headers);
   parsed_request->ipp = std::move(ipp_message);
-  parsed_request->data = std::move(ipp_data.value());
+  parsed_request->data = std::move(*ipp_data);
 
   DVLOG(1) << "Finished parsing IPP request.";
-  std::move(callback).Run(mojom::IppParserResult::SUCCESS,
-                          std::move(parsed_request));
+  std::move(callback).Run(std::move(parsed_request));
 }
 
 }  // namespace chrome

chrome/services/cups_ipp_parser/ipp_parser.h

@@ -10,22 +10,24 @@
 
 namespace chrome {
 
-// This class implements the chrome.mojom.IppParser interface.
-// It is intended to operate under the heavily jailed, out-of-process
-// cups_ipp_parser service, parsing incoming requests before passing
-// them to CUPS.
+// chrome.mojom.IppParser handler.
+//
+// This handler accepts incoming IPP requests as arbitrary buffers, parses
+// the contents using libCUPS, and yields a chrome::mojom::IppRequest. It is
+// intended to operate under the heavily jailed, out-of-process CupsIppParser
+// Service.
 class IppParser : public chrome::mojom::IppParser {
  public:
   explicit IppParser(
       std::unique_ptr<service_manager::ServiceContextRef> service_ref);
   ~IppParser() override;
 
  private:
-  // chrome::mojom::IppParser
+  // chrome::mojom::IppParser override.
   // Checks that |to_parse| is formatted as a valid IPP request, per RFC2910
   // Calls |callback| with a fully parsed IPP request on success, empty on
   // failure.
-  void ParseIpp(const std::string& to_parse,
+  void ParseIpp(const std::vector<uint8_t>& to_parse,
                 ParseIppCallback callback) override;
 
   const std::unique_ptr<service_manager::ServiceContextRef> service_ref_;

chrome/services/cups_ipp_parser/public/cpp/ipp_converter.cpp

@@ -378,4 +378,9 @@ std::vector<char> ConvertToCharBuffer(base::span<const uint8_t> byte_buffer) {
   return char_buffer;
 }
 
+std::string ConvertToString(base::span<const uint8_t> byte_buffer) {
+  std::vector<char> char_buffer = ConvertToCharBuffer(byte_buffer);
+  return std::string(char_buffer.begin(), char_buffer.end());
+}
+
 }  // namespace ipp_converter

chrome/services/cups_ipp_parser/public/cpp/ipp_converter.h

@@ -91,6 +91,7 @@ chrome::mojom::IppMessagePtr ConvertIppToMojo(ipp_t* ipp);
 // Common converters for working with arbitrary byte buffers.
 std::vector<uint8_t> ConvertToByteBuffer(base::StringPiece char_buffer);
 std::vector<char> ConvertToCharBuffer(base::span<const uint8_t> byte_buffer);
+std::string ConvertToString(base::span<const uint8_t> byte_buffer);
 
 }  // namespace ipp_converter
 

chrome/services/cups_ipp_parser/public/mojom/ipp_parser.mojom

@@ -2,17 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Interface defining an IPP, Internet Printing Protocol, validator to check
-// the format of incoming requests before passing them to CUPS.
+// Defines a mojom representation for an IPP, Internet Printing Protocol,
+// request, IppRequest, and an interface for parsing arbitrary requests into
+// this format, IppParser.
 
 module chrome.mojom;
 
-[Extensible]
-enum IppParserResult {
-  SUCCESS = 0,
-  FAILURE = 1
-};
-
 [Extensible]
 enum ValueType {
   BOOLEAN,
@@ -53,6 +48,10 @@ struct IppRequest {
   array<uint8> data;
 };
 
+// Implemented by the CupsIppParserService; supports parsing arbitrary IPP
+// requests.
 interface IppParser {
-  ParseIpp(string to_parse) => (IppParserResult success, IppRequest? request);
+  // Treats |to_parse| as an IPP request and attempts to parse into IppRequest
+  // format. Returns empty IppRequest on failure.
+  ParseIpp(array<uint8> to_parse) => (IppRequest? request);
 };