From 31eddc5b70ff2158102af6c72849c3a500c4d002 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
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 <kdudka@redhat.com>
---
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;
PRBool added;
@@ -1221,24 +1220,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 <kdudka@redhat.com>
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 <kdudka@redhat.com>
---
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
@@ -1222,7 +1222,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;
@@ -1242,6 +1242,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 <kdudka@redhat.com>
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 <kdudka@redhat.com>
---
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
@@ -1232,7 +1232,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 <kdudka@redhat.com>
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 <kdudka@redhat.com>
---
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
@@ -402,16 +402,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
@@ -421,8 +422,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 <kdudka@redhat.com>
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 <kdudka@redhat.com>
---
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
@@ -421,6 +421,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