From 781295ae151fb709e613d1d30f47f31426f8008e Mon Sep 17 00:00:00 2001
From: Ziga Seilnacht <ziga.seilnacht@gmail.com>
Date: Tue, 22 Dec 2009 14:58:01 +0100
Subject: Additional error checks and a refcount fix for
 global_passphrase_callback.

There were two really big problems in this function: the first one was the
silent truncation of passphrases, the second was the refcounting bug,
which kept the passphrase in memory until the process exited. See tests
for details.
---
 OpenSSL/crypto/crypto.c     | 13 ++++++++--
 OpenSSL/test/test_crypto.py | 58 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 68 insertions(+), 3 deletions(-)

diff --git a/OpenSSL/crypto/crypto.c b/OpenSSL/crypto/crypto.c
index d9ce52e..ec4d0b3 100644
--- a/OpenSSL/crypto/crypto.c
+++ b/OpenSSL/crypto/crypto.c
@@ -45,19 +45,28 @@ global_passphrase_callback(char *buf, int len, int rwflag, void *cb_arg)
 
     func = (PyObject *)cb_arg;
     argv = Py_BuildValue("(i)", rwflag);
+    if (argv == NULL) {
+        return 0;
+    }
     ret = PyEval_CallObject(func, argv);
     Py_DECREF(argv);
     if (ret == NULL)
         return 0;
     if (!PyBytes_Check(ret))
     {
+        Py_DECREF(ret);
         PyErr_SetString(PyExc_ValueError, "String expected");
         return 0;
     }
     nchars = PyBytes_Size(ret);
-    if (nchars > len)
-        nchars = len;
+    if (nchars > len) {
+        Py_DECREF(ret);
+        PyErr_SetString(PyExc_ValueError,
+                        "passphrase returned by callback is too long");
+        return 0;
+    }
     strncpy(buf, PyBytes_AsString(ret), nchars);
+    Py_DECREF(ret);
     return nchars;
 }
 
diff --git a/OpenSSL/test/test_crypto.py b/OpenSSL/test/test_crypto.py
index 4438f8c..e108528 100644
--- a/OpenSSL/test/test_crypto.py
+++ b/OpenSSL/test/test_crypto.py
@@ -7,7 +7,7 @@ Unit tests for :py:mod:`OpenSSL.crypto`.
 
 from unittest import main
 
-import os, re
+import os, re, sys
 from subprocess import PIPE, Popen
 from datetime import datetime, timedelta
 
@@ -2075,6 +2075,33 @@ class FunctionTests(TestCase):
         self.assertRaises(ValueError, dump_privatekey, 100, key)
 
 
+    def test_load_privatekey_passphraseCallbackLeak(self):
+        """
+        L{crypto.load_privatekey} should not leak a reference to the
+        passphrase when the passphrase is provided by a callback.
+        """
+        def cb(ignored):
+            return encryptedPrivateKeyPEMPassphrase
+
+        startCount = sys.getrefcount(encryptedPrivateKeyPEMPassphrase)
+        for i in range(100):
+            load_privatekey(FILETYPE_PEM, encryptedPrivateKeyPEM, cb)
+        endCount = sys.getrefcount(encryptedPrivateKeyPEMPassphrase)
+        self.assert_(endCount - startCount < 5, endCount - startCount)
+
+
+    def test_load_privatekey_passphraseCallbackLength(self):
+        """
+        L{crypto.load_privatekey} should raise an error when the passphrase
+        provided by the callback is too long, not silently truncate it.
+        """
+        def cb(ignored):
+            return "a" * 1025
+
+        self.assertRaises(ValueError,
+            load_privatekey, FILETYPE_PEM, encryptedPrivateKeyPEM, cb)
+
+
     def test_dump_privatekey_passphrase(self):
         """
         :py:obj:`dump_privatekey` writes an encrypted PEM when given a passphrase.
@@ -2180,6 +2207,35 @@ class FunctionTests(TestCase):
             dump_privatekey, FILETYPE_PEM, key, "blowfish", cb)
 
 
+    def test_dump_privatekey_passphraseCallbackLeak(self):
+        """
+        L{crypto.dump_privatekey} should not leak a reference to the
+        passphrase when the passphrase is provided by a callback.
+        """
+        def cb(ignored):
+            return encryptedPrivateKeyPEMPassphrase
+
+        startCount = sys.getrefcount(encryptedPrivateKeyPEMPassphrase)
+        key = load_privatekey(FILETYPE_PEM, cleartextPrivateKeyPEM)
+        for i in range(100):
+            dump_privatekey(FILETYPE_PEM, key, "blowfish", cb)
+        endCount = sys.getrefcount(encryptedPrivateKeyPEMPassphrase)
+        self.assert_(endCount - startCount < 5, endCount - startCount)
+
+
+    def test_dump_privatekey_passphraseCallbackLength(self):
+        """
+        L{crypto.dump_privatekey} should raise an error when the passphrase
+        provided by the callback is too long, not silently truncate it.
+        """
+        def cb(ignored):
+            return "a" * 1025
+
+        key = load_privatekey(FILETYPE_PEM, cleartextPrivateKeyPEM)
+        self.assertRaises(ValueError,
+            dump_privatekey, FILETYPE_PEM, key, "blowfish", cb)
+
+
     def test_load_pkcs7_data(self):
         """
         :py:obj:`load_pkcs7_data` accepts a PKCS#7 string and returns an instance of
-- 
cgit v1.2.1