Whilst conducting some testing with OpenSSL 0.9.7beta4 and the nCipher
chil plugin, I observed the following issues:
1. Lack of threadsafety if app fails to support new OpenSSL dynamic locks
At the moment hw_ncipher.c uses the new OpenSSL dynamic lock code inorder
to implement the hwcrhk locking upcalls. However if the OpenSSL application
fails to implement the dynamic upcalls, i.e.
CRYPTO_get_dynlock_create_callback() returns NULL, then the hwcrhk engine
code becomes non threadsafe.
This is a serious problem given that it is unlikely that existing
applications such as apache will implement dynamic upcalls anytime soon,
after all OpenSSL dynamic locks are only required by hw_ncipher.c at the
moment.
Here are some errors from apache when configured using --with-mpm=worker and
with SSLCryptoDevice chil set in ssl.conf.
[Thu Nov 28 15:53:11 2002] [notice] Apache/2.0.43 (Unix) mod_ssl/2.0.43 \
OpenSSL/0.9.7-beta4 configured -- resuming normal operations
httpd: ../setup.c:472: nfast_hwch_doneconnection: Assertion `hwctx->conn\
datas[remember].inuse > 0' failed.
[Thu Nov 28 15:56:31 2002] [notice] child pid 17526 exit signal Aborted \
(6)
httpd: ../setup.c:472: nfast_hwch_doneconnection: Assertion `hwctx->conn\
datas[remember].inuse > 0' failed.
httpd: ../setup.c:472: nfast_hwch_doneconnection: Assertion `hwctx->conn\
datas[remember].inuse > 0' failed.
httpd: ../setup.c:472: nfast_hwch_doneconnection: Assertion `hwctx->conn\
datas[remember].inuse > 0' failed.
There is a simple workaround that one can use in hw_ncipher.c to support
multithreaded programs in the case when only static locks are supported
by the application. The workaround use the maxmutexes member of
HWCryptoHook_InitInfo to configure the hwcrhk library to only ever request
one lock context.
This workaround is contained in the patch.
2. Missing emsg in call to p_hwcrhk_ModExpCRT() in hw_ncipher.c
The p_hwcrhk_ModExpCRT() function call made in hwcrhk_rsa_mod_exp()
forgets to pass in &emsg as the last argument. This means that errors
messages from chil occuring in this function are lost.
This bug is fixed in the patch.
3. hwcrhk_log_message() in hw_ncipher.c
This fails to write a newline character to the logstream after each
log message. So for instance setting debug in s_client produces:
$ ./openssl s_client -engine chil -debug -connect localhost:443
[12370] Loading nfhwch 1.5 flags=00000010 bignums=4ll mutexes getpas\
sphrase getphystoken maxmutexes=1 maxsimultaneous=1000 (hwcrhk 1.5.0\
cam1 hwcrhk/build 2.0.6cam1 hwcrhk/nfast 1.77.9cam1 hwcrhk/nfast/bui\
ld 2.0.6cam1 hwcrhk/sworld 1.14.3cam1 hwcrhk/sworld/build 2.0.6cam1 \
hwcrhk/sworld/nfast 1.77.9cam1 hwcrhk/sworld/nfast/build 2.0.6cam1)n\
Cipher nFast HWCryptoHook 1.5engine "chil" set.
CONNECTED(00000004)
write to 08181FE8 [08182040] (148 bytes => 148 (0x94))
0000 - 80 92 01 03 01 00 69 00-00 00 20 00 00 39 00 00 ......i... ..9..
0010 - 38 00 00 35 00 00 16 00-00 13 00 00 0a 07 00 c0 8..5............
0020 - 00 00 33 00 00 32 00 00-2f 00 00 07 05 00 80 03 ..3..2../.......
0030 - 00 80 00 00 66 00 00 05-00 00 04 01 00 80 08 00 ....f...........
0040 - 80 00 00 63 00 00 62 00-00 61 00 00 15 00 00 12 ...c..b..a......
0050 - 00 00 09 06 00 40 00 00-65 00 00 64 00 00 60 00 [email protected]..`.
0060 - 00 14 00 00 11 00 00 08-00 00 06 04 00 80 00 00 ................
0070 - 03 02 00 80 86 a5 6b 75-67 16 d1 78 3d 13 c9 62 ......kug..x=..b
0080 - 6e 84 8e ff ff 05 d9 ee-cf d3 5e 0e f4 59 56 23 n.........^..YV#
0090 - e9 5d 2d 5b .]-[
...
This bug is fixed in the patch.
4. hwcrhk_insert_card() in hw_ncipher.c
$ createocs 1 0 testcardset 1 0
Insert new operator card 1 into module 1 slot 0 and press return...
Passphrase for new operator card 1: <NO PASSPHRASE>
cardset created; hkltu = a0f043a70b4f2fd9392149f95390b273e7f72f50
$ generatekey --batch hwcrhk ident=testkey
nCipher KM key generation/import utility
key generation/import parameter(s):
protect Protected by TOKEN
ident Key identifier testkey
type Key type RSA
size Key size 1024
paramsreadfile Group parameters file (DH only)
recovery Recovery feature 1
Using Operator Card Set `testcardset'.
Loaded Operator Card Set, using card #1.
Generating fresh key ...
Key generated and stored.
<NOW TAKE THE CARDSET OUT OF THE HSM'S SLOT>
$ ./openssl req -engine chil -keyform ENGINE -key testkey -new
engine "chil" set.
unable to load Private Key
12099:error:2806D069:lib(40):GENERAL_ALLOCATE_PROMPT:no result
buffer:ui_lib.c:150:
12099:error:81069066:hwcrhk engine:HWCRHK_LOAD_PRIVKEY:chil
error:hw_ncipher.c:792:Failed to load key (codes: m1b0s0SE m1b0s1SE
m1b0s0SE m1b0s1SE m1b0CN-1 m1BN)
12099:error:26096080:engine routines:ENGINE_load_private_key:failed loading
private key:eng_pkey.c:117:
The bug is in the following code:
int UI_dup_info_string(UI *ui, const char *text)
{
char *text_copy=NULL;
if (text)
{
text_copy=BUF_strdup(text);
if (text_copy == NULL)
{
UIerr(UI_F_UI_DUP_INFO_STRING,ERR_R_MALLOC_FAILURE);
return -1;
}
}
return general_allocate_string(ui, text_copy, 1, UIT_INFO, 0, NULL,
0, 0, NULL);
}
this code calls general_allocate_string() with a NULL result_buf which
causes the error above to be thrown by general_allocate_prompt().
Unfortunately I have not had enough time to understand the ui_lib code
sufficiently well to attempt to patch this problem. In any event the
ability to use with-nfast to preload an ncipher hwcrhk key makes this
a non-urgent problem.
I hope that this mail is helpful,
Bertie
diff -ur openssl-0.9.7-beta4/crypto/cryptlib.c
openssl-0.9.7-beta4-patched/crypto/cryptlib.c
--- openssl-0.9.7-beta4/crypto/cryptlib.c Tue Nov 12 13:20:47 2002
+++ openssl-0.9.7-beta4-patched/crypto/cryptlib.c Fri Nov 29 08:50:40 2002
@@ -104,7 +104,8 @@
"dynlock",
"engine",
"ui",
-#if CRYPTO_NUM_LOCKS != 32
+ "hwcrhk",
+#if CRYPTO_NUM_LOCKS != 33
# error "Inconsistency between crypto.h and cryptlib.c"
#endif
};
Only in openssl-0.9.7-beta4-patched/crypto: cryptlib.o
diff -ur openssl-0.9.7-beta4/crypto/crypto.h
openssl-0.9.7-beta4-patched/crypto/crypto.h
--- openssl-0.9.7-beta4/crypto/crypto.h Fri Nov 15 22:41:37 2002
+++ openssl-0.9.7-beta4-patched/crypto/crypto.h Fri Nov 29 08:50:45 2002
@@ -130,7 +130,8 @@
#define CRYPTO_LOCK_DYNLOCK 29
#define CRYPTO_LOCK_ENGINE 30
#define CRYPTO_LOCK_UI 31
-#define CRYPTO_NUM_LOCKS 32
+#define CRYPTO_LOCK_HWCRHK 32
+#define CRYPTO_NUM_LOCKS 33
#define CRYPTO_LOCK 1
#define CRYPTO_UNLOCK 2
diff -ur openssl-0.9.7-beta4/crypto/engine/hw_ncipher.c
openssl-0.9.7-beta4-patched/crypto/engine/hw_ncipher.c
--- openssl-0.9.7-beta4/crypto/engine/hw_ncipher.c Wed Nov 13 15:29:00 2002
+++ openssl-0.9.7-beta4-patched/crypto/engine/hw_ncipher.c Tue Dec 3 14:17:01
+2002
@@ -91,11 +91,17 @@
static int hwcrhk_finish(ENGINE *e);
static int hwcrhk_ctrl(ENGINE *e, int cmd, long i, void *p, void (*f)());
-/* Functions to handle mutexes */
+/* Functions to handle mutexes if have dynamic locks */
static int hwcrhk_mutex_init(HWCryptoHook_Mutex*, HWCryptoHook_CallerContext*);
static int hwcrhk_mutex_lock(HWCryptoHook_Mutex*);
static void hwcrhk_mutex_unlock(HWCryptoHook_Mutex*);
static void hwcrhk_mutex_destroy(HWCryptoHook_Mutex*);
+/* Functions to handle mutexes if only have static locks */
+static int hwcrhk_static_mutex_init(HWCryptoHook_Mutex *m,
+ HWCryptoHook_CallerContext *c);
+static int hwcrhk_static_mutex_lock(HWCryptoHook_Mutex *m);
+static void hwcrhk_static_mutex_unlock(HWCryptoHook_Mutex *m);
+static void hwcrhk_static_mutex_destroy(HWCryptoHook_Mutex *m);
/* BIGNUM stuff */
static int hwcrhk_mod_exp(BIGNUM *r, const BIGNUM *a, const BIGNUM *p,
@@ -560,16 +566,26 @@
/* Check if the application decided to support dynamic locks,
and if it does, use them. */
- if (disable_mutex_callbacks == 0 &&
- CRYPTO_get_dynlock_create_callback() != NULL &&
- CRYPTO_get_dynlock_lock_callback() != NULL &&
- CRYPTO_get_dynlock_destroy_callback() != NULL)
+ if (disable_mutex_callbacks == 0)
+ {
+ if ( CRYPTO_get_dynlock_create_callback() != NULL &&
+ CRYPTO_get_dynlock_lock_callback() != NULL &&
+ CRYPTO_get_dynlock_destroy_callback() != NULL)
{
- hwcrhk_globals.mutex_init = hwcrhk_mutex_init;
+ hwcrhk_globals.mutex_init = hwcrhk_mutex_init;
hwcrhk_globals.mutex_acquire = hwcrhk_mutex_lock;
hwcrhk_globals.mutex_release = hwcrhk_mutex_unlock;
hwcrhk_globals.mutex_destroy = hwcrhk_mutex_destroy;
}
+ else
+ {
+ hwcrhk_globals.maxmutexes = 1; /* Only have one lock */
+ hwcrhk_globals.mutex_init = hwcrhk_static_mutex_init;
+ hwcrhk_globals.mutex_acquire = hwcrhk_static_mutex_lock;
+ hwcrhk_globals.mutex_release = hwcrhk_static_mutex_unlock;
+ hwcrhk_globals.mutex_destroy = hwcrhk_static_mutex_destroy;
+ }
+ }
/* Try and get a context - if not, we may have a DSO but no
* accelerator! */
@@ -1022,7 +1038,7 @@
/* Perform the operation */
ret = p_hwcrhk_ModExpCRT(hwcrhk_context, m_a, m_p, m_q,
- m_dmp1, m_dmq1, m_iqmp, &m_r, NULL);
+ m_dmp1, m_dmq1, m_iqmp, &m_r, &rmsg);
/* Convert the response */
r->top = m_r.size / sizeof(BN_ULONG);
@@ -1173,6 +1189,26 @@
CRYPTO_destroy_dynlockid(mt->lockid);
}
+/* Mutex upcalls to use if the application does not support
+ * dynamic locks
+ */
+
+static int hwcrhk_static_mutex_init(HWCryptoHook_Mutex *m,
+ HWCryptoHook_CallerContext *c)
+ {
+ return 0;
+ }
+static int hwcrhk_static_mutex_lock(HWCryptoHook_Mutex *m)
+ {
+ CRYPTO_w_lock(CRYPTO_LOCK_HWCRHK);
+ return 0;
+ }
+static void hwcrhk_static_mutex_unlock(HWCryptoHook_Mutex *m)
+ {
+ CRYPTO_w_unlock(CRYPTO_LOCK_HWCRHK);
+ }
+static void hwcrhk_static_mutex_destroy(HWCryptoHook_Mutex *m) { }
+
static int hwcrhk_get_pass(const char *prompt_info,
int *len_io, char *buf,
HWCryptoHook_PassphraseContext *ppctx,
@@ -1320,7 +1356,10 @@
lstream=*(BIO **)logstr;
if (lstream)
{
- BIO_write(lstream, message, strlen(message));
+ if ( BIO_write(lstream, message, strlen(message)) > 0 )
+ {
+ BIO_write(lstream,"\n",1);
+ }
}
CRYPTO_w_unlock(CRYPTO_LOCK_BIO);
}
diff -ur openssl-0.9.7-beta4/include/openssl/crypto.h
openssl-0.9.7-beta4-patched/include/openssl/crypto.h
--- openssl-0.9.7-beta4/include/openssl/crypto.h Fri Nov 15 22:41:37 2002
+++ openssl-0.9.7-beta4-patched/include/openssl/crypto.h Fri Nov 29 08:50:45
+2002
@@ -130,7 +130,8 @@
#define CRYPTO_LOCK_DYNLOCK 29
#define CRYPTO_LOCK_ENGINE 30
#define CRYPTO_LOCK_UI 31
-#define CRYPTO_NUM_LOCKS 32
+#define CRYPTO_LOCK_HWCRHK 32
+#define CRYPTO_NUM_LOCKS 33
#define CRYPTO_LOCK 1
#define CRYPTO_UNLOCK 2
