[engine_pkcs11] Various fixes and features

classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|

[engine_pkcs11] Various fixes and features

Petr Pisar
Hello,

while testing TLS client authentication using a cryprographical token in my
project (libisds over cURL over OpenSSL with Athena USB token under OpenSC),
I found a lot of bugs in the engine_pkcs11 plug-in for OpenSSL.

Some of the bugs are so serious that they prevent from using the token through
OpenSSL and can lead even to a segmentation fault. So I deciced to fix them
and post the pathes here in hope the engine_pkcs11 maintainer will review them
and merge them.

Here is a short description, patches will be sent as replies:

[PATCH 1/9] Unify PIN freeing
[PATCH 2/9] Free PIN storage where needed

These two patches fix memory leaks when storing a PIN code.

[PATCH 3/9] Use user interface correctly

This fixes a crash (segmenation fault) when loading a private key. Current
code could never use a PIN passed from OpenSSL because of wrong usage of the
user interface call-back data. I send a fix to cURL library
<http://thread.gmane.org/gmane.comp.web.curl.library/40222> too and I tested
the colaboration between cURL and engine_pkcs11 successfully.

[PATCH 4/9] Hexadecimal ID string contains colons

A certificate/key object hexadecimal ID is printed with colons (ab:cd:..)
everywhere. Let's allow engine_pkcs11 to recognize it. Contrary current parser
expects the colons by can not recognize such string as an ID. I believe it
was not possible to use the hexadecimal ID before.

[PATCH 5/9] Find token if no slot was specified

Identifier wihout a slot number (e.g. a plain ID) always resulted to slot
number 0. This searches all slots now.

[PATCH 6/9] Search for a certificate by a label

Searching a certificate by a label did not work and worked differently than
searching a key. This caused a lot of confusion why OpenSSL can locate the key
but it cannot locate the certificate.

[PATCH 7/9] Decouple loging into the token
[PATCH 8/9] Implement ENGINE_load_ssl_client_cert()
[PATCH 9/9] Add load_ssl_client_cert test

These tree patches implement ENGINE_load_ssl_client_cert() interface which
allows automatic negotion of client certificate in TLS authenticatinon. The
ninth patch provides a test.

-- Petr

------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/9] Unify PIN freeing

Petr Pisar
---
 src/engine_pkcs11.c | 45 +++++++++++++++++----------------------------
 1 file changed, 17 insertions(+), 28 deletions(-)

diff --git a/src/engine_pkcs11.c b/src/engine_pkcs11.c
index c1b8fbb..6e248e4 100644
--- a/src/engine_pkcs11.c
+++ b/src/engine_pkcs11.c
@@ -66,6 +66,18 @@ int set_module(const char *modulename)
  return 1;
 }
 
+
+/* Free PIN storage in secure way. */
+static void free_pin(void)
+{
+ if (pin != NULL) {
+ OPENSSL_cleanse(pin, pin_length);
+ free(pin);
+ pin = NULL;
+ pin_length = 0;
+ }
+}
+
 /**
  * Set the PIN used for login. A copy of the PIN shall be made.
  *
@@ -158,12 +170,7 @@ int pkcs11_finish(ENGINE * engine)
  PKCS11_CTX_free(ctx);
  ctx = NULL;
  }
- if (pin != NULL) {
- OPENSSL_cleanse(pin, pin_length);
- free(pin);
- pin = NULL;
- pin_length = 0;
- }
+ free_pin();
  return 1;
 }
 
@@ -183,12 +190,7 @@ int pkcs11_init(ENGINE * engine)
 
 int pkcs11_rsa_finish(RSA * rsa)
 {
- if (pin) {
- OPENSSL_cleanse(pin, pin_length);
- free(pin);
- pin = NULL;
- pin_length = 0;
- }
+ free_pin();
  if (module) {
  free(module);
  module = NULL;
@@ -691,12 +693,7 @@ static EVP_PKEY *pkcs11_load_key(ENGINE * e, const char *s_slot_key_id,
  if (tok->secureLogin) {
  /* Free the PIN if it has already been
    assigned (i.e, cached by get_pin) */
- if (pin != NULL) {
- OPENSSL_cleanse(pin, pin_length);
- free(pin);
- pin = NULL;
- pin_length = 0;
- }
+ free_pin();
  } else if (pin == NULL) {
  pin = (char *)calloc(MAX_PIN_LENGTH, sizeof(char));
  pin_length = MAX_PIN_LENGTH;
@@ -704,10 +701,7 @@ static EVP_PKEY *pkcs11_load_key(ENGINE * e, const char *s_slot_key_id,
  fail("Could not allocate memory for PIN");
  }
  if (!get_pin(ui_method, callback_data) ) {
- OPENSSL_cleanse(pin, pin_length);
- free(pin);
- pin = NULL;
- pin_length = 0;
+ free_pin();
  fail("No pin code was entered");
  }
  }
@@ -715,12 +709,7 @@ static EVP_PKEY *pkcs11_load_key(ENGINE * e, const char *s_slot_key_id,
  /* Now login in with the (possibly NULL) pin */
  if (PKCS11_login(slot, 0, pin)) {
  /* Login failed, so free the PIN if present */
- if (pin != NULL) {
- OPENSSL_cleanse(pin, pin_length);
- free(pin);
- pin = NULL;
- pin_length = 0;
- }
+ free_pin();
  fail("Login failed\n");
  }
  /* Login successful, PIN retained in case further logins are
--
1.8.1.5


------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/9] Free PIN storage where needed

Petr Pisar
In reply to this post by Petr Pisar
---
 src/engine_pkcs11.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/engine_pkcs11.c b/src/engine_pkcs11.c
index 6e248e4..0c0b383 100644
--- a/src/engine_pkcs11.c
+++ b/src/engine_pkcs11.c
@@ -101,6 +101,7 @@ int set_pin(const char *_pin)
 
  /* Copy the PIN. If the string cannot be copied, NULL
    shall be returned and errno shall be set. */
+ free_pin();
  pin = strdup(_pin);
  if (pin != NULL)
  pin_length = strlen(pin);
@@ -127,6 +128,7 @@ static int get_pin(UI_METHOD * ui_method, void *callback_data)
 
  /* pin in the call back data, copy and use */
  if (mycb != NULL && mycb->password) {
+ free_pin();
  pin = (char *)calloc(MAX_PIN_LENGTH, sizeof(char));
  if (!pin)
  return 0;
@@ -142,6 +144,11 @@ static int get_pin(UI_METHOD * ui_method, void *callback_data)
  if (callback_data != NULL)
  UI_set_app_data(ui, callback_data);
 
+ free_pin();
+ pin = (char *)calloc(MAX_PIN_LENGTH, sizeof(char));
+ if (!pin)
+ return 0;
+ pin_length = MAX_PIN_LENGTH;
  if (!UI_add_input_string
     (ui, "PKCS#11 token PIN: ", 0, pin, 1, MAX_PIN_LENGTH)) {
  fprintf(stderr, "UI_add_input_string failed\n");
--
1.8.1.5


------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 3/9] Use user interface correctly

Petr Pisar
In reply to this post by Petr Pisar
Call-back data passed by the OpenSSL from an application are application
specific data opaque to an engine.

Previous engine_pkcs11 code tried to use the call-back data which could
result in crash. This patch fixes it.
---
 src/engine_pkcs11.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/src/engine_pkcs11.c b/src/engine_pkcs11.c
index 0c0b383..829b59d 100644
--- a/src/engine_pkcs11.c
+++ b/src/engine_pkcs11.c
@@ -115,34 +115,24 @@ int inc_verbose(void)
  return 1;
 }
 
-/* either get the pin code from the supplied callback data, or get the pin
- * via asking our self. In both cases keep a copy of the pin code in the
- * pin variable (strdup'ed copy). */
+/* Get the PIN via asking user interface. The supplied call-back data are
+ * passed to the user interface implemented by an application. Only the
+ * application knows how to interpret the call-back data.
+ * A (strdup'ed) copy of the PIN code will be stored in the pin variable. */
 static int get_pin(UI_METHOD * ui_method, void *callback_data)
 {
  UI *ui;
- struct {
- const void *password;
- const char *prompt_info;
- } *mycb = callback_data;
-
- /* pin in the call back data, copy and use */
- if (mycb != NULL && mycb->password) {
- free_pin();
- pin = (char *)calloc(MAX_PIN_LENGTH, sizeof(char));
- if (!pin)
- return 0;
- strncpy(pin,mycb->password,MAX_PIN_LENGTH);
- pin_length = MAX_PIN_LENGTH;
- return 1;
- }
 
  /* call ui to ask for a pin */
  ui = UI_new();
+ if (ui == NULL) {
+ fprintf(stderr, "UI_new failed\n");
+ return 0;
+ }
  if (ui_method != NULL)
  UI_set_method(ui, ui_method);
  if (callback_data != NULL)
- UI_set_app_data(ui, callback_data);
+ UI_add_user_data(ui, callback_data);
 
  free_pin();
  pin = (char *)calloc(MAX_PIN_LENGTH, sizeof(char));
@@ -150,7 +140,8 @@ static int get_pin(UI_METHOD * ui_method, void *callback_data)
  return 0;
  pin_length = MAX_PIN_LENGTH;
  if (!UI_add_input_string
-    (ui, "PKCS#11 token PIN: ", 0, pin, 1, MAX_PIN_LENGTH)) {
+    (ui, "PKCS#11 token PIN: ", UI_INPUT_FLAG_DEFAULT_PWD,
+    pin, 1, MAX_PIN_LENGTH)) {
  fprintf(stderr, "UI_add_input_string failed\n");
  UI_free(ui);
  return 0;
--
1.8.1.5


------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 4/9] Hexadecimal ID string contains colons

Petr Pisar
In reply to this post by Petr Pisar
The hexadecimal ID string is supposed to separate nibbles by a colon, so to
recognize the string as such.

Plain <ID> will clash with <slot>:<ID> syntax but there is no help. User
should use slot_<slot>-id_<ID> instead.
---
 src/engine_pkcs11.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/engine_pkcs11.c b/src/engine_pkcs11.c
index 829b59d..65289a7 100644
--- a/src/engine_pkcs11.c
+++ b/src/engine_pkcs11.c
@@ -257,7 +257,7 @@ static int parse_slot_id_string(const char *slot_id, int *slot,
  return 0;
 
  /* support for several formats */
-#define HEXDIGITS "01234567890ABCDEFabcdef"
+#define HEXDIGITS "01234567890ABCDEFabcdef:"
 #define DIGITS "0123456789"
 
  /* first: pure hex number (id, slot is 0) */
--
1.8.1.5


------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 5/9] Find token if no slot was specified

Petr Pisar
In reply to this post by Petr Pisar
label_<label> allows to search for any token, let's behave the same way with
id_<ID> or <ID> too.

Without this patch, id_<ID> did not work despite the label_<label> worked.
Especially if the only used slot number is different from 0.
---
 src/engine_pkcs11.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/engine_pkcs11.c b/src/engine_pkcs11.c
index 65289a7..42d90e2 100644
--- a/src/engine_pkcs11.c
+++ b/src/engine_pkcs11.c
@@ -260,14 +260,14 @@ static int parse_slot_id_string(const char *slot_id, int *slot,
 #define HEXDIGITS "01234567890ABCDEFabcdef:"
 #define DIGITS "0123456789"
 
- /* first: pure hex number (id, slot is 0) */
+ /* first: pure hex number (id, slot is undefined) */
  if (strspn(slot_id, HEXDIGITS) == strlen(slot_id)) {
  /* ah, easiest case: only hex. */
  if ((strlen(slot_id) + 1) / 2 > *id_len) {
  fprintf(stderr, "id string too long!\n");
  return 0;
  }
- *slot = 0;
+ *slot = -1;
  return hex_to_bin(slot_id, id, id_len);
  }
 
@@ -298,7 +298,7 @@ static int parse_slot_id_string(const char *slot_id, int *slot,
  return hex_to_bin(slot_id + i, id, id_len);
  }
 
- /* third: id_<id>  */
+ /* third: id_<id>, slot is undefined */
  if (strncmp(slot_id, "id_", 3) == 0) {
  if (strspn(slot_id + 3, HEXDIGITS) + 3 != strlen(slot_id)) {
  fprintf(stderr, "could not parse string!\n");
@@ -309,12 +309,13 @@ static int parse_slot_id_string(const char *slot_id, int *slot,
  fprintf(stderr, "id string too long!\n");
  return 0;
  }
- *slot = 0;
+ *slot = -1;
  return hex_to_bin(slot_id + 3, id, id_len);
  }
 
- /* label_<label>  */
+ /* label_<label>, slot is undefined */
  if (strncmp(slot_id, "label_", 6) == 0) {
+ *slot = -1;
  *label = strdup(slot_id + 6);
  return *label != NULL;
  }
--
1.8.1.5


------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 6/9] Search for a certificate by a label

Petr Pisar
In reply to this post by Petr Pisar
Previously, it was not possible to load a certificate by a label because it
alwayes searched by undefined ID value. This has been fixed to behave in the
same way as searching for a key.
---
 src/engine_pkcs11.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/engine_pkcs11.c b/src/engine_pkcs11.c
index 42d90e2..34b65d6 100644
--- a/src/engine_pkcs11.c
+++ b/src/engine_pkcs11.c
@@ -497,13 +497,19 @@ static X509 *pkcs11_load_cert(ENGINE * e, const char *s_slot_cert_id)
  fprintf(stderr, "Found %u cert%s:\n", cert_count,
  (cert_count <= 1) ? "" : "s");
  }
- if ((s_slot_cert_id && *s_slot_cert_id) && (cert_id_len != 0)) {
+ if ((s_slot_cert_id && *s_slot_cert_id) && (cert_id_len != 0 || cert_label != NULL)) {
  for (n = 0; n < cert_count; n++) {
  PKCS11_CERT *k = certs + n;
 
- if (cert_id_len != 0 && k->id_len == cert_id_len &&
-    memcmp(k->id, cert_id, cert_id_len) == 0) {
- selected_cert = k;
+ if (cert_label == NULL) {
+ if (cert_id_len != 0 && k->id_len == cert_id_len &&
+    memcmp(k->id, cert_id, cert_id_len) == 0) {
+    selected_cert = k;
+ }
+ } else {
+ if (strcmp(k->label, cert_label) == 0) {
+    selected_cert = k;
+ }
  }
  }
  } else {
--
1.8.1.5


------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 7/9] Decouple loging into the token

Petr Pisar
In reply to this post by Petr Pisar
---
 src/engine_pkcs11.c | 101 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 60 insertions(+), 41 deletions(-)

diff --git a/src/engine_pkcs11.c b/src/engine_pkcs11.c
index 34b65d6..1c1842d 100644
--- a/src/engine_pkcs11.c
+++ b/src/engine_pkcs11.c
@@ -39,6 +39,7 @@
 #endif
 
 #define fail(msg) { fprintf(stderr,msg); return NULL;}
+#define fail0(msg) { fprintf(stderr,msg); return 0;}
 
 /** The maximum length of an internally-allocated PIN */
 #define MAX_PIN_LENGTH   32
@@ -545,6 +546,63 @@ int load_cert_ctrl(ENGINE * e, void *p)
  return 1;
 }
 
+/*
+ * Log-into the token if necesary.
+ *
+ * @slot is PKCS11 slot to log in
+ * @tok is PKCS11 token to log in (??? could be derived as @slot->token)
+ * @ui_method is OpenSSL user inteface which is used to ask for a password
+ * @callback_data are application data to the user interface
+ * @return 1 on success, 0 on error.
+ */
+static int pkcs11_login(PKCS11_SLOT *slot, PKCS11_TOKEN *tok, UI_METHOD *ui_method, void *callback_data)
+{
+ /* Perform login to the token if required */
+ if (tok->loginRequired) {
+ /* If the token has a secure login (i.e., an external keypad),
+   then use a NULL pin. Otherwise, check if a PIN exists. If
+   not, allocate and obtain a new PIN. */
+ if (tok->secureLogin) {
+ /* Free the PIN if it has already been
+   assigned (i.e, cached by get_pin) */
+ free_pin();
+ } else if (pin == NULL) {
+ pin = (char *)calloc(MAX_PIN_LENGTH, sizeof(char));
+ pin_length = MAX_PIN_LENGTH;
+ if (pin == NULL) {
+ fail0("Could not allocate memory for PIN\n");
+ }
+ if (!get_pin(ui_method, callback_data) ) {
+ free_pin();
+ fail0("No pin code was entered\n");
+ }
+ }
+
+ /* Now login in with the (possibly NULL) pin */
+ if (PKCS11_login(slot, 0, pin)) {
+ /* Login failed, so free the PIN if present */
+ free_pin();
+ fail0("Login failed\n");
+ }
+ /* Login successful, PIN retained in case further logins are
+   required. This will occur on subsequent calls to the
+   pkcs11_load_key function. Subsequent login calls should be
+   relatively fast (the token should maintain its own login
+   state), although there may still be a slight performance
+   penalty. We could maintain state noting that successful
+   login has been performed, but this state may not be updated
+   if the token is removed and reinserted between calls. It
+   seems safer to retain the PIN and peform a login on each
+   call to pkcs11_load_key, even if this may not be strictly
+   necessary. */
+ /* TODO when does PIN get freed after successful login? */
+ /* TODO confirm that multiple login attempts do not introduce
+   significant performance penalties */
+
+ }
+ return 1;
+}
+
 static EVP_PKEY *pkcs11_load_key(ENGINE * e, const char *s_slot_key_id,
  UI_METHOD * ui_method, void *callback_data,
  int isPrivate)
@@ -691,47 +749,8 @@ static EVP_PKEY *pkcs11_load_key(ENGINE * e, const char *s_slot_key_id,
  }
 
  /* Perform login to the token if required */
- if (tok->loginRequired) {
- /* If the token has a secure login (i.e., an external keypad),
-   then use a NULL pin. Otherwise, check if a PIN exists. If
-   not, allocate and obtain a new PIN. */
- if (tok->secureLogin) {
- /* Free the PIN if it has already been
-   assigned (i.e, cached by get_pin) */
- free_pin();
- } else if (pin == NULL) {
- pin = (char *)calloc(MAX_PIN_LENGTH, sizeof(char));
- pin_length = MAX_PIN_LENGTH;
- if (pin == NULL) {
- fail("Could not allocate memory for PIN");
- }
- if (!get_pin(ui_method, callback_data) ) {
- free_pin();
- fail("No pin code was entered");
- }
- }
-
- /* Now login in with the (possibly NULL) pin */
- if (PKCS11_login(slot, 0, pin)) {
- /* Login failed, so free the PIN if present */
- free_pin();
- fail("Login failed\n");
- }
- /* Login successful, PIN retained in case further logins are
-   required. This will occur on subsequent calls to the
-   pkcs11_load_key function. Subsequent login calls should be
-   relatively fast (the token should maintain its own login
-   state), although there may still be a slight performance
-   penalty. We could maintain state noting that successful
-   login has been performed, but this state may not be updated
-   if the token is removed and reinserted between calls. It
-   seems safer to retain the PIN and peform a login on each
-   call to pkcs11_load_key, even if this may not be strictly
-   necessary. */
- /* TODO when does PIN get freed after successful login? */
- /* TODO confirm that multiple login attempts do not introduce
-   significant performance penalties */
-
+ if (!pkcs11_login(slot, tok, ui_method, callback_data)) {
+ return NULL;
  }
 
  /* Make sure there is at least one private key on the token */
--
1.8.1.5


------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 8/9] Implement ENGINE_load_ssl_client_cert()

Petr Pisar
In reply to this post by Petr Pisar
This feature allows to select approriate certificate and private key for SSL
client authentication. An application provides list of certificate
authorities known by the server and this code will return suitable certificate
and private key. If more certificates are suitable, user will be asked to
select one of them.
---
 src/engine_pkcs11.c | 256 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/engine_pkcs11.h |   4 +
 src/hw_pkcs11.c     |   3 +-
 3 files changed, 262 insertions(+), 1 deletion(-)

diff --git a/src/engine_pkcs11.c b/src/engine_pkcs11.c
index 1c1842d..992e43e 100644
--- a/src/engine_pkcs11.c
+++ b/src/engine_pkcs11.c
@@ -31,6 +31,7 @@
 #include <openssl/crypto.h>
 #include <openssl/objects.h>
 #include <openssl/engine.h>
+#include <openssl/x509v3.h>
 #include <libp11.h>
 #include "engine_pkcs11.h"
 
@@ -43,6 +44,7 @@
 
 /** The maximum length of an internally-allocated PIN */
 #define MAX_PIN_LENGTH   32
+#define MAX_MESSAGE_LENGTH 256
 
 static PKCS11_CTX *ctx;
 
@@ -827,3 +829,257 @@ EVP_PKEY *pkcs11_load_private_key(ENGINE * e, const char *s_key_id,
  fail("PKCS11_get_private_key returned NULL\n");
  return pk;
 }
+
+/*
+ * Return true if certificate issuer is listed in X509_NAME stack or if the
+ * stack is empty. Return false otherwise.
+ *
+ * @cert is a valid PKCS11 certificate
+ * @issuer_dns is a possibly empty stack of issuer names
+ */
+static int pkcs11_cert_issuer_matches(PKCS11_CERT *cert, STACK_OF(X509_NAME) *issuer_dns)
+{
+ int count;
+ int i;
+
+ if (NULL == issuer_dns)
+ return 1;
+ count = sk_X509_NAME_num(issuer_dns);
+ if (count <= 0)
+ return 1;
+ for (i = 0; i < count; i++) {
+ if (!X509_NAME_cmp(
+ sk_X509_NAME_value(issuer_dns, i),
+ X509_get_issuer_name(cert->x509)))
+ return 1;
+ }
+ return 0;
+}
+
+
+/*
+ * Check if certificate can be used by an SSL client.
+ *
+ * @cert is a valid PKCS11 certificate.
+ * @return 1 if the certificate purpose allowes that.
+ * @return 0 if the certificate purpose prohibits that.
+ * @return -1 in case of an error.
+ */
+static int pkcs11_cert_is_for_ssl_client(PKCS11_CERT *cert)
+{
+ /* XXX: We have to work on a temporary copy because
+ * X509_check_purpose() modified the X509. */
+ X509 *copy = X509_dup(cert->x509);
+ int suitable;
+
+ if (NULL == copy)
+ return -1;
+ suitable = X509_check_purpose(copy, X509_PURPOSE_SSL_CLIENT, 0);
+ X509_free(copy);
+
+ return suitable;
+}
+
+/*
+ * Ask user to select a certificate from array of certificates if more
+ * certificates are listed. Otherwise selects the one certificate without
+ * asking.
+ *
+ * @certs is array of pointers to a PKCS11 certificate
+ * @cerrs_count is number of pointers in the array
+ * @ui_method is user interface to use to ask an user
+ * @callback_data are application data for the user interface
+ * @return selected certificate or NULL in case of an empty list or an error.
+ */
+static PKCS11_CERT *pkcs11_select_certificate(PKCS11_CERT **certs,
+ int certs_count, UI_METHOD *ui_method, void *callback_data)
+{
+ UI *ui;
+ char message[MAX_MESSAGE_LENGTH];
+ int i;
+
+ /* No certificate list */
+ if (NULL == certs || 0 == certs_count)
+ return NULL;
+
+ /* Exactly one certificate */
+ if (1 == certs_count)
+ return certs[0];
+
+ /* More certificates, ask the user */
+ ui = UI_new();
+ if (ui == NULL) {
+ fail("UI_new failed\n");
+ }
+ if (ui_method != NULL)
+ UI_set_method(ui, ui_method);
+ if (callback_data != NULL)
+ UI_add_user_data(ui, callback_data);
+
+ UI_add_info_string(ui, "Available certificates:\n");
+ for (i = 0; i < certs_count; i++) {
+ char *dn = NULL;
+ if (certs[i]->x509)
+ dn = X509_NAME_oneline(X509_get_subject_name
+       (certs[i]->x509), NULL, 0);
+ snprintf(message, MAX_MESSAGE_LENGTH - 1, "%2u. %s (%s)\n",
+ i + 1, certs[i]->label, dn);
+ message[MAX_MESSAGE_LENGTH-1] = '\0';
+ UI_dup_info_string(ui, message);
+ if (dn) {
+ OPENSSL_free(dn);
+ }
+ }
+
+ message[0] = '\0';
+ if (!UI_add_input_string(ui, "Select certificate by number: ",
+    UI_INPUT_FLAG_ECHO, message, 0, MAX_MESSAGE_LENGTH-1)) {
+ UI_free(ui);
+ fail("UI_add_input_string failed\n");
+ }
+ if (UI_process(ui)) {
+ UI_free(ui);
+ fail("UI_process failed\n");
+ }
+ UI_free(ui);
+
+ /* Parse the response */
+ i = atoi(message);
+ if (i < 1 || i > certs_count) {
+ fail("Could not select a certificate because of wrong number\n");
+ }
+
+ return certs[i - 1];
+}
+
+/*
+ * This is ENGINE_SSL_CLIENT_CERT_PTR OpenSSL engine call-back used to select
+ * a certificate and a corresponding private key appropriate for SSL client.
+ *
+ * @engine is this engine context
+ * @ssl is current SSL connection in client authentication phase
+ * @ca_dn is stack of certificate authority distinguished names advertised by
+ * the SSL server. This list constrains certificate to return.
+ * @pcert is a memory to store pointer to selected X509 certificate. Only one
+ * certificate can be returned.
+ * @pkey us a memory to store pointer to selected private key
+ * @pother has unkown semantics. Not implemented.
+ * @ui_method is an user interface to select certificate by the user if there
+ * are more certificates available issued by one of @ca_dn.
+ * @return 1 in case of success, otherwise 0.
+ */
+int pkcs11_load_ssl_client_cert(ENGINE *e, SSL *ssl,
+ STACK_OF(X509_NAME) *ca_dn, X509 **pcert, EVP_PKEY **pkey,
+ STACK_OF(X509) **pother, UI_METHOD *ui_method, void *callback_data)
+{
+ PKCS11_SLOT *slot_list, *slot;
+ PKCS11_SLOT *found_slot = NULL;
+ PKCS11_TOKEN *tok;
+ PKCS11_CERT *certs, *selected_cert = NULL;
+ PKCS11_CERT **suitable_certs = NULL;
+ PKCS11_KEY *key;
+ X509 *x509;
+
+ unsigned int slot_count, cert_count, n, suitable_certs_count;
+
+ if (NULL != pcert)
+    *pcert = NULL;
+ if (NULL != pkey)
+    *pkey = NULL;
+ if (NULL != pother)
+    *pother = NULL;
+
+ if (NULL == e)
+    return 0;
+
+ /* Enumerate certificates */
+ if (PKCS11_enumerate_slots(ctx, &slot_list, &slot_count) < 0)
+ fail0("failed to enumerate slots\n");
+ if (!(slot = PKCS11_find_token(ctx, slot_list, slot_count))) {
+ PKCS11_release_all_slots(ctx, slot_list, slot_count);
+ fail0("didn't find any tokens\n");
+ }
+ tok = slot->token;
+ if (tok == NULL) {
+ PKCS11_release_all_slots(ctx, slot_list, slot_count);
+ fail0("Found empty token\n");
+ }
+
+ if (PKCS11_enumerate_certs(tok, &certs, &cert_count)) {
+ PKCS11_release_all_slots(ctx, slot_list, slot_count);
+ fail0("unable to enumerate certificates\n");
+ }
+
+ /* Select suitable certificates */
+ suitable_certs = malloc(cert_count * sizeof(*suitable_certs));
+ if (NULL == suitable_certs) {
+ PKCS11_release_all_slots(ctx, slot_list, slot_count);
+ fail0("not enough memory to select certificates\n");
+ }
+
+ for (n = 0, suitable_certs_count = 0; n < cert_count; n++) {
+ PKCS11_CERT *k = certs + n;
+ if (pkcs11_cert_issuer_matches(k, ca_dn)) {
+ int suitable = pkcs11_cert_is_for_ssl_client(k);
+ /* ???: Exclude expired certificates */
+ if (1 == suitable) {
+ /* Suitable certificate found */
+ suitable_certs[suitable_certs_count++] = k;
+ } else if (0 != suitable) {
+ free(suitable_certs);
+ PKCS11_release_all_slots(ctx, slot_list,
+    slot_count);
+ fail0("Error while checking a certificate is "
+ "allowed for an SSL client\n");
+ }
+ }
+ }
+
+ /* Let user to select if more certificates are suitable */
+ selected_cert = pkcs11_select_certificate(suitable_certs, suitable_certs_count,
+ ui_method, callback_data);
+ free(suitable_certs);
+
+ /* Store selected certificate */
+ if (NULL == selected_cert) {
+ PKCS11_release_all_slots(ctx, slot_list, slot_count);
+ fail0("No suitable certificate found\n");
+ }
+ if (NULL != pcert) {
+ *pcert = X509_dup(selected_cert->x509);
+ if (NULL == *pcert) {
+ PKCS11_release_all_slots(ctx, slot_list, slot_count);
+ fail0("could not copy selected certificate\n");
+ }
+ }
+
+ if (NULL == pkey) {
+    /* No private key requested by an application */
+    PKCS11_release_all_slots(ctx, slot_list, slot_count);
+    return (1);
+ }
+
+ /* Find a private key corresponding to the certificate */
+ if (!pkcs11_login(slot, tok, ui_method, callback_data)) {
+ PKCS11_release_all_slots(ctx, slot_list, slot_count);
+ if (NULL != *pcert) {
+ X509_free(*pcert);
+ *pcert = NULL;
+ }
+ fail0("could not log in to access private key corresponding to selected certificate\n");
+ }
+ key = PKCS11_find_key(selected_cert);
+ if (NULL == key) {
+ PKCS11_release_all_slots(ctx, slot_list, slot_count);
+ if (NULL != *pcert) {
+ X509_free(*pcert);
+ *pcert = NULL;
+ }
+ fail0("could not find private key corresponding to selected certificate\n");
+ }
+ /* ???: Duplicate EVP_PKEY as the X509 */
+ *pkey = PKCS11_get_private_key(key);
+
+ PKCS11_release_all_slots(ctx, slot_list, slot_count);
+ return (1);
+}
diff --git a/src/engine_pkcs11.h b/src/engine_pkcs11.h
index 2159330..3bc55fb 100644
--- a/src/engine_pkcs11.h
+++ b/src/engine_pkcs11.h
@@ -55,4 +55,8 @@ EVP_PKEY *pkcs11_load_public_key(ENGINE * e, const char *s_key_id,
 EVP_PKEY *pkcs11_load_private_key(ENGINE * e, const char *s_key_id,
   UI_METHOD * ui_method, void *callback_data);
 
+int pkcs11_load_ssl_client_cert(ENGINE *e, SSL *ssl,
+ STACK_OF(X509_NAME) *ca_dn, X509 **pcert, EVP_PKEY **pkey,
+ STACK_OF(X509) **pother, UI_METHOD *ui_method, void *callback_data);
+
 #endif
diff --git a/src/hw_pkcs11.c b/src/hw_pkcs11.c
index 24806ff..f6e45d5 100644
--- a/src/hw_pkcs11.c
+++ b/src/hw_pkcs11.c
@@ -193,7 +193,8 @@ static int bind_helper(ENGINE * e)
     !ENGINE_set_BN_mod_exp(e, BN_mod_exp) ||
 #endif
     !ENGINE_set_load_pubkey_function(e, pkcs11_load_public_key) ||
-    !ENGINE_set_load_privkey_function(e, pkcs11_load_private_key)) {
+    !ENGINE_set_load_privkey_function(e, pkcs11_load_private_key) ||
+    !ENGINE_set_load_ssl_client_cert_function(e, pkcs11_load_ssl_client_cert)) {
  return 0;
  } else {
  return 1;
--
1.8.1.5


------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 9/9] Add load_ssl_client_cert test

Petr Pisar
In reply to this post by Petr Pisar
The best way is to execute

cd test && OPENSSL_CONF=./openssl.cnf ./load_ssl_client_cert [CA_FILE...]

This allows you to test code working with issuing CA names too.
---
 Makefile.am                 |  2 +-
 configure.ac                |  1 +
 test/Makefile.am            | 15 +++++++
 test/load_ssl_client_cert.c | 98 +++++++++++++++++++++++++++++++++++++++++++++
 test/openssl.cnf            | 20 +++++++++
 5 files changed, 135 insertions(+), 1 deletion(-)
 create mode 100644 test/Makefile.am
 create mode 100644 test/load_ssl_client_cert.c
 create mode 100644 test/openssl.cnf

diff --git a/Makefile.am b/Makefile.am
index 6c3f91c..2a0b3b5 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -15,7 +15,7 @@ MAINTAINERCLEANFILES = \
  $(srcdir)/packaged
 EXTRA_DIST = svnignore
 
-SUBDIRS = src doc
+SUBDIRS = src doc test
 
 dist_noinst_SCRIPTS = bootstrap
 dist_doc_DATA = NEWS
diff --git a/configure.ac b/configure.ac
index 484f509..0bcc8ba 100644
--- a/configure.ac
+++ b/configure.ac
@@ -308,6 +308,7 @@ AC_CONFIG_FILES([
  doc/nonpersistent/Makefile
  src/Makefile
  src/versioninfo.rc
+    test/Makefile
 ])
 AC_OUTPUT
 
diff --git a/test/Makefile.am b/test/Makefile.am
new file mode 100644
index 0000000..d9679b2
--- /dev/null
+++ b/test/Makefile.am
@@ -0,0 +1,15 @@
+MAINTAINERCLEANFILES = \
+ Makefile.in
+
+OPENSSL_EXTRA_CFLAGS = \
+ -DOPENSSL_THREADS -D_REENTRANT -DDSO_DLFCN -DHAVE_DLFCN_H \
+ -DOPENSSL_NO_KRB5 -DL_ENDIAN -DTERMIO -DENGINE_DYNAMIC_SUPPORT \
+ -DSHA1_ASM -DMD5_ASM -DRMD160_ASM
+AM_CFLAGS = $(OPENSSL_EXTRA_CFLAGS) $(OPENSSL_CFLAGS)
+LDADD = $(OPENSSL_LIBS)
+AM_LDFLAGS = $(OPENSSL_EXTRA_LDFLAGS)
+
+noinst_PROGRAMS = load_ssl_client_cert
+
+load_ssl_client_cert_SOURCES = load_ssl_client_cert.c
+
diff --git a/test/load_ssl_client_cert.c b/test/load_ssl_client_cert.c
new file mode 100644
index 0000000..21dfe2f
--- /dev/null
+++ b/test/load_ssl_client_cert.c
@@ -0,0 +1,98 @@
+#include <stdio.h>
+#include <unistd.h>
+#include <openssl/conf.h>
+#include <openssl/engine.h>
+#include <openssl/pem.h>
+
+STACK_OF(X509_NAME) *load_ca_dns(int argc, char **argv) {
+    STACK_OF(X509_NAME) *ca_dns = NULL;
+    BIO *in;
+    X509 *cert;
+    X509_NAME *name;
+    int i;
+    
+    for (i = 1; i < argc; i++) {
+        in = BIO_new_file(argv[i], "r");
+        if (NULL == in) {
+            fprintf(stderr, "Could not read %s\n", argv[i]);
+            continue;
+        }
+        cert = PEM_read_bio_X509(in, NULL, 0, NULL);
+        BIO_free(in);
+        if (NULL == cert) {
+            fprintf(stderr, "Could not read %s\n", argv[i]);
+            continue;
+        }
+        name = X509_NAME_dup(X509_get_subject_name(cert));
+        X509_free(cert);
+        if (NULL == name) {
+            fprintf(stderr, "Could not get issuer from %s\n", argv[i]);
+            X509_free(cert);
+            continue;
+        }
+        if (NULL == ca_dns)
+            ca_dns = sk_X509_NAME_new_null();
+        sk_X509_NAME_push(ca_dns, name);
+    }
+    return ca_dns;
+}
+
+
+int main(int argc, char ** argv) {
+    ENGINE *e;
+    const char *engine_id = "pkcs11";
+    STACK_OF(X509_NAME) *ca_dns;
+    X509 *cert = NULL;
+    EVP_PKEY *pkey = NULL;
+    int retval;
+
+    printf("Testing %s\n", argv[0]);
+
+    ENGINE_load_builtin_engines();
+    OPENSSL_load_builtin_modules();
+    if (CONF_modules_load_file(getenv("OPENSSL_CONF"), NULL, 0) <= 0) {
+        fprintf(stderr, "Could not load modules defined in the "
+                "configuration file\n");
+        exit(EXIT_FAILURE);
+    }
+
+    e = ENGINE_by_id(engine_id);
+    if(!e) {
+        fprintf(stderr, "The engine isn't available\n");
+        exit(EXIT_FAILURE);
+    }
+    if(!ENGINE_init(e)) {
+        fprintf(stderr, "The engine couldn't ne initilized\n");
+        ENGINE_free(e);
+        exit(EXIT_FAILURE);
+    }
+
+    ca_dns = load_ca_dns(argc, argv);
+    retval = ENGINE_load_ssl_client_cert(e, NULL, ca_dns, &cert, &pkey, NULL, NULL, NULL);
+    sk_X509_NAME_free(ca_dns);
+
+    if (!retval) {
+        fprintf(stderr, "ENGINE_load_ssl_client_cert() failed\n");
+        ENGINE_finish(e);
+        ENGINE_free(e);
+        exit(EXIT_FAILURE);
+    }
+    if (NULL != cert) {
+        printf("A certificate returned:\n");
+        X509_print_fp(stdout, cert);
+        X509_free(cert);
+    } else {
+        printf("No certificate returned\n");
+    }
+    if (NULL != pkey) {
+        printf("A private key returned\n");
+        /*EVP_PKEY_free(pkey);*/
+    } else {
+        printf("No private key returned\n");
+    }
+
+    ENGINE_finish(e);
+    ENGINE_free(e);
+    printf("Ok.\n");
+    exit(EXIT_SUCCESS);
+}
diff --git a/test/openssl.cnf b/test/openssl.cnf
new file mode 100644
index 0000000..c4fd1e4
--- /dev/null
+++ b/test/openssl.cnf
@@ -0,0 +1,20 @@
+#HOME = .
+RANDFILE = $ENV::HOME/.rnd
+
+openssl_conf = openssl_def
+
+[openssl_def]
+engines = engine_section
+
+[engine_section]
+pkcs11 = pkcs11_engine
+
+[pkcs11_engine]
+engine_id = pkcs11
+#dynamic_path = /usr/lib/engines/engine_pkcs11.so
+dynamic_path = $ENV::HOME/engine_pkcs11/src/.libs/engine_pkcs11.so
+MODULE_PATH = /usr/lib/opensc-pkcs11.so
+#PIN = Bar
+#VERBOSE = 1
+init = 0
+
--
1.8.1.5


------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 7/9] Decouple loging into the token

NdK-3
In reply to this post by Petr Pisar
Il 30/08/2013 16:45, Petr Písař ha scritto:

> + /* Login successful, PIN retained in case further logins are
> +   required. This will occur on subsequent calls to the
> +   pkcs11_load_key function. Subsequent login calls should be
> +   relatively fast (the token should maintain its own login
> +   state), although there may still be a slight performance
> +   penalty. We could maintain state noting that successful
> +   login has been performed, but this state may not be updated
> +   if the token is removed and reinserted between calls. It
> +   seems safer to retain the PIN and peform a login on each
> +   call to pkcs11_load_key, even if this may not be strictly
> +   necessary. */
> + /* TODO when does PIN get freed after successful login? */
> + /* TODO confirm that multiple login attempts do not introduce
> +   significant performance penalties */
TODO: have "some" method to completely disable caching (think "token
with HOTP login)...

BYtE,
 Diego.

------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 7/9] Decouple loging into the token

Petr Pisar
On Sat, Aug 31, 2013 at 08:24:36AM +0200, NdK wrote:

> Il 30/08/2013 16:45, Petr Písař ha scritto:
>
> > + /* Login successful, PIN retained in case further logins are
> > +   required. This will occur on subsequent calls to the
> > +   pkcs11_load_key function. Subsequent login calls should be
> > +   relatively fast (the token should maintain its own login
> > +   state), although there may still be a slight performance
> > +   penalty. We could maintain state noting that successful
> > +   login has been performed, but this state may not be updated
> > +   if the token is removed and reinserted between calls. It
> > +   seems safer to retain the PIN and peform a login on each
> > +   call to pkcs11_load_key, even if this may not be strictly
> > +   necessary. */
> > + /* TODO when does PIN get freed after successful login? */
> > + /* TODO confirm that multiple login attempts do not introduce
> > +   significant performance penalties */
> TODO: have "some" method to completely disable caching (think "token
> with HOTP login)...
>
I have just moved the code including the comments into a separate function.

Actually the PIN caching is weired. E.g. there is a non-standard PIN CTRL
command that pre-sets the PIN. I don't like it and I don't think controlling
the caching with yet another private CTRL is good.

One could augment the dialogue in the get_pin() function to ask user if he
wants to cache the PIN. But what about non-interactive log-in?

Or one could cache the context with loged-in slot instead of caching mere
password. This would work if each process used it's own OpenSSL context.
But then how to deal with some `authentication managers' that centrilize the
functionality into one daemon instance that proxies application requests. Then
you could not distinguish when a token is accessed on behalf of different
application.

Actually I know almost nothing about p11 or the PKCS11 design regarding
sharing log-in state. I worry one behaviour cannot fit to all.

-- Petr

------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel

attachment0 (237 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [engine_pkcs11] Various fixes and features

Anthony Foiani
In reply to this post by Petr Pisar
Petr --

On Fri, Aug 30, 2013 at 8:45 AM, Petr Písař <[hidden email]> wrote:
> Hello,
>
> while testing TLS client authentication using a cryprographical token in my
> project (libisds over cURL over OpenSSL with Athena USB token under OpenSC),
> I found a lot of bugs in the engine_pkcs11 plug-in for OpenSSL.

Indeed.  :(

> Some of the bugs are so serious that they prevent from using the token through
> OpenSSL and can lead even to a segmentation fault. So I deciced to fix them
> and post the pathes here in hope the engine_pkcs11 maintainer will review them
> and merge them.

If you're already using this set of patches in production, you might
also be interested in my patchset that fixes a severe memory leak with
the use of on-token private keys through the engine interface:

https://github.com/OpenSC/engine_pkcs11/pull/3

It requires a small change in API -- but without it, I was losing
megabytes of memory per hour.

Thanks for your code -- I hope it gets integrated soon!

Best regards,
Tony

------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 7/9] Decouple loging into the token

Petr Pisar
In reply to this post by Petr Pisar
On Sat, Aug 31, 2013 at 09:05:05AM +0200, Petr Pisar wrote:
> On Sat, Aug 31, 2013 at 08:24:36AM +0200, NdK wrote:
> > TODO: have "some" method to completely disable caching (think "token
> > with HOTP login)...
> >
[...]
>
> Actually the PIN caching is weired. E.g. there is a non-standard PIN CTRL
> command that pre-sets the PIN. I don't like it and I don't think controlling
> the caching with yet another private CTRL is good.
>
And last but not least, you can have more tokens pluged in each with different
PIN. Then the caching is obviously wrong because it caches one PIN for all of
them.

I think the caching should be disabled in the engine_pkcs11 by default.
Application can cache the password instead and fill it automatically through
the OpenSSL user interface.

-- Petr

------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel

attachment0 (237 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [engine_pkcs11] Various fixes and features

Douglas E. Engert
In reply to this post by Petr Pisar
Looks like it is time for some overhaul of engine_pkcs11...

You have some good patches here.

The OpenSC code including the engine_pkcs11 is maintained
at https://github.com/OpenSC

The best way to get these patches added would be to fork the
  https://github.com/OpenSC/engine_pkcs11
add your patches to your fork, and submit pull requests.

Anthony Foiani also has forked engine_pkcs11 and his changes
also look good too, so you may want to build with his mods
as well.

I also have mods to engine_pkcs11 for doing ECDSA, which
has been on the back burner since 2011. I have recently
been helping Sanaullah to get these working with softhsm.

(I have been waiting for OpenSSL bug report #2459 02/23/2011
to be addressed. I might have a way to get around this,
or at least get the OpenSSL's attention.)

I am getting ready to submit these engine_patches,
and would like use both yours and Anthony's patches.



On 8/30/2013 9:45 AM, Petr Písař wrote:

> Hello,
>
> while testing TLS client authentication using a cryprographical token in my
> project (libisds over cURL over OpenSSL with Athena USB token under OpenSC),
> I found a lot of bugs in the engine_pkcs11 plug-in for OpenSSL.
>
> Some of the bugs are so serious that they prevent from using the token through
> OpenSSL and can lead even to a segmentation fault. So I deciced to fix them
> and post the pathes here in hope the engine_pkcs11 maintainer will review them
> and merge them.
>
> Here is a short description, patches will be sent as replies:
>
> [PATCH 1/9] Unify PIN freeing
> [PATCH 2/9] Free PIN storage where needed
>
> These two patches fix memory leaks when storing a PIN code.
>
> [PATCH 3/9] Use user interface correctly
>
> This fixes a crash (segmenation fault) when loading a private key. Current
> code could never use a PIN passed from OpenSSL because of wrong usage of the
> user interface call-back data. I send a fix to cURL library
> <http://thread.gmane.org/gmane.comp.web.curl.library/40222> too and I tested
> the colaboration between cURL and engine_pkcs11 successfully.
>
> [PATCH 4/9] Hexadecimal ID string contains colons
>
> A certificate/key object hexadecimal ID is printed with colons (ab:cd:..)
> everywhere. Let's allow engine_pkcs11 to recognize it. Contrary current parser
> expects the colons by can not recognize such string as an ID. I believe it
> was not possible to use the hexadecimal ID before.
>
> [PATCH 5/9] Find token if no slot was specified
>
> Identifier wihout a slot number (e.g. a plain ID) always resulted to slot
> number 0. This searches all slots now.
>
> [PATCH 6/9] Search for a certificate by a label
>
> Searching a certificate by a label did not work and worked differently than
> searching a key. This caused a lot of confusion why OpenSSL can locate the key
> but it cannot locate the certificate.
>
> [PATCH 7/9] Decouple loging into the token
> [PATCH 8/9] Implement ENGINE_load_ssl_client_cert()
> [PATCH 9/9] Add load_ssl_client_cert test
>
> These tree patches implement ENGINE_load_ssl_client_cert() interface which
> allows automatic negotion of client certificate in TLS authenticatinon. The
> ninth patch provides a test.
>
> -- Petr
>
> ------------------------------------------------------------------------------
> Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
> Discover the easy way to master current and previous Microsoft technologies
> and advance your career. Get an incredible 1,500+ hours of step-by-step
> tutorial videos with LearnDevNow. Subscribe today and save!
> http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
> _______________________________________________
> Opensc-devel mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/opensc-devel
>

--

  Douglas E. Engert  <[hidden email]>
  Argonne National Laboratory
  9700 South Cass Avenue
  Argonne, Illinois  60439
  (630) 252-5444

------------------------------------------------------------------------------
How ServiceNow helps IT people transform IT departments:
1. Consolidate legacy IT systems to a single system of record for IT
2. Standardize and globalize service processes across IT
3. Implement zero-touch automation to replace manual, redundant tasks
http://pubads.g.doubleclick.net/gampad/clk?id=51271111&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: [engine_pkcs11] Various fixes and features

Anthony Foiani
Douglas --

On Tue, Sep 10, 2013 at 9:28 AM, Douglas E. Engert <[hidden email]> wrote:
> Looks like it is time for some overhaul of engine_pkcs11...

It'll be nice to see it get some love.

> I also have mods to engine_pkcs11 for doing ECDSA, which
> has been on the back burner since 2011. I have recently
> been helping Sanaullah to get these working with softhsm.

If the project I'm on gets an opportunity to do a version 1.1, we will
definitely want to have the option to use ECDSA, so that would be
great.

> (I have been waiting for OpenSSL bug report #2459 02/23/2011
> to be addressed. I might have a way to get around this,
> or at least get the OpenSSL's attention.)

Yeah, I'm a little mystified that openssl just ignores patches sent to
their list.  :(

> I am getting ready to submit these engine_patches,
> and would like use both yours and Anthony's patches.

You are of course welcome to take those patches at any time.  I don't
have a lot of time, but if there are particular deficiencies, I can
try to address them.

Thanks for your efforts!

Best regards,
Anthony Foiani

------------------------------------------------------------------------------
How ServiceNow helps IT people transform IT departments:
1. Consolidate legacy IT systems to a single system of record for IT
2. Standardize and globalize service processes across IT
3. Implement zero-touch automation to replace manual, redundant tasks
http://pubads.g.doubleclick.net/gampad/clk?id=51271111&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: [engine_pkcs11] Various fixes and features

Douglas E. Engert


On 9/10/2013 10:37 AM, Anthony Foiani wrote:

> Douglas --
>
> On Tue, Sep 10, 2013 at 9:28 AM, Douglas E. Engert <[hidden email]> wrote:
>> Looks like it is time for some overhaul of engine_pkcs11...
>
> It'll be nice to see it get some love.
>
>> I also have mods to engine_pkcs11 for doing ECDSA, which
>> has been on the back burner since 2011. I have recently
>> been helping Sanaullah to get these working with softhsm.
>
> If the project I'm on gets an opportunity to do a version 1.1, we will
> definitely want to have the option to use ECDSA, so that would be
> great.
>
>> (I have been waiting for OpenSSL bug report #2459 02/23/2011
>> to be addressed. I might have a way to get around this,
>> or at least get the OpenSSL's attention.)
>
> Yeah, I'm a little mystified that openssl just ignores patches sent to
> their list.  :(

#2459 does not have a patch. But since the structure that needs to be
exposed has not changed in 2 years, I will be submitting a patch.

>
>> I am getting ready to submit these engine_patches,
>> and would like use both yours and Anthony's patches.
>
> You are of course welcome to take those patches at any time.  I don't
> have a lot of time, but if there are particular deficiencies, I can
> try to address them.
>
> Thanks for your efforts!
>
> Best regards,
> Anthony Foiani
>

--

  Douglas E. Engert  <[hidden email]>
  Argonne National Laboratory
  9700 South Cass Avenue
  Argonne, Illinois  60439
  (630) 252-5444

------------------------------------------------------------------------------
How ServiceNow helps IT people transform IT departments:
1. Consolidate legacy IT systems to a single system of record for IT
2. Standardize and globalize service processes across IT
3. Implement zero-touch automation to replace manual, redundant tasks
http://pubads.g.doubleclick.net/gampad/clk?id=51271111&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: [engine_pkcs11] Various fixes and features

Anthony Foiani
Douglas --

On Tue, Sep 10, 2013 at 9:39 AM, Douglas E. Engert <[hidden email]> wrote:
>
> On 9/10/2013 10:37 AM, Anthony Foiani wrote:
>
>> Yeah, I'm a little mystified that openssl just ignores patches sent to
>> their list.  :(
>
> #2459 does not have a patch. But since the structure that needs to be
> exposed has not changed in 2 years, I will be submitting a patch.

Ah, I was actually referring to a different issue, where I sent them a
one-line, obviously-correct patch:

http://permalink.gmane.org/gmane.comp.encryption.openssl.devel/23019

Crickets.  I just assumed that's how they responded in general.  :(

Thanks again for your work on this topic -- it's very welcome!

Best,
Anthony Foiani

------------------------------------------------------------------------------
How ServiceNow helps IT people transform IT departments:
1. Consolidate legacy IT systems to a single system of record for IT
2. Standardize and globalize service processes across IT
3. Implement zero-touch automation to replace manual, redundant tasks
http://pubads.g.doubleclick.net/gampad/clk?id=51271111&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: [engine_pkcs11] Various fixes and features

Petr Pisar
In reply to this post by Douglas E. Engert
On Tue, Sep 10, 2013 at 10:28:55AM -0500, Douglas E. Engert wrote:

> Looks like it is time for some overhaul of engine_pkcs11...
>
> You have some good patches here.
>
> The OpenSC code including the engine_pkcs11 is maintained
> at https://github.com/OpenSC
>
> The best way to get these patches added would be to fork the
>   https://github.com/OpenSC/engine_pkcs11
> add your patches to your fork, and submit pull requests.
>
Done <https://github.com/OpenSC/engine_pkcs11/pull/5>.

-- Petr

------------------------------------------------------------------------------
LIMITED TIME SALE - Full Year of Microsoft Training For Just $49.99!
1,500+ hours of tutorials including VisualStudio 2012, Windows 8, SharePoint
2013, SQL 2012, MVC 4, more. BEST VALUE: New Multi-Library Power Pack includes
Mobile, Cloud, Java, and UX Design. Lowest price ever! Ends 9/22/13.
http://pubads.g.doubleclick.net/gampad/clk?id=64545871&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel

attachment0 (237 bytes) Download Attachment