Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improper TLS CRL handling 1/2 in mod_tls of ProFTPD master HEAD #859

Closed
debrouxl opened this issue Nov 4, 2019 · 2 comments
Assignees
Labels
Milestone

Comments

@debrouxl
Copy link

@debrouxl debrouxl commented Nov 4, 2019

This is the 2nd of 4 bugs in the tls_verify_crl() function. The code fails to perform a CRL lookup by issuer (as the comment right above the block states), it instead performs a second lookup by subject. As a result, in our tests, after crash bugs 1 and 4 (issue #858 and issue #861) were fixed, ProFTPD still failed to take into account a valid CRL and break a connection.

The patch is as follows:

@@ -5982,11 +5990,11 @@
   /* Try to retrieve a CRL corresponding to the _issuer_ of
    * the current certificate in order to check for revocation.
    */

 #if OPENSSL_VERSION_NUMBER >= 0x10100000L
-  crls = X509_STORE_CTX_get1_crls(store_ctx, subject);
+  crls = X509_STORE_CTX_get1_crls(store_ctx, issuer);
 #elif OPENSSL_VERSION_NUMBER >= 0x10000000L
-  crls = X509_STORE_get1_crls(store_ctx, subject);
+  crls = X509_STORE_get1_crls(store_ctx, issuer);
 #else
   /* Your OpenSSL is before 1.0.0.  You really need to upgrade. */
   crls = NULL;

At least two other pieces of similar code, which contain the same comment as this one, are getting it right:
https://github.com/CESNET/libnetconf2/blob/master/src/session_server_tls.c
https://github.com/copiousfreetime/stunnel/blob/master/src/verify.c (outdated stunnel < 5.24)

FWIW, 4 years ago, stunnel got rid of custom CRL handling code and started relying on OpenSSL's built-in handling instead. That was between 5.23 and 5.24, compare src/verify.c from https://www.usenix.org.uk/mirrors/stunnel/archive/5.x/stunnel-5.23.tar.gz and https://www.usenix.org.uk/mirrors/stunnel/archive/5.x/stunnel-5.24.tar.gz .

I hit this issue in the summer of 2018, after fixing the two crashes (issue #858 and issue #861) when dealing with TLS CRLs using CentOS 7's ProFTPD 1.3.5e package against OpenSSL 1.0.2*.
I quickly reported the issues privately, but ProFTPD's TLS CRL handling remains broken on all branches more than a year later... I'm aware that TLS CRLs are highly unpopular, and that only system administrators are supposed to define them, but clearly, low-profile responsible disclosure didn't work here :)
Public reports, and CVE ID assignments (for which I'll use this issue as reference), piling onto the recent higher risk issue #846 (CVE-2019-18217) and older vulnerabilities should help the downstream propagation of all fixes, at least if any downstream provides security support for ProFTPD.

@Castaglia Castaglia self-assigned this Nov 24, 2019
@Castaglia Castaglia added this to the 1.3.7 milestone Nov 24, 2019
Castaglia added a commit that referenced this issue Nov 24, 2019
…r for

lookups, and guarding against null pointers.
Castaglia added a commit that referenced this issue Nov 24, 2019
Issue #859, #861: Fix handling of CRL lookups by properly using issue…
Castaglia added a commit that referenced this issue Nov 24, 2019
Castaglia added a commit that referenced this issue Nov 24, 2019
…r for

lookups, and guarding against null pointers.
Castaglia added a commit that referenced this issue Nov 24, 2019
@Castaglia

This comment has been minimized.

Copy link
Member

@Castaglia Castaglia commented Nov 24, 2019

Fixed in master, and backported to the 1.3.6 branch. Thanks!

@Castaglia Castaglia closed this Nov 24, 2019
@debrouxl

This comment has been minimized.

Copy link
Author

@debrouxl debrouxl commented Nov 26, 2019

CVE-2019-19270 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.