Adding OCSP Verification

Adding OCSP Verification

BUG=

[svaldez, 2016-03-31 15:11:13]

svaldez@chromium.org changed reviewers:
+ eroman@chromium.org, rsleevi@chromium.org

[svaldez, 2016-03-31 15:11:13]

This is the verification code that was pulled out of the OCSP Parsing CL.

[eroman, 2016-04-06 00:25:40]

Will be slow to get to this, but haven't forgotten :)

[svaldez, 2016-05-02 20:23:14]

On 2016/04/06 00:25:40, eroman wrote:
> Will be slow to get to this, but haven't forgotten :)

Just checking on this. Not super high priority, but it would be nice to get this
through so that it exists for the Must-Staple work.

[eroman, 2016-05-02 22:36:51]

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
File net/cert/internal/parse_ocsp.cc (right):

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.cc:539: // Checks that the ResponderID |id| matches
the certificate |cert|.
Same comment here -- justify what parts of the standard this is implementing.

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.cc:541: const ParsedTbsCertificate& cert) {
WARN_UNUSED_RESULT

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.cc:545: if
(!der::Parser(id.name).ReadTag(der::kSequence, &name_rdn) ||
Is this sufficient?
The assumption is that id.name is a single SEQUENCE right?
However it doesn't look like the parsing code for ResponderID enforces that
(i.e. I believe there can be unconsumed in it here).

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.cc:549: } else {
don't include the else after a short-circuit.

Also would this make more sense as a switch rather than if/else, in case the
enum for for some reason changed?

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.cc:553: if (!parser.ReadSequence(&spki_parser))
Same thing as elsewhere -- describe intent.
Looks like you are doing SPKI parsing here.

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.cc:561: HashValue key_hash(HASH_VALUE_SHA1);
I am not familiar with OCSP (beyond what I read to review this CL just now)...
is this a problem that the protocol implicitly uses SHA1 here?

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.cc:569: bool VerifyOCSPResponse(const OCSPResponse&
response,
Please documentation throughout this implementation that references the
normative requirements in the standard these are implementing.

This is hard to read as written since I don't know what parts of the standard it
is aiming to implement, or review for omissions.

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.cc:571: SimpleSignaturePolicy
signature_policy(1024);
Why this signature policy? 1024-bit RSA is weak. Does this need to be baked in
here, or user supplied?

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.cc:573: OCSPResponseData response_data;
I didn't review the rest of this in detail -- will wait for spec references and
comments.

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
File net/cert/internal/parse_ocsp.h (right):

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.h:281: // path to the issuer |issuer_cert|.
Expand the documentation to reference the RFC that this implementation follows
-- presumably RFC 6960.

Also mention this information in the CL description.

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.h:284: const ParsedCertificate& issuer_cert);
Please mark this as WARN_UNUSED_RESULT.

In fact I recommend the same for all the other functions in this file...
But especially this one.

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.h:288: #endif  // NET_CERT_INTERNAL_PARSE_OCSP_H_
parse_ocsp.h isn't the best name for this file given it handles both parsing and
verification.

At a minimum please add a TODO by VerifyOCSPResponse to clean this up (we will
need to do a file re-organization soon anyway).

[svaldez, 2016-05-04 15:31:20]

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
File net/cert/internal/parse_ocsp.cc (right):

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.cc:539: // Checks that the ResponderID |id| matches
the certificate |cert|.
On 2016/05/02 22:36:50, eroman wrote:
> Same comment here -- justify what parts of the standard this is implementing.

Done.

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.cc:541: const ParsedTbsCertificate& cert) {
On 2016/05/02 22:36:50, eroman wrote:
> WARN_UNUSED_RESULT

Done.

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.cc:545: if
(!der::Parser(id.name).ReadTag(der::kSequence, &name_rdn) ||
On 2016/05/02 22:36:50, eroman wrote:
> Is this sufficient?
> The assumption is that id.name is a single SEQUENCE right?
> However it doesn't look like the parsing code for ResponderID enforces that
> (i.e. I believe there can be unconsumed in it here).

The outer name I believe is guaranteed to be a single sequence containing a
bunch of RDNs.

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.cc:549: } else {
On 2016/05/02 22:36:50, eroman wrote:
> don't include the else after a short-circuit.
> 
> Also would this make more sense as a switch rather than if/else, in case the
> enum for for some reason changed?

Fixed. Due to the lack of extensability with the ASN.1 structure here, we're
guaranteed for this not to change.

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.cc:553: if (!parser.ReadSequence(&spki_parser))
On 2016/05/02 22:36:50, eroman wrote:
> Same thing as elsewhere -- describe intent.
> Looks like you are doing SPKI parsing here.

Done.

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.cc:561: HashValue key_hash(HASH_VALUE_SHA1);
On 2016/05/02 22:36:51, eroman wrote:
> I am not familiar with OCSP (beyond what I read to review this CL just now)...
> is this a problem that the protocol implicitly uses SHA1 here?
This is safe since we are only using the ResponderID to look up the correct
certificate, instead of any sort of verification. If there's a collision
(accidental or malicious), an attacker would also need to forge the actual OCSP
response signature from an *authorized* responder.

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.cc:569: bool VerifyOCSPResponse(const OCSPResponse&
response,
On 2016/05/02 22:36:51, eroman wrote:
> Please documentation throughout this implementation that references the
> normative requirements in the standard these are implementing.
> 
> This is hard to read as written since I don't know what parts of the standard
it
> is aiming to implement, or review for omissions.

Done.

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.cc:571: SimpleSignaturePolicy
signature_policy(1024);
On 2016/05/02 22:36:51, eroman wrote:
> Why this signature policy? 1024-bit RSA is weak. Does this need to be baked in
> here, or user supplied?

Done.

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.cc:573: OCSPResponseData response_data;
On 2016/05/02 22:36:50, eroman wrote:
> I didn't review the rest of this in detail -- will wait for spec references
and
> comments.

Done.

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
File net/cert/internal/parse_ocsp.h (right):

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.h:281: // path to the issuer |issuer_cert|.
On 2016/05/02 22:36:51, eroman wrote:
> Expand the documentation to reference the RFC that this implementation follows
> -- presumably RFC 6960.
> 
> Also mention this information in the CL description.

Done.

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.h:284: const ParsedCertificate& issuer_cert);
On 2016/05/02 22:36:51, eroman wrote:
> Please mark this as WARN_UNUSED_RESULT.
> 
> In fact I recommend the same for all the other functions in this file...
> But especially this one.

Done.

https://codereview.chromium.org/1849773002/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_ocsp.h:288: #endif  // NET_CERT_INTERNAL_PARSE_OCSP_H_
On 2016/05/02 22:36:51, eroman wrote:
> parse_ocsp.h isn't the best name for this file given it handles both parsing
and
> verification.
> 
> At a minimum please add a TODO by VerifyOCSPResponse to clean this up (we will
> need to do a file re-organization soon anyway).

Done.

[svaldez, 2016-05-04 16:33:08]

The CQ bit was checked by svaldez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-04 16:33:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1849773002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1849773002/40001

[commit-bot: I haz the power, 2016-05-04 16:45:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-04 16:46:00]

Dry run: Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[svaldez, 2016-05-31 14:59:19]

Friendly ping and adding dadrian as an FYI.

[eroman, 2016-05-31 19:12:47]

https://codereview.chromium.org/1849773002/diff/60001/net/cert/internal/ocsp.cc
File net/cert/internal/ocsp.cc (right):

https://codereview.chromium.org/1849773002/diff/60001/net/cert/internal/ocsp....
net/cert/internal/ocsp.cc:547: WARN_UNUSED_RESULT bool CheckResponder(const
OCSPResponseData::ResponderID& id,
Can you use a more specific name -- how about ResponderIdMatches(ID, Cert), to
reflect what check on the responder it is doing? (In particular it was confusing
when seeing the caller, as I thought the actual responder check that the "else"
branch was doing would go here).

https://codereview.chromium.org/1849773002/diff/60001/net/cert/internal/ocsp....
net/cert/internal/ocsp.cc:552: if (!der::Parser(id.name).ReadTag(der::kSequence,
&name_rdn) ||
Can you add documentation for ResponderID indicating this assumption? (i.e. that
ResponderID after parsing is a single SEQUENCE?)

https://codereview.chromium.org/1849773002/diff/60001/net/cert/internal/ocsp....
net/cert/internal/ocsp.cc:556: }
For future proofing my preference is a switch(). But could also use this
structure and add an else or a DCHECK() for the remaining possibility for
id.type.

https://codereview.chromium.org/1849773002/diff/60001/net/cert/internal/ocsp....
net/cert/internal/ocsp.cc:558: // Otherwise we extract the SPKI and compare the
public key hash to key
avoid "we" in comments.

https://codereview.chromium.org/1849773002/diff/60001/net/cert/internal/ocsp....
net/cert/internal/ocsp.cc:600: // authority, the OCSP Response will contain the
responder certificate.
Is there any normative reference for this logic?

I couldn't find it in 6960 at least, which just said:

   The responder MAY include certificates in the certs field of
   BasicOCSPResponse that help the OCSP client verify the responder's
   signature.

Which says nothing about their ordering, validity, or interpretation.

This code checks only signatures of certificates (nothing else, including
presence of critical unsupported extensions).

Also, should it permit a chain of certs from the issuer to the responder? (here
the responder is assumed to be directly signed by issuer).

https://codereview.chromium.org/1849773002/diff/60001/net/cert/internal/ocsp....
net/cert/internal/ocsp.cc:642: break;
Can you pull this out to a separate function instead of the "break" and "found"
logic?

https://codereview.chromium.org/1849773002/diff/60001/net/cert/internal/ocsp.h
File net/cert/internal/ocsp.h (right):

https://codereview.chromium.org/1849773002/diff/60001/net/cert/internal/ocsp....
net/cert/internal/ocsp.h:289: NET_EXPORT_PRIVATE bool VerifyOCSPResponse(
IMPORTANT: How does this prevent replay attacks?

I don't see the use of a nonce, or enforcement of response time anywhere, which
implies an OCSP response could simply be replayed and accepted here ad
infinitum, after it has been revoked.

Should this function take a timestamp ?

https://codereview.chromium.org/1849773002/diff/60001/net/cert/internal/ocsp_...
File net/cert/internal/ocsp_unittest.cc (right):

https://codereview.chromium.org/1849773002/diff/60001/net/cert/internal/ocsp_...
net/cert/internal/ocsp_unittest.cc:34: OCSPFailure ParseOCSP(const std::string&
file_name) {
Please rename this as well, to reflect the other changes.

(i.e. call it VerifyOCSP() or something).

https://codereview.chromium.org/1849773002/diff/60001/net/cert/internal/ocsp_...
net/cert/internal/ocsp_unittest.cc:65: const SimpleSignaturePolicy policy(1024);
Do we need to allow 1024-bit RSA keys in these tests? (which are known to be
weak).

[eroman, 2017-09-19 20:18:07]

Unless you plan on reviving this CL, I will take it over as I am doing work in
this area.