From c4bd40a483c7962beb4a8b1c7f84d647de66bf0c Mon Sep 17 00:00:00 2001 From: Kamil Dudka Date: Tue, 29 Aug 2017 13:08:11 +0200 Subject: [PATCH 1/5] util: fix the return value type of ReadDERFromFile() SECStatus is an inappropriate type for returning the count of objects. Upstream-commit: 01c13264e59050ea34aefc3d2a38e9ead247276c Signed-off-by: Kamil Dudka --- src/ckpem.h | 6 ++---- src/pinst.c | 15 +++++---------- src/pobject.c | 10 ++++------ src/util.c | 7 +++---- 4 files changed, 14 insertions(+), 24 deletions(-) diff --git a/src/ckpem.h b/src/ckpem.h index e6ecc5f..be9977a 100644 --- a/src/ckpem.h +++ b/src/ckpem.h @@ -259,10 +259,8 @@ pem_CreateMDObject #define NSS_PEM_ARRAY_SIZE(x) ((sizeof (x))/(sizeof ((x)[0]))) /* Read DER encoded data from a PEM file or a binary (der-encoded) file. */ -/* NOTE: Discrepancy with the the way callers use of the return value as a count - * Fix this when we sync. up with the cleanup work being done at nss-pem project. - */ -SECStatus ReadDERFromFile(SECItem ***derlist, char *filename, PRBool ascii, int *cipher, char **ivstring, PRBool certsonly); +int ReadDERFromFile(SECItem ***derlist, char *filename, PRBool ascii, + int *cipher, char **ivstring, PRBool certsonly); /* Fetch an attribute of the specified type. */ const NSSItem * pem_FetchAttribute ( pemInternalObject *io, CK_ATTRIBUTE_TYPE type, CK_RV *pError); diff --git a/src/pinst.c b/src/pinst.c index 25485b1..a5901f0 100644 --- a/src/pinst.c +++ b/src/pinst.c @@ -475,14 +475,12 @@ AddCertificate(char *certfile, char *keyfile, PRBool cacert, pemInternalObject *o = NULL; CK_RV error = 0; int objid, i = 0; - int nobjs = 0; SECItem **objs = NULL; char *ivstring = NULL; int cipher; - /* TODO: Fix discrepancy between our usage of the return value as - * as an int (a count) and the declaration as a SECStatus. */ - nobjs = (int) ReadDERFromFile(&objs, certfile, PR_TRUE, &cipher, &ivstring, PR_TRUE /* certs only */); + int nobjs = ReadDERFromFile(&objs, certfile, /* ascii */ PR_TRUE, &cipher, + &ivstring, /* certs only */ PR_TRUE); if (nobjs <= 0) { NSS_ZFreeIf(objs); return CKR_GENERAL_ERROR; @@ -517,12 +515,9 @@ AddCertificate(char *certfile, char *keyfile, PRBool cacert, if (o != NULL && keyfile != NULL) { /* add the private key */ SECItem **keyobjs = NULL; - int kobjs = 0; - /* TODO: Fix discrepancy between our usage of the return value as - * as an int and the declaration as a SECStatus. */ - kobjs = - (int) ReadDERFromFile(&keyobjs, keyfile, PR_TRUE, &cipher, - &ivstring, PR_FALSE); + int kobjs = ReadDERFromFile(&keyobjs, keyfile, /* ascii */ PR_TRUE, + &cipher, &ivstring, + /*certs only */ PR_FALSE); if (kobjs < 1) { found_error = PR_TRUE; } else { diff --git a/src/pobject.c b/src/pobject.c index 918b327..d80092a 100644 --- a/src/pobject.c +++ b/src/pobject.c @@ -1165,11 +1165,8 @@ pem_CreateObject } if (objClass == CKO_CERTIFICATE) { - /* TODO: Fix discrepancy between our usage of the return value as - * as an int and the declaration as a SECStatus. Typecasting as a - * temporary workaround. - */ - nobjs = (int) ReadDERFromFile(&derlist, filename, PR_TRUE, &cipher, &ivstring, PR_TRUE /* certs only */); + nobjs = ReadDERFromFile(&derlist, filename, /* ascii */ PR_TRUE, + &cipher, &ivstring, /* certs only */ PR_TRUE); if (nobjs < 1) goto loser; @@ -1213,7 +1210,8 @@ pem_CreateObject SECItem certDER; PRBool added; - nobjs = ReadDERFromFile(&derlist, filename, PR_TRUE, &cipher, &ivstring, PR_FALSE /* keys only */); + nobjs = ReadDERFromFile(&derlist, filename, /* ascii */ PR_TRUE, + &cipher, &ivstring, /* keys only */ PR_FALSE); if (nobjs < 1) goto loser; diff --git a/src/util.c b/src/util.c index ee9e0a4..9bad613 100644 --- a/src/util.c +++ b/src/util.c @@ -144,10 +144,9 @@ static SECStatus ConvertAsciiToZAllocItem(SECItem *der, const char *ascii) return rv; } -/* FIX: Returns a SECStatus yet callers take result as a count */ -SECStatus -ReadDERFromFile(SECItem *** derlist, char *filename, PRBool ascii, - int *cipher, char **ivstring, PRBool certsonly) +/* returns count of objects read, or -1 on error */ +int ReadDERFromFile(SECItem *** derlist, char *filename, PRBool ascii, + int *cipher, char **ivstring, PRBool certsonly) { SECStatus rv; PRFileDesc *inFile; -- 2.17.2 From 729f16122f0bae216583323389aa6ef1418e4c37 Mon Sep 17 00:00:00 2001 From: Kamil Dudka Date: Thu, 20 Dec 2018 18:42:33 +0100 Subject: [PATCH 2/5] ckpem: make pem_objs a list instead of array Since libcurl started to use PK11_CreateManagedGenericObject(), the calls to PK11_DestroyGenericObject() started to take affect at the level of nss-pem. Internal objects were removed on each call of curl_easy_cleanup() and allocated again on each call of curl_easy_perform(). This would be fine if the corresponding items of the global pem_objects array were reused, which was, however, not the case. Consequently, the array grew unboundedly while most of its items were NULL pointers. This caused severe performance regression in libcurl-based applications. This commit replaces the global array by a global linked list, which allows to delete arbitrary items instead of keeping NULL pointers. For compatibility reasons, the original array indexes are stored in the list nodes. Bug: https://bugzilla.redhat.com/1659108 Fixes #2 Upstream-commit: 8fc25a6b062a34d09473d4f80058fffda37f41a2 Signed-off-by: Kamil Dudka --- src/ckpem.h | 11 ++- src/list.h | 200 +++++++++++++++++++++++++++++++++++++++++++++++++ src/pfind.c | 19 ++--- src/pinst.c | 56 +++++++------- src/pobject.c | 27 +++---- src/psession.c | 11 +-- 6 files changed, 258 insertions(+), 66 deletions(-) create mode 100644 src/list.h diff --git a/src/ckpem.h b/src/ckpem.h index be9977a..caad74b 100644 --- a/src/ckpem.h +++ b/src/ckpem.h @@ -39,6 +39,8 @@ #ifndef CKPEM_H #define CKPEM_H +#include "list.h" + #define USE_UTIL_DIRECTLY #include @@ -197,9 +199,14 @@ struct pemInternalObjectStr { char *nickname; NSSCKMDObject mdObject; CK_SLOT_ID slotID; - CK_ULONG gobjIndex; int refCount; + /* all internal objects are linked in a global list */ + struct list_head gl_list; + + /* we represent sparse array as list but keep its elements indexed */ + long arrayIdx; + /* used by pem_mdFindObjects_Next */ CK_BBOOL extRef; @@ -208,7 +215,7 @@ struct pemInternalObjectStr { pemObjectListItem *list; }; -NSS_EXTERN_DATA pemInternalObject **pem_objs; +NSS_EXTERN_DATA struct list_head pem_objs; NSS_EXTERN_DATA int pem_nobjs; NSS_EXTERN_DATA int token_needsLogin[]; NSS_EXTERN_DATA NSSCKMDSlot *lastEventSlot; diff --git a/src/list.h b/src/list.h new file mode 100644 index 0000000..7b4aec4 --- /dev/null +++ b/src/list.h @@ -0,0 +1,200 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _LINUX_LIST_H +#define _LINUX_LIST_H + +/* simplified for our use case */ +#define READ_ONCE(x) (x) +#define WRITE_ONCE(dst, src) (dst = (src)) +#define typeof(x) __typeof__(x) +#define POISON_POINTER_DELTA 0 + +/* following code is taken from of v4.20-rc7-202-g1d51b4b1d3f2 */ + +/** + * container_of - cast a member of a structure out to the containing structure + * @ptr: the pointer to the member. + * @type: the type of the container struct this is embedded in. + * @member: the name of the member within the struct. + * + */ +#define container_of(ptr, type, member) ({ \ + void *__mptr = (void *)(ptr); \ + ((type *)(__mptr - offsetof(type, member))); }) + +/* + * These are non-NULL pointers that will result in page faults + * under normal circumstances, used to verify that nobody uses + * non-initialized list entries. + */ +#define LIST_POISON1 ((void *) 0x100 + POISON_POINTER_DELTA) +#define LIST_POISON2 ((void *) 0x200 + POISON_POINTER_DELTA) + +struct list_head { + struct list_head *next, *prev; +}; + +/* + * Simple doubly linked list implementation. + * + * Some of the internal functions ("__xxx") are useful when + * manipulating whole lists rather than single entries, as + * sometimes we already know the next/prev entries and we can + * generate better code by using them directly rather than + * using the generic single-entry routines. + */ + +#define LIST_HEAD_INIT(name) { &(name), &(name) } + +#define LIST_HEAD(name) \ + struct list_head name = LIST_HEAD_INIT(name) + +static inline void INIT_LIST_HEAD(struct list_head *list) +{ + WRITE_ONCE(list->next, list); + list->prev = list; +} + +/* + * Insert a new entry between two known consecutive entries. + * + * This is only for internal list manipulation where we know + * the prev/next entries already! + */ +static inline void __list_add(struct list_head *new, + struct list_head *prev, + struct list_head *next) +{ + next->prev = new; + new->next = next; + new->prev = prev; + WRITE_ONCE(prev->next, new); +} + +/** + * list_add - add a new entry + * @new: new entry to be added + * @head: list head to add it after + * + * Insert a new entry after the specified head. + * This is good for implementing stacks. + */ +static inline void list_add(struct list_head *new, struct list_head *head) +{ + __list_add(new, head, head->next); +} + + +/** + * list_add_tail - add a new entry + * @new: new entry to be added + * @head: list head to add it before + * + * Insert a new entry before the specified head. + * This is useful for implementing queues. + */ +static inline void list_add_tail(struct list_head *new, struct list_head *head) +{ + __list_add(new, head->prev, head); +} + +/* + * Delete a list entry by making the prev/next entries + * point to each other. + * + * This is only for internal list manipulation where we know + * the prev/next entries already! + */ +static inline void __list_del(struct list_head * prev, struct list_head * next) +{ + next->prev = prev; + WRITE_ONCE(prev->next, next); +} + +/** + * list_del - deletes entry from list. + * @entry: the element to delete from the list. + * Note: list_empty() on entry does not return true after this, the entry is + * in an undefined state. + */ +static inline void __list_del_entry(struct list_head *entry) +{ + __list_del(entry->prev, entry->next); +} + +static inline void list_del(struct list_head *entry) +{ + __list_del_entry(entry); + entry->next = LIST_POISON1; + entry->prev = LIST_POISON2; +} + +/** + * list_entry - get the struct for this entry + * @ptr: the &struct list_head pointer. + * @type: the type of the struct this is embedded in. + * @member: the name of the list_head within the struct. + */ +#define list_entry(ptr, type, member) \ + container_of(ptr, type, member) + +/** + * list_first_entry - get the first element from a list + * @ptr: the list head to take the element from. + * @type: the type of the struct this is embedded in. + * @member: the name of the list_head within the struct. + * + * Note, that list is expected to be not empty. + */ +#define list_first_entry(ptr, type, member) \ + list_entry((ptr)->next, type, member) + +/** + * list_last_entry - get the last element from a list + * @ptr: the list head to take the element from. + * @type: the type of the struct this is embedded in. + * @member: the name of the list_head within the struct. + * + * Note, that list is expected to be not empty. + */ +#define list_last_entry(ptr, type, member) \ + list_entry((ptr)->prev, type, member) + +/** + * list_next_entry - get the next element in list + * @pos: the type * to cursor + * @member: the name of the list_head within the struct. + */ +#define list_next_entry(pos, member) \ + list_entry((pos)->member.next, typeof(*(pos)), member) + +/** + * list_prev_entry - get the prev element in list + * @pos: the type * to cursor + * @member: the name of the list_head within the struct. + */ +#define list_prev_entry(pos, member) \ + list_entry((pos)->member.prev, typeof(*(pos)), member) + +/** + * list_for_each_entry - iterate over list of given type + * @pos: the type * to use as a loop cursor. + * @head: the head for your list. + * @member: the name of the list_head within the struct. + */ +#define list_for_each_entry(pos, head, member) \ + for (pos = list_first_entry(head, typeof(*pos), member); \ + &pos->member != (head); \ + pos = list_next_entry(pos, member)) + +/** + * list_for_each_entry_reverse - iterate backwards over list of given type. + * @pos: the type * to use as a loop cursor. + * @head: the head for your list. + * @member: the name of the list_head within the struct. + */ +#define list_for_each_entry_reverse(pos, head, member) \ + for (pos = list_last_entry(head, typeof(*pos), member); \ + &pos->member != (head); \ + pos = list_prev_entry(pos, member)) + +#endif diff --git a/src/pfind.c b/src/pfind.c index 2aff8ff..3f0fdcb 100644 --- a/src/pfind.c +++ b/src/pfind.c @@ -253,12 +253,13 @@ collect_objects(CK_ATTRIBUTE_PTR pTemplate, size_t result_array_capacity = 0; pemObjectType type = pemRaw; CK_OBJECT_CLASS objClass = pem_GetObjectClass(pTemplate, ulAttributeCount); + pemInternalObject *obj = NULL; *pError = CKR_OK; plog("collect_objects slot #%ld, ", slotID); plog("%d attributes, ", ulAttributeCount); - plog("%d objects to look through.\n", pem_nobjs); + plog("%d objects created in total.\n", pem_nobjs); plog("Looking for: "); /* * now determine type of the object @@ -299,22 +300,18 @@ collect_objects(CK_ATTRIBUTE_PTR pTemplate, } /* find objects */ - for (i = 0; i < pem_nobjs; i++) { + list_for_each_entry(obj, &pem_objs, gl_list) { int match = 1; /* matches type if type not specified */ - if (NULL == pem_objs[i]) - continue; - - plog(" %d type = %d\n", i, pem_objs[i]->type); + plog(" %d type = %d\n", i, obj->type); if (type != pemAll) { /* type specified - must match given type */ - match = (type == pem_objs[i]->type); + match = (type == obj->type); } if (match) { - match = (slotID == pem_objs[i]->slotID) && - (CK_TRUE == pem_match(pTemplate, ulAttributeCount, pem_objs[i])); + match = (slotID == obj->slotID) && + (CK_TRUE == pem_match(pTemplate, ulAttributeCount, obj)); } if (match) { - pemInternalObject *o = pem_objs[i]; pemInternalObject **new_result_array = (pemInternalObject **) ensure_array_capacity(*result_array, &result_array_capacity, @@ -328,7 +325,7 @@ collect_objects(CK_ATTRIBUTE_PTR pTemplate, if (*result_array != new_result_array) { *result_array = new_result_array; } - (*result_array)[ result_array_entries ] = o; + (*result_array)[ result_array_entries ] = obj; result_array_entries++; } } diff --git a/src/pinst.c b/src/pinst.c index a5901f0..810db5a 100644 --- a/src/pinst.c +++ b/src/pinst.c @@ -50,7 +50,7 @@ static PRBool pemInitialized = PR_FALSE; -pemInternalObject **pem_objs; +LIST_HEAD(pem_objs); int pem_nobjs = 0; int token_needsLogin[NUM_SLOTS]; NSSCKMDSlot *lastEventSlot; @@ -362,13 +362,9 @@ derEncodingsMatch(CK_OBJECT_CLASS objClass, pemInternalObject * obj, static CK_RV LinkSharedKeyObject(int oldKeyIdx, int newKeyIdx) { - int i; - for (i = 0; i < pem_nobjs; i++) { + pemInternalObject *obj; + list_for_each_entry(obj, &pem_objs, gl_list) { CK_RV rv; - pemInternalObject *obj = pem_objs[i]; - if (NULL == obj) - continue; - if (atoi(obj->id.data) != oldKeyIdx) continue; @@ -381,13 +377,26 @@ LinkSharedKeyObject(int oldKeyIdx, int newKeyIdx) return CKR_OK; } +/* return pointer to the internal object with given arrayIdx */ +static pemInternalObject * +FindObjectByArrayIdx(const long arrayIdx) +{ + pemInternalObject *obj; + + list_for_each_entry(obj, &pem_objs, gl_list) + if (arrayIdx == obj->arrayIdx) + return obj; + + return NULL; +} + pemInternalObject * AddObjectIfNeeded(CK_OBJECT_CLASS objClass, pemObjectType type, SECItem * certDER, SECItem * keyDER, const char *filename, int objid, CK_SLOT_ID slotID, PRBool *pAdded) { - int i; + pemInternalObject *curObj; const char *nickname = strrchr(filename, '/'); if (nickname @@ -401,11 +410,7 @@ AddObjectIfNeeded(CK_OBJECT_CLASS objClass, *pAdded = PR_FALSE; /* first look for the object in pem_objs, it might be already there */ - for (i = 0; i < pem_nobjs; i++) { - pemInternalObject *const curObj = pem_objs[i]; - if (NULL == curObj) - continue; - + list_for_each_entry(curObj, &pem_objs, gl_list) { /* Comparing DER encodings is dependable and frees the PEM module * from having to require clients to provide unique nicknames. */ @@ -419,11 +424,11 @@ AddObjectIfNeeded(CK_OBJECT_CLASS objClass, * the key object is shared by multiple client certificates, such * an assumption does not hold. We have to update the references. */ - LinkSharedKeyObject(pem_nobjs, i); + LinkSharedKeyObject(pem_nobjs, curObj->arrayIdx); if (CKO_CERTIFICATE == objClass) { const int ref = atoi(curObj->id.data); - if (0 < ref && ref < pem_nobjs && !pem_objs[ref]) { + if (0 < ref && ref < pem_nobjs && !FindObjectByArrayIdx(ref)) { /* The certificate we are going to reuse refers to an * object that has already been removed. Make it refer * to the object that will be added next (private key). @@ -433,7 +438,8 @@ AddObjectIfNeeded(CK_OBJECT_CLASS objClass, } } - plog("AddObjectIfNeeded: re-using internal object #%i\n", i); + plog("AddObjectIfNeeded: re-using internal object #%li\n", + curObj->arrayIdx); curObj->refCount ++; return curObj; } @@ -448,18 +454,9 @@ AddObjectIfNeeded(CK_OBJECT_CLASS objClass, /* initialize pointers to functions */ pem_CreateMDObject(NULL, io, NULL); - io->gobjIndex = pem_nobjs; - - /* add object to global array */ - if (pem_objs) - pem_objs = NSS_ZRealloc(pem_objs, - (pem_nobjs + 1) * sizeof(pemInternalObject *)); - else - pem_objs = NSS_ZNEWARRAY(NULL, pemInternalObject *, 1); - if (!pem_objs) - return NULL; - pem_objs[pem_nobjs] = io; - pem_nobjs++; + /* add object to global list */ + io->arrayIdx = pem_nobjs++; + list_add_tail(&io->gl_list, &pem_objs); if (pAdded) *pAdded = PR_TRUE; @@ -748,8 +745,7 @@ pem_Finalize if (!pemInitialized) return; - NSS_ZFreeIf(pem_objs); - pem_objs = NULL; + INIT_LIST_HEAD(&pem_objs); pem_nobjs = 0; PR_AtomicSet(&pemInitialized, PR_FALSE); diff --git a/src/pobject.c b/src/pobject.c index d80092a..f118f5d 100644 --- a/src/pobject.c +++ b/src/pobject.c @@ -672,10 +672,8 @@ pem_DestroyInternalObject return; } - if (NULL != pem_objs) - /* remove reference to self from the global array */ - pem_objs[io->gobjIndex] = NULL; - + /* remove self from the global list */ + list_del(&io->gl_list); NSS_ZFreeIf(io); return; } @@ -1206,7 +1204,7 @@ pem_CreateObject goto loser; } } else if (objClass == CKO_PRIVATE_KEY) { - int i; + pemInternalObject *curObj; SECItem certDER; PRBool added; @@ -1220,30 +1218,27 @@ pem_CreateObject /* Brute force: find the id of the certificate, if any, in this slot */ objid = -1; - for (i = pem_nobjs - 1; 0 <= i; i--) { - if (NULL == pem_objs[i]) - continue; - - if (slotID != pem_objs[i]->slotID) + list_for_each_entry_reverse(curObj, &pem_objs, gl_list) { + if (slotID != curObj->slotID) continue; - if (pem_objs[i]->type != pemCert) + if (curObj->type != pemCert) continue; - if (atoi(pem_objs[i]->id.data) != pem_nobjs) + if (atoi(curObj->id.data) != pem_nobjs) /* not a certificate that refers to the key being added */ continue; objid = pem_nobjs; - certDER.data = NSS_ZAlloc(NULL, pem_objs[i]->derCert->len); + certDER.data = NSS_ZAlloc(NULL, curObj->derCert->len); if (certDER.data == NULL) goto loser; - certDER.len = pem_objs[i]->derCert->len; + certDER.len = curObj->derCert->len; memcpy(certDER.data, - pem_objs[i]->derCert->data, - pem_objs[i]->derCert->len); + curObj->derCert->data, + curObj->derCert->len); break; } diff --git a/src/psession.c b/src/psession.c index 4e94a7f..13a5e5d 100644 --- a/src/psession.c +++ b/src/psession.c @@ -241,7 +241,7 @@ pem_mdSession_Login NSSLOWKEYPrivateKey *lpk = NULL; PLArenaPool *arena; SECItem plain; - int i; + pemInternalObject *curObj; fwSlot = NSSCKFWToken_GetFWSlot(fwToken); slotID = NSSCKFWSlot_GetSlotID(fwSlot); @@ -256,12 +256,9 @@ pem_mdSession_Login token_needsLogin[slotID - 1] = PR_FALSE; /* Find the right key object */ - for (i = 0; i < pem_nobjs; i++) { - if (NULL == pem_objs[i]) - continue; - - if ((slotID == pem_objs[i]->slotID) && (pem_objs[i]->type == pemBareKey)) { - io = pem_objs[i]; + list_for_each_entry(curObj, &pem_objs, gl_list) { + if ((slotID == curObj->slotID) && (curObj->type == pemBareKey)) { + io = curObj; break; } } -- 2.17.2 From 6d968e0f54bfd48029086ae41067e6024bde76a2 Mon Sep 17 00:00:00 2001 From: Kamil Dudka Date: Fri, 21 Dec 2018 13:43:47 +0100 Subject: [PATCH 3/5] ckpem: define pem_nobjs and objid as long to avoid wraparound Fixes #2 Upstream-commit: 5a14b625f529b35f997f60f4ff302f0bfc6ddc16 Signed-off-by: Kamil Dudka --- src/ckpem.h | 4 ++-- src/pfind.c | 2 +- src/pinst.c | 29 +++++++++++++++-------------- src/pobject.c | 4 ++-- 4 files changed, 20 insertions(+), 19 deletions(-) diff --git a/src/ckpem.h b/src/ckpem.h index caad74b..5d05087 100644 --- a/src/ckpem.h +++ b/src/ckpem.h @@ -216,7 +216,7 @@ struct pemInternalObjectStr { }; NSS_EXTERN_DATA struct list_head pem_objs; -NSS_EXTERN_DATA int pem_nobjs; +NSS_EXTERN_DATA long pem_nobjs; NSS_EXTERN_DATA int token_needsLogin[]; NSS_EXTERN_DATA NSSCKMDSlot *lastEventSlot; @@ -304,7 +304,7 @@ PRBool pem_ParseString(const char *inputstring, const char delimiter, pemInternalObject * AddObjectIfNeeded(CK_OBJECT_CLASS objClass, pemObjectType type, - SECItem *certDER, SECItem *keyDER, const char *filename, int objid, + SECItem *certDER, SECItem *keyDER, const char *filename, long objid, CK_SLOT_ID slotID, PRBool *pAdded); void pem_DestroyInternalObject (pemInternalObject *io); diff --git a/src/pfind.c b/src/pfind.c index 3f0fdcb..958c0d5 100644 --- a/src/pfind.c +++ b/src/pfind.c @@ -259,7 +259,7 @@ collect_objects(CK_ATTRIBUTE_PTR pTemplate, plog("collect_objects slot #%ld, ", slotID); plog("%d attributes, ", ulAttributeCount); - plog("%d objects created in total.\n", pem_nobjs); + plog("%ld objects created in total.\n", pem_nobjs); plog("Looking for: "); /* * now determine type of the object diff --git a/src/pinst.c b/src/pinst.c index 810db5a..87bf7c2 100644 --- a/src/pinst.c +++ b/src/pinst.c @@ -51,7 +51,7 @@ static PRBool pemInitialized = PR_FALSE; LIST_HEAD(pem_objs); -int pem_nobjs = 0; +long pem_nobjs = 0L; int token_needsLogin[NUM_SLOTS]; NSSCKMDSlot *lastEventSlot; @@ -182,12 +182,12 @@ GetCertFields(unsigned char *cert, int cert_length, } static CK_RV -assignObjectID(pemInternalObject *o, int objid) +assignObjectID(pemInternalObject *o, const long objid) { - char id[16]; + char id[24]; int len; - sprintf(id, "%d", objid); + sprintf(id, "%ld", objid); len = strlen(id) + 1; /* zero terminate */ o->id.size = len; o->id.data = NSS_ZAlloc(NULL, len); @@ -202,7 +202,7 @@ static pemInternalObject * CreateObject(CK_OBJECT_CLASS objClass, pemObjectType type, SECItem * certDER, SECItem * keyDER, const char *filename, - int objid, CK_SLOT_ID slotID) + long objid, CK_SLOT_ID slotID) { pemInternalObject *o; SECItem subject; @@ -226,17 +226,17 @@ CreateObject(CK_OBJECT_CLASS objClass, switch (objClass) { case CKO_CERTIFICATE: - plog("Creating cert nick %s id %d in slot %ld\n", nickname, objid, slotID); + plog("Creating cert nick %s id %ld in slot %ld\n", nickname, objid, slotID); memset(&o->u.cert, 0, sizeof(o->u.cert)); break; case CKO_PRIVATE_KEY: - plog("Creating key id %d in slot %ld\n", objid, slotID); + plog("Creating key id %ld in slot %ld\n", objid, slotID); memset(&o->u.key, 0, sizeof(o->u.key)); /* more unique nicknames - https://bugzilla.redhat.com/689031#c66 */ nickname = filename; break; case CKO_NETSCAPE_TRUST: - plog("Creating trust nick %s id %d in slot %ld\n", nickname, objid, slotID); + plog("Creating trust nick %s id %ld in slot %ld\n", nickname, objid, slotID); memset(&o->u.trust, 0, sizeof(o->u.trust)); break; } @@ -360,12 +360,12 @@ derEncodingsMatch(CK_OBJECT_CLASS objClass, pemInternalObject * obj, } static CK_RV -LinkSharedKeyObject(int oldKeyIdx, int newKeyIdx) +LinkSharedKeyObject(const long oldKeyIdx, const long newKeyIdx) { pemInternalObject *obj; list_for_each_entry(obj, &pem_objs, gl_list) { CK_RV rv; - if (atoi(obj->id.data) != oldKeyIdx) + if (atol(obj->id.data) != oldKeyIdx) continue; NSS_ZFreeIf(obj->id.data); @@ -394,7 +394,7 @@ pemInternalObject * AddObjectIfNeeded(CK_OBJECT_CLASS objClass, pemObjectType type, SECItem * certDER, SECItem * keyDER, const char *filename, - int objid, CK_SLOT_ID slotID, PRBool *pAdded) + long objid, CK_SLOT_ID slotID, PRBool *pAdded) { pemInternalObject *curObj; @@ -427,7 +427,7 @@ AddObjectIfNeeded(CK_OBJECT_CLASS objClass, LinkSharedKeyObject(pem_nobjs, curObj->arrayIdx); if (CKO_CERTIFICATE == objClass) { - const int ref = atoi(curObj->id.data); + const long ref = atol(curObj->id.data); if (0 < ref && ref < pem_nobjs && !FindObjectByArrayIdx(ref)) { /* The certificate we are going to reuse refers to an * object that has already been removed. Make it refer @@ -471,7 +471,8 @@ AddCertificate(char *certfile, char *keyfile, PRBool cacert, { pemInternalObject *o = NULL; CK_RV error = 0; - int objid, i = 0; + long objid; + int i = 0; SECItem **objs = NULL; char *ivstring = NULL; int cipher; @@ -746,7 +747,7 @@ pem_Finalize return; INIT_LIST_HEAD(&pem_objs); - pem_nobjs = 0; + pem_nobjs = 0L; PR_AtomicSet(&pemInitialized, PR_FALSE); } diff --git a/src/pobject.c b/src/pobject.c index f118f5d..9a8321c 100644 --- a/src/pobject.c +++ b/src/pobject.c @@ -1089,7 +1089,7 @@ pem_CreateObject SECItem **derlist = NULL; int nobjs = 0; int i; - int objid; + long objid; #if 0 pemToken *token; #endif @@ -1225,7 +1225,7 @@ pem_CreateObject if (curObj->type != pemCert) continue; - if (atoi(curObj->id.data) != pem_nobjs) + if (atol(curObj->id.data) != pem_nobjs) /* not a certificate that refers to the key being added */ continue; -- 2.17.2 From 513ac624ec4c640f53f2bcdee11dc332358b77d7 Mon Sep 17 00:00:00 2001 From: Kamil Dudka Date: Fri, 21 Dec 2018 14:26:01 +0100 Subject: [PATCH 4/5] ckpem: keep objid as number to avoid use of atol() Fixes #2 Upstream-commit: 188c63f7b824e256d20b70ee080ea88d96d8e9c6 Signed-off-by: Kamil Dudka --- src/ckpem.h | 1 + src/pinst.c | 5 +++-- src/pobject.c | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/ckpem.h b/src/ckpem.h index 5d05087..e2f18cf 100644 --- a/src/ckpem.h +++ b/src/ckpem.h @@ -194,6 +194,7 @@ struct pemInternalObjectStr { CK_OBJECT_CLASS objClass; NSSItem hashKey; NSSItem id; + long objid; unsigned char hashKeyData[128]; SECItem *derCert; char *nickname; diff --git a/src/pinst.c b/src/pinst.c index 87bf7c2..26f3ac2 100644 --- a/src/pinst.c +++ b/src/pinst.c @@ -195,6 +195,7 @@ assignObjectID(pemInternalObject *o, const long objid) return CKR_HOST_MEMORY; memcpy(o->id.data, id, len); + o->objid = objid; return CKR_OK; } @@ -365,7 +366,7 @@ LinkSharedKeyObject(const long oldKeyIdx, const long newKeyIdx) pemInternalObject *obj; list_for_each_entry(obj, &pem_objs, gl_list) { CK_RV rv; - if (atol(obj->id.data) != oldKeyIdx) + if (obj->objid != oldKeyIdx) continue; NSS_ZFreeIf(obj->id.data); @@ -427,7 +428,7 @@ AddObjectIfNeeded(CK_OBJECT_CLASS objClass, LinkSharedKeyObject(pem_nobjs, curObj->arrayIdx); if (CKO_CERTIFICATE == objClass) { - const long ref = atol(curObj->id.data); + const long ref = curObj->objid; if (0 < ref && ref < pem_nobjs && !FindObjectByArrayIdx(ref)) { /* The certificate we are going to reuse refers to an * object that has already been removed. Make it refer diff --git a/src/pobject.c b/src/pobject.c index 9a8321c..2ffffed 100644 --- a/src/pobject.c +++ b/src/pobject.c @@ -1225,7 +1225,7 @@ pem_CreateObject if (curObj->type != pemCert) continue; - if (atol(curObj->id.data) != pem_nobjs) + if (curObj->objid != pem_nobjs) /* not a certificate that refers to the key being added */ continue; -- 2.17.2 From db140f2d9259ac73d59c50a658d444e418b81c3c Mon Sep 17 00:00:00 2001 From: Kamil Dudka Date: Wed, 16 Jan 2019 16:19:50 +0100 Subject: [PATCH 5/5] pfind: fix use of uninitialized var in plog() call Detected by static analysis: Error: UNINIT (CWE-457): nss-pem-1.0.3/src/pfind.c:251: var_decl: Declaring variable "i" without initializer. nss-pem-1.0.3/src/pfind.c:305: uninit_use_in_call: Using uninitialized value "i" when calling "plog". 303| list_for_each_entry(obj, &pem_objs, gl_list) { 304| int match = 1; /* matches type if type not specified */ 305|-> plog(" %d type = %d\n", i, obj->type); 306| if (type != pemAll) { 307| /* type specified - must match given type */ Error: CLANG_WARNING: nss-pem-1.0.3/src/pfind.c:305:9: warning: Function call argument is an uninitialized value nss-pem-1.0.3/src/pfind.c:364:9: note: Assuming 'fwSlot' is not equal to null nss-pem-1.0.3/src/pfind.c:364:5: note: Taking false branch nss-pem-1.0.3/src/pfind.c:370:9: note: Assuming 'arena' is not equal to null nss-pem-1.0.3/src/pfind.c:370:5: note: Taking false branch nss-pem-1.0.3/src/pfind.c:375:9: note: Assuming 'rv' is not equal to null nss-pem-1.0.3/src/pfind.c:375:5: note: Taking false branch nss-pem-1.0.3/src/pfind.c:381:9: note: Assuming 'fo' is not equal to null nss-pem-1.0.3/src/pfind.c:381:5: note: Taking false branch nss-pem-1.0.3/src/pfind.c:395:9: note: Calling 'collect_objects' nss-pem-1.0.3/src/pfind.c:251:5: note: 'i' declared without an initial value nss-pem-1.0.3/src/pfind.c:267:5: note: Control jumps to 'case 0:' at line 293 nss-pem-1.0.3/src/pfind.c:296:9: note: Execution continues on line 303 nss-pem-1.0.3/src/pfind.c:305:9: note: Function call argument is an uninitialized value Error: COMPILER_WARNING: nss-pem-1.0.3/src/pfind.c: scope_hint: In function 'pem_FindObjectsInit' nss-pem-1.0.3/src/pfind.c:305:13: warning: 'i' may be used uninitialized in this function [-Wmaybe-uninitialized] nss-pem-1.0.3/src/pfind.c:251:14: note: 'i' was declared here Upstream-commit: 5c05ed268675c7544b1a51198e1e9e566be17608 Signed-off-by: Kamil Dudka --- src/pfind.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/pfind.c b/src/pfind.c index 958c0d5..83d5b89 100644 --- a/src/pfind.c +++ b/src/pfind.c @@ -248,7 +248,6 @@ collect_objects(CK_ATTRIBUTE_PTR pTemplate, pemInternalObject *** result_array, CK_RV * pError, CK_SLOT_ID slotID) { - PRUint32 i; size_t result_array_entries = 0; size_t result_array_capacity = 0; pemObjectType type = pemRaw; @@ -302,7 +301,7 @@ collect_objects(CK_ATTRIBUTE_PTR pTemplate, /* find objects */ list_for_each_entry(obj, &pem_objs, gl_list) { int match = 1; /* matches type if type not specified */ - plog(" %d type = %d\n", i, obj->type); + plog(" %ld type = %d\n", obj->arrayIdx, obj->type); if (type != pemAll) { /* type specified - must match given type */ match = (type == obj->type); -- 2.17.2