From a4c64c8c212273112670bf5a206170c4ce5e18c0 Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Mon, 20 May 2019 15:08:23 -0400 Subject: [PATCH] Initialize C_Initialize's reader list to NULs. valgrind --track-origins=true says: ==13529== Conditional jump or move depends on uninitialised value(s) ==13529== at 0x4AF92D1: PK11_MakeString (pk11slot.c:1073) ==13529== by 0x4AFA5AA: PK11_InitSlot (pk11slot.c:1456) ==13529== by 0x4AE315E: secmod_LoadPKCS11Module (pk11load.c:563) ==13529== by 0x4AEF68C: SECMOD_LoadModule (pk11pars.c:1838) ==13529== by 0x4AEF7C7: SECMOD_LoadModule (pk11pars.c:1874) ==13529== by 0x4ABCB6A: nss_InitModules (nssinit.c:464) ==13529== by 0x4ABCB6A: nss_Init (nssinit.c:689) ==13529== by 0x4ABD17C: NSS_Init (nssinit.c:824) ==13529== by 0x4059C0: main (pesign.c:354) ==13529== Uninitialised value was created by a stack allocation ==13529== at 0x484D175: C_Initialize (in /usr/lib64/libykcs11.so.1.5.0) This is the result of a combination of two problems. In ykcs11/utils.c:parse_readers(), the code does: for (i = 0; i < len; i++) if (readers[i] == '\0' && i != len - 1) { But in ykcs11/ykcs11.c:C_Initialize(), the parts of readers[] that are initialized are only the parts that have been populated; the rest of the array is still just whatever value is on the stack. Additionally, in lib/ykpiv.c:ykpiv_list_readers(), which populates the array, the length is updated only in the case where the buffer is smaller than the data, not when there is additional buffer but no data: if (num_readers > *len) { num_readers = (pcsc_word)*len; } The result is that if the amount of reader data is smaller than 2048 bytes, PK11_InitSlot() will try to find reader data in the rest of the array, which has not been initialized. This patch adds an initialization for the data to set it all '\0', and also updates the length when there is excess buffer available. Signed-off-by: Peter Jones --- lib/ykpiv.c | 2 ++ ykcs11/ykcs11.c | 2 ++ 2 files changed, 4 insertions(+) diff --git a/lib/ykpiv.c b/lib/ykpiv.c index 67d8f79..b1b0a1f 100644 --- a/lib/ykpiv.c +++ b/lib/ykpiv.c @@ -520,6 +520,8 @@ ykpiv_rc ykpiv_list_readers(ykpiv_state *state, char *readers, size_t *len) { if (num_readers > *len) { num_readers = (pcsc_word)*len; + } else if (num_readers < *len) { + *len = (size_t)num_readers; } rc = SCardListReaders(state->context, NULL, readers, &num_readers); diff --git a/ykcs11/ykcs11.c b/ykcs11/ykcs11.c index 6bbdb1a..9a817bf 100644 --- a/ykcs11/ykcs11.c +++ b/ykcs11/ykcs11.c @@ -86,6 +86,8 @@ CK_DEFINE_FUNCTION(CK_RV, C_Initialize)( DIN; + memset(readers, '\0', sizeof(readers)); + if (piv_state != NULL) return CKR_CRYPTOKI_ALREADY_INITIALIZED;