Closed Bug 901698 Opened 11 years ago Closed 9 years ago

implement OCSP-must-staple (off by default)

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: keeler, Assigned: mgoodwin)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 7 obsolete files)

OCSP-must-staple will basically be HSTS for OCSP stapling.

14:23   briansmith | the important point to add is that, when a site has the
                   | must-staple flag, we must not do TLS intolerance fallback
                   | past TLS 1.0
14:23   briansmith | because otherwise the server can't staple the OCSP
                   | response.
We can't turn this on until bug 915930 is fixed. I will file a separate bug for turning this on by default that depends on that bug.
Summary: implement OCSP-must-staple → implement OCSP-must-staple (off by default)
The dependency on bug 839310 was moved to bug 921907.
No longer depends on: 839310
Some clarification was requested about this feature, so:
> There seem to be
> two ideas for OCSP must-staple. One is basically HSTS but for OCSP
> stapling (i.e. it's an HTTP header) - this is what we're working on. The
> other seems to be an x509v3 extension, which is not what we're currently
> working on (although it wouldn't be too difficult, once we have the
> other pieces in place).

(Work towards a standard for the x509v3 extension is at http://tools.ietf.org/html/draft-hallambaker-muststaple-00 )
Using a preloaded list for must-staple suffers from the same scalability problem that affects the preloading for HSTS and Chrome's crlset. If must-staple is implemented that way it will never be useful for the majority of websites and can only improve the security of a select few large sites. The only scalable solution is the x509v3 extension.
I don't see why the x509v3 extension is anymore scalable than the HTTP-header*. AFAIK both the HTTP-header and the x509 solution "suffer" from TOFU like HSTS does. Still it would be an improvement if I could inform the visitors of my websites that subsequent requests must have a stapled OCSP response.

* unless the Must-Staple flag is set in some root certificates that ship with the browser.
I didn't mean to suggest that the HTTP header isn't scalable. It's problem (as you mention) is the first visit. I meant that HSTS preloading (to solve the first visit issue) and crlset aren't scalable (because you need to ship it with the binary). The x509 solution is of all the solution the only one which is both scalable and doesn't have the first visit problem.
I see, I didn't realize that the must-staple flag will be set in a stolen certificate.

Still I wonder if the header-route is the most realistic path into bringing this feature into reality. Can anyone elaborate on the status of standardizing any of these technologies? As mentioned in #921907 there is not much to find in the ietf drafts. I have the feeling there is not much progress, if any.
Update: contrary to my belief there is activity on the X.509 must-staple front. The X.509 extension draft is renamed from "OCSP Stapling Required" to "TLS Feature Extension". The most recent one is released three months ago: http://datatracker.ietf.org/doc/draft-hallambaker-tlsfeature/
I think the header route _is_ the most realistic path for making this happen. An x509 extension will only be implemented by a few CAs, and it's hard to imagine any CA making it the _default_ for certificate issuance in the next 5 years. Even if I purchase a certificate with the extension, what does it mean for my site? There is not mechanism for 'max-age' there, an attacker using a cert from another CA will _not_ require the staple.  

The IETF link at https://wiki.mozilla.org/CA:ImprovingRevocation#OCSP_Must-Staple was replying to me in 2013. Even had the x509 extension been standardized in a timely manner (it hasn't) it doesn't address the above shortcoming.

See also: https://www.ietf.org/mail-archive/web/websec/current/msg02297.html
Attached patch bug-901968.0.patch (obsolete) — Splinter Review
I think Tom makes some pretty good points here, so I would be willing to consider an HSTS-based solution to this problem.

However, I was on a plane this evening with a copy of draft-hallambaker-tlsfeature open, so I took a hack at implementing that flavor.  The result is this very drafty draft patch.

I have verified that this patch builds, but have not tested it with an actual certificate.
A couple of points about HSTS vs. X.509:

* It doesn't really matter that an attacker can swap in a cert without must-staple.  Must-staple is only an optimization that lets the browser fail faster.  If you're looking for the "must get revocation info" indicator, that's in the HSTS spec - https://tools.ietf.org/html/rfc6797#section-8.4 

* There's no reason that a site couldn't indicate must-staple BOTH in HSTS and in its certificate, and no real cost besides engineering in supporting them both.  In particular, there's no ambiguity in the signal to the browser; you just require stapling if either HSTS or the cert says so.

* I could envision some site operators objecting to having to do all the stuff HSTS requires, just to get the benefit of this optimization.  (I could also envision an argument for using this as an incentive to do HSTS, so maybe this is a wash.)

There may ultimately be some utility in adding a must-staple directive to HSTS, possibly along with other TLS policies.  But given the above, it seems worth moving forward with the certificate-based one anyway.

(BTW, the above assumes that header == HSTS.  I think that's right; we don't want to mint a whole new header just for this thing.)
Attached patch wip-bug-901698-testcases.patch (obsolete) — Splinter Review
WIP - This starts work on test cases, and forward-ports rbarnes' patch up to the current tip.

Note: The key test, mustStaple when the OCSP is not stapled, fails in this patch. Something's not quite happy yet.

Keeler: It won't be until next Wednesday that I can continue on this, but if you can in particular give me any feedback on the way I'm adding the extension in security/manager/ssl/tests/unit/tlsserver/generate_certs.sh, I'd be much obliged.
Assignee: dkeeler → jjones
Attachment #8609172 - Attachment is obsolete: true
Attachment #8650296 - Flags: feedback?(dkeeler)
Comment on attachment 8650296 [details] [diff] [review]
wip-bug-901698-testcases.patch

Review of attachment 8650296 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking good. I'm suggesting a slightly different approach (see comments). Let me know what you think when you get back to this.

::: security/manager/ssl/tests/unit/head_psm.js
@@ -62,5 @@
>  const SEC_ERROR_OCSP_INVALID_SIGNING_CERT               = SEC_ERROR_BASE + 144;
>  const SEC_ERROR_POLICY_VALIDATION_FAILED                = SEC_ERROR_BASE + 160; // -8032
>  const SEC_ERROR_OCSP_BAD_SIGNATURE                      = SEC_ERROR_BASE + 157;
>  const SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED       = SEC_ERROR_BASE + 176;
> -

I think we should keep this blank line for clarity (to separate the SEC_ERROR_s from the SSL_ERROR_s)

::: security/manager/ssl/tests/unit/tlsserver/generate_certs.sh
@@ +338,5 @@
>  make_EE_with_nsCertType nsCertTypeNotCritical 'CN=nsCertType Not Critical' testCA "localhost,*.example.com" "n"
>  make_EE_with_nsCertType nsCertTypeCriticalWithExtKeyUsage 'CN=nsCertType Critical With extKeyUsage' testCA "localhost,*.example.com" "y" "--extKeyUsage serverAuth"
>  
> +# ASN.1 encoding of ext-TLSFeatures with option 5, OCSP Must-Staple
> +echo -ne '\x30\x03\x02\x01\x05' > "$ASN1_MUSTSTAPLE"

This looks like it would work, but rather than put more effort into this, we should convert this directory to use pycert (see attachment 8646288 [details] [diff] [review] in bug 1183718 for part of what would need to change - the other part would be converting everything in this file to its own certspec/keyspec file). I realize that's a fair bit of work, but that's what we're moving towards anyway. (We should maybe do that in a separate bug that blocks this.)

::: security/pkix/lib/pkixcheck.cpp
@@ +970,5 @@
> +// (Empty extensions are also rejected.)
> +//
> +// If the mustStaple parameter is provided, then it is set to indicate
> +// whether OCSP stapling is required by the extension.
> +Result CheckRequiredTLSFeatures(const Input* requiredTLSFeatures,

nit: Result on its own line
We should also have some mozilla::pkix gtests for this. I'm thinking:
* successful case (i.e. just statusRequest)
* empty sequence
* requiredFeature != statusRequest
* multiple requiedFeatures
  * including statusRequest
  * not including statusRequest
  * duplicate statusRequests (what's the right thing to do in that case?)
  * duplicate non-statusRequest

@@ +971,5 @@
> +//
> +// If the mustStaple parameter is provided, then it is set to indicate
> +// whether OCSP stapling is required by the extension.
> +Result CheckRequiredTLSFeatures(const Input* requiredTLSFeatures,
> +                                /*optional out*/ bool* mustStaple) {

I don't think we need to pass this boolean out parameter around. Instead, we should pass in the stapledOCSPResponse from BuildForward and return an error if requiredFeature == statusRequest && !stapledOCSPResponse. We should probably also add a new error type for "required TLS feature OCSP stapling not present".

@@ +985,5 @@
> +  }
> +
> +  Reader input(*requiredTLSFeatures);
> +  return der::NestedOf(input, der::SEQUENCE, der::INTEGER,
> +                            der::EmptyAllowed::No, [&](Reader& r) {

nit: indentation
Attachment #8650296 - Flags: feedback?(dkeeler) → feedback+
Assignee: jjones → mgoodwin
draft-hallambaker-tlsfeature is now RFC 7633.

Assuming I'm reading it correctly, the official OID for the certificate extension is 1.3.6.1.5.5.7.0.51.24.  (The WIP patch uses a slightly different OID at the moment).
(In reply to Rob Stradling from comment #17)
> the official OID for the certificate extension is 1.3.6.1.5.5.7.0.51.24

Errm, no. 1.3.6.1.5.5.7.1.24 (as used in the patch) is correct, see https://www.iana.org/assignments/smi-numbers/smi-numbers.xhtml#smi-numbers-1.3.6.1.5.5.7.1.

1.3.6.1.5.5.7.0.51 is the OID for the PKIX1Explicit-2009 module for RFC 5280 (id-mod-pkix1-explicit-02 under https://www.iana.org/assignments/smi-numbers/smi-numbers.xhtml#smi-numbers-1.3.6.1.5.5.7.0 - see also RFC 7299).
Ah, you're right.  1.3.6.1.5.5.7.1.24 is the correct OID.  Thanks Kaspar.

(RFC7633 imports "id-pe" from the PKIX1Explicit-2009 module, but due to insufficient sleep or caffeine (or both) I'd misread the import as a definition).
Attached patch Bug 901698.patch (obsolete) — Splinter Review
I think I've addressed the comments on the previous approach - I'm still working on tests but feedback on this will be useful in the mean time.
Attachment #8650296 - Attachment is obsolete: true
Attachment #8681319 - Flags: feedback?(dkeeler)
Comment on attachment 8681319 [details] [diff] [review]
Bug 901698.patch

Review of attachment 8681319 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great - just a few comments.

::: security/manager/locales/en-US/chrome/pipnss/nsserrors.properties
@@ +318,5 @@
>  MOZILLA_PKIX_ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE=A certificate that is not yet valid was used to issue the server's certificate.
>  MOZILLA_PKIX_ERROR_SIGNATURE_ALGORITHM_MISMATCH=The signature algorithm in the signature field of the certificate does not match the algorithm in its signatureAlgorithm field.
>  MOZILLA_PKIX_ERROR_OCSP_RESPONSE_FOR_CERT_MISSING=The OCSP response does not include a status for the certificate being verified.
>  MOZILLA_PKIX_ERROR_VALIDITY_TOO_LONG=The server presented a certificate that is valid for too long.
> +MOZILLA_PKIX_ERROR_OCSP_STAPLING_NOT_PRESENT=Required TLS feature OCSP stapling not present

nit: these are generally full sentences (with the included period)

::: security/pkix/lib/pkixbuild.cpp
@@ +233,5 @@
>        return rv;
>      }
>      Duration validityDuration(notAfter, notBefore);
> +
> +    rv = CheckRequiredTLSFeatures(subject.GetRequiredTLSFeatures(),

I was expecting this to be in CheckIssuerIndependentProperties. Is it necessary for it to be here?

::: security/pkix/lib/pkixcheck.cpp
@@ +991,5 @@
> +Result
> +CheckRequiredTLSFeatures(const Input* requiredTLSFeatures,
> +                         const Input* stapledOCSPResponse) {
> +  // RFC 6066 10.2: ExtensionType status_request
> +  static uint8_t statusRequest = 5;

Maybe const? Also, maybe naming this "status_request" would better indicate that it's a special value rather than a variable.

@@ +999,5 @@
> +  }
> +
> +  Reader input(*requiredTLSFeatures);
> +  return der::NestedOf(input, der::SEQUENCE, der::INTEGER,
> +                            der::EmptyAllowed::No, [&](Reader& r) {

nit: alignment

@@ +1006,5 @@
> +    if (rv != Success) {
> +      return rv;
> +    }
> +
> +    if (requiredFeature == statusRequest) {

Instead of structuring the logic like this, let's test for each of the error conditions and return early.

i.e.:

if (requiredFeature != statusRequest) {
  return Result::ERROR...
}
if (!stapledOCSPResponse) {
  return Result::ERROR...
}
return Success;

@@ +1012,5 @@
> +        return Result::ERROR_OCSP_STAPLING_NOT_PRESENT;
> +      }
> +      return Success;
> +    } else {
> +      return Result::ERROR_EXTENSION_VALUE_INVALID;

Generally this library returns only one type of error per Check... function. We could just use a more generic "ERROR_REQUIRED_TLS_FEATURE_MISSING" error.
Attachment #8681319 - Flags: feedback?(dkeeler) → feedback+
Attached patch Bug 901698.patch (obsolete) — Splinter Review
Attachment #8681319 - Attachment is obsolete: true
Attachment #8682016 - Flags: feedback?(dkeeler)
Attached patch Bug 901698 tests.patch (obsolete) — Splinter Review
(In reply to David Keeler [:keeler] (use needinfo?) from comment #21)
> I was expecting this to be in CheckIssuerIndependentProperties. Is it
> necessary for it to be here?

Not specifically; the call site was inherited from jcj's patch. I can modify CheckIssuerIndependentProperties to pass the stapledOCSPResponse in from BuildForward if you'd prefer?
Attached patch Bug 901698.patch (obsolete) — Splinter Review
Moved the check to inside CheckIssuerIndependentProperties
Attachment #8682016 - Attachment is obsolete: true
Attachment #8682016 - Flags: feedback?(dkeeler)
Attachment #8682098 - Flags: feedback?(dkeeler)
Attached patch Bug 901698 tests.patch (obsolete) — Splinter Review
Attachment #8682018 - Attachment is obsolete: true
Bug 901698 - Implement OCSP-must-staple; r?keeler
Attachment #8682598 - Flags: review?(dkeeler)
Bug 901698 - Some tests for OCSP-must-staple; r?keeler
Attachment #8682599 - Flags: review?(dkeeler)
https://reviewboard.mozilla.org/r/24101/#review21579

(so I don't forget)

::: security/pkix/lib/pkixbuild.cpp:236
(Diff revision 1)
> +

Whitespace

::: security/pkix/lib/pkixocsp.cpp:127
(Diff revision 1)
> -                                        CertPolicyId::anyPolicy, 0,
> +                                        CertPolicyId::anyPolicy, 0, NULL,

nullptr
https://reviewboard.mozilla.org/r/24103/#review21581

::: security/manager/ssl/tests/unit/tlsserver/cmd/OCSPStaplingServer.cpp:80
(Diff revision 1)
> +      strstr(host->mHostName, "ocsp-stapling-must-staple") == host->mHostName) {

Indentation
Attachment #8682098 - Attachment is obsolete: true
Attachment #8682098 - Flags: feedback?(dkeeler)
Attachment #8682492 - Attachment is obsolete: true
Comment on attachment 8682598 [details]
MozReview Request: Bug 901698 - Implement OCSP-must-staple; r?keeler

https://reviewboard.mozilla.org/r/24101/#review21545

While reviewing this, I realized we might run into a bit of a problem with certificate verifications outside of the TLS handshake. That is, in the places other than the TLS handshake where we verify certificates, we don't know whether or not the original connection included a stapled OCSP response. In some cases it probably doesn't matter (e.g. an email signing/encryption certificate shouldn't have this extension, so it's fine if it fails to verify), but we might have to do something like stash the stapled response on the SSLStatus or something (in which case I imagine we'd want to save it in the cache as well).

::: security/pkix/lib/pkixcheck.h:60
(Diff revision 1)
> +Result CheckRequiredTLSFeatures(const Input* requiredTLSFeatures,

Does this need to be externally visible for the gtests? Otherwise, I don't think we want this here.

::: security/pkix/lib/pkixcheck.cpp:993
(Diff revision 1)
> +// The only feature that we can process a requirement for is OCSP stapling,

Might be good to add that "status_request" means OCSP stapling.

::: security/pkix/lib/pkixcheck.cpp:997
(Diff revision 1)
> +// If the mustStaple parameter is provided, then it is set to indicate

Is this an old comment? It doesn't seem relevant now.

::: security/pkix/lib/pkixcheck.cpp:1000
(Diff revision 1)
> +CheckRequiredTLSFeatures(const Input* requiredTLSFeatures,

Reading the spec again, it looks like this doesn't implement section 4.2.2 (i.e. nested required TLS features). Luckikly, since we only support status_request, we don't really have to do any set math, but I think we should enforce that if an ancestor CA has the extension, everything down to the EE should have it too.

::: security/pkix/lib/pkixcheck.cpp:1013
(Diff revision 1)
> +    Result rv = r.Read(requiredFeature);

It would be good to add a test that this code rejects the extension if it looks something like this:

30 04 02 02 05 05 (i.e. a SEQUENCE of INTEGER with value 05 05)
Attachment #8682598 - Flags: review?(dkeeler)
Comment on attachment 8682599 [details]
MozReview Request: Bug 901698 - Some tests for OCSP-must-staple; r?keeler

https://reviewboard.mozilla.org/r/24103/#review21563

::: security/manager/ssl/tests/unit/ocsp_certs/must-staple-ee.pem.certspec:3
(Diff revision 1)
> +extension:subjectAlternativeName:localhost,*.example.com,*.pinning.example.com,*.include-subdomains.pinning.example.com,*.exclude-subdomains.pinning.example.com

Let's just limit this to '\*.example.com'

::: security/manager/ssl/tests/unit/ocsp_certs/must-staple-ee.pem.certspec:4
(Diff revision 1)
> +extension:authorityInformationAccess:http://localhost:8888/

For now, since we don't really enforce the presence of the AIA, let's just leave it out.

::: security/manager/ssl/tests/unit/test_ocsp_stapling.js:166
(Diff revision 1)
> +                MOZILLA_PKIX_ERROR_REQUIRED_TLS_FEATURE_MISSING, false);

Hmmm - looking at the spec again, section 4.3.3 might indicate that if the client disables OCSP stapling, it's not a failure if the server certificate requires it but the server doesn't send it. The language isn't clear, though - it could either mean an intersection or union:

"A client MUST treat a certificate with a TLS feature extension as an invalid certificate if the features offered by the server do not contain all features present in both the client's ClientHello message and the TLS feature extension."

So, going back to the implementation, we may have to disable processing this extension if OCSP stapling is disabled. (And, to reflect this in the tests, if aStaplingEnabled is false, this should pass.)

::: security/manager/ssl/tests/unit/tlsserver/cmd/OCSPStaplingServer.cpp:80
(Diff revision 1)
> +      strstr(host->mHostName, "ocsp-stapling-must-staple") == host->mHostName) {

I think at this point we should just generalize this. That is, instead of overloading the mAdditionalCertName field for both generating an OCSP response and maybe using a different server certificate, we should have a separate field for the server certificate nickname (it can be nullptr by default, in which case the server will use DEFAULT_CERT_NICKNAME).

::: security/pkix/test/gtest/pkixcheck_CheckRequiredTLSFeatures_tests.cpp:39
(Diff revision 1)
> +  ByteString stapledOCSPResponse;

This is never not just 'ByteString()', right? I imagine we can just omit it and create it if necessary in CheckRequiredTLSFeatures, right?

::: security/pkix/test/gtest/pkixcheck_CheckRequiredTLSFeatures_tests.cpp:62
(Diff revision 1)
> +  0x30, 0x06, 0x02, 0x01, 0x06, 0x02, 0x01, 0x06

Like I said in the other review, we should have something like:

0x30, 0x04, 0x02, 0x02, 0x05, 0x05

(i.e. the INTEGER is 2 bytes, not 1)

Also, a 0-byte INTEGER might be good:

0x30, 0x02, 0x02, 0x00
Comment on attachment 8682598 [details]
MozReview Request: Bug 901698 - Implement OCSP-must-staple; r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24101/diff/1-2/
Attachment #8682598 - Flags: review?(dkeeler)
Attachment #8682599 - Flags: review?(dkeeler)
Comment on attachment 8682599 [details]
MozReview Request: Bug 901698 - Some tests for OCSP-must-staple; r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24103/diff/1-2/
https://reviewboard.mozilla.org/r/24103/#review21935

::: security/manager/ssl/tests/unit/test_ocsp_stapling.js:169
(Diff revision 2)
> +  //              PRErrorCodeSuccess, false);

This needs enabling again. Also need some tests for the ocsp-must-staple enabled patch
Attachment #8682599 - Flags: review?(dkeeler) → review+
Comment on attachment 8682599 [details]
MozReview Request: Bug 901698 - Some tests for OCSP-must-staple; r?keeler

https://reviewboard.mozilla.org/r/24103/#review22097

This looks good.

::: security/manager/ssl/tests/unit/tlsserver/cmd/OCSPStaplingServer.cpp:40
(Diff revision 2)
> -  { "ocsp-stapling-with-intermediate.example.com", ORTGood, "ocspEEWithIntermediate" },
> +  { "ocsp-stapling-with-intermediate.example.com", ORTGood, "ocspEEWithIntermediate", "ocspEEWithIntermediate" },

Can you investigate if the first "ocspEEWithIntermediate" parameter can be nullptr here?

::: security/pkix/test/gtest/pkixcheck_CheckRequiredTLSFeatures_tests.cpp:38
(Diff revision 2)
> +  bool includeTLSInfo;

Looks like includeTLSInfo is always false, so it doesn't seem necessary as a parameter.

::: security/pkix/test/gtest/pkixcheck_CheckRequiredTLSFeatures_tests.cpp:72
(Diff revision 2)
> +  0x30, 0x04, 0x02, 0x02, 0x00, 0x05

This case should fail - 00 05 is an invalid INTEGER encoding for DER.

::: security/pkix/test/gtest/pkixcheck_CheckRequiredTLSFeatures_tests.cpp:76
(Diff revision 2)
> +  0x30, 0x04, 0x02, 0x00

The length of the SEQUENCE should be 02 instead of 04 here.
Comment on attachment 8682598 [details]
MozReview Request: Bug 901698 - Implement OCSP-must-staple; r?keeler

https://reviewboard.mozilla.org/r/24101/#review22101

For anyone following along (and for posterity), we discussed modifying this to be more like a combination of CheckNameConstraints and CheckCertHostname. That is, there is one part that is checked during path building (i.e. if an issuing certificate has this extension, every certificate below it in the chain has it as well) and another part that is checked in VerifySSLServerCert (i.e. if the client asked for OCSP stapling and the end-entity certificate requires it, that the server included it).

::: security/manager/ssl/nsNSSComponent.cpp:881
(Diff revision 2)
> +  bool ocspMustStapleEnabled = Preferences::GetBool("security.ssl.enable_ocsp_must_staple",

This will need a corresponding line like https://dxr.mozilla.org/mozilla-central/rev/5dbceb9638a01828296241661ff3c644d894f118/security/manager/ssl/nsNSSComponent.cpp#1367

::: security/pkix/lib/pkixcheck.cpp:861
(Diff revision 2)
> +    // RFC6066 implies there could be two-byte ExtensionType values...

We talked about just matching exactly the bytes of the expected extension value, but maybe a middle-ground solution would be to keep most of this and then do r.MatchRest(status_request_bytes) (where status_request_bytes = [5]). (In particular, this will ensure that there are no extra bytes smuggled in there.)
Attachment #8682598 - Flags: review?(dkeeler)
Attachment #8682598 - Flags: review?(dkeeler)
Comment on attachment 8682598 [details]
MozReview Request: Bug 901698 - Implement OCSP-must-staple; r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24101/diff/2-3/
Comment on attachment 8682599 [details]
MozReview Request: Bug 901698 - Some tests for OCSP-must-staple; r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24103/diff/2-3/
I just remembered; I was using ERROR_POLICY_VALIDATION_FAILED as a placeholder for whatever the correct error should be in the case of a chain missing TLSFeature extensions on a child that's present for a parent.

This won't be the correct error so:
- I can use the REQUIRED_TLS_FEATURE_MISSING one (though technically the wording fits, it confusing that this could be a chain validation issue *or* a missing feature from the handshake)
- I can create a new one. 

Which would you prefer. If the latter, what should it be called?
Flags: needinfo?(dkeeler)
Comment on attachment 8682598 [details]
MozReview Request: Bug 901698 - Implement OCSP-must-staple; r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24101/diff/3-4/
Comment on attachment 8682599 [details]
MozReview Request: Bug 901698 - Some tests for OCSP-must-staple; r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24103/diff/3-4/
Comment on attachment 8682598 [details]
MozReview Request: Bug 901698 - Implement OCSP-must-staple; r?keeler

https://reviewboard.mozilla.org/r/24101/#review22389

I think this will be the best architecture going forward. I have a few suggestions for simplifications, however.

::: security/certverifier/CertVerifier.h:60
(Diff revision 4)
> +  static const Flags FLAG_OCSP_MUST_STAPLE_ENABLED;

I think it would be best to fail closed with these. That is, default to having them enabled. If we're in a context that doesn't have access to the TLS handshake data, we can pass a flag that says to not check for a stapled OCSP response.

::: security/certverifier/CertVerifier.cpp:606
(Diff revision 4)
> +  Input* responseInputPtr;

This should be initialized to nullptr.

::: security/certverifier/CertVerifier.cpp:615
(Diff revision 4)
> +    if (stapledOCSPResponseInput.GetLength() > 0) {

I'm not sure we need this check - the parts of the code that consume the stapled OCSP response should enforce the validity of the input. I would add a test for it, though.

::: security/certverifier/CertVerifier.cpp:622
(Diff revision 4)
> +      flags & FLAG_OCSP_MUST_STAPLE_ENABLED) {

It seems like we can collapse the two flag values into one, since if either is false, we won't enforce OCSP must staple.

::: security/certverifier/CertVerifier.cpp:626
(Diff revision 4)
> +  result = VerifyTLSFeaturesAreSatisfied(peerCertInput, responseInputPtr,

This should happen before CheckCertHostname, since BAD_CERT_DOMAIN is overridable.

::: security/certverifier/CertVerifier.cpp:627
(Diff revision 4)
> +                                         tlsFlags);

I mention this elsewhere, but I don't think we should pass the flags in to VerifyTLSFeaturesAreSatisfied. If ocsp stapling or must staple is disabled, we should just not call that function.

::: security/manager/ssl/SSLServerCertVerification.cpp:1236
(Diff revision 4)
>    rv = certVerifier.VerifySSLServerCert(cert, stapledOCSPResponse,

There are a few other places where we call VerifySSLServerCert (in particular, nsNSSSocketInfo::IsAcceptableForHost). We need to make sure we're passing in the correct flags there (or make the flags restrictive by default and assume that both stapling and must staple are enabled).

::: security/pkix/include/pkix/pkix.h:159
(Diff revision 4)
> +Result VerifyTLSFeaturesAreSatisfied(Input& cert,

In this area of code "verify" usually has to do with a signature. Since there's no signatures here, "check" might be a better verb (cf. CheckCertHostname)

::: security/pkix/include/pkix/pkix.h:161
(Diff revision 4)
> +                                     int flags);

I don't think we should pass flags into this function. If stapling or must staple is disabled, VerifySSLServerCert can just skip calling this function.

::: security/pkix/lib/pkixcheck.cpp:858
(Diff revision 4)
> +CheckTLSFeatures(const BackCert& firstChild) {

Technically we only need to check each (subject, potentialIssuer) pair, so I think we can simplify this (i.e. no loop is necessary).

::: security/pkix/lib/pkixcheck.cpp:871
(Diff revision 4)
> +        return Result::ERROR_POLICY_VALIDATION_FAILED;

I think using the same error code (i.e. using "ERROR_REQUIRED_TLS_FEATURE_MISSING") is ok.

::: security/pkix/lib/pkixcheck.cpp:877
(Diff revision 4)
> +    Result rv = hasFeature(*requiredTLSFeatures, status_request, seen);

This checks for the presence of status_request but not for the presence of any other features that may be in the parent cert's extension. For the time being, I think we can get away with just checking that if the parent has the extension then it is not empty and its contents match that of the child's extension.

::: security/pkix/lib/pkixverify.cpp:124
(Diff revision 4)
> +    Result rv = r.Read(requiredFeature);

I would do something more like this:

const static uint8_t status_request_bytes\[\] = \{ status_request \};

if (\!r.MatchRest(status_request_bytes)) \{ // (in particular, this makes sure there are no more bytes after the one we're interested in)
  return Result::ERROR_REQUIRED_TLS_FEATURE_MISSING;
\}

if (\!stapledOCSPResponse) \{
  return Result::ERROR_REQUIRED_TLS_FEATURE_MISSING;
\}

return Success;
Attachment #8682598 - Flags: review?(dkeeler)
Comment on attachment 8682599 [details]
MozReview Request: Bug 901698 - Some tests for OCSP-must-staple; r?keeler

https://reviewboard.mozilla.org/r/24103/#review22383

This looks good. I have some ideas for a few more things we should test, though.

::: security/manager/ssl/tests/unit/ocsp_certs/test-must-staple-int.pem.certspec:3
(Diff revision 4)
> +validity:20150101-20250101

Do we need to hard-code the validity here?

::: security/manager/ssl/tests/unit/pycert.py:549
(Diff revision 4)
> +        if feature != 'OCSPMustStaple':

It might be nice to have a testcase where an intermediate has the set of features \{5, 6\} (or something) but the end-entity only has \{5\} (or no extension at all, or an empty one).

::: security/pkix/test/gtest/pkixocsp_VerifyTLSFeaturesInternal_tests.cpp:40
(Diff revision 4)
> +  bool omitResponse;

How about instead of omitResponse we just try both cases in each testcase? That is, we can have an expectedResultWithResponse and expectedResultWithoutResponse or something.

::: security/pkix/test/gtest/pkixocsp_VerifyTLSFeaturesInternal_tests.cpp:41
(Diff revision 4)
> +  int flags;

As a result of changes in the other patch, I don't think we'll need the flags here.

::: security/pkix/test/gtest/pkixocsp_VerifyTLSFeaturesInternal_tests.cpp:113
(Diff revision 4)
> +    ByteString stapledOCSPResponse = BS(statusRequest);

This might end up confusing us in the future. Maybe just use the bytes \[30, 00\] (i.e. an empty DER SEQUENCE)?
Attachment #8682599 - Flags: review+
(In reply to Mark Goodwin [:mgoodwin] from comment #40)
> I just remembered; I was using ERROR_POLICY_VALIDATION_FAILED as a
> placeholder for whatever the correct error should be in the case of a chain
> missing TLSFeature extensions on a child that's present for a parent.
> 
> This won't be the correct error so:
> - I can use the REQUIRED_TLS_FEATURE_MISSING one (though technically the
> wording fits, it confusing that this could be a chain validation issue *or*
> a missing feature from the handshake)
> - I can create a new one. 
> 
> Which would you prefer. If the latter, what should it be called?

I think we can go with REQUIRED_TLS_FEATURE_MISSING for now, and reconsider if it becomes a problem.
Flags: needinfo?(dkeeler)
Comment on attachment 8682598 [details]
MozReview Request: Bug 901698 - Implement OCSP-must-staple; r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24101/diff/4-5/
Attachment #8682598 - Flags: review?(dkeeler)
Attachment #8682599 - Flags: review?(dkeeler)
Comment on attachment 8682599 [details]
MozReview Request: Bug 901698 - Some tests for OCSP-must-staple; r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24103/diff/4-5/
https://reviewboard.mozilla.org/r/24103/#review22557

::: security/manager/ssl/tests/unit/test_ocsp_stapling.js:197
(Diff revision 5)
> +  // Some tests to ensure that TLSFeature chain checks work correctly

Disable stapling and move with the other chain tests above
Attachment #8682598 - Flags: review?(dkeeler) → review+
Comment on attachment 8682598 [details]
MozReview Request: Bug 901698 - Implement OCSP-must-staple; r?keeler

https://reviewboard.mozilla.org/r/24101/#review22561

This looks great. Just a few nits.

::: security/certverifier/CertVerifier.cpp:589
(Diff revision 5)
> +  Input* responseInputPtr;

= nullptr

::: security/certverifier/CertVerifier.cpp:607
(Diff revision 5)
> +   }

nit: looks like this } needs to move one space right.

::: security/manager/ssl/SSLServerCertVerification.cpp:1232
(Diff revision 5)
> +  if (!infoObject->SharedState().IsOCSPMustStapleEnabled()) {

Might be more concise to combine these two ifs into one or statement.

::: security/pkix/include/pkix/pkix.h:153
(Diff revision 5)
> +// status_request (OCSP stapling) so web reject any extension that specifies a

s/web/we/

::: security/pkix/lib/pkixcheck.h:58
(Diff revision 5)
> +// Check that each child in the chain meets the TLS Feature (rfc7633)

nit: update documentation

::: security/pkix/lib/pkixcheck.cpp:834
(Diff revision 5)
> +CheckTLSFeatures(const BackCert& subject, BackCert& potentialIssuer) {

nit: { on the next line

::: security/pkix/lib/pkixcheck.cpp:852
(Diff revision 5)
> +                         const Input* stapledOCSPResponse) {

nit: indentation, { on the next line

::: security/pkix/lib/pkixcheck.cpp:869
(Diff revision 5)
> +        stapledOCSPResponse->GetLength() == 0) {

I still think we shouldn't do any validation of the stapled OCSP response other than that it's present (i.e. non-null) here. The revocation checking code should be responsible for validating it (although we should test that the right thing happens here - I think there's an ORTEmpty type of OCSP response in the test code).

::: security/pkix/lib/pkixcheck.cpp:879
(Diff revision 5)
> +                              const Input* stapledOCSPResponse) {

nit: indentation, { on the next line
Comment on attachment 8682599 [details]
MozReview Request: Bug 901698 - Some tests for OCSP-must-staple; r?keeler

https://reviewboard.mozilla.org/r/24103/#review22563

Great!

::: security/manager/ssl/tests/unit/ocsp_certs/multi-tls-feature-good-ee.pem.certspec:4
(Diff revision 5)
> +extension:TLSFeature:OCSPMustStaple, 6

tiny nit: other extensions that consist of lists generally don't have spaces after the commas, and it's nice to be consistent. Also, I'm wondering if there is a reserved testing value for this, since 6 might end up being used for something.
Also, now that I think about it, shouldn't this fail? The extension mandates that TLS feature 6 be present, but since we don't know what that is, we can't know if it's present or not (and, indeed, it probably isn't).
Oh, I guess this is a test for ocsp-must-staple-disabled-mode? The name isn't very clear in that case.
Also, we should probably document somewhere that disabling OCSP must-staple doesn't actually disable enforcing nested feature requirements (which I think is fine - if a CA shoots themselves in the foot by messing that up, that's probably grounds for seeing what else they're doing wrong).

::: security/manager/ssl/tests/unit/pycert.py:548
(Diff revision 5)
> +    def addTLSFeature(self, features, critical):

It would be nice to document this extension at the top of this file.

::: security/pkix/test/gtest/moz.build:15
(Diff revision 5)
> +    'pkixcheck_TLSFeaturesSatisfiedInternal_tests.cpp',

The OCSPStaplingServer tests are good, but I think we can do a lot more (and faster) with some additional gtests (for instance, there aren't any gtests for CheckTLSFeaturesAreSatisfied here). This should be a follow-up, though.

::: security/pkix/test/gtest/pkixcheck_TLSFeaturesSatisfiedInternal_tests.cpp:96
(Diff revision 5)
> +TEST_P(pkixcheck_TLSFeaturesSatisfiedInternal, TLSFeaturesSatisfiedInternal) {

nit: { on the next line

::: security/pkix/test/gtest/pkixcheck_TLSFeaturesSatisfiedInternal_tests.cpp:123
(Diff revision 5)
> +TEST_F(pkixcheck_TLSFeaturesSatisfiedInternal, EmptyInput) {

nit: { on the next line
Attachment #8682599 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/mozilla-central/rev/801655542a12
https://hg.mozilla.org/mozilla-central/rev/21e0e988e3bd
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Blocks: 1396925
You need to log in before you can comment on or make changes to this bug.