Adding OCSP Parser

net/cert/internal/parse_ocsp.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef NET_CERT_INTERNAL_PARSE_OCSP_H_
+#define NET_CERT_INTERNAL_PARSE_OCSP_H_
+
+#include <string>
+#include <vector>
+
+#include "base/memory/scoped_ptr.h"
+#include "net/base/hash_value.h"
+#include "net/cert/internal/parse_certificate.h"
+#include "net/cert/internal/signature_algorithm.h"
+#include "net/der/input.h"
+#include "net/der/parse_values.h"
+#include "net/der/parser.h"
+#include "net/der/tag.h"
+
+namespace net {
+
+// OCSPCertID contains a representation of a DER-encoded RFC 6960 "CertID".
+//
+// CertID ::= SEQUENCE {
+//    hashAlgorithm           AlgorithmIdentifier,
+//    issuerNameHash          OCTET STRING, -- Hash of issuer's DN
+//    issuerKeyHash           OCTET STRING, -- Hash of issuer's public key
+//    serialNumber            CertificateSerialNumber
+// }
+struct OCSPCertID {
+  OCSPCertID();
+  ~OCSPCertID();
+
+  DigestAlgorithm hash_algorithm;
+  der::Input issuer_name_hash;
+  der::Input issuer_key_hash;
+  der::Input serial_number;
+};
+
+// OCSPCertStatus contains a representation of a DER-encoded RFC 6960
+// "CertStatus".
+//
+// CertStatus ::= CHOICE {
+//      good        [0]     IMPLICIT NULL,
+//      revoked     [1]     IMPLICIT RevokedInfo,
+//      unknown     [2]     IMPLICIT UnknownInfo
+// }
+//
+// RevokedInfo ::= SEQUENCE {
+//      revocationTime              GeneralizedTime,
+//      revocationReason    [0]     EXPLICIT CRLReason OPTIONAL
+// }
+//
+// UnknownInfo ::= NULL
+struct OCSPCertStatus {
+  enum class Status {
+    GOOD,
+    REVOKED,
+    UNKNOWN,
+  };
+
+  enum class RevocationReason {

[eroman, 2016/02/13 00:56:50]

Where is this specified?

Same comment as the other enum: When relying on explicit values of the enum
provide them in the definition rather than relying on their definition order.

[svaldez, 2016/02/16 17:25:11]

Done.

+    UNSPECIFIED,
+    KEY_COMPROMISE,
+    CA_COMPROMISE,
+    AFFILIATION_CHANGED,
+    SUPERSEDED,
+    CESSATION_OF_OPERATION,
+    CERTIFICATE_HOLD,
+    UNUSED,
+    REMOVE_FROM_CRL,
+    PRIVILEGE_WITHDRAWN,
+    A_COMPROMISE,
+
+    REVOCATION_REASON_MAX
+  };
+
+  Status status;
+  der::GeneralizedTime revocation_time;
+  bool has_reason;
+  RevocationReason revocation_reason;
+};
+
+// OCSPSingleResponse contains a representation of a DER-encoded RFC 6960
+// "SingleResponse". The 'certID' and 'singleExtensions' fields are pointers

[eroman, 2016/02/13 00:56:50]

Would be clearer to reference them by name |cert_id_tlv|, |extensions|

[svaldez, 2016/02/16 17:25:11]

Done.

+// to the original object and are only valid as long as it is alive. They also
+// aren't verified until they are parsed.
+//
+// SingleResponse ::= SEQUENCE {
+//      certID                       CertID,
+//      certStatus                   CertStatus,
+//      thisUpdate                   GeneralizedTime,
+//      nextUpdate         [0]       EXPLICIT GeneralizedTime OPTIONAL,
+//      singleExtensions   [1]       EXPLICIT Extensions OPTIONAL
+// }
+struct OCSPSingleResponse {
+  OCSPSingleResponse();
+  ~OCSPSingleResponse();
+
+  der::Input cert_id_tlv;
+  OCSPCertStatus cert_status;
+  der::GeneralizedTime this_update;
+  bool has_next_update;
+  der::GeneralizedTime next_update;
+  bool has_extensions;
+  der::Input extensions;
+};
+
+// OCSPResponseData contains a representation of a DER-encoded RFC 6960
+// "ResponseData". The 'responses' and 'extensions' fields are pointers to the

[eroman, 2016/02/13 00:56:50]

Are these referring to the RFC 6960 names, or the code names? Because either
'responses' or 'extensions' is inconsistent in that case. If referencing
variable names surround with |bars|.

[svaldez, 2016/02/16 17:25:11]

Done.

+// original object and are only valid as long as it is alive. They also aren't
+// verified until they are parsed into OCSPSingleResponse and ParsedExtensions.
+//
+// ResponseData ::= SEQUENCE {
+//      version              [0] EXPLICIT Version DEFAULT v1,
+//      responderID              ResponderID,
+//      producedAt               GeneralizedTime,
+//      responses                SEQUENCE OF SingleResponse,
+//      responseExtensions   [1] EXPLICIT Extensions OPTIONAL
+// }
+struct OCSPResponseData {
+  enum class ResponderType { NAME, KEY_HASH };
+
+  struct ResponderID {
+    ResponderType type;
+    der::Input name;
+    HashValue key_hash;
+  };
+
+  OCSPResponseData();
+  ~OCSPResponseData();
+
+  uint8_t version;
+  OCSPResponseData::ResponderID responder_id;
+  der::GeneralizedTime produced_at;
+  std::vector<der::Input> responses;
+  bool has_extensions;
+  der::Input extensions;
+};
+
+// OCSPResponse contains a representation of a DER-encoded RFC 6960
+// "OCSPResponse" and the corresponding "BasicOCSPResponse". The 'data' field

[eroman, 2016/02/13 00:56:50]

'data' --> |data| ?

[svaldez, 2016/02/16 17:25:11]

Done.

+// is a pointer to the original object and are only valid as long is it is
+// alive. The 'data' field isn't verified until it is parsed into an

[eroman, 2016/02/13 00:56:50]

same

[svaldez, 2016/02/16 17:25:11]

Done.

+// OCSPResponseData.
+//
+// OCSPResponse ::= SEQUENCE {
+//      responseStatus         OCSPResponseStatus,
+//      responseBytes          [0] EXPLICIT ResponseBytes OPTIONAL
+// }
+//
+// BasicOCSPResponse       ::= SEQUENCE {
+//      tbsResponseData      ResponseData,
+//      signatureAlgorithm   AlgorithmIdentifier,
+//      signature            BIT STRING,
+//      certs            [0] EXPLICIT SEQUENCE OF Certificate OPTIONAL
+// }
+struct NET_EXPORT OCSPResponse {
+  enum class ResponseStatus {
+    SUCCESSFUL,
+    MALFORMED_REQUEST,
+    INTERNAL_ERROR,
+    TRY_LATER,
+    SIG_REQUIRED,
+    UNAUTHORIZED,
+
+    RESPONSE_STATUS_MAX,

[eroman, 2016/02/13 00:56:50]

Several things here:
  (1) Since this relies on the enum value for de-serialization, write the values
explicitly (protects against someone deciding to re-order these, and makes it
clear the correspondence).

  (2) Use a LAST value rather than *_MAX. I generally don't like introducing
dummy enum values since it means they have to be accounted for in a switch()
statement. Whereas aliasing the same value doesn't have that issue, albeit
requires a manual update.

   LAST = UNAUTHORIZED,

[svaldez, 2016/02/16 17:25:11]

Done.

+  };
+
+  OCSPResponse();
+  ~OCSPResponse();
+
+  ResponseStatus status;
+  der::Input data;
+  scoped_ptr<SignatureAlgorithm> signature_algorithm;
+  der::BitString signature;
+  std::vector<der::Input> certs;
+};
+
+// From RFC 6960:
+//
+// id-pkix-ocsp           OBJECT IDENTIFIER ::= { id-ad-ocsp }
+// id-pkix-ocsp-basic     OBJECT IDENTIFIER ::= { id-pkix-ocsp 1 }
+//
+// In dotted notation: 1.3.6.1.5.5.7.48.1.1
+NET_EXPORT der::Input BasicOCSPResponseOid();
+
+// Parses a DER-encoded OCSP "CertID" as specified by RFC 6960. Returns true on
+// success and sets the results in |out|.
+//
+// On failure |out| has an undefined state. Some of its fields may have been
+// updated during parsing, whereas others may not have been changed.
+NET_EXPORT_PRIVATE bool ParseOCSPCertID(der::Input raw_tlv, OCSPCertID* out);
+
+// Parses a DER-encoded OCSP "SingleResponse" as specified by RFC 6960. Returns
+// true on success and sets the results in |out|. The resulting |out|
+// references data from |raw_tlv| and is only valid for the lifetime of
+// |raw_tlv|.
+//
+// On failure |out| has an undefined state. Some of its fields may have been
+// updated during parsing, whereas others may not have been changed.
+NET_EXPORT_PRIVATE bool ParseOCSPSingleResponse(der::Input raw_tlv,

[eroman, 2016/02/13 00:56:50]

Plz change to |const der::Input&| for consistency with the other callsites that
largely use const der::Input&

(Performance difference is pretty "meh", so I won't argue on those grounds.)

[svaldez, 2016/02/16 17:25:11]

Done.

+                                                OCSPSingleResponse* out);
+
+// Parses a DER-encoded OCSP "ResponseData" as specified by RFC 6960. Returns
+// true on success and sets the results in |out|. The resulting |out|
+// references data from |raw_tlv| and is only valid for the lifetime of
+// |raw_tlv|.
+//
+// On failure |out| has an undefined state. Some of its fields may have been
+// updated during parsing, whereas others may not have been changed.
+NET_EXPORT_PRIVATE bool ParseOCSPResponseData(der::Input raw_tlv,
+                                              OCSPResponseData* out);
+
+// Parses a DER-encoded "OCSPResponse" as specified by RFC 6960. Returns true
+// on success and sets the results in |out|. The resulting |out|
+// references data from |ocsp_response| and is only valid for the lifetime of
+// |ocsp_response|.
+//
+// On failure |out| has an undefined state. Some of its fields may have been
+// updated during parsing, whereas others may not have been changed.
+NET_EXPORT_PRIVATE bool ParseOCSPResponse(der::Input ocsp_response,
+                                          OCSPResponse* out);
+
+// Verifies that the OCSP Response |response| is signed and has a valid trust
+// path to the issuer |issuer_cert|.
+NET_EXPORT_PRIVATE bool VerifyOCSPResponse(
+    const OCSPResponse* response,

[eroman, 2016/02/13 00:56:50]

(1) Why pointers instead of references? What is the relevance of nullptr?
(2) I don't think this belongs in parse_ocsp.h as it is not a parsing routine
(3) Please split this off to a separate changelist. It will need tests of a
different nature, and I would prefer to review it separately.

[svaldez, 2016/02/16 17:25:11]

I'll split it off to a separate CL once I'm done running/fixing all the Alexa
sites through the parser. I'm not a fan of having both a parse_ocsp and
verify_ocsp file, since users should never be able to do one without the other,
and parsing will still need an issuer and certificate to be provided, in order
to parse out the correct OCSP responses. Would it make more sense to rename the
files to be ocsp_response and then just have all the OCSP related code in there?

[eroman, 2016/02/16 23:42:25]

I think there can be a logical separation between parsing and verification. But
we can discuss that more on the CL that adds the verification. I am fine with
changing the filenames in a later step.

+    const ParsedCertificate* issuer_cert);
+
+// Checks the certificate status of |cert| based on the OCSPResponseData
+// |response_data| and sets the results in |out|.
+//
+// On failure |out| has an undefined state. Some of its fields may have been
+// updated during parsing, whereas others may not have been changed.
+NET_EXPORT_PRIVATE bool GetOCSPCertStatus(const OCSPResponseData* response_data,

[eroman, 2016/02/13 00:56:50]

Why are the inputs pointers rather than references? If null is being interpreted
as having special significance it should be called out in the documentation.

[svaldez, 2016/02/16 17:25:11]

Done.

+                                          const ParsedCertificate* issuer,
+                                          const ParsedCertificate* cert,
+                                          OCSPCertStatus* out);
+
+}  // namespace net
+
+#endif  // NET_CERT_INTERNAL_PARSE_OCSP_H_