From d1602a056e7ef9e950ef4cea5cebaf31280e65b63400b1aa7f28a6d0e97fe230 Mon Sep 17 00:00:00 2001 From: Stanislav Brabec Date: Mon, 10 Sep 2018 15:04:41 +0000 Subject: [PATCH] Accepting request 629902 from home:vitezslav_cizek:branches:security:chipcard - Address security issues found by X41 D-Sec audit (bsc#1105012) * Authentication Replay * Buffer Overflow * Memory not cleaned properly before free() - add patches: * 0001-verify-using-a-nonce-from-the-system-not-the-card.patch * 0002-fixed-buffer-overflow-with-long-home-directory.patch * 0003-fixed-wiping-secrets-with-OpenSSL_cleanse.patch OBS-URL: https://build.opensuse.org/request/show/629902 OBS-URL: https://build.opensuse.org/package/show/security:chipcard/pam_pkcs11?expand=0&rev=24 --- ...a-nonce-from-the-system-not-the-card.patch | 103 +++++++++++++++ ...er-overflow-with-long-home-directory.patch | 36 +++++ ...-wiping-secrets-with-OpenSSL_cleanse.patch | 124 ++++++++++++++++++ pam_pkcs11.changes | 12 ++ pam_pkcs11.spec | 6 + 5 files changed, 281 insertions(+) create mode 100644 0001-verify-using-a-nonce-from-the-system-not-the-card.patch create mode 100644 0002-fixed-buffer-overflow-with-long-home-directory.patch create mode 100644 0003-fixed-wiping-secrets-with-OpenSSL_cleanse.patch diff --git a/0001-verify-using-a-nonce-from-the-system-not-the-card.patch b/0001-verify-using-a-nonce-from-the-system-not-the-card.patch new file mode 100644 index 0000000..ea8a7a4 --- /dev/null +++ b/0001-verify-using-a-nonce-from-the-system-not-the-card.patch @@ -0,0 +1,103 @@ +From cc51b3e2720ea862d500cab2ea517518ff39a497 Mon Sep 17 00:00:00 2001 +From: Frank Morgner +Date: Fri, 25 May 2018 23:46:41 +0200 +Subject: [PATCH 1/3] verify using a nonce from the system, not the card + +Thanks to Eric Sesterhenn from X41 D-SEC GmbH +for reporting the problem. +--- + src/common/pkcs11_lib.c | 66 +++++++++++++++++------------------------ + 1 file changed, 28 insertions(+), 38 deletions(-) + +diff --git a/src/common/pkcs11_lib.c b/src/common/pkcs11_lib.c +index 46a93bd..d4433f2 100644 +--- a/src/common/pkcs11_lib.c ++++ b/src/common/pkcs11_lib.c +@@ -131,6 +131,34 @@ memcmp_pad_max(void *d1, size_t d1_len, void *d2, size_t d2_len, + return (0); + } + ++int get_random_value(unsigned char *data, int length) ++{ ++ static const char *random_device = "/dev/urandom"; ++ int rv, fh, l; ++ ++ DBG2("reading %d random bytes from %s", length, random_device); ++ fh = open(random_device, O_RDONLY); ++ if (fh == -1) { ++ set_error("open() failed: %s", strerror(errno)); ++ return -1; ++ } ++ ++ l = 0; ++ while (l < length) { ++ rv = read(fh, data + l, length - l); ++ if (rv <= 0) { ++ close(fh); ++ set_error("read() failed: %s", strerror(errno)); ++ return -1; ++ } ++ l += rv; ++ } ++ close(fh); ++ DBG5("random-value[%d] = [%02x:%02x:%02x:...:%02x]", length, data[0], ++ data[1], data[2], data[length - 1]); ++ return 0; ++} ++ + + #ifdef HAVE_NSS + /* +@@ -834,16 +862,6 @@ int sign_value(pkcs11_handle_t *h, cert_object_t *cert, CK_BYTE *data, + return 0; + } + +-int get_random_value(unsigned char *data, int length) +-{ +- SECStatus rv = PK11_GenerateRandom(data,length); +- if (rv != SECSuccess) { +- DBG1("couldn't generate random number: %s", SECU_Strerror(PR_GetError())); +- } +- return (rv == SECSuccess) ? 0 : -1; +-} +- +- + struct tuple_str { + PRErrorCode errNum; + const char * errString; +@@ -1778,32 +1796,4 @@ int sign_value(pkcs11_handle_t *h, cert_object_t *cert, CK_BYTE *data, + (*signature)[0], (*signature)[1], (*signature)[2], (*signature)[*signature_length - 1]); + return 0; + } +- +-int get_random_value(unsigned char *data, int length) +-{ +- static const char *random_device = "/dev/urandom"; +- int rv, fh, l; +- +- DBG2("reading %d random bytes from %s", length, random_device); +- fh = open(random_device, O_RDONLY); +- if (fh == -1) { +- set_error("open() failed: %s", strerror(errno)); +- return -1; +- } +- +- l = 0; +- while (l < length) { +- rv = read(fh, data + l, length - l); +- if (rv <= 0) { +- close(fh); +- set_error("read() failed: %s", strerror(errno)); +- return -1; +- } +- l += rv; +- } +- close(fh); +- DBG5("random-value[%d] = [%02x:%02x:%02x:...:%02x]", length, data[0], +- data[1], data[2], data[length - 1]); +- return 0; +-} + #endif /* HAVE_NSS */ +-- +2.18.0 + diff --git a/0002-fixed-buffer-overflow-with-long-home-directory.patch b/0002-fixed-buffer-overflow-with-long-home-directory.patch new file mode 100644 index 0000000..e51f7cb --- /dev/null +++ b/0002-fixed-buffer-overflow-with-long-home-directory.patch @@ -0,0 +1,36 @@ +From a37fe986997b2d2fefc350c43650cc8193389235 Mon Sep 17 00:00:00 2001 +From: Frank Morgner +Date: Fri, 25 May 2018 23:53:44 +0200 +Subject: [PATCH 2/3] fixed buffer overflow with long home directory + +Thanks to Eric Sesterhenn from X41 D-SEC GmbH +for reporting the issue. +--- + src/mappers/openssh_mapper.c | 4 ++-- + 1 file changed, 2 insertions(+), 2 deletions(-) + +diff --git a/src/mappers/openssh_mapper.c b/src/mappers/openssh_mapper.c +index b9e09f7..ed0a409 100644 +--- a/src/mappers/openssh_mapper.c ++++ b/src/mappers/openssh_mapper.c +@@ -311,7 +311,7 @@ _DEFAULT_MAPPER_END + */ + static int openssh_mapper_match_user(X509 *x509, const char *user, void *context) { + struct passwd *pw; +- char filename[512]; ++ char filename[PATH_MAX]; + if (!x509) return -1; + if (!user) return -1; + pw = getpwnam(user); +@@ -333,7 +333,7 @@ static char * openssh_mapper_find_user(X509 *x509, void *context, int *match) { + /* parse list of users until match */ + setpwent(); + while((pw=getpwent()) != NULL) { +- char filename[512]; ++ char filename[PATH_MAX]; + DBG1("Trying to match certificate with user: '%s'",pw->pw_name); + if ( is_empty_str(pw->pw_dir) ) { + DBG1("User '%s' has no home directory",pw->pw_name); +-- +2.18.0 + diff --git a/0003-fixed-wiping-secrets-with-OpenSSL_cleanse.patch b/0003-fixed-wiping-secrets-with-OpenSSL_cleanse.patch new file mode 100644 index 0000000..3ca79d7 --- /dev/null +++ b/0003-fixed-wiping-secrets-with-OpenSSL_cleanse.patch @@ -0,0 +1,124 @@ +From a0c9b6ffc020944f03f57e7de66ad4363d52125d Mon Sep 17 00:00:00 2001 +From: Frank Morgner +Date: Sat, 26 May 2018 00:10:49 +0200 +Subject: [PATCH 3/3] fixed wiping secrets with OpenSSL_cleanse() + +Thanks to Eric Sesterhenn from X41 D-SEC GmbH +for reporting the problems. +--- + src/common/pkcs11_lib.c | 15 ++++++++++++--- + src/common/pkcs11_lib.h | 1 + + src/pam_pkcs11/pam_pkcs11.c | 10 +++++----- + 3 files changed, 18 insertions(+), 8 deletions(-) + +diff --git a/src/common/pkcs11_lib.c b/src/common/pkcs11_lib.c +index d4433f2..912de05 100644 +--- a/src/common/pkcs11_lib.c ++++ b/src/common/pkcs11_lib.c +@@ -63,7 +63,7 @@ int pkcs11_pass_login(pkcs11_handle_t *h, int nullok) + + /* perform pkcs #11 login */ + rv = pkcs11_login(h, pin); +- memset(pin, 0, strlen(pin)); ++ cleanse(pin, strlen(pin)); + if (rv != 0) { + set_error("pkcs11_login() failed: %s", get_error()); + return -1; +@@ -159,6 +159,15 @@ int get_random_value(unsigned char *data, int length) + return 0; + } + ++void cleanse(void *ptr, size_t len) ++{ ++#ifdef HAVE_OPENSSL ++ OPENSSL_cleanse(ptr, len); ++#else ++ memset(ptr, 0, len); ++#endif ++} ++ + + #ifdef HAVE_NSS + /* +@@ -637,7 +646,7 @@ void release_pkcs11_module(pkcs11_handle_t *h) + if (h->module) { + SECMOD_DestroyModule(h->module); + } +- memset(h, 0, sizeof(pkcs11_handle_t)); ++ cleanse(h, sizeof(pkcs11_handle_t)); + free(h); + + /* if we initialized NSS, then we need to shut it down */ +@@ -1199,7 +1208,7 @@ void release_pkcs11_module(pkcs11_handle_t *h) + /* release all allocated memory */ + if (h->slots != NULL) + free(h->slots); +- memset(h, 0, sizeof(pkcs11_handle_t)); ++ cleanse(h, 0, sizeof(pkcs11_handle_t)); + free(h); + } + +diff --git a/src/common/pkcs11_lib.h b/src/common/pkcs11_lib.h +index 27ed910..637a0c1 100644 +--- a/src/common/pkcs11_lib.h ++++ b/src/common/pkcs11_lib.h +@@ -67,6 +67,7 @@ PKCS11_EXTERN int sign_value(pkcs11_handle_t *h, cert_object_t *, + unsigned char *data, unsigned long length, + unsigned char **signature, unsigned long *signature_length); + PKCS11_EXTERN int get_random_value(unsigned char *data, int length); ++PKCS11_EXTERN void cleanse(void *ptr, size_t len); + + #undef PKCS11_EXTERN + +diff --git a/src/pam_pkcs11/pam_pkcs11.c b/src/pam_pkcs11/pam_pkcs11.c +index d6ca475..3f2b6ab 100644 +--- a/src/pam_pkcs11/pam_pkcs11.c ++++ b/src/pam_pkcs11/pam_pkcs11.c +@@ -108,7 +108,7 @@ static int pam_prompt(pam_handle_t *pamh, int style, char **response, char *fmt, + *response = strdup(resp[0].resp); + } + /* overwrite memory and release it */ +- memset(resp[0].resp, 0, strlen(resp[0].resp)); ++ cleanse(resp[0].resp, strlen(resp[0].resp)); + free(&resp[0]); + return PAM_SUCCESS; + } +@@ -191,7 +191,7 @@ static int pam_get_pwd(pam_handle_t *pamh, char **pwd, char *text, int oitem, in + return PAM_CRED_INSUFFICIENT; + *pwd = strdup(resp[0].resp); + /* overwrite memory and release it */ +- memset(resp[0].resp, 0, strlen(resp[0].resp)); ++ cleanse(resp[0].resp, strlen(resp[0].resp)); + free(&resp[0]); + /* save password if variable nitem is set */ + if ((nitem == PAM_AUTHTOK) || (nitem == PAM_OLDAUTHTOK)) { +@@ -517,7 +517,7 @@ PAM_EXTERN int pam_sm_authenticate(pam_handle_t *pamh, int flags, int argc, cons + /* check password length */ + if (!configuration->nullok && strlen(password) == 0) { + release_pkcs11_module(ph); +- memset(password, 0, strlen(password)); ++ cleanse(password, strlen(password)); + free(password); + pam_syslog(pamh, LOG_ERR, + "password length is zero but the 'nullok' argument was not defined."); +@@ -543,7 +543,7 @@ PAM_EXTERN int pam_sm_authenticate(pam_handle_t *pamh, int flags, int argc, cons + /* erase and free in-memory password data asap */ + if (password) + { +- memset(password, 0, strlen(password)); ++ cleanse(password, strlen(password)); + free(password); + } + if (rv != 0) { +@@ -831,7 +831,7 @@ PAM_EXTERN int pam_sm_authenticate(pam_handle_t *pamh, int flags, int argc, cons + return PAM_SUCCESS; + + /* quick and dirty fail exit point */ +- memset(password, 0, strlen(password)); ++ cleanse(password, strlen(password)); + free(password); /* erase and free in-memory password data */ + + auth_failed_nopw: +-- +2.18.0 + diff --git a/pam_pkcs11.changes b/pam_pkcs11.changes index 6ba6e0c..51f4d0c 100644 --- a/pam_pkcs11.changes +++ b/pam_pkcs11.changes @@ -1,3 +1,15 @@ +------------------------------------------------------------------- +Fri Aug 17 10:12:31 UTC 2018 - vcizek@suse.com + +- Address security issues found by X41 D-Sec audit (bsc#1105012) + * Authentication Replay + * Buffer Overflow + * Memory not cleaned properly before free() +- add patches: + * 0001-verify-using-a-nonce-from-the-system-not-the-card.patch + * 0002-fixed-buffer-overflow-with-long-home-directory.patch + * 0003-fixed-wiping-secrets-with-OpenSSL_cleanse.patch + ------------------------------------------------------------------- Mon Jul 23 17:36:18 CEST 2018 - sbrabec@suse.com diff --git a/pam_pkcs11.spec b/pam_pkcs11.spec index 0927630..2bcf4bf 100644 --- a/pam_pkcs11.spec +++ b/pam_pkcs11.spec @@ -36,6 +36,9 @@ Patch1: %{name}-0.5.3-nss-conf.patch Patch3: %{name}-0.6.0-nss-autoconf.patch # PATCH-FIX-UPSTEAM-PENDING pam_pkcs11-crl-check.patch https://github.com/OpenSC/pam_pkcs11/pull/26 -- Fix segfault and fetch problems when checking CRLs. Patch4: %{name}-crl-check.patch +Patch5: 0001-verify-using-a-nonce-from-the-system-not-the-card.patch +Patch6: 0002-fixed-buffer-overflow-with-long-home-directory.patch +Patch7: 0003-fixed-wiping-secrets-with-OpenSSL_cleanse.patch BuildRequires: curl-devel BuildRequires: docbook-xsl-stylesheets BuildRequires: doxygen @@ -91,6 +94,9 @@ authentication. %patch1 -p1 %patch3 -p1 %patch4 -p1 +%patch5 -p1 +%patch6 -p1 +%patch7 -p1 cp -a %{SOURCE1} common-auth-smartcard sed -i s:/lib/:/%{_lib}/:g etc/pam_pkcs11.conf.example.in etc/pkcs11_eventmgr.conf.example # make dist was not called and cannot be called on a non git snapshot.