certdb: use certutil and match_hostname for cert verification #490

HonzaCholasta · 2017-02-21T09:24:16Z

Use certutil and ssl.match_hostname calls instead of python-nss for
certificate verification.

tiran

Couple of simplifications

tiran · 2017-02-21T10:16:45Z

ipalib/x509.py

+            else:
+                name = ava.dotted_string
+            match_ava = (name, ava.value)
+            match_rdn.append(match_ava)


Surplus () in match_ava = (name, ava.value). How about you collapse both lines into one? match_rdn.append((name, ava.value)).

Sure, why not.

tiran · 2017-02-21T10:17:17Z

ipalib/x509.py

+    else:
+        match_cert['subjectAltName'] = match_san = []
+        for value in san.get_values_for_type(cryptography.x509.DNSName):
+            match_dnsname = ('DNS', str(value))


Why do you use str() here? It's either unnecessary (if value is already a str) or wrong (if value is bytes or DNSName instance). Since get_values_for_type returns the value attribute of DNSName instances and DNSName.value is guaranteed to be text, the call is not necessary.

I don't remember TBH - this patch has been laying on my HDD for quite a while.

tiran · 2017-02-21T10:17:54Z

ipalib/x509.py

+
+    try:
+        ssl.match_hostname(match_cert, hostname)
+    except ssl.SSLError as e:


match_hostname raises a subclass of ValueError. It's part of the API. There is no need to catch SSLError.

tiran · 2017-02-21T10:25:38Z

ipalib/x509.py

+            if ava.oid == cryptography.x509.oid.NameOID.COMMON_NAME:
+                name = 'commonName'
+            else:
+                name = ava.dotted_string


You can safely omit other attribute value assertion. Python's match_hostname will both ignore other AVAs and never support AVAs OID in dotted string notation.

Makes sense.

tiran · 2017-02-21T10:34:24Z

Do we ensure that the function is always called with an IDN A-Label encoded hostname? ssl.match_hostname assumes that all parts are A-labels, not U-labels.

HonzaCholasta · 2017-02-21T11:12:12Z

@tiran, how do I ensure that?

tiran · 2017-02-21T11:18:33Z

The hostname must be ASCII text. Something like hostname.encode('ascii') should catch non-ASCII text and Python 3 bytes.

tiran

Hostname verification is a mess. I got it wrong multiple times, caused at least one CVE and found multiple other CVEs. :/

tiran · 2017-02-21T21:06:15Z

ipalib/x509.py

+            value = DNSName(value).ToASCII()
+            match_san.append(('DNS', value))
+
+    ssl.match_hostname(match_cert, hostname)


ssl.match_hostname expects an IDN A-label, use DNSName(hostname).ToASCII() here.

tiran · 2017-02-22T07:04:09Z

ipalib/x509.py

+    else:
+        match_cert['subjectAltName'] = match_san = []
+        for value in san.value.get_values_for_type(cryptography.x509.DNSName):
+            value = DNSName(value).ToASCII()


You must not our DNSName class here. It's an invalid and therefore unsafe conversion. I fear we cannot convert back safely. In general IDNA is not a bijective encoding. This is especially true for different kinds of IDNA. PyCA cryptography uses IDNA 2008 UTS46, DNSName uses IDNA 2003.

Consider this example:

>>> from ipapython.dnsutil import DNSName >>> import idna >>> asn1_string = 'xn--strae-oqa.de' >>> pyca_dnsname = idna.decode(asn1_string) >>> DNSName(pyca_dnsname).ToASCII() u'strasse.de'

I talked to @reaperhulk about the issue. Paul pointed me to pyca/cryptography#3357.

Yes, hostname verification is a mess. I got it wrong multiple times, caused at least one CVE and found multiple other CVEs. :/

HonzaCholasta · 2017-03-08T10:18:05Z

I think this PR is ready now.

stlaz · 2017-03-27T07:06:47Z

@tiran Could you please finish the review? I guess we can omit the change in .spec.in for the review time being.

stlaz · 2017-03-28T15:27:08Z

I tried to use the wonderful github tool to resolve conflicts to make this more review-friendly but I guess it kind of missed the magic, it's ready for review anyway, please, finish it.

tiran · 2017-03-28T16:16:17Z

github magic is bad magic :/ It still shows up as 'conflicting' for me.

I'll try to find time to review the issue tomorrow, Thursday latest.

tiran · 2017-03-29T09:44:40Z

ipapython/certdb.py

+        cert = x509.load_certificate(cert, x509.DER)
+
+        if not cert.subject:
+            raise ValueError("has empty subject")


RFC 5280 section 4.1.2.6 mandates subject only for certs with basic constraint CA: TRUE. Please check for basic constraint first.

tiran · 2017-03-29T09:50:45Z

ipapython/certdb.py

+            raise ValueError("missing basic constraints")
+
+        if not bc.ca:
+            raise ValueError("not a CA certificate")


How is the verify_ca_cert_validity method used? Is it used to check for just any CA cert or also used to verify that the CA cert can be used to sign an intermediate CA in Dogtag? In the latter case, this should also check that path_length is >= 1.

tiran · 2017-03-29T09:54:59Z

ipapython/certdb.py

+
+        if not bc.ca:
+            raise ValueError("not a CA certificate")
+


Validation is missing one important field. As of RFC 5280 section 4.2.1.3, CA certs must have a key usage extension with at least keyCertSign bit set.

tiran · 2017-03-29T09:58:06Z

ipalib/x509.py

+    match_cert = {}
+
+    match_cert['subject'] = match_subject = []
+    for rdn in cert.subject.rdns:


As of RFC 5280 section 4.1.2.6, the subject field of an EE cert is optional and may not be present, https://no-subject.badssl.com/

I think this needs an additional check to verify that cert.subject is present and/or not None.

What RFC 5280 actually states is that the subject might be empty, not that the subject field is optional.

Empty subject has an empty list of RDNs:

>>> from cryptography.hazmat.backends import default_backend >>> from cryptography.x509 import load_pem_x509_certificate >>> load_pem_x509_certificate(open('no-subject.crt', 'rb').read(), default_backend()).subject.rdns []

i.e. no extra check is necessary.

tiran · 2017-03-29T10:05:23Z

ipalib/x509.py

+
+    values = get_san_a_label_dns_names(cert)
+    if values:
+        match_cert['subjectAltName'] = match_san = []


As of RFC 6125 section 6.4.4. you also have to take SRV-ID and URI-ID into account. Since ssl.match_hostname supports IP addresses, you also need to include iPAddress fields or validate that hostname never looks like an IP address.

This will cause a security bug as soon as https://bugs.python.org/issue28196 is fixed.

ssl.match_hostname in Python 2 supports only DNS SANs, so I didn't bother including support for anything else.

I guess adding support for other SAN types won't hurt, that is if we don't mind the different behavior in Python 2 vs 3.

tiran · 2017-03-29T10:06:37Z

ipalib/x509.py

+        cert.tbs_certificate_bytes,
+        asn1Spec=rfc2459.TBSCertificate()
+    )[0]
+    OID_SAN = univ.ObjectIdentifier('2.5.29.17')


Please use SUBJECT_ALTERNATIVE_NAME from cryptography.x509.oid.

tiran · 2017-03-29T19:47:05Z

Your PR is going to remove the last import from python-nss. Awesome!

Please remove the requirement from ipapython/setup.py and freeipa.spec.in, too.

Use certutil and ssl.match_hostname calls instead of python-nss for certificate verification.

Remove the unused python-nss dependency.

HonzaCholasta · 2017-03-30T12:03:03Z

Awesome indeed!

As for your suggestions to improve the validation, I completely agree with them, but the focus of this PR is to refactor the current validation not to use python-nss, which it delivers. Could you please file a ticket for the improvements, so that it gets more visibility and can be properly tracked?

tiran

ACK!

I created ticket https://pagure.io/freeipa/issue/6840 to track the remaining nit-picks. Since all validation functions are only used for sanity checks and user feedback, it is safe to handle them at a later point.

MartinBasti · 2017-03-31T10:20:58Z

master:

9183cf2 certdb: use certutil and match_hostname for cert verification
2b33230 setup, pylint, spec file: drop python-nss dependency

tiran requested changes Feb 21, 2017

View reviewed changes

tiran requested changes Feb 22, 2017

View reviewed changes

HonzaCholasta changed the title ~~[WIP] certdb: use certutil and match_hostname for cert verification~~ certdb: use certutil and match_hostname for cert verification Mar 8, 2017

tiran self-assigned this Mar 28, 2017

tiran requested changes Mar 29, 2017

View reviewed changes

Jan Cholasta added 2 commits March 30, 2017 10:23

certdb: use certutil and match_hostname for cert verification

4b31511

Use certutil and ssl.match_hostname calls instead of python-nss for certificate verification.

setup, pylint, spec file: drop python-nss dependency

25c3bd2

Remove the unused python-nss dependency.

tiran approved these changes Mar 31, 2017

View reviewed changes

tiran added the ack Pull Request approved, can be merged label Mar 31, 2017

MartinBasti added the pushed Pull Request has already been pushed label Mar 31, 2017

MartinBasti closed this Mar 31, 2017

certdb: use certutil and match_hostname for cert verification #490

certdb: use certutil and match_hostname for cert verification #490

HonzaCholasta commented Feb 21, 2017 •

edited by MartinBasti

tiran left a comment

tiran Feb 21, 2017

HonzaCholasta Feb 21, 2017

tiran Feb 21, 2017

HonzaCholasta Feb 21, 2017

tiran Feb 21, 2017

HonzaCholasta Feb 21, 2017

tiran Feb 21, 2017

HonzaCholasta Feb 21, 2017

tiran commented Feb 21, 2017

HonzaCholasta commented Feb 21, 2017

tiran commented Feb 21, 2017

tiran left a comment

tiran Feb 21, 2017

tiran Feb 22, 2017

HonzaCholasta commented Mar 8, 2017

stlaz commented Mar 27, 2017

stlaz commented Mar 28, 2017

tiran commented Mar 28, 2017

tiran Mar 29, 2017

tiran Mar 29, 2017

tiran Mar 29, 2017

tiran Mar 29, 2017

HonzaCholasta Mar 30, 2017 •

edited

tiran Mar 29, 2017

HonzaCholasta Mar 30, 2017

tiran Mar 29, 2017

tiran commented Mar 29, 2017

HonzaCholasta commented Mar 30, 2017

tiran left a comment

MartinBasti commented Mar 31, 2017

certdb: use certutil and match_hostname for cert verification #490

certdb: use certutil and match_hostname for cert verification #490

Conversation

HonzaCholasta commented Feb 21, 2017 • edited by MartinBasti

tiran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiran commented Feb 21, 2017

HonzaCholasta commented Feb 21, 2017

tiran commented Feb 21, 2017

tiran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HonzaCholasta commented Mar 8, 2017

stlaz commented Mar 27, 2017

stlaz commented Mar 28, 2017

tiran commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HonzaCholasta Mar 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiran commented Mar 29, 2017

HonzaCholasta commented Mar 30, 2017

tiran left a comment

Choose a reason for hiding this comment

MartinBasti commented Mar 31, 2017

HonzaCholasta commented Feb 21, 2017 •

edited by MartinBasti

HonzaCholasta Mar 30, 2017 •

edited