[Bug 1371] Add PKCS#11 (Smartcards) support into OpenSSH

Sat Apr 26 18:47:02 EST 2008

https://bugzilla.mindrot.org/show_bug.cgi?id=1371

--- Comment #24 from Damien Miller <djm at mindrot.org>  2008-04-26 18:46:59 ---
(From update of attachment 1467)

Is there a reason behind having to add providers before adding
identities? I think it would be less cumbersome to specify the
provider+key in one go. The common case would be adding all the keys in
a provider, right?

>diff -urNp ssh.org/pkcs11.c ssh/pkcs11.c
>--- ssh.org/pkcs11.c	1970-01-01 02:00:00.000000000 +0200
>+++ ssh/pkcs11.c	2008-01-09 13:26:02.000000000 +0200
>@@ -0,0 +1,944 @@
>+/*
>+ * Copyright(c) 2005-2006 Alon Bar-Lev.  All rights reserved.
>+ *
>+ * The _ssh_from_x509 is dirived of Tatu and Markus work.
>+ * Copyright(c) 2006 Alon bar-Lev <alon.barlev at gmail.com>.  All rights reserved.
>+ * Copyright(c) 2000, 2001 Markus Friedl.  All rights reserved.
>+ * Copyright(c) 1995 Tatu Ylonen <ylo at cs.hut.fi>, Espoo, Finland

I don't think Tatu Ylonen ever used the BSD license. If code is derived
from his sources, it is absolutely essential that his license is
preserved.

>+static char *
>+_ssh_from_x509(X509 *);
>+
>+static time_t
>+__mytime(void)
>+{
>+	return time(NULL);
>+}
>+
>+static void
>+__mysleep(const unsigned long usec)
>+{
>+	usleep((unsigned)usec);
>+}
>+
>+static int
>+__mygettimeofday(struct timeval *tv)
>+{
>+	return gettimeofday(tv, NULL);
>+}

Please do not prefix functions with underscores, declaring them static
is enough.

>+static void
>+_pkcs11_openssh_log(void *const global_data, unsigned flags,
>+	const char *const format, va_list args)
>+{
>+	(void) global_data;

Please do not avoid compiler warnings like this. Just add a comment "/*
ARGSUSED */" before each function that does not use all its arguments.

>+static PKCS11H_BOOL
>+_pkcs11_ssh_pin_prompt(void *const global_data,
>+	void *const user_data, const pkcs11h_token_id_t token,
>+	const unsigned retry, char *const pin, const size_t pin_max)
>+{
>+	char prompt[1024];
>+	char *passphrase = NULL;
>+	PKCS11H_BOOL ret = FALSE;
>+
>+	(void) global_data;
>+	(void) user_data;
>+	(void) retry;
>+
>+	if (snprintf(prompt, sizeof(prompt), "Please enter PIN for token '%s': ",
>+		token->display) < 0)
>+		goto cleanup;
>+
>+	passphrase = read_passphrase(prompt, RP_ALLOW_EOF);
>+
>+	if (passphrase == NULL || strlen(passphrase) == 0 ||
>+		strlen(passphrase) > pin_max-1)
>+		goto cleanup;
>+	
>+	strncpy(pin, passphrase, pin_max);

Please use strlcpy here. If you need the trailing bytes of pin zeroed,
then please use an explicit bzero() call.

>+static int
>+_pkcs11_convert_to_ssh_key(const pkcs11h_certificate_id_t certificate_id, Key **const key,
>+	char **const comment, const int pin_cache_period)
...
>+	if ((rv = pkcs11h_certificate_getCertificateBlob(certificate,
>+				NULL, &temp)) != CKR_OK) {

What is the purpose of this function call? You don't every receive a
certificate blob and you don't use the blob length. Is it just to
ensure that the provider is able to return a certificate?

>+	if ((rv = pkcs11h_certificate_getCertificateId(certificate,
>+		&certificate_id_new)) != CKR_OK) {

Why duplicate the certificate ID? 

(Sorry if these are obvious questions, but I can't find any
documentation on pkcs11-helper other than some doxygen stuff that
doesn't tell much much more than the headers themselves)

>+PKCS11Provider *
>+pkcs11_parse_provider(const char *const info)
...
>+	/*
>+	 * provider[:protected_authentication[:private_mode[:cert_is_private]]]
>+	 * string:1|0:hex:1|0
>+	 */

Is this complexity needed? Under what circumstances will users need to
specify more than just "provider"?

>+int
>+pkcs11_get_key(const PKCS11Id *const id, Key **const key,
>+	char **const comment)
...
>+	if (id->cert_file != NULL && id->cert_file[0] != '\x0') {

Where does the cert_file come from? Is it created implicitly by the
library? Does it need to be cleaned up?

>+int
>+pkcs11_get_keys(Key ***const keys, char ***const comments)
...
>+	if((internal_keys = xmalloc((PKCS11_MAX_KEYS+1)*sizeof(*internal_keys))) == NULL) {

Please use xcalloc() instead of xmalloc(x * y).

>+void
>+pkcs11_show_ids(void)

Can some of this (perhaps just provider & DN) go into the comment, so
"ssh-keygen -l" does the right thing automatically?

>+static char *
>+_ssh_from_x509(X509 * x509)
>+{
>+#define PUT_32BIT(cp, value) ( \
>+	(cp)[0] = (unsigned char)((value) >> 24), \
>+	(cp)[1] = (unsigned char)((value) >> 16), \
>+	(cp)[2] = (unsigned char)((value) >> 8), \
>+	(cp)[3] = (unsigned char)((value) >> 0) )

Please use put_u32() in misc.[ch] if you need it at all (see below).

>+	bytes_name = strlen(keyname);
>+	bytes_exponent = (size_t)BN_num_bytes(pubkey->pkey.rsa->e);
>+	bytes_modulus = (size_t)BN_num_bytes(pubkey->pkey.rsa->n);
>+
>+	blobsize = (4 + bytes_name + 4 + ((unsigned)bytes_exponent + 1) + 4 +
>+		((unsigned)bytes_modulus + 1) + 1);
>+
>+	if ((blob = (unsigned char *) xmalloc(blobsize)) == NULL)
>+		goto cleanup;
>+
>+	if ((buffer = (unsigned char *) xmalloc(blobsize)) == NULL)
>+		goto cleanup;
>+
>+	bp = blob;
>+
>+	PUT_32BIT(bp, bytes_name), bp += 4;
>+	memcpy(bp, keyname, bytes_name), bp += (ssize_t)bytes_name;
>+
>+	BN_bn2bin(pubkey->pkey.rsa->e, buffer);
>+	if (buffer[0] & 0x80) {
>+		// highest bit set would indicate a negative number.
>+		// to avoid this, we have to spend an extra byte:
>+		PUT_32BIT(bp, bytes_exponent + 1), bp += 4;
>+		*(bp++) = 0;
>+	} else
>+		PUT_32BIT(bp, bytes_exponent), bp += 4;
>+
>+	memcpy(bp, buffer, bytes_exponent), bp += (ssize_t)bytes_exponent;
>+
>+	BN_bn2bin(pubkey->pkey.rsa->n, buffer);
>+	if (buffer[0] & 0x80) {
>+		PUT_32BIT(bp, bytes_modulus + 1), bp += 4;
>+		*(bp++) = 0;
>+	} else
>+		PUT_32BIT(bp, bytes_modulus), bp += 4;
>+
>+	memcpy(bp, buffer, bytes_modulus), bp += (ssize_t)bytes_modulus;

Please use the OpenSSH buffer.h functions here, they do all the math
for you:

Buffer b;

buffer_init(&b);
buffer_put_cstring(&b, keyname);
buffer_put_bignum2(&b, pubkey->pkey.rsa->e);
buffer_put_bignum2(&b, pubkey->pkey.rsa->n);
if (BIO_write(bio2, buffer_ptr(&b), buffer_len(&b)) == -1)
        goto cleanup;

>+	if ((n = BIO_read(bio, ret + (ssize_t)strlen(ret),
>+		(int)(retsize - strlen(ret) - 1))) == -1)
>+		goto cleanup;

What are you reading back here?

>diff -urNp ssh.org/pkcs11.h ssh/pkcs11.h
>--- ssh.org/pkcs11.h	1970-01-01 02:00:00.000000000 +0200
>+++ ssh/pkcs11.h	2008-01-09 12:59:05.000000000 +0200
...
>+typedef struct {
>+	char *provider;
>+	int protected_authentication;
>+	unsigned private_mode;
>+	int cert_is_private;
>+} PKCS11Provider;
>+
>+typedef struct {
>+	char *id;
>+	int pin_cache_period;
>+	char *cert_file;
>+}	PKCS11Id;

Unless you need to follow some external naming convention, please use
all lowercase names, e.g. "struct pkcs11_provider".

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.