Xavier Hsinyuan
2024-Dec-21 18:15 UTC
[PATCH 0/2] Fix Memory Management Issue in `ssh-sk-helper` with External SK Libraries
Hi,
Sometimes, users might find that the `ssh-sk-helper` crashes after
enrolling a new key when using external SK libraries.
Currently, the memory returned by SK APIs is freed by the host, but
external libraries may have their own methods of handling memory. For
instance some external libraries are linked against a foreign libc
statically. As a result, the `ssh-sk-helper` would have issues if it
tries to free memory allocated by external libraries.
To resolve this problem, here are two patches:
1 - A patch introduces new APIs to free memory allocated by
external libraries, helping to prevent unexpected memory-related
behavior when using external SK libraries.
2 - A patch adopts new APIs for bundled `sk-usbhid` and `sk-dummy`.
Thank you for your time and feedback.
Best regards,
Xavier Hsinyuan
------
Xavier Hsinyuan (2):
Introduce new SK APIs for freeing memory
Adopt new SecurityKey API for sk-usbhid and sk-dummy
regress/misc/sk-dummy/sk-dummy.c | 35 +++++++++++++-
sk-api.h | 11 ++++-
sk-usbhid.c | 55 +++++++++++++++++++++
ssh-sk.c | 83 ++++++++++++++------------------
4 files changed, 136 insertions(+), 48 deletions(-)
--
2.39.5
Xavier Hsinyuan
2024-Dec-21 18:15 UTC
[PATCH 1/2] Introduce new SK APIs for freeing memory
External SecurityKey middleware libraries may use different memory
allocation methods for SK API responses. `ssh-sk-helper` may exhibit
unexpected behavior if it frees memory allocated by external SK
libraries.
Fixed by introducing new SK APIs for freeing memory allocated by and
returned from external SK libraries.
---
sk-api.h | 11 ++++++++++-
ssh-sk.c | 34 +++++++++++++++++++++++++++++++---
2 files changed, 41 insertions(+), 4 deletions(-)
diff --git a/sk-api.h b/sk-api.h
index 08f567a9e..b61f7235c 100644
--- a/sk-api.h
+++ b/sk-api.h
@@ -79,7 +79,7 @@ struct sk_option {
uint8_t required;
};
-#define SSH_SK_VERSION_MAJOR 0x000a0000 /* current API version */
+#define SSH_SK_VERSION_MAJOR 0x000b0000 /* current API version */
#define SSH_SK_VERSION_MAJOR_MASK 0xffff0000
/* Return the version of the middleware API */
@@ -100,4 +100,13 @@ int sk_sign(uint32_t alg, const uint8_t *data, size_t
data_len,
int sk_load_resident_keys(const char *pin, struct sk_option **options,
struct sk_resident_key ***rks, size_t *nrks);
+/* Free sk_sign_response allocated by provider */
+void sk_free_enroll_response(struct sk_enroll_response *enroll_resp);
+
+/* Free sk_sign_response allocated by provider */
+void sk_free_sign_response(struct sk_sign_response *sign_resp);
+
+/* Free sk_resident_key allocated by provider */
+void sk_free_sk_resident_keys(struct sk_resident_key **rks, size_t nrks);
+
#endif /* _SK_API_H */
diff --git a/ssh-sk.c b/ssh-sk.c
index a2a7d7206..19ac9dda8 100644
--- a/ssh-sk.c
+++ b/ssh-sk.c
@@ -78,6 +78,15 @@ struct sshsk_provider {
/* Enumerate resident keys */
int (*sk_load_resident_keys)(const char *pin, struct sk_option **opts,
struct sk_resident_key ***rks, size_t *nrks);
+
+ /* Free sk_sign_response allocated by provider */
+ void (*sk_free_enroll_response)(struct sk_enroll_response *ptr);
+
+ /* Free sk_sign_response allocated by provider */
+ void (*sk_free_sign_response)(struct sk_sign_response *ptr);
+
+ /* Free sk_resident_key allocated by provider */
+ void (*sk_free_sk_resident_keys)(struct sk_resident_key **rks, size_t nrks);
};
/* Built-in version */
@@ -171,6 +180,25 @@ sshsk_open(const char *path)
"failed: %s", path, dlerror());
goto fail;
}
+
+ if ((ret->sk_free_enroll_response = dlsym(ret->dlhandle,
+ "sk_free_enroll_response")) == NULL) {
+ error("Provider \"%s\" dlsym(sk_free_enroll_response) "
+ "failed: %s", path, dlerror());
+ goto fail;
+ }
+ if ((ret->sk_free_sign_response = dlsym(ret->dlhandle,
+ "sk_free_sign_response")) == NULL) {
+ error("Provider \"%s\" dlsym(sk_free_sign_response) "
+ "failed: %s", path, dlerror());
+ goto fail;
+ }
+ if ((ret->sk_free_sk_resident_keys = dlsym(ret->dlhandle,
+ "sk_free_sk_resident_keys")) == NULL) {
+ error("Provider \"%s\" dlsym(sk_free_sk_resident_keys) "
+ "failed: %s", path, dlerror());
+ goto fail;
+ }
/* success */
return ret;
fail:
@@ -568,9 +596,9 @@ sshsk_enroll(int type, const char *provider_path, const char
*device,
r = 0;
out:
sshsk_free_options(opts);
+ skp->sk_free_enroll_response(resp);
sshsk_free(skp);
sshkey_free(key);
- sshsk_free_enroll_response(resp);
explicit_bzero(randchall, sizeof(randchall));
return r;
}
@@ -746,8 +774,8 @@ sshsk_sign(const char *provider_path, struct sshkey *key,
r = 0;
out:
sshsk_free_options(opts);
+ skp->sk_free_sign_response(resp);
sshsk_free(skp);
- sshsk_free_sign_response(resp);
sshbuf_free(sig);
sshbuf_free(inner_sig);
return r;
@@ -883,8 +911,8 @@ sshsk_load_resident(const char *provider_path, const char
*device,
r = 0;
out:
sshsk_free_options(opts);
+ skp->sk_free_sk_resident_keys(rks, nrks);
sshsk_free(skp);
- sshsk_free_sk_resident_keys(rks, nrks);
sshkey_free(key);
sshsk_free_resident_key(srk);
sshsk_free_resident_keys(srks, nsrks);
--
2.39.5
Xavier Hsinyuan
2024-Dec-21 18:15 UTC
[PATCH 2/2] Adopt new SecurityKey API for sk-usbhid and sk-dummy
---
regress/misc/sk-dummy/sk-dummy.c | 35 +++++++++++++++++++-
sk-usbhid.c | 55 ++++++++++++++++++++++++++++++++
ssh-sk.c | 49 ++++------------------------
3 files changed, 95 insertions(+), 44 deletions(-)
diff --git a/regress/misc/sk-dummy/sk-dummy.c b/regress/misc/sk-dummy/sk-dummy.c
index 347b21227..de96e7ef5 100644
--- a/regress/misc/sk-dummy/sk-dummy.c
+++ b/regress/misc/sk-dummy/sk-dummy.c
@@ -50,7 +50,7 @@
/* #define SK_DEBUG 1 */
-#if SSH_SK_VERSION_MAJOR != 0x000a0000
+#if SSH_SK_VERSION_MAJOR != 0x000b0000
# error SK API has changed, sk-dummy.c needs an update
#endif
@@ -59,6 +59,9 @@
# define sk_enroll ssh_sk_enroll
# define sk_sign ssh_sk_sign
# define sk_load_resident_keys ssh_sk_load_resident_keys
+# define sk_free_enroll_response ssh_sk_free_enroll_response
+# define sk_free_sign_response ssh_sk_free_sign_response
+# define sk_free_resident_keys ssh_sk_free_resident_keys
#endif /* !SK_STANDALONE */
static void skdebug(const char *func, const char *fmt, ...)
@@ -541,3 +544,33 @@ sk_load_resident_keys(const char *pin, struct sk_option
**options,
{
return SSH_SK_ERR_UNSUPPORTED;
}
+
+void
+sk_free_enroll_response(struct sk_enroll_response *enroll_resp)
+{
+ if (enroll_resp == NULL)
+ return;
+ freezero(enroll_resp->key_handle, enroll_resp->key_handle_len);
+ freezero(enroll_resp->public_key, enroll_resp->public_key_len);
+ freezero(enroll_resp->signature, enroll_resp->signature_len);
+ freezero(enroll_resp->attestation_cert,
enroll_resp->attestation_cert_len);
+ freezero(enroll_resp->authdata, enroll_resp->authdata_len);
+ freezero(enroll_resp, sizeof(*enroll_resp));
+}
+
+void
+sk_free_sign_response(struct sk_sign_response *sign_resp)
+{
+ if (sign_resp == NULL)
+ return;
+ freezero(sign_resp->sig_r, sign_resp->sig_r_len);
+ freezero(sign_resp->sig_s, sign_resp->sig_s_len);
+ freezero(sign_resp, sizeof(*sign_resp));
+}
+
+/* sk_load_resident_keys returns SSH_SK_ERR_UNSUPPORTED */
+void
+sk_free_sk_resident_keys(struct sk_resident_key **rks, size_t nrks)
+{
+ return;
+}
diff --git a/sk-usbhid.c b/sk-usbhid.c
index 427431b9a..01c68c842 100644
--- a/sk-usbhid.c
+++ b/sk-usbhid.c
@@ -90,6 +90,9 @@
# define sk_enroll ssh_sk_enroll
# define sk_sign ssh_sk_sign
# define sk_load_resident_keys ssh_sk_load_resident_keys
+# define sk_free_enroll_response ssh_sk_free_enroll_response
+# define sk_free_sign_response ssh_sk_free_sign_response
+# define sk_free_sk_resident_keys ssh_sk_free_sk_resident_keys
#endif /* !SK_STANDALONE */
#include "sk-api.h"
@@ -134,6 +137,15 @@ int sk_sign(uint32_t alg, const uint8_t *data, size_t
data_len,
int sk_load_resident_keys(const char *pin, struct sk_option **options,
struct sk_resident_key ***rks, size_t *nrks);
+/* Free sk_sign_response allocated by provider */
+void sk_free_enroll_response(struct sk_enroll_response *enroll_resp);
+
+/* Free sk_sign_response allocated by provider */
+void sk_free_sign_response(struct sk_sign_response *sign_resp);
+
+/* Free sk_resident_key allocated by provider */
+void sk_free_sk_resident_keys(struct sk_resident_key **rks, size_t nrks);
+
static void skdebug(const char *func, const char *fmt, ...)
__attribute__((__format__ (printf, 2, 3)));
@@ -1479,4 +1491,47 @@ sk_load_resident_keys(const char *pin, struct sk_option
**options,
return ret;
}
+void
+sk_free_enroll_response(struct sk_enroll_response *enroll_resp)
+{
+ if (enroll_resp == NULL)
+ return;
+ freezero(enroll_resp->key_handle, enroll_resp->key_handle_len);
+ freezero(enroll_resp->public_key, enroll_resp->public_key_len);
+ freezero(enroll_resp->signature, enroll_resp->signature_len);
+ freezero(enroll_resp->attestation_cert,
enroll_resp->attestation_cert_len);
+ freezero(enroll_resp->authdata, enroll_resp->authdata_len);
+ freezero(enroll_resp, sizeof(*enroll_resp));
+}
+
+void
+sk_free_sign_response(struct sk_sign_response *sign_resp)
+{
+ if (sign_resp == NULL)
+ return;
+ freezero(sign_resp->sig_r, sign_resp->sig_r_len);
+ freezero(sign_resp->sig_s, sign_resp->sig_s_len);
+ freezero(sign_resp, sizeof(*sign_resp));
+}
+
+void
+sk_free_sk_resident_keys(struct sk_resident_key **rks, size_t nrks)
+{
+ size_t i;
+
+ if (nrks == 0 || rks == NULL)
+ return;
+ for (i = 0; i < nrks; i++) {
+ free(rks[i]->application);
+ freezero(rks[i]->user_id, rks[i]->user_id_len);
+ freezero(rks[i]->key.key_handle, rks[i]->key.key_handle_len);
+ freezero(rks[i]->key.public_key, rks[i]->key.public_key_len);
+ freezero(rks[i]->key.signature, rks[i]->key.signature_len);
+ freezero(rks[i]->key.attestation_cert,
+ rks[i]->key.attestation_cert_len);
+ freezero(rks[i], sizeof(**rks));
+ }
+ free(rks);
+}
+
#endif /* ENABLE_SK_INTERNAL */
diff --git a/ssh-sk.c b/ssh-sk.c
index 19ac9dda8..9cc5bd4c1 100644
--- a/ssh-sk.c
+++ b/ssh-sk.c
@@ -101,6 +101,9 @@ int ssh_sk_sign(int alg, const uint8_t *message, size_t
message_len,
struct sk_sign_response **sign_response);
int ssh_sk_load_resident_keys(const char *pin, struct sk_option **opts,
struct sk_resident_key ***rks, size_t *nrks);
+void ssh_sk_free_enroll_response(struct sk_enroll_response *enroll_resp);
+void ssh_sk_free_sign_response(struct sk_sign_response *enroll_resp);
+void ssh_sk_free_sk_resident_keys(struct sk_resident_key **rks, size_t nrks);
static void
sshsk_free(struct sshsk_provider *p)
@@ -137,6 +140,9 @@ sshsk_open(const char *path)
ret->sk_enroll = ssh_sk_enroll;
ret->sk_sign = ssh_sk_sign;
ret->sk_load_resident_keys = ssh_sk_load_resident_keys;
+ ret->sk_free_enroll_response = ssh_sk_free_enroll_response;
+ ret->sk_free_sign_response = ssh_sk_free_sign_response;
+ ret->sk_free_sk_resident_keys = ssh_sk_free_sk_resident_keys;
return ret;
#else
error("internal security key support not enabled");
@@ -206,29 +212,6 @@ fail:
return NULL;
}
-static void
-sshsk_free_enroll_response(struct sk_enroll_response *r)
-{
- if (r == NULL)
- return;
- freezero(r->key_handle, r->key_handle_len);
- freezero(r->public_key, r->public_key_len);
- freezero(r->signature, r->signature_len);
- freezero(r->attestation_cert, r->attestation_cert_len);
- freezero(r->authdata, r->authdata_len);
- freezero(r, sizeof(*r));
-}
-
-static void
-sshsk_free_sign_response(struct sk_sign_response *r)
-{
- if (r == NULL)
- return;
- freezero(r->sig_r, r->sig_r_len);
- freezero(r->sig_s, r->sig_s_len);
- freezero(r, sizeof(*r));
-}
-
#ifdef WITH_OPENSSL
/* Assemble key from response */
static int
@@ -781,26 +764,6 @@ sshsk_sign(const char *provider_path, struct sshkey *key,
return r;
}
-static void
-sshsk_free_sk_resident_keys(struct sk_resident_key **rks, size_t nrks)
-{
- size_t i;
-
- if (nrks == 0 || rks == NULL)
- return;
- for (i = 0; i < nrks; i++) {
- free(rks[i]->application);
- freezero(rks[i]->user_id, rks[i]->user_id_len);
- freezero(rks[i]->key.key_handle, rks[i]->key.key_handle_len);
- freezero(rks[i]->key.public_key, rks[i]->key.public_key_len);
- freezero(rks[i]->key.signature, rks[i]->key.signature_len);
- freezero(rks[i]->key.attestation_cert,
- rks[i]->key.attestation_cert_len);
- freezero(rks[i], sizeof(**rks));
- }
- free(rks);
-}
-
static void
sshsk_free_resident_key(struct sshsk_resident_key *srk)
{
--
2.39.5