From f8f5a6b003981bb824329dc987d101977beda7ca Mon Sep 17 00:00:00 2001 From: djm Date: Wed, 19 Jul 2023 14:02:27 +0000 Subject: [PATCH] Ensure FIDO/PKCS11 libraries contain expected symbols This checks via nlist(3) that candidate provider libraries contain one of the symbols that we will require prior to dlopen(), which can cause a number of side effects, including execution of constructors. Feedback deraadt; ok markus --- usr.bin/ssh/misc.c | 31 ++++++++++++++++++++++++++++++- usr.bin/ssh/misc.h | 3 ++- usr.bin/ssh/ssh-pkcs11.c | 6 +++++- usr.bin/ssh/ssh-sk.c | 9 ++++++--- 4 files changed, 43 insertions(+), 6 deletions(-) diff --git a/usr.bin/ssh/misc.c b/usr.bin/ssh/misc.c index a128ea80e7e5..3f5ed04b9e95 100644 --- a/usr.bin/ssh/misc.c +++ b/usr.bin/ssh/misc.c @@ -1,4 +1,4 @@ -/* $OpenBSD: misc.c,v 1.183 2023/07/14 07:44:21 dtucker Exp $ */ +/* $OpenBSD: misc.c,v 1.184 2023/07/19 14:02:27 djm Exp $ */ /* * Copyright (c) 2000 Markus Friedl. All rights reserved. * Copyright (c) 2005-2020 Damien Miller. All rights reserved. @@ -40,6 +40,7 @@ #include #include #include +#include #include #include #include @@ -2814,3 +2815,31 @@ ptimeout_isset(struct timespec *pt) { return pt->tv_sec != -1; } + +/* + * Returns zero if the library at 'path' contains symbol 's', nonzero + * otherwise. + */ +int +lib_contains_symbol(const char *path, const char *s) +{ + struct nlist nl[2]; + int ret = -1, r; + + memset(nl, 0, sizeof(nl)); + nl[0].n_name = xstrdup(s); + nl[1].n_name = NULL; + if ((r = nlist(path, nl)) == -1) { + error_f("nlist failed for %s", path); + goto out; + } + if (r != 0 || nl[0].n_value == 0 || nl[0].n_type == 0) { + error_f("library %s does not contain symbol %s", path, s); + goto out; + } + /* success */ + ret = 0; + out: + free(nl[0].n_name); + return ret; +} diff --git a/usr.bin/ssh/misc.h b/usr.bin/ssh/misc.h index 902cf56cbe20..94082238d4fc 100644 --- a/usr.bin/ssh/misc.h +++ b/usr.bin/ssh/misc.h @@ -1,4 +1,4 @@ -/* $OpenBSD: misc.h,v 1.102 2023/03/03 02:37:58 dtucker Exp $ */ +/* $OpenBSD: misc.h,v 1.103 2023/07/19 14:02:27 djm Exp $ */ /* * Author: Tatu Ylonen @@ -96,6 +96,7 @@ int parse_absolute_time(const char *, uint64_t *); void format_absolute_time(uint64_t, char *, size_t); int path_absolute(const char *); int stdfd_devnull(int, int, int); +int lib_contains_symbol(const char *, const char *); struct passwd *pwcopy(struct passwd *); const char *ssh_gai_strerror(int); diff --git a/usr.bin/ssh/ssh-pkcs11.c b/usr.bin/ssh/ssh-pkcs11.c index da2efd13d62e..e9aada6b8982 100644 --- a/usr.bin/ssh/ssh-pkcs11.c +++ b/usr.bin/ssh/ssh-pkcs11.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssh-pkcs11.c,v 1.57 2023/07/19 13:55:53 djm Exp $ */ +/* $OpenBSD: ssh-pkcs11.c,v 1.58 2023/07/19 14:02:27 djm Exp $ */ /* * Copyright (c) 2010 Markus Friedl. All rights reserved. * Copyright (c) 2014 Pedro Martelletto. All rights reserved. @@ -1507,6 +1507,10 @@ pkcs11_register_provider(char *provider_id, char *pin, debug_f("provider already registered: %s", provider_id); goto fail; } + if (lib_contains_symbol(provider_id, "C_GetFunctionList") != 0) { + error("provider %s is not a PKCS11 library", provider_id); + goto fail; + } /* open shared pkcs11-library */ if ((handle = dlopen(provider_id, RTLD_NOW)) == NULL) { error("dlopen %s failed: %s", provider_id, dlerror()); diff --git a/usr.bin/ssh/ssh-sk.c b/usr.bin/ssh/ssh-sk.c index e5e72f27849a..d6eea2e054ba 100644 --- a/usr.bin/ssh/ssh-sk.c +++ b/usr.bin/ssh/ssh-sk.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssh-sk.c,v 1.39 2022/07/20 03:29:14 djm Exp $ */ +/* $OpenBSD: ssh-sk.c,v 1.40 2023/07/19 14:02:27 djm Exp $ */ /* * Copyright (c) 2019 Google LLC * @@ -113,15 +113,18 @@ sshsk_open(const char *path) ret->sk_load_resident_keys = ssh_sk_load_resident_keys; return ret; } + if (lib_contains_symbol(path, "sk_api_version") != 0) { + error("provider %s is not an OpenSSH FIDO library", path); + goto fail; + } if ((ret->dlhandle = dlopen(path, RTLD_NOW)) == NULL) { error("Provider \"%s\" dlopen failed: %s", path, dlerror()); goto fail; } if ((ret->sk_api_version = dlsym(ret->dlhandle, "sk_api_version")) == NULL) { - error("Provider \"%s\" dlsym(sk_api_version) failed: %s", + fatal("Provider \"%s\" dlsym(sk_api_version) failed: %s", path, dlerror()); - goto fail; } version = ret->sk_api_version(); debug_f("provider %s implements version 0x%08lx", ret->path,