Closed Bug 1252891 Opened 9 years ago Closed 6 years ago

Implement certUsageIPSec as defined in RFC 4945

Categories

(NSS :: Libraries, defect, P2)

Product:
NSS
Component:
Libraries
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: KaiE, Assigned: KaiE)

Details

Attachments

(1 file, 7 obsolete files)

1252891-v7.patch
33.66 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
This has been originally requested at https://bugzilla.redhat.com/show_bug.cgi?id=1212132 After talking to the people who reported that bug, here's my summary of the requirements. As of today, NSS offers a limited amount of certUsage scenarios for the existing certificate verification APIs offered. None of them covers the requirements of RFC 4945, see https://tools.ietf.org/html/rfc4945#section-5 Summarizing the apparent highlights: - if any EKU are present, they must be completely ignored - empty KU is allowed - if KU is present, either digitalSignature or nonRepudiation (or both) must be present, and any additional usages are tolerated. The Libreswan project currently uses the CERT_PKIXVerifyCert interface, as it provides several features the classic NSS verify don't offer. I think this means, we should add the new usage profile to both the classic and the NSS libpkix code.
I'm not sure, from this bug description, what you're asking, since you indicate that libreswan currently uses CERT_PKIXVerifyCert Further, I'm opposed to adding new usage profiles to classic code without strong justification and use case. Given that Firefox now uses mozilla::pkix, and Chrome and (apparently) libreswan use libpkix, we should be careful about how much *new* functionality we introduce to legacy - because then it's not really legacy anymore.
libreswan wants to use whatever the recommended proper interface is with NSS to perform certificate validation with CRL and OCSP support. This includes the ability to have multiple CA certs in a single NSS db and request a validation with one specific CA only. We further need to be able to ignore (missing or present) EKU's and support empty KU's as per above quoted RFC. We don't care if this is old, new or legacy. Give us the tools and we'll do the job :)
I suppose the question is how Kai phrased it; that is, I read his report as being "libpkix is already sufficient, but we should add this to legacy" I'm uncertain both you and Kai's remark about EKUs being present should be ignored, and I would be strongly opposed to "ignoring EKUs" in libpkix (or any other NSS validator), regardless of what RFC 4945 says. The use of the EKU field is a key part in preventing certificate misuse within the WebPKI and elsewhere (as shown by Flame), and so a product direction that NSS should take should be "safe by default", and that means: - If no EKU is present (in the leaf or the CAs), then accept/continue - If an EKU is present (leaf or CA), and does not match the IKE EKU, reject - If an EKU is present (leaf or CA), and it's the anyEKU, accept/continue [to the extent it's compatible with the first two] That is, I'm strongly opposed to adding any NSS flags that change the current EKU processing model, in which the acceptable EKUs for a leaf is the intersection of the EKUs of the issuing change.
So you want to limit NSS to TLS use only? Should we as libreswan be looking for another library that does implement RFCs properly?
> So you want to limit NSS to TLS use only? No, that's not what I said at all. I'm opposed to ignoring EKUs, simply that. It's extremely relevant to the WebPKI, and while PKIX did not specify it, it's a common behaviour in a significant number of libraries (Windows included), that regardless of what RFC 4945 recommends, it's simply not a reasonable security decision. Do some in the PKIX working group oppose that view? Sure. But this was a debate there even 16 years ago, when the then-Netscape Navigator and then-IE 3.0 harmonized on that behaviour. It's a vital tool for scoping authority and preventing misissuance, so despite what 4945 says, I'm opposed to adding a flag to disable that security check. I am opposed to adding IKE-specific logic (such as consistency checks of the KU) into the path building logic; those sorts of per-context use cases arguably belong in the application library on evaluating the leaf cert after RFC 5280-validation has succeeded. To the extent 4945 *disagrees* with behaviours of RFC 5280, I am opposed to adding that behaviour to NSS.
Note: What I said in Comment #3 is restating what RFC 4945 says at the top of Page 31, and which I believe is in conflict with how Kai described it - which is why I tried stating it alternatively in comment #3.
As with browsers, we do not control all kinds of IKE server and client implementations. therefor we must follow the RFCs with respect to ignoring EKU's. I understand you do not like this. That does change the reality that for NSS use with IKE/IPsec, ignoring EKU's is required for proper functioning. Note that practically all certificate setups for IKE/IPsec are privately generated non-public CAs and usually are EE certificates/keys (PKCS#12 bundles) generated by the CA (as in the CA has a copy of the enduser private key). So a lot of what is important in WebPKI is irrelevant to these kind of deployments. It seems the considerations in comment 3# are focused on a system as used by TLS, where there are multiple parties to this system that act independently (CAs, subCAs, operators and clients) As for the it being "legacy", I agree that deployments without EKU should be considered legacy. And anyone generating certificates properly (eg using certutil of NSS) will not run into these problems. So it seems appropriate to add this to "legacy code". Having an IKE profile mode allows us to divert from TLS assumptions that are currently hidden in the generic PKIX validation code.
Having a cert usage that ignores eku should not interfere with normal operation as long as you can't accidentally get it. We can make clear our support does not denote endorcements of public CA's improperly issuing such certificates. I agree I wouldn't want the browser to use that usage. bob
There are few questions we need to answer, which came up when I worked on this enhancement. (1) When verifying a certificate, and searching for a trusted issuer, NSS uses the concept of trust categories. We currently have the supported categories of SSL/TLS, email security (and code signing, which is probably going away). We need to decide which category of trust flags should apply to trust a certificate for the IPSec scenario. It seems that the developers of libreswan have already made the decision, if a certificate is trustworthy for SSL/TLS, they apparently want to accept that certificate for IPSec, too. So, I suggest, when we implement certUsageIPSec, we check that the issueing CA is trusted for SSL/TLS. (2) Server side trust vs client side trust. NSS supports two variations of the SSL/TLS trust flags. One is for a "server" (trust flag 'C'), which means, this CA is trusted to issue certificates that are intended to identify a server, with the name of the server given in the Subject(AltName) attribute. This is used when verifying a certificate for certUsageSSLServer. Another one is for a "client" (trust flag 'T'), which means, any client authentication certificates issued by that CA will be trusted locally to grant access for incoming client connections. This is used when verifying a certificate for certUsageSSLClient. I understand that until now, where the IPSec usage in NSS wasn't available, you have configured both certificate trust flags ("CT") to allow verification to succeed with all variations of certificates. The question we need to answer is, which trust flag category should be required when verifying for certUsageIPSec ? The answer to this question probably depends on how certificate based authentication works with IPSec, and I don't know the answer here, so please educate us, if necessary. I'll start with a simple assumption, so please correct me if this isn't correct. I assume that you never use CAs from the "public web PKI". Rather, I assume that you always use a local, private CA, which has been specifically created for the purpose of authentication both IPSec peers to each other. If this simple assumption is correct, then it should be sufficient to require that both sides configure the involved CA with trust flag 'C'. This is what the initial patch assumes and requires. (It ignores whether the 'T' trust flag is present or not.) (Please comment, if you believe it's necessary that we support a trust database with trust flag 'T' (and without 'C') to be sufficient for verifying for certUsageIPSec. Supporting both trust flags would require more changes, as currently NSS always requires exactly one of multiple alternatives, never a choice.)
Attached patch patch v1 (obsolete) — — Splinter Review
This patch implements the patch for all current certificate engines. (Since the usages verification definitions, and the code implementing the checks, is shared between the classic engine and the libpkix engine, it would be difficult to implement code that does it for the libpkix code path, only.) The patch enhances the test suite to create a couple of test certificates, with the combinations of key usages as explained in the initial comment in this bug, and tests them with all variations of our verification engines (as offered by the vfychain command and the -p option). The test suite expects that most certificates are accepted, and only the NoMatch test certificate is rejected (has KU extension without any of the expected usages). The test suite output gave me the expected results. Drive-By change: The current end entity test certificates created by chains.sh don't set a basic constraints extension. The patch fixes it to set it (in function create_cert_req).
Attachment #8812800 - Flags: review?(rrelyea)
Paul, if you can, please test this patch, and give feedback. Also, please contribute descriptions of additional test certificates, if you believe the existing list in the tests_results/*/*/chains/IPSec directory should be extended.
Tuomo suggested that the patch should be updated to use IPsec (not IPSec). I'll attach an updated version that fixes that everywhere.
Attached patch patch v2 (obsolete) — — Splinter Review
Attachment #8812800 - Attachment is obsolete: true
Attachment #8812800 - Flags: review?(rrelyea)
Attachment #8812819 - Flags: review?(rrelyea)
Attached patch patch v3 (obsolete) — — Splinter Review
changed remaining "IP Sec" to "IPsec", too.
Attachment #8812819 - Attachment is obsolete: true
Attachment #8812819 - Flags: review?(rrelyea)
Attachment #8813257 - Flags: review?(rrelyea)
Priority: -- → P2
Comment on attachment 8813257 [details] [diff] [review] patch v3 Review of attachment 8813257 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/certhigh/certvfypkix.c @@ +177,5 @@ > ekuIndexCodeSigner, > ekuIndexEmail, > ekuIndexTimeStamp, > ekuIndexStatusResponder, > + ekuIndexIPsec, If I understand correctly, this enum is used for indexing ekuOidStrings[] (defined in libpkix/pkix/checker/pkix_ekuchecker.c). However, I see no new OID assigned for IPsec.
"IP Security IKE Intermediate" EKU is OID 1.3.6.1.5.5.8.2.2 I'm not sure if this is official or unofficial. But I think the IKE RFCs do not put these constraints in, only some vendors (read: Microsoft)
(In reply to Daiki Ueno [:ueno] from comment #15) > If I understand correctly, this enum is used for indexing ekuOidStrings[] > (defined in libpkix/pkix/checker/pkix_ekuchecker.c). However, I see no new > OID assigned for IPsec. Daiki, thanks for reviewing. I just noticed the code isn't used, it's wrapped in "#ifdef NOTDEF". You're right, if this code were used, adding ekuIndexIPsec, and adding the mapping from certUsageIPsec to ekuIndexIPsec would cause a problem in cert_NssCertificateUsageToPkixKUAndEKU. But that function is never called. To avoid this inconsistency in the unused code, we can omit the changes to file certvfypkix.c, which are unnecessary anyway. I'll attach an updated patch with that part removed, and that also fixes the code formatting as reported in this try build: https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=a6be5f3e50cfd169382b9a585f6a4578cdecadee
QA Contact: franziskuskiefer
Attached patch 1252891-v4.patch (obsolete) — — Splinter Review
Attachment #8813257 - Attachment is obsolete: true
Attachment #8813257 - Flags: review?(rrelyea)
Attached patch 1252891-v5.patch (obsolete) — — Splinter Review
I was given an example certificate that doesn't work with patch v4. That cert neither qualifies as an SSL server cert, nor as an SSL client cert, and the NSS checks are failing. Patch v4 implements a test for "certificate type", which is based on EKU. That existing code (cert_ComputeCertType) doesn't know how to handle the example cert. It returns zero. Based on RFC4945 section 5.1.3.12 it might be OK to ignore the EKU attribute for the IKE profile. The NSS code in CERT_KeyUsageAndTypeForCertUsage could potentially be changed to set requiredCertType=0. We could modify the NSS cert verification to ignore the result of cert_ComputeCertType if requiredCertType is zero. However, IMHO, if we decided to ignore EKU for certificateUsageIPSec, we should still reject the cert, if the EKU extension is marked critical. I'm attaching a patch v5 that accepts the example cert and ignores the EKU. It only checks the KU. The patch v5 doesn't yet implement rejection of a critical EKU with this certUsage. Patch v5 also modifies a general code path in cert verification, to allow the zero requiredCertType. We should carefully review to ensure this change doesn't have any side effects. Optionally, we could fully implement the EKU checks as described in that RFC section.
Attachment #9017734 - Attachment is obsolete: true
Attached patch 1252891-v6.patch (obsolete) — — Splinter Review
This patch v6 contains the following changes, compared to v5: It uses a less invasive modification for CERT_VerifyCertificate and PKIX_PL_Cert_VerifyCertAndKeyType. The logic is changed for the IPSec usage, only. It implements EKU handling for IPSec IKE. If the EKU isn't critical, any EKU value is accepted. If the EKU is critical, the following values are accepted: - the values described in RFC 4945 section 5.1.3.12 (id-kp-ipsecIKE or anyExtendedKeyUsage) - the value suggested by Paul in comment 16 (1.3.6.1.5.5.8.2.2) I've manually tested this code to work with the test certificate I've been given (using certutil -V -u I, and vfychain -pp -u 12) TODO: add automated tests for certificates with these extension values
Attached patch 1252891-v6b.patch (obsolete) — — Splinter Review
I realize there has been a typo in all earlier patches. I wrote IPsec. We probably want IPSec instead.
IPsec is correct spelling. "The spelling "IPsec" is preferred and used throughout this and all related IPsec standards. All other capitalizations of IPsec (e.g., IPSEC, IPSec, ipsec) are deprecated. However, any capitalization of the sequence of letters "IPsec" should be understood to refer to the IPsec protocols." https://tools.ietf.org/html/rfc4301#section-1.1
There is one more OID to add, it's a brother of the IPsec IKE Intermediate (iKEIntermediate): .1.3.6.1.5.5.8.2.1 (iKEEnd) They both come from the same draft document: https://tools.ietf.org/html/draft-ietf-ipsec-pki-req-02
Comment on attachment 9018204 [details] [diff] [review] 1252891-v6b.patch ok, obsoleting the "IPSec" patch, keeping "IPsec"
Attachment #9018204 - Attachment is obsolete: true
Attached patch 1252891-v7.patch — — Splinter Review
Patch v7 adds the OID mentioned in comment 23, and renames a few additional occurrences of IPSec to IPsec.
Attachment #9017825 - Attachment is obsolete: true
Attachment #9018202 - Attachment is obsolete: true
Comment on attachment 9018223 [details] [diff] [review] 1252891-v7.patch Review of attachment 9018223 [details] [diff] [review]: ----------------------------------------------------------------- I'll attach a patch with some additional tests, but the patch is correct and passes tests and the nss-try tests (except the new oids get flagged in the abi test, but new oids are fine as long as the are added to the end of the list). ::: tests/chains/scenarios/ipsec.cfg @@ +13,5 @@ > + > +entity NoKU > + type EE > + issuer CA1 > + We need some tests that also set the eku values for IPsec. That requires changes to certutil to handle the new eku oids added to secoid.c
Attachment #9018223 - Flags: review+
This change fails ABI checks. Please make sure that the changes are legit and update the corresponding ABI txt files.
Flags: needinfo?(rrelyea)
Updated. I didn't realize the ABI whitelists were right there in the NSS tree. I had verified that the change was legit (see comment 26 ) https://hg.mozilla.org/projects/nss/rev/0d97145d524ab35b8bc2a4a8aea60a83bd244f14
Flags: needinfo?(rrelyea)
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.41

It looks like this patch is not ignoring EKU's when they are marked critical. This is causing problems already :/

The RFC test is not clear to me:

https://tools.ietf.org/html/rfc4945#section-5.1.3.12

Conforming IKE implementations are not required to support EKU. If a
critical EKU extension appears in a certificate and EKU is not
supported by the implementation, then RFC 3280 requires that the
certificate be rejected. Implementations that do support EKU MUST
support the following logic for certificate validation:

o If no EKU extension, continue.

o If EKU present AND contains either id-kp-ipsecIKE or
anyExtendedKeyUsage, continue.

o Otherwise, reject cert.

It seems the bullet list kinda conflicts the paragraph above it

So now if a certificate has serverAuth (for compatibility before NSS/others supported the IPsec profile, and they happen to have made that EKU critical, then NSS rejects the cert.

libreswan with older NSS would attempt to validate as server, and on failure validate as client, as one of these would pass. libreswan with the newer NSS with ipsec profiles now rejects these.

I suggest that NSS for IPsec profiles does not reject critical EKU's

note also that a lot of VPN serves offer TLS and IKE/IPsec, re-using the same certificate. So that is another reason why authServer might appear.

Paul, file a new bug for the new issue, and mark it as dependent on this bug. New issues should be discussed in new bugs.

I forgot to say "please"

I will, just talking to Hubert Kario to ensure we fully understand the issue before I file the report.

I've openned bug 1537927

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: