From 31eddc5b70ff2158102af6c72849c3a500c4d002 Mon Sep 17 00:00:00 2001 From: Kamil Dudka Date: Fri, 3 Aug 2018 15:05:22 +0200 Subject: [PATCH 1/5] pem_CreateObject: rewrite the conditions in the cert search loop ... to make the code more readable. No changes in behavior are intended by this commit. Upstream-commit: 1d51c2337bc7156e3dfd7a8087ac4328f71174e3 Signed-off-by: Kamil Dudka --- src/pobject.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/src/pobject.c b/src/pobject.c index e8891d0..c8539f2 100644 --- a/src/pobject.c +++ b/src/pobject.c @@ -1209,7 +1209,6 @@ pem_CreateObject goto loser; } } else if (objClass == CKO_PRIVATE_KEY) { - /* Brute force: find the id of the certificate, if any, in this slot */ int i; SECItem certDER; CK_SESSION_HANDLE hSession; @@ -1222,24 +1221,28 @@ pem_CreateObject certDER.len = 0; /* in case there is no equivalent cert */ certDER.data = NULL; + /* Brute force: find the id of the certificate, if any, in this slot */ objid = -1; for (i = 0; i < pem_nobjs; i++) { if (NULL == pem_objs[i]) continue; - if ((slotID == pem_objs[i]->slotID) && (pem_objs[i]->type == pemCert)) { - objid = atoi(pem_objs[i]->id.data); - certDER.data = - (void *) NSS_ZAlloc(NULL, pem_objs[i]->derCert->len); + if (slotID != pem_objs[i]->slotID) + continue; - if (certDER.data == NULL) - goto loser; + if (pem_objs[i]->type != pemCert) + continue; - certDER.len = pem_objs[i]->derCert->len; - memcpy(certDER.data, - pem_objs[i]->derCert->data, - pem_objs[i]->derCert->len); - } + objid = atoi(pem_objs[i]->id.data); + certDER.data = NSS_ZAlloc(NULL, pem_objs[i]->derCert->len); + + if (certDER.data == NULL) + goto loser; + + certDER.len = pem_objs[i]->derCert->len; + memcpy(certDER.data, + pem_objs[i]->derCert->data, + pem_objs[i]->derCert->len); } /* We're just adding a key, we'll assume the cert is next */ -- 2.17.1 From 476e1a7db2b35146db6c1fb5c0cd230f84778583 Mon Sep 17 00:00:00 2001 From: Kamil Dudka Date: Fri, 3 Aug 2018 15:09:01 +0200 Subject: [PATCH 2/5] pem_CreateObject: fix memory leak in the cert search loop If we search the array in reverse direction and stop on the first match, we end up with the same results but without leaking certDER.data in each iteration after the first match. Upstream-commit: e85b6f903fa38a34672428be114741d397f17fe2 Signed-off-by: Kamil Dudka --- src/pobject.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/pobject.c b/src/pobject.c index c8539f2..34d0b5a 100644 --- a/src/pobject.c +++ b/src/pobject.c @@ -1223,7 +1223,7 @@ pem_CreateObject /* Brute force: find the id of the certificate, if any, in this slot */ objid = -1; - for (i = 0; i < pem_nobjs; i++) { + for (i = pem_nobjs - 1; 0 <= i; i--) { if (NULL == pem_objs[i]) continue; @@ -1243,6 +1243,7 @@ pem_CreateObject memcpy(certDER.data, pem_objs[i]->derCert->data, pem_objs[i]->derCert->len); + break; } /* We're just adding a key, we'll assume the cert is next */ -- 2.17.1 From dddc9331e35520b772b499475f5f2a67eeac8fef Mon Sep 17 00:00:00 2001 From: Kamil Dudka Date: Fri, 3 Aug 2018 15:12:46 +0200 Subject: [PATCH 3/5] pem_CreateObject: check object ID in the cert search loop We need to find a certificate that refers to the key being added, which is not necessarily the last certificate added to the slot. Bug: https://bugzilla.redhat.com/1610998 Upstream-commit: 5e6d9ce0d638eb6c9f25f31366960db2f8031716 Signed-off-by: Kamil Dudka --- src/pobject.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/pobject.c b/src/pobject.c index 34d0b5a..918b327 100644 --- a/src/pobject.c +++ b/src/pobject.c @@ -1233,7 +1233,11 @@ pem_CreateObject if (pem_objs[i]->type != pemCert) continue; - objid = atoi(pem_objs[i]->id.data); + if (atoi(pem_objs[i]->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); if (certDER.data == NULL) -- 2.17.1 From ed88392c385d7a8733891fb736e9b7456331b617 Mon Sep 17 00:00:00 2001 From: Kamil Dudka Date: Fri, 3 Aug 2018 15:22:23 +0200 Subject: [PATCH 4/5] AddObjectIfNeeded: use a pointer to avoid code duplication No changes in behavior intended by this commit. Upstream-commit: 0eafa24fdf0b54e5a63a76a7c63dc4000253c971 Signed-off-by: Kamil Dudka --- src/pinst.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/pinst.c b/src/pinst.c index 5ac0ff3..c54cbe2 100644 --- a/src/pinst.c +++ b/src/pinst.c @@ -401,16 +401,17 @@ AddObjectIfNeeded(CK_OBJECT_CLASS objClass, /* first look for the object in pem_objs, it might be already there */ for (i = 0; i < pem_nobjs; i++) { - if (NULL == pem_objs[i]) + pemInternalObject *const curObj = pem_objs[i]; + if (NULL == curObj) continue; /* Comparing DER encodings is dependable and frees the PEM module * from having to require clients to provide unique nicknames. */ - if ((pem_objs[i]->objClass == objClass) - && (pem_objs[i]->type == type) - && (pem_objs[i]->slotID == slotID) - && derEncodingsMatch(objClass, pem_objs[i], certDER, keyDER)) { + if ((curObj->objClass == objClass) + && (curObj->type == type) + && (curObj->slotID == slotID) + && derEncodingsMatch(objClass, curObj, certDER, keyDER)) { /* While adding a client certificate we (wrongly?) assumed that the * key object will follow right after the cert object. However, if @@ -420,8 +421,8 @@ AddObjectIfNeeded(CK_OBJECT_CLASS objClass, LinkSharedKeyObject(pem_nobjs, i); plog("AddObjectIfNeeded: re-using internal object #%i\n", i); - pem_objs[i]->refCount ++; - return pem_objs[i]; + curObj->refCount ++; + return curObj; } } -- 2.17.1 From 9f0b5facee73afe5578e98010128a72236e6c370 Mon Sep 17 00:00:00 2001 From: Kamil Dudka Date: Fri, 3 Aug 2018 16:00:50 +0200 Subject: [PATCH 5/5] AddObjectIfNeeded: update object ID while reusing a certificate ... in case it refers to an object that has already been removed Bug: https://bugzilla.redhat.com/1610998 Upstream-commit: e14465a1238dcf7364dc07a1438d24111ccad3b1 Signed-off-by: Kamil Dudka --- src/pinst.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/pinst.c b/src/pinst.c index c54cbe2..25485b1 100644 --- a/src/pinst.c +++ b/src/pinst.c @@ -420,6 +420,18 @@ AddObjectIfNeeded(CK_OBJECT_CLASS objClass, */ LinkSharedKeyObject(pem_nobjs, i); + if (CKO_CERTIFICATE == objClass) { + const int ref = atoi(curObj->id.data); + if (0 < ref && ref < pem_nobjs && !pem_objs[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). + */ + NSS_ZFreeIf(curObj->id.data); + assignObjectID(curObj, pem_nobjs); + } + } + plog("AddObjectIfNeeded: re-using internal object #%i\n", i); curObj->refCount ++; return curObj; -- 2.17.1