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

DoS (nullptr dereference) 2/2 with CRLs in mod_tls of ProFTPD master HEAD #861

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

Comments

@debrouxl
Copy link

debrouxl commented Nov 4, 2019

This is the 4th of 4 bugs in the tls_verify_crl() function. The code fails to take into account an empty CRL, for which sk_X509_REVOKED_value() returned NULL in my tests. It proceeds to dereferencing the NULL pointer, crashing the application.

My patch is as follows:

@@ -6005,7 +6014,8 @@
         ASN1_INTEGER *sn;
 
         revoked = sk_X509_REVOKED_value(X509_CRL_get_REVOKED(crl), j);
+        if (!revoked) continue;
 #if OPENSSL_VERSION_NUMBER >= 0x10100000L
         sn = X509_REVOKED_get0_serialNumber(revoked);
 #else
         sn = revoked->serialNumber;

Both of the other code bases which I noticed were getting the issuer CRL lookup right (second bug, issue #859) fail to check the return value against NULL as well:
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 crash in the summer of 2018, after fixing the first crash (issue #858) 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
…r for

lookups, and guarding against null pointers.
@Castaglia
Copy link
Member

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

@debrouxl
Copy link
Author

CVE-2019-19269 .

Sashan pushed a commit to Sashan/proftpd that referenced this issue Dec 16, 2019
…ly using issuer for

lookups, and guarding against null pointers.
Sashan pushed a commit to Sashan/proftpd that referenced this issue Dec 16, 2019
…ly using issuer for

lookups, and guarding against null pointers.
Sashan pushed a commit to Sashan/proftpd that referenced this issue Dec 16, 2019
…ly using issuer for

lookups, and guarding against null pointers.
chrisrd added a commit to chrisrd/proftpd that referenced this issue Jul 23, 2020
This fix was previously applied in commit 81cc5dc, however this was
subsequently overwritten by commit fce40d7 "Implement proper SNI
support".
Castaglia added a commit that referenced this issue Jul 25, 2020
Reapply Issue #859, #861: Fix handling of CRL lookups
Castaglia pushed a commit that referenced this issue Jul 25, 2020
This fix was previously applied in commit 81cc5dc, however this was
subsequently overwritten by commit fce40d7 "Implement proper SNI
support".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants