fix segfaults with X509.sign and X509Req.sign with certain PKeys
diff --git a/ChangeLog b/ChangeLog
index 60e708c..8fb41e3 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -2,6 +2,11 @@
* src/crypto/x509.c: Add getters and setters for the notBefore and
notAfter attributes of X509s.
+ * src/crypto/pkey.h, src/crypto/pkey.c, src/crypto/x509req.c,
+ src/crypto/x509.c: Track the initialized and public/private state
+ of EVP_PKEY structures underlying the crypto_PKeyObj type and
+ reject X509Req signature operations on keys not suitable for the
+ task.
2008-03-06 Jean-Paul Calderone <exarkun@twistedmatrix.com>
diff --git a/src/crypto/crypto.c b/src/crypto/crypto.c
index 257b478..5326198 100644
--- a/src/crypto/crypto.c
+++ b/src/crypto/crypto.c
@@ -584,10 +584,16 @@
static PyObject *
crypto_PKey(PyObject *spam, PyObject *args)
{
+ crypto_PKeyObj *py_pkey;
+
if (!PyArg_ParseTuple(args, ":PKey"))
return NULL;
- return (PyObject *)crypto_PKey_New(EVP_PKEY_new(), 1);
+ py_pkey = crypto_PKey_New(EVP_PKEY_new(), 1);
+ if (py_pkey) {
+ py_pkey->initialized = 0;
+ }
+ return py_pkey;
}
static char crypto_X509Extension_doc[] = "\n\
diff --git a/src/crypto/pkey.c b/src/crypto/pkey.c
index 0c2bea6..d06ff96 100644
--- a/src/crypto/pkey.c
+++ b/src/crypto/pkey.c
@@ -58,8 +58,7 @@
FAIL();
if (!EVP_PKEY_assign_RSA(self->pkey, rsa))
FAIL();
- Py_INCREF(Py_None);
- return Py_None;
+ break;
case crypto_TYPE_DSA:
if ((dsa = DSA_generate_parameters(bits, NULL, 0, NULL, NULL, NULL, NULL)) == NULL)
@@ -68,12 +67,16 @@
FAIL();
if (!EVP_PKEY_assign_DSA(self->pkey, dsa))
FAIL();
- Py_INCREF(Py_None);
- return Py_None;
- }
+ break;
- PyErr_SetString(crypto_Error, "No such key type");
- return NULL;
+ default:
+ PyErr_SetString(crypto_Error, "No such key type");
+ return NULL;
+
+ }
+ self->initialized = 1;
+ Py_INCREF(Py_None);
+ return Py_None;
}
static char crypto_PKey_bits_doc[] = "\n\
@@ -148,6 +151,14 @@
self->pkey = pkey;
self->dealloc = dealloc;
+ self->only_public = 0;
+
+ /*
+ * Heuristic. Most call-sites pass an initialized EVP_PKEY. Not
+ * necessarily the case that they will, though. That's part of why this is
+ * a hack. -exarkun
+ */
+ self->initialized = 1;
return self;
}
diff --git a/src/crypto/pkey.h b/src/crypto/pkey.h
index d46e360..0227629 100644
--- a/src/crypto/pkey.h
+++ b/src/crypto/pkey.h
@@ -19,7 +19,29 @@
typedef struct {
PyObject_HEAD
+
+ /*
+ * A pointer to the underlying OpenSSL structure.
+ */
EVP_PKEY *pkey;
+
+ /*
+ * A flag indicating the underlying pkey object has no private parts (so it
+ * can't sign, for example). This is a bit of a temporary hack.
+ * Public-only should be represented as a different type. -exarkun
+ */
+ int only_public;
+
+ /*
+ * A flag indicating whether the underlying pkey object has no meaningful
+ * data in it whatsoever. This is a temporary hack. It should be
+ * impossible to create PKeys in an unusable state. -exarkun
+ */
+ int initialized;
+
+ /*
+ * A flag indicating whether pkey will be freed when this object is freed.
+ */
int dealloc;
} crypto_PKeyObj;
diff --git a/src/crypto/x509.c b/src/crypto/x509.c
index ff1f3de..a10c53a 100644
--- a/src/crypto/x509.c
+++ b/src/crypto/x509.c
@@ -309,6 +309,7 @@
{
crypto_PKeyObj *crypto_PKey_New(EVP_PKEY *, int);
EVP_PKEY *pkey;
+ crypto_PKeyObj *py_pkey;
if (!PyArg_ParseTuple(args, ":get_pubkey"))
return NULL;
@@ -319,7 +320,11 @@
return NULL;
}
- return (PyObject *)crypto_PKey_New(pkey, 1);
+ py_pkey = crypto_PKey_New(pkey, 1);
+ if (py_pkey != NULL) {
+ py_pkey->only_public = 1;
+ }
+ return py_pkey;
}
static char crypto_X509_set_pubkey_doc[] = "\n\
@@ -568,6 +573,16 @@
&digest_name))
return NULL;
+ if (pkey->only_public) {
+ PyErr_SetString(PyExc_ValueError, "Key has only public part");
+ return NULL;
+ }
+
+ if (!pkey->initialized) {
+ PyErr_SetString(PyExc_ValueError, "Key is uninitialized");
+ return NULL;
+ }
+
if ((digest = EVP_get_digestbyname(digest_name)) == NULL)
{
PyErr_SetString(PyExc_ValueError, "No such digest method");
diff --git a/src/crypto/x509req.c b/src/crypto/x509req.c
index 7ad1a14..143e4aa 100644
--- a/src/crypto/x509req.c
+++ b/src/crypto/x509req.c
@@ -56,6 +56,7 @@
{
crypto_PKeyObj *crypto_PKey_New(EVP_PKEY *, int);
EVP_PKEY *pkey;
+ crypto_PKeyObj *py_pkey;
if (!PyArg_ParseTuple(args, ":get_pubkey"))
return NULL;
@@ -66,7 +67,11 @@
return NULL;
}
- return (PyObject *)crypto_PKey_New(pkey, 1);
+ py_pkey = crypto_PKey_New(pkey, 1);
+ if (py_pkey != NULL) {
+ py_pkey->only_public = 1;
+ }
+ return py_pkey;
}
static char crypto_X509Req_set_pubkey_doc[] = "\n\
@@ -117,6 +122,16 @@
&digest_name))
return NULL;
+ if (pkey->only_public) {
+ PyErr_SetString(PyExc_ValueError, "Key has only public part");
+ return NULL;
+ }
+
+ if (!pkey->initialized) {
+ PyErr_SetString(PyExc_ValueError, "Key is uninitialized");
+ return NULL;
+ }
+
if ((digest = EVP_get_digestbyname(digest_name)) == NULL)
{
PyErr_SetString(PyExc_ValueError, "No such digest method");
diff --git a/src/ssl/context.c b/src/ssl/context.c
index 6cbebbe..482cc1a 100644
--- a/src/ssl/context.c
+++ b/src/ssl/context.c
@@ -129,43 +129,36 @@
SSL *ssl;
ssl_ConnectionObj *conn;
crypto_X509Obj *cert;
- int errnum, errdepth, c_ret;
-
+ int errnum, errdepth, c_ret, use_thread_state;
+
+ // Get Connection object to check thread state
+ ssl = (SSL *)X509_STORE_CTX_get_app_data(x509_ctx);
+ conn = (ssl_ConnectionObj *)SSL_get_app_data(ssl);
+
+ use_thread_state = conn->tstate != NULL;
+ if (use_thread_state)
+ MY_END_ALLOW_THREADS(conn->tstate);
+
cert = crypto_X509_New(X509_STORE_CTX_get_current_cert(x509_ctx), 0);
errnum = X509_STORE_CTX_get_error(x509_ctx);
errdepth = X509_STORE_CTX_get_error_depth(x509_ctx);
- ssl = (SSL *)X509_STORE_CTX_get_app_data(x509_ctx);
- conn = (ssl_ConnectionObj *)SSL_get_app_data(ssl);
-
+
argv = Py_BuildValue("(OOiii)", (PyObject *)conn, (PyObject *)cert,
errnum, errdepth, ok);
Py_DECREF(cert);
- if (conn->tstate != NULL)
- {
- /* We need to get back our thread state before calling the callback */
- MY_END_ALLOW_THREADS(conn->tstate);
- ret = PyEval_CallObject(conn->context->verify_callback, argv);
- MY_BEGIN_ALLOW_THREADS(conn->tstate);
- }
- else
- {
- ret = PyEval_CallObject(conn->context->verify_callback, argv);
- }
+ ret = PyEval_CallObject(conn->context->verify_callback, argv);
Py_DECREF(argv);
- if (ret == NULL)
- return 0;
-
- if (PyObject_IsTrue(ret))
- {
+ if (ret != NULL && PyObject_IsTrue(ret)) {
X509_STORE_CTX_set_error(x509_ctx, X509_V_OK);
+ Py_DECREF(ret);
c_ret = 1;
- }
- else
+ } else {
c_ret = 0;
+ }
- Py_DECREF(ret);
-
+ if (use_thread_state)
+ MY_BEGIN_ALLOW_THREADS(conn->tstate);
return c_ret;
}
diff --git a/test/test_crypto.py b/test/test_crypto.py
index 5935cc5..b9a7b1b 100644
--- a/test/test_crypto.py
+++ b/test/test_crypto.py
@@ -263,10 +263,51 @@
-class X509ReqTests(TestCase, _Python23TestCaseHelper):
+class _PKeyInteractionTestsMixin:
+ """
+ Tests which involve another thing and a PKey.
+ """
+ def signable(self):
+ """
+ Return something with a C{set_pubkey}, C{set_pubkey}, and C{sign} method.
+ """
+ raise NotImplementedError()
+
+
+ def test_signWithUngenerated(self):
+ """
+ L{X509Req.sign} raises L{ValueError} when pass a L{PKey} with no parts.
+ """
+ request = self.signable()
+ key = PKey()
+ self.assertRaises(ValueError, request.sign, key, 'MD5')
+
+
+ def test_signWithPublicKey(self):
+ """
+ L{X509Req.sign} raises L{ValueError} when pass a L{PKey} with no
+ private part as the signing key.
+ """
+ request = self.signable()
+ key = PKey()
+ key.generate_key(TYPE_RSA, 512)
+ request.set_pubkey(key)
+ pub = request.get_pubkey()
+ self.assertRaises(ValueError, request.sign, pub, 'MD5')
+
+
+
+class X509ReqTests(TestCase, _PKeyInteractionTestsMixin, _Python23TestCaseHelper):
"""
Tests for L{OpenSSL.crypto.X509Req}.
"""
+ def signable(self):
+ """
+ Create and return a new L{X509Req}.
+ """
+ return X509Req()
+
+
def test_construction(self):
"""
L{X509Req} takes no arguments and returns an L{X509ReqType} instance.
@@ -296,10 +337,17 @@
-class X509Tests(TestCase, _Python23TestCaseHelper):
+class X509Tests(TestCase, _PKeyInteractionTestsMixin, _Python23TestCaseHelper):
"""
Tests for L{OpenSSL.crypto.X509}.
"""
+ def signable(self):
+ """
+ Create and return a new L{X509}.
+ """
+ return X509()
+
+
def test_construction(self):
"""
L{X509} takes no arguments and returns an instance of L{X509Type}.