Memory leak in OpenSSL.crypto.X509Req.get_extentions

[Have-a-question-about-this-project]

This leak only happens if a CSR is generated with a 'req_extensions' section containing things like X509v3 Basic Constraints: CA:FALSE X509v3 Key Usage: Digital Signature, Non Repudiation, Key Encipherment X509v3 Subject Alternative Name: DNS:c62, DNS:c62.mydomain.com, IP Address:10.1.3.1

Tested with cryptography-2.1.4 + pyOpenSSL-17.5.0 and cryptography-2.8 + pyOpenSSL-19.1.0

import os ; pid=os.getpid() ; open("/tmp/test.pid", 'w').write("%s" %(pid))
from OpenSSL.crypto import load_certificate_request, FILETYPE_PEM
csr=open('/tmp/test.csr', 'r').read()
x509r = load_certificate_request(FILETYPE_PEM, csr)
while 1: extensions = x509r.get_extensions()`

Then watch it leak with

/root# watch -n.1 cat /proc/$(</tmp/test.pid)/status

Running similar under valgrind gets:

==00:00:00:19.803 18621== 228,567 (31,968 direct, 196,599 indirect) bytes in 999 blocks are definitely lost in loss record 1,191 of 1,194
==00:00:00:19.803 18621==    at 0x40327B3: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==00:00:00:19.803 18621==    by 0x6A8DFFC: ??? (in /usr/lib/libcrypto.so.1.0.0)
==00:00:00:19.803 18621==    by 0x6A8E6B9: CRYPTO_malloc (in /usr/lib/libcrypto.so.1.0.0)
==00:00:00:19.803 18621==    by 0x69FCB88: sk_new (in /usr/lib/libcrypto.so.1.0.0)
==00:00:00:19.803 18621==    by 0x69FCB64: sk_new_null (in /usr/lib/libcrypto.so.1.0.0)
==00:00:00:19.803 18621==    by 0x6A296A9: ??? (in /usr/lib/libcrypto.so.1.0.0)
==00:00:00:19.803 18621==    by 0x6A29534: ??? (in /usr/lib/libcrypto.so.1.0.0)
==00:00:00:19.803 18621==    by 0x6A284DF: ??? (in /usr/lib/libcrypto.so.1.0.0)
==00:00:00:19.803 18621==    by 0x6A292EC: ASN1_item_ex_d2i (in /usr/lib/libcrypto.so.1.0.0)
==00:00:00:19.803 18621==    by 0x6A282C5: ASN1_item_d2i (in /usr/lib/libcrypto.so.1.0.0)
==00:00:00:19.803 18621==    by 0x6A42AD0: X509_REQ_get_extensions (in /usr/lib/libcrypto.so.1.0.0)
==00:00:00:19.803 18621==    by 0x713993C: ??? (in /usr/lib/python2.7/site-packages/cryptography/hazmat/bindings/_openssl.so)
==00:00:00:19.803 18621==
==00:00:00:19.803 18621== 258,048 bytes in 54 blocks are still reachable in loss record 1,192 of 1,194
==00:00:00:19.803 18621==    at 0x40327B3: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==00:00:00:19.803 18621==    by 0x4AB7B3D: dictresize (dictobject.c:643)
==00:00:00:19.803 18621==    by 0x4B236BD: PyEval_EvalFrameEx (ceval.c:1941)
==00:00:00:19.803 18621==    by 0x4B25734: PyEval_EvalCodeEx (ceval.c:3265)
==00:00:00:19.803 18621==    by 0x4B259C8: PyEval_EvalCode (ceval.c:667)
==00:00:00:19.803 18621==    by 0x4B39471: PyImport_ExecCodeModuleEx (import.c:709)
==00:00:00:19.803 18621==    by 0x4B39F5D: load_compiled_module (import.c:837)
==00:00:00:19.803 18621==    by 0x4B3A750: import_submodule (import.c:2700)
==00:00:00:19.803 18621==    by 0x4B3A997: load_next (import.c:2515)
==00:00:00:19.803 18621==    by 0x4B3B380: import_module_level (import.c:2224)
==00:00:00:19.803 18621==    by 0x4B3B380: PyImport_ImportModuleLevel (import.c:2288)
==00:00:00:19.803 18621==    by 0x4B1BED7: builtin___import__ (bltinmodule.c:49)
==00:00:00:19.803 18621==    by 0x4A7BF42: PyObject_Call (abstract.c:2529)
==00:00:00:19.803 18621==
==00:00:00:19.803 18621== 422,912 bytes in 448 blocks are still reachable in loss record 1,193 of 1,194
==00:00:00:19.803 18621==    at 0x40327B3: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==00:00:00:19.803 18621==    by 0x4B5A8BD: _PyObject_GC_Malloc (gcmodule.c:1499)
==00:00:00:19.803 18621==    by 0x4AD6739: PyType_GenericAlloc (typeobject.c:761)
==00:00:00:19.803 18621==    by 0x4AE2D37: type_new (typeobject.c:2314)
==00:00:00:19.803 18621==    by 0x4ADB294: type_call (typeobject.c:729)
==00:00:00:19.803 18621==    by 0x4A7BF42: PyObject_Call (abstract.c:2529)
==00:00:00:19.803 18621==    by 0x4A7C977: PyObject_CallFunctionObjArgs (abstract.c:2756)
==00:00:00:19.803 18621==    by 0x4B22127: build_class (ceval.c:4644)
==00:00:00:19.803 18621==    by 0x4B22127: PyEval_EvalFrameEx (ceval.c:1929)
==00:00:00:19.803 18621==    by 0x4B25734: PyEval_EvalCodeEx (ceval.c:3265)
==00:00:00:19.803 18621==    by 0x4B259C8: PyEval_EvalCode (ceval.c:667)
==00:00:00:19.803 18621==    by 0x4B39471: PyImport_ExecCodeModuleEx (import.c:709)
==00:00:00:19.803 18621==    by 0x4B39F5D: load_compiled_module (import.c:837)
==00:00:00:19.803 18621==
==00:00:00:19.803 18621== 3,145,728 bytes in 1 blocks are still reachable in loss record 1,194 of 1,194
==00:00:00:19.803 18621==    at 0x40327B3: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==00:00:00:19.803 18621==    by 0x4AB7B3D: dictresize (dictobject.c:643)
==00:00:00:19.803 18621==    by 0x4ACA8F1: PyString_InternInPlace (stringobject.c:4759)
==00:00:00:19.803 18621==    by 0x4B3DED8: r_object (marshal.c:822)
==00:00:00:19.803 18621==    by 0x4B3CF02: r_object (marshal.c:886)
==00:00:00:19.803 18621==    by 0x4B3D7C8: r_object (marshal.c:1019)
==00:00:00:19.803 18621==    by 0x4B42EDF: PyMarshal_ReadObjectFromFile (marshal.c:1168)
==00:00:00:19.803 18621==    by 0x4B42FC0: PyMarshal_ReadLastObjectFromFile (marshal.c:1154)
==00:00:00:19.803 18621==    by 0x4B39F25: read_compiled_module (import.c:801)
==00:00:00:19.803 18621==    by 0x4B39F25: load_compiled_module (import.c:831)
==00:00:00:19.803 18621==    by 0x4B3A750: import_submodule (import.c:2700)
==00:00:00:19.803 18621==    by 0x4B3A997: load_next (import.c:2515)
==00:00:00:19.803 18621==    by 0x4B3B380: import_module_level (import.c:2224)
==00:00:00:19.803 18621==    by 0x4B3B380: PyImport_ImportModuleLevel (import.c:2288)
==00:00:00:19.803 18621==
==00:00:00:19.803 18621== LEAK SUMMARY:
==00:00:00:19.803 18621==    definitely lost: 32,000 bytes in 1,003 blocks
==00:00:00:19.803 18621==    indirectly lost: 196,599 bytes in 9,663 blocks
==00:00:00:19.803 18621==      possibly lost: 8,624 bytes in 14 blocks
==00:00:00:19.803 18621==    still reachable: 6,157,763 bytes in 8,318 blocks
==00:00:00:19.803 18621==         suppressed: 0 bytes in 0 blocks

-----BEGIN CERTIFICATE REQUEST-----
MIIFKTCCAxECAQAwgZMxCzAJBgNVBAYTAkdCMRAwDgYDVQQIEwdCcmlzdG9sMRAw
DgYDVQQHEwdCcmlzdG9sMRIwEAYDVQQKEwlTdG9yTWFnaWMxDjAMBgNVBAsTBWlT
Q1NJMRgwFgYDVQQDEw9jNjIuZXhzZXF1aS5jb20xIjAgBgkqhkiG9w0BCQEWE2Fk
bWluQHN0b3JtYWdpYy5jb20wggIiMA0GCSqGSIb3DQEBAQUAA4ICDwAwggIKAoIC
AQC+mmO21HxoPbJj9IZGCmLUSE+hhlCp4laZOncZ085YH+71hHDu5oMm7Vgay21C
gh2STQ3dPmM6NN3Z3zJ50Pc7emovYNHX7+Csb0i08+p2xz2FefmmOpESB2r5o7zU
J6dXK1Hpm8gZ+ojFO3+eFtR6ExUYYtHqxFHMQFRjyKnYp2j73Y8fNGCb4CKU/dzI
SxXRzejUUZpHjQT+na0OZz3PsqRaoH+Pkq4UNCwJZBCQKfx1sbzmpHIdhML6fMPc
+xamp90kPEEo1BYcjQefgM5VXzuCknBs0tc7nTGdZQgGYgQXzKWIEt4s+ZLSDWIU
U3kVIqsIOrytz1aZE5lgH9fHmHcaP5XGzyu8p6VHGK+C94aVeGIAs98/ttk/6nnf
wi5XkomUa4AUqJHCNZyACCD/ZmCFN4MQggHmSJTU9ZWsljyzh9Mu7Gim/MwGPiWm
U2Iii3443nj5MaRATEA1EqICm2YQMEXTHH5Jt9NYy1U3k+b4XNMWTXz2HHmhUVb2
hqyBwKiPvLYSj00zNDWKyW/y258IQ6VwcBDxW0MVw01DBUtMkfM3BXg4Vq+aGvRs
ogHWluSCg1qQU+9PjXaWM75y5rII2QXtrxjYrxuKdo3o1qKPuJPSIyKXJLZ0M4li
jpRFl4vwQAx/mTIEmIVwuLOblszq7Yg7IQtQd30h2HP14wIDAQABoFAwTgYJKoZI
hvcNAQkOMUEwPzAJBgNVHRMEAjAAMAsGA1UdDwQEAwIF4DAlBgNVHREEHjAcggNj
NjKCD2M2Mi5leHNlcXVpLmNvbYcECgEDATANBgkqhkiG9w0BAQsFAAOCAgEAkFic
/bpM8nxQ/j+aVgKEx2Oo7TLd6X1XSMOmUBDe/Ye7o0m3TUOs3IFdOpR3L+59YGaz
vgL3IievUbna0HmHpK/OHQJaeK+QHjzj9eluujXqP1sqndd7wSUS98be6g5iWuqq
N8rACSI49pxdetBoeeFOsrJ/feMkUWmRiuZWyVKebeaLKmBxpp/UUQIdvdho6WH1
3DThMW1FzxFi9ondcE0cg6hxZzk/fH7yPjDbflp71ZnNGcgwKvyOV9YMzV/On4Iy
yH3mBmdGbZ1xBeG367l/UH/YAmp4y1QQsqFdZWVlDs3l9qbsquYepXlw33d/KDnI
Kykr63C2hL0hC2y/umRSCJvEjiVuRviJceyrhNKFDrAeVmrvmPsmCrHBDPI6Xxhb
FrQKyX4KOoCoMIyj6GXyzPvr5hL+yUWhAyVIaf12YPWNdZNukAHM5N59C8C8A2qw
V6LJJyjsyZgqEnZBfAL8DFvcIhC6ReVdZPDIDHhoa0HomfFEgrxrRuJgFN/rZM0O
mFUTLtkyEnnO6z9XLOz9svGJYuW/z/UbMATVDyQWjB6ZjrQ4E9Fr25955+JLKLmU
H4pWlr5CLXOVNT6hl4HEcGyTCAEoP/Izg46zS1cNye1t00zG9zBHDD62rEXfTkYb
d8WKdTJXydQ6X8Eu911FGBFCjXGK8cTM6M2IxE0=
-----END CERTIFICATE REQUEST-----

[Have-a-question-about-this-project]

I've written a patch that seems to fix the issue. I based it on the implementation of the call to sk_X509_EXTENSION_value in cryptography-2.8/src/cryptography/hazmat/backends/openssl/x509.py _CertificateSigningRequest.extensions

--- pyOpenSSL-19.1.0/src/OpenSSL/crypto.py      2020-03-13 13:47:15.854098906 +0000
+++ pyOpenSSL-19.1.0/src/OpenSSL/crypto.py      2020-03-13 13:47:33.534290040 +0000
@@ -1000,6 +1000,7 @@
             ext = X509Extension.__new__(X509Extension)
             ext._extension = _lib.sk_X509_EXTENSION_value(native_exts_obj, i)
             exts.append(ext)
+        _lib.sk_X509_EXTENSION_pop_free(native_exts_obj, _ffi.addressof(_lib._original_lib, "X509_EXTENSION_free"))
         return exts

     def sign(self, pkey, digest):

[Have-a-question-about-this-project]

That patch isn't right, I'm getting a segv after applying it and running more extensive testing.

[Have-a-question-about-this-project]

The previous didn't work as it freed the entire extension list when we still needed it.

This patch appears to work for me:

--- pyOpenSSL-19.1.0/src/OpenSSL/crypto.py      2020-03-13 17:55:46.404632944 +0000
+++ pyOpenSSL-19.1.0/src/OpenSSL/crypto.py      2020-03-13 17:56:29.349115599 +0000
@@ -998,8 +998,10 @@
         native_exts_obj = _lib.X509_REQ_get_extensions(self._req)
         for i in range(_lib.sk_X509_EXTENSION_num(native_exts_obj)):
             ext = X509Extension.__new__(X509Extension)
-            ext._extension = _lib.sk_X509_EXTENSION_value(native_exts_obj, i)
+            ext._extension = _lib.sk_X509_EXTENSION_delete(native_exts_obj, 0)
+            ext._extension = _ffi.gc(ext._extension, _ffi.addressof(_lib._original_lib, "X509_EXTENSION_free"))
             exts.append(ext)
+        _lib.sk_X509_EXTENSION_pop_free(native_exts_obj, _ffi.addressof(_lib._original_lib, "X509_EXTENSION_free"))
         return exts

     def sign(self, pkey, digest):

This patch removes the extension elements from the list that we were keeping a reference to, wrapping them with a garbage collect wrapper, then we free the empty extension list.

[alex]

This patch isn't correct, calling get_extensions a second time needs to return correct results, which it won't if you pop everything from it.

[Have-a-question-about-this-project]

Its popping from the native_exts_obj which we allocate every time with _lib.X509_REQ_get_extensions and were leaking.

Heres what happens when you call it twice:

>>> import sys, os ; pid=os.getpid() ; open("/tmp/test.pid", 'w').write("%s" %(pid))
>>> from OpenSSL.crypto import load_certificate_request, FILETYPE_PEM
>>> csr=open('/opt/exsequi/etc/ssl/user_certificate/user.csr', 'r').read()
>>> import sys, os ; pid=os.getpid() ; open("/tmp/test.pid", 'w').write("%s" %(pid))
>>> from OpenSSL.crypto import load_certificate_request, FILETYPE_PEM
>>> csr=open('/opt/exsequi/etc/ssl/user_certificate/user.csr', 'r').read()
>>> x509r = load_certificate_request(FILETYPE_PEM, csr)
>>> extensions = x509r.get_extensions()
>>> extensions
[<OpenSSL.crypto.X509Extension object at 0x7f33e8bd4910>, <OpenSSL.crypto.X509Extension object at 0x7f33e8bd4950>, <OpenSSL.crypto.X509Extension object at 0x7f33e8bd4890>]
>>> a = [{'name':e.get_short_name(), 'data':e.get_data()} for e in extensions]
>>> extensions = x509r.get_extensions()
>>> extensions
[<OpenSSL.crypto.X509Extension object at 0x7f33e8bd49d0>, <OpenSSL.crypto.X509Extension object at 0x7f33e8bd4990>, <OpenSSL.crypto.X509Extension object at 0x7f33e8bd48d0>]
>>> b = [{'name':e.get_short_name(), 'data':e.get_data()} for e in extensions]
>>> p(a)
[{'data': '0\x00', 'name': 'basicConstraints'},
 {'data': '\x03\x02\x05\xe0', 'name': 'keyUsage'},
 {'data': '0\x1c\x82\x03c62\x82\x0fc62.exsequi.com\x87\x04\n\x01\x03\x01',
  'name': 'subjectAltName'}]
>>> p(b)
[{'data': '0\x00', 'name': 'basicConstraints'},
 {'data': '\x03\x02\x05\xe0', 'name': 'keyUsage'},
 {'data': '0\x1c\x82\x03c62\x82\x0fc62.exsequi.com\x87\x04\n\x01\x03\x01',
  'name': 'subjectAltName'}]
>>> a == b
True
>>>

[facutuesca]

This was fixed by https://github.com/pyca/pyopenssl/pull/967 and can now be closed.

cc @reaperhulk @mhils