pyOpenSSL

Merge lp://staging/~zseil/pyopenssl/privatekey-callback-fixes into lp://staging/~exarkun/pyopenssl/trunk

  1. privatekey-callback-fixes
  2. Merge into trunk
Proposed by Ziga Seilnacht
Status: Needs review
Proposed branch: lp://staging/~zseil/pyopenssl/privatekey-callback-fixes
Merge into: lp://staging/~exarkun/pyopenssl/trunk
Diff against target: 385 lines (+197/-66)
2 files modified
src/crypto/crypto.c (+91/-66)
test/test_crypto.py (+106/-0)
To merge this branch: bzr merge lp://staging/~zseil/pyopenssl/privatekey-callback-fixes
Reviewer Review Type Date Requested Status
Jean-Paul Calderone Pending
Review via email: mp+16516@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
Ziga Seilnacht (zseil) wrote :

This branch fixes the bugs mentioned in Bug #499628 and contains the tests for the parts that can be tested without introducing some kind of C level fault injector.

The branch contains two backwards incompatible changes:
 - {load, dump}_privatekey now raise an error when called with a passphrase and FILETYPE_ASN1
 - they raise an error when the passphrase callback returns a passphrase longer than 1024 bytes.

I can change load_privatekey to only issue a warning in these conditions, but I think that the current behaviour is too dangerous for dump_privatekey and should be changed to raise an error immediately.

Reply

Unmerged revisions

131. By Ziga Seilnacht

Unify code formatting in recently changed functions to what seems to be the currently preferred style.

130. By Ziga Seilnacht

Add a few more error checks around OpenSSL API calls.

These errors can only occur in low memory conditions, so there
is no reasonable way to test them.

129. By Ziga Seilnacht

Raise an error if a passphrase is used with a private key format that does not support encryption.

Otherwise users might get an unpleasant surprise once they learn that their private key, which they
thought was secure, is in fact readable by everyone.

128. By Ziga Seilnacht

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.

127. By Ziga Seilnacht

Don't overwrite the error raised by the callback.

126. By Ziga Seilnacht

Whitespace cleanup.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/crypto/crypto.c'
2--- src/crypto/crypto.c 2009-07-17 17:50:12 +0000
3+++ src/crypto/crypto.c 2009-12-22 23:42:20 +0000
4@@ -31,22 +31,75 @@
5
6 func = (PyObject *)cb_arg;
7 argv = Py_BuildValue("(i)", rwflag);
8+ if (argv == NULL) {
9+ return 0;
10+ }
11 ret = PyEval_CallObject(func, argv);
12 Py_DECREF(argv);
13- if (ret == NULL)
14+ if (ret == NULL) {
15 return 0;
16- if (!PyString_Check(ret))
17- {
18+ }
19+ if (!PyString_Check(ret)) {
20+ Py_DECREF(ret);
21 PyErr_SetString(PyExc_ValueError, "String expected");
22 return 0;
23 }
24 nchars = PyString_Size(ret);
25- if (nchars > len)
26- nchars = len;
27+ if (nchars > len) {
28+ Py_DECREF(ret);
29+ PyErr_SetString(PyExc_ValueError,
30+ "passphrase returned by callback is too long");
31+ return 0;
32+ }
33 strncpy(buf, PyString_AsString(ret), nchars);
34+ Py_DECREF(ret);
35 return nchars;
36 }
37
38+static PyObject *
39+raise_current_error(void)
40+{
41+ if (PyErr_Occurred()) {
42+ /*
43+ * The python exception from callback is more informative than
44+ * OpenSSL's error.
45+ */
46+ flush_error_queue();
47+ return NULL;
48+ }
49+ exception_from_error_queue(crypto_Error);
50+ return NULL;
51+}
52+
53+static int
54+setup_callback(int type, PyObject *pw, pem_password_cb **cb, void **cb_arg)
55+{
56+ if (pw == NULL) {
57+ *cb = NULL;
58+ *cb_arg = NULL;
59+ return 1;
60+ }
61+ if (type != X509_FILETYPE_PEM) {
62+ PyErr_SetString(PyExc_ValueError,
63+ "only FILETYPE_PEM key format supports encryption");
64+ return 0;
65+ }
66+ if (PyString_Check(pw)) {
67+ *cb = NULL;
68+ *cb_arg = PyString_AsString(pw);
69+ }
70+ else if (PyCallable_Check(pw)) {
71+ *cb = global_passphrase_callback;
72+ *cb_arg = pw;
73+ }
74+ else {
75+ PyErr_SetString(PyExc_TypeError,
76+ "Last argument must be string or callable");
77+ return 0;
78+ }
79+ return 1;
80+}
81+
82 static char crypto_load_privatekey_doc[] = "\n\
83 Load a private key from a buffer\n\
84 \n\
85@@ -71,31 +124,20 @@
86 BIO *bio;
87 EVP_PKEY *pkey;
88
89- if (!PyArg_ParseTuple(args, "is#|O:load_privatekey", &type, &buffer, &len, &pw))
90- return NULL;
91-
92- if (pw != NULL)
93- {
94- if (PyString_Check(pw))
95- {
96- cb = NULL;
97- cb_arg = PyString_AsString(pw);
98- }
99- else if (PyCallable_Check(pw))
100- {
101- cb = global_passphrase_callback;
102- cb_arg = pw;
103- }
104- else
105- {
106- PyErr_SetString(PyExc_TypeError, "Last argument must be string or callable");
107- return NULL;
108- }
109+ if (!PyArg_ParseTuple(args, "is#|O:load_privatekey",
110+ &type, &buffer, &len, &pw)) {
111+ return NULL;
112+ }
113+ if (!setup_callback(type, pw, &cb, &cb_arg)) {
114+ return NULL;
115 }
116
117 bio = BIO_new_mem_buf(buffer, len);
118- switch (type)
119- {
120+ if (bio == NULL) {
121+ exception_from_error_queue(crypto_Error);
122+ return NULL;
123+ }
124+ switch (type) {
125 case X509_FILETYPE_PEM:
126 pkey = PEM_read_bio_PrivateKey(bio, NULL, cb, cb_arg);
127 break;
128@@ -111,10 +153,8 @@
129 }
130 BIO_free(bio);
131
132- if (pkey == NULL)
133- {
134- exception_from_error_queue(crypto_Error);
135- return NULL;
136+ if (pkey == NULL) {
137+ return raise_current_error();
138 }
139
140 return (PyObject *)crypto_PKey_New(pkey, 1);
141@@ -150,49 +190,32 @@
142 crypto_PKeyObj *pkey;
143
144 if (!PyArg_ParseTuple(args, "iO!|sO:dump_privatekey", &type,
145- &crypto_PKey_Type, &pkey, &cipher_name, &pw))
146+ &crypto_PKey_Type, &pkey, &cipher_name, &pw)) {
147 return NULL;
148-
149- if (cipher_name != NULL && pw == NULL)
150- {
151+ }
152+ if (cipher_name != NULL && pw == NULL) {
153 PyErr_SetString(PyExc_ValueError, "Illegal number of arguments");
154 return NULL;
155 }
156- if (cipher_name != NULL)
157- {
158+ if (cipher_name != NULL) {
159 cipher = EVP_get_cipherbyname(cipher_name);
160- if (cipher == NULL)
161- {
162+ if (cipher == NULL) {
163 PyErr_SetString(PyExc_ValueError, "Invalid cipher name");
164 return NULL;
165 }
166- if (PyString_Check(pw))
167- {
168- cb = NULL;
169- cb_arg = PyString_AsString(pw);
170- }
171- else if (PyCallable_Check(pw))
172- {
173- cb = global_passphrase_callback;
174- cb_arg = pw;
175- }
176- else
177- {
178- PyErr_SetString(PyExc_TypeError, "Last argument must be string or callable");
179+ if (!setup_callback(type, pw, &cb, &cb_arg)) {
180 return NULL;
181 }
182 }
183
184 bio = BIO_new(BIO_s_mem());
185- switch (type)
186- {
187+ if (bio == NULL) {
188+ exception_from_error_queue(crypto_Error);
189+ return NULL;
190+ }
191+ switch (type) {
192 case X509_FILETYPE_PEM:
193 ret = PEM_write_bio_PrivateKey(bio, pkey->pkey, cipher, NULL, 0, cb, cb_arg);
194- if (PyErr_Occurred())
195- {
196- BIO_free(bio);
197- return NULL;
198- }
199 break;
200
201 case X509_FILETYPE_ASN1:
202@@ -201,8 +224,12 @@
203
204 case X509_FILETYPE_TEXT:
205 rsa = EVP_PKEY_get1_RSA(pkey->pkey);
206+ if (rsa == NULL) {
207+ ret = 0;
208+ break;
209+ }
210 ret = RSA_print(bio, rsa, 0);
211- RSA_free(rsa);
212+ RSA_free(rsa);
213 break;
214
215 default:
216@@ -211,11 +238,9 @@
217 return NULL;
218 }
219
220- if (ret == 0)
221- {
222+ if (ret == 0) {
223 BIO_free(bio);
224- exception_from_error_queue(crypto_Error);
225- return NULL;
226+ return raise_current_error();
227 }
228
229 buf_len = BIO_get_mem_data(bio, &temp);
230@@ -450,8 +475,8 @@
231 if (!PyArg_ParseTuple(args, "is#:load_pkcs7_data", &type, &buffer, &len))
232 return NULL;
233
234- /*
235- * Try to read the pkcs7 data from the bio
236+ /*
237+ * Try to read the pkcs7 data from the bio
238 */
239 bio = BIO_new_mem_buf(buffer, len);
240 switch (type)
241
242=== modified file 'test/test_crypto.py'
243--- test/test_crypto.py 2009-09-01 14:35:50 +0000
244+++ test/test_crypto.py 2009-12-22 23:42:20 +0000
245@@ -7,6 +7,8 @@
246 from unittest import main
247
248 import os, re
249+import sys
250+
251 from os import popen2
252 from datetime import datetime, timedelta
253
254@@ -1354,6 +1356,18 @@
255 load_privatekey, FILETYPE_PEM, encryptedPrivateKeyPEM, "quack")
256
257
258+ def test_load_privatekey_passphraseWrongType(self):
259+ """
260+ L{load_privatekey} raises C{ValueError} when it is passed a passphrase
261+ with a private key encoded in a format, that doesn't support
262+ encryption.
263+ """
264+ key = load_privatekey(FILETYPE_PEM, cleartextPrivateKeyPEM)
265+ blob = dump_privatekey(FILETYPE_ASN1, key)
266+ self.assertRaises(ValueError,
267+ load_privatekey, FILETYPE_ASN1, blob, "secret")
268+
269+
270 def test_load_privatekey_passphrase(self):
271 """
272 L{load_privatekey} can create a L{PKey} object from an encrypted PEM
273@@ -1395,6 +1409,45 @@
274 self.assertEqual(called, [False])
275
276
277+ def test_load_privatekey_passphraseCallbackError(self):
278+ """
279+ L{load_privatekey} should not overwrite the exception raised
280+ by the passphrase callback.
281+ """
282+ def cb(ignored):
283+ raise ArithmeticError
284+
285+ self.assertRaises(ArithmeticError,
286+ load_privatekey, FILETYPE_PEM, encryptedPrivateKeyPEM, cb)
287+
288+
289+ def test_load_privatekey_passphraseCallbackLeak(self):
290+ """
291+ L{crypto.load_privatekey} should not leak a reference to the
292+ passphrase when the passphrase is provided by a callback.
293+ """
294+ def cb(ignored):
295+ return encryptedPrivateKeyPEMPassphrase
296+
297+ startCount = sys.getrefcount(encryptedPrivateKeyPEMPassphrase)
298+ for i in range(100):
299+ load_privatekey(FILETYPE_PEM, encryptedPrivateKeyPEM, cb)
300+ endCount = sys.getrefcount(encryptedPrivateKeyPEMPassphrase)
301+ self.assert_(endCount - startCount < 5, endCount - startCount)
302+
303+
304+ def test_load_privatekey_passphraseCallbackLength(self):
305+ """
306+ L{crypto.load_privatekey} should raise an error when the passphrase
307+ provided by the callback is too long, not silently truncate it.
308+ """
309+ def cb(ignored):
310+ return "a" * 1025
311+
312+ self.assertRaises(ValueError,
313+ load_privatekey, FILETYPE_PEM, encryptedPrivateKeyPEM, cb)
314+
315+
316 def test_dump_privatekey_passphrase(self):
317 """
318 L{dump_privatekey} writes an encrypted PEM when given a passphrase.
319@@ -1409,6 +1462,17 @@
320 self.assertEqual(loadedKey.bits(), key.bits())
321
322
323+ def test_dump_privatekey_passphraseWrongType(self):
324+ """
325+ L{dump_privatekey} raises C{ValueError} when it is passed a passphrase
326+ with a private key encoded in a format, that doesn't support
327+ encryption.
328+ """
329+ key = load_privatekey(FILETYPE_PEM, cleartextPrivateKeyPEM)
330+ self.assertRaises(ValueError,
331+ dump_privatekey, FILETYPE_ASN1, key, "blowfish", "secret")
332+
333+
334 def test_dump_certificate(self):
335 """
336 L{dump_certificate} writes PEM, DER, and text.
337@@ -1485,6 +1549,48 @@
338 self.assertEqual(loadedKey.bits(), key.bits())
339
340
341+ def test_dump_privatekey_passphraseCallbackError(self):
342+ """
343+ L{dump_privatekey} should not overwrite the exception raised
344+ by the passphrase callback.
345+ """
346+ def cb(ignored):
347+ raise ArithmeticError
348+
349+ key = load_privatekey(FILETYPE_PEM, cleartextPrivateKeyPEM)
350+ self.assertRaises(ArithmeticError,
351+ dump_privatekey, FILETYPE_PEM, key, "blowfish", cb)
352+
353+
354+ def test_dump_privatekey_passphraseCallbackLeak(self):
355+ """
356+ L{crypto.dump_privatekey} should not leak a reference to the
357+ passphrase when the passphrase is provided by a callback.
358+ """
359+ def cb(ignored):
360+ return encryptedPrivateKeyPEMPassphrase
361+
362+ startCount = sys.getrefcount(encryptedPrivateKeyPEMPassphrase)
363+ key = load_privatekey(FILETYPE_PEM, cleartextPrivateKeyPEM)
364+ for i in range(100):
365+ dump_privatekey(FILETYPE_PEM, key, "blowfish", cb)
366+ endCount = sys.getrefcount(encryptedPrivateKeyPEMPassphrase)
367+ self.assert_(endCount - startCount < 5, endCount - startCount)
368+
369+
370+ def test_dump_privatekey_passphraseCallbackLength(self):
371+ """
372+ L{crypto.dump_privatekey} should raise an error when the passphrase
373+ provided by the callback is too long, not silently truncate it.
374+ """
375+ def cb(ignored):
376+ return "a" * 1025
377+
378+ key = load_privatekey(FILETYPE_PEM, cleartextPrivateKeyPEM)
379+ self.assertRaises(ValueError,
380+ dump_privatekey, FILETYPE_PEM, key, "blowfish", cb)
381+
382+
383 def test_load_pkcs7_data(self):
384 """
385 L{load_pkcs7_data} accepts a PKCS#7 string and returns an instance of

Subscribers

People subscribed via source and target branches

to status/vote changes: