Bug 1681099, pk11wrap: Tighten certificate lookup based on PKCS #11 URI, r=kjacobs,rrelyea

Previously we only used the "object" attribute (mapped to CKA_LABEL) to find certificates by PKCS #11 URI. This updates the logic to match also with "id" (mapped to CKA_ID) and reject the request if a "type" attribute is present with the value other than "cert".

Note: as "id" may not be null-terminated, the PKCS #11 URI API had to be revamped to allow binary blobs. This is still not perfect because PK11URIAttribute doesn't have a length field of value.

Differential Revision: https://phabricator.services.mozilla.com/D98940

7 files changed, 353 insertions, 60 deletions

diff --git a/automation/abi-check/expected-report-libnssutil3.so.txt b/automation/abi-check/expected-report-libnssutil3.so.txt
index e69de29bb..ba634d9ab 100644
--- a/automation/abi-check/expected-report-libnssutil3.so.txt
+++ b/automation/abi-check/expected-report-libnssutil3.so.txt
@@ -0,0 +1,6 @@
+
+2 Added functions:
+
+  'function const SECItem* PK11URI_GetPathAttributeItem(PK11URI*, const char*)'    {PK11URI_GetPathAttributeItem@@NSSUTIL_3.82}
+  'function const SECItem* PK11URI_GetQueryAttributeItem(PK11URI*, const char*)'    {PK11URI_GetQueryAttributeItem@@NSSUTIL_3.82}
+
diff --git a/gtests/pk11_gtest/pk11_module_unittest.cc b/gtests/pk11_gtest/pk11_module_unittest.cc
index eb467c3c4..42b59c605 100644
--- a/gtests/pk11_gtest/pk11_module_unittest.cc
+++ b/gtests/pk11_gtest/pk11_module_unittest.cc
@@ -45,7 +45,7 @@ TEST_F(Pkcs11ModuleTest, ListSlots) {
       PK11_GetAllTokens(CKM_INVALID_MECHANISM, PR_FALSE, PR_FALSE, nullptr));
   ASSERT_NE(nullptr, slots);
 
-  PK11SlotListElement* element = PK11_GetFirstSafe(slots.get());
+  PK11SlotListElement *element = PK11_GetFirstSafe(slots.get());
   ASSERT_NE(nullptr, element);
 
   // These tokens are always present.
@@ -81,6 +81,39 @@ TEST_F(Pkcs11ModuleTest, PublicCertificatesToken) {
   EXPECT_TRUE(PK11_IsFriendly(slot2.get()));
 }
 
+TEST_F(Pkcs11ModuleTest, PublicCertificatesTokenLookup) {
+  const std::string kCertUrl =
+      "pkcs11:id=%10%11%12%13%14%15%16%17%18%19%1a%1b%1c%1d%1e%1f";
+
+  ScopedCERTCertList certsByUrl(
+      PK11_FindCertsFromURI(kCertUrl.c_str(), nullptr));
+  EXPECT_NE(nullptr, certsByUrl.get());
+
+  size_t count = 0;
+  CERTCertificate *certByUrl = nullptr;
+  for (CERTCertListNode *node = CERT_LIST_HEAD(certsByUrl);
+       !CERT_LIST_END(node, certsByUrl); node = CERT_LIST_NEXT(node)) {
+    if (count == 0) {
+      certByUrl = node->cert;
+    }
+    count++;
+  }
+  EXPECT_EQ(1UL, count);
+  EXPECT_NE(nullptr, certByUrl);
+
+  EXPECT_EQ(
+      0, strcmp(certByUrl->nickname, "Test PKCS11 Public Certs Token:cert2"));
+}
+
+TEST_F(Pkcs11ModuleTest, PublicCertificatesTokenLookupNoMatch) {
+  const std::string kCertUrl =
+      "pkcs11:id=%00%01%02%03%04%05%06%07%08%09%0a%0b%0c%0d%0e%0e";
+
+  ScopedCERTCertList certsByUrl(
+      PK11_FindCertsFromURI(kCertUrl.c_str(), nullptr));
+  EXPECT_EQ(nullptr, certsByUrl.get());
+}
+
 #if defined(_WIN32)
 #include <windows.h>
 
diff --git a/gtests/pkcs11testmodule/pkcs11testmodule.cpp b/gtests/pkcs11testmodule/pkcs11testmodule.cpp
index dbb574442..4bd558c0d 100644
--- a/gtests/pkcs11testmodule/pkcs11testmodule.cpp
+++ b/gtests/pkcs11testmodule/pkcs11testmodule.cpp
@@ -76,13 +76,79 @@ CK_RV Test_C_GetFunctionList(CK_FUNCTION_LIST_PTR_PTR) { return CKR_OK; }
 
 static int tokenPresent = 0;
 
-// The token in slot 4 has 2 objects. Both of them are profile object
-// and identified by object ID 1 or 2.
+// The token in slot 4 has 4 objects:
+// 1. CKO_PROFILE with CKP_PUBLIC_CERTIFICATES_TOKEN
+// 2. CKO_PROFILE with CKP_BASELINE_PROVIDER
+// 3. CKO_CERTIFICATE with CKA_ID "\x00..\x0f"
+// 4. CKO_CERTIFICATE with CKA_ID "\x10..\x1f"
 static bool readingProfile = false;
 static const CK_PROFILE_ID profiles[] = {CKP_PUBLIC_CERTIFICATES_TOKEN,
                                          CKP_BASELINE_PROVIDER};
 static int profileIndex = 0;
 
+static bool readingCert = false;
+static const unsigned char certId1[] = {
+  0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
+  0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f
+};
+static const char *certLabel1 = "cert1";
+static const unsigned char certId2[] = {
+  0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17,
+  0x18, 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f
+};
+static const char *certLabel2 = "cert2";
+
+static const unsigned char certValue[] = {
+  0x30, 0x82, 0x01, 0x54, 0x30, 0x81, 0xfc, 0xa0, 0x03, 0x02, 0x01, 0x02,
+  0x02, 0x02, 0x0e, 0x42, 0x30, 0x0a, 0x06, 0x08, 0x2a, 0x86, 0x48, 0xce,
+  0x3d, 0x04, 0x03, 0x02, 0x30, 0x15, 0x31, 0x13, 0x30, 0x11, 0x06, 0x03,
+  0x55, 0x04, 0x03, 0x13, 0x0a, 0x45, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65,
+  0x20, 0x43, 0x41, 0x30, 0x1e, 0x17, 0x0d, 0x32, 0x30, 0x31, 0x32, 0x31,
+  0x39, 0x30, 0x39, 0x30, 0x32, 0x34, 0x39, 0x5a, 0x17, 0x0d, 0x32, 0x31,
+  0x30, 0x33, 0x31, 0x39, 0x30, 0x39, 0x30, 0x32, 0x34, 0x39, 0x5a, 0x30,
+  0x15, 0x31, 0x13, 0x30, 0x11, 0x06, 0x03, 0x55, 0x04, 0x03, 0x13, 0x0a,
+  0x45, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x20, 0x43, 0x41, 0x30, 0x59,
+  0x30, 0x13, 0x06, 0x07, 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x02, 0x01, 0x06,
+  0x08, 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x03, 0x01, 0x07, 0x03, 0x42, 0x00,
+  0x04, 0x24, 0xd1, 0x96, 0xcc, 0x72, 0x36, 0xbb, 0xd6, 0x04, 0x36, 0x14,
+  0x59, 0x9a, 0x27, 0x24, 0x6b, 0x03, 0x7c, 0x02, 0x69, 0x68, 0x50, 0x70,
+  0x52, 0xe5, 0x5f, 0xe1, 0xf1, 0xd4, 0x0a, 0x00, 0x18, 0x76, 0x14, 0xa3,
+  0xed, 0x7d, 0xc5, 0x0a, 0xfe, 0xe4, 0x6f, 0x09, 0xf8, 0xcd, 0xe8, 0x5a,
+  0x39, 0x81, 0xf4, 0xcc, 0x25, 0xbe, 0x26, 0x76, 0xe1, 0x23, 0x52, 0x09,
+  0x6f, 0xbd, 0xf1, 0x75, 0xbe, 0xa3, 0x3c, 0x30, 0x3a, 0x30, 0x14, 0x06,
+  0x09, 0x60, 0x86, 0x48, 0x01, 0x86, 0xf8, 0x42, 0x01, 0x01, 0x01, 0x01,
+  0xff, 0x04, 0x04, 0x03, 0x02, 0x02, 0x04, 0x30, 0x12, 0x06, 0x03, 0x55,
+  0x1d, 0x13, 0x01, 0x01, 0xff, 0x04, 0x08, 0x30, 0x06, 0x01, 0x01, 0xff,
+  0x02, 0x01, 0x00, 0x30, 0x0e, 0x06, 0x03, 0x55, 0x1d, 0x0f, 0x01, 0x01,
+  0xff, 0x04, 0x04, 0x03, 0x02, 0x07, 0x80, 0x30, 0x0a, 0x06, 0x08, 0x2a,
+  0x86, 0x48, 0xce, 0x3d, 0x04, 0x03, 0x02, 0x03, 0x47, 0x00, 0x30, 0x44,
+  0x02, 0x20, 0x76, 0x56, 0x09, 0xe9, 0x79, 0xc2, 0x62, 0x28, 0xfc, 0x48,
+  0xf8, 0xac, 0x73, 0xbb, 0xe1, 0xe5, 0x79, 0x93, 0x78, 0x05, 0x4b, 0x45,
+  0x08, 0xcf, 0x10, 0x9f, 0x0d, 0xb9, 0x50, 0x7d, 0x70, 0x24, 0x02, 0x20,
+  0x27, 0x52, 0xe7, 0x9e, 0x42, 0xe3, 0xb2, 0x4d, 0xbb, 0x7d, 0xa3, 0x81,
+  0x5f, 0x7f, 0x0f, 0x3a, 0x55, 0x34, 0xfa, 0x86, 0x35, 0xcb, 0x68, 0x4f,
+  0xad, 0x67, 0x67, 0x05, 0x36, 0xcb, 0x11, 0x4d
+};
+static const unsigned char certSerial[] = {
+  0x02, 0x02, 0x0e, 0x42
+};
+static const unsigned char certIssuer[] = {
+  0x30, 0x15, 0x31, 0x13, 0x30, 0x11, 0x06, 0x03, 0x55, 0x04, 0x03, 0x13,
+  0x0a, 0x45, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x20, 0x43, 0x41
+};
+
+static const struct cert {
+  const unsigned char *id;
+  size_t idLen;
+  const char *label;
+} certs[] = {
+  { certId1, sizeof(certId1), certLabel1 },
+  { certId2, sizeof(certId2), certLabel2 }
+};
+static int certIndex = 0;
+static CK_OBJECT_HANDLE certHandle = CK_INVALID_HANDLE;
+static bool certIdGiven = false;
+
 CK_RV Test_C_GetSlotList(CK_BBOOL limitToTokensPresent,
                          CK_SLOT_ID_PTR pSlotList, CK_ULONG_PTR pulCount) {
   if (!pulCount) {
@@ -316,22 +382,78 @@ CK_RV Test_C_GetAttributeValue(CK_SESSION_HANDLE hSession,
                                CK_OBJECT_HANDLE hObject,
                                CK_ATTRIBUTE_PTR pTemplate, CK_ULONG ulCount) {
   if (hSession == 4) {
-    assert(hObject >= 1 &&
-           hObject - 1 < sizeof(profiles) / sizeof(profiles[0]));
-    for (CK_ULONG count = 0; count < ulCount; count++) {
-      if (pTemplate[count].type == CKA_PROFILE_ID) {
-        if (pTemplate[count].pValue) {
-          assert(pTemplate[count].ulValueLen == sizeof(CK_ULONG));
-          CK_ULONG value = profiles[hObject - 1];
-          memcpy(pTemplate[count].pValue, &value, sizeof(value));
+    switch (hObject) {
+    case 1:
+    case 2:
+      for (CK_ULONG count = 0; count < ulCount; count++) {
+        if (pTemplate[count].type == CKA_PROFILE_ID) {
+          if (pTemplate[count].pValue) {
+            assert(pTemplate[count].ulValueLen == sizeof(CK_ULONG));
+            CK_ULONG value = profiles[hObject - 1];
+            memcpy(pTemplate[count].pValue, &value, sizeof(value));
+          } else {
+            pTemplate[count].ulValueLen = sizeof(CK_ULONG);
+          }
         } else {
-          pTemplate[count].ulValueLen = sizeof(CK_ULONG);
+          pTemplate[count].ulValueLen = (CK_ULONG)-1;
+        }
+      }
+      return CKR_OK;
+    case 3:
+    case 4:
+      for (CK_ULONG count = 0; count < ulCount; count++) {
+        switch (pTemplate[count].type) {
+        case CKA_TOKEN:
+          if (pTemplate[count].pValue) {
+            assert(pTemplate[count].ulValueLen == sizeof(CK_BBOOL));
+            CK_BBOOL value = true;
+            memcpy(pTemplate[count].pValue, &value, sizeof(value));
+          } else {
+            pTemplate[count].ulValueLen = sizeof(CK_BBOOL);
+          }
+          break;
+
+        case CKA_LABEL: {
+          const char *label = certs[hObject - 3].label;
+          size_t labelLen = strlen(label);
+          if (pTemplate[count].pValue) {
+            if (pTemplate[count].ulValueLen >= labelLen) {
+              memcpy(pTemplate[count].pValue, label, labelLen);
+            } else {
+              pTemplate[count].ulValueLen = CK_UNAVAILABLE_INFORMATION;
+            }
+          } else {
+            pTemplate[count].ulValueLen = labelLen;
+          }
+          break;
+        }
+
+#define BYTEARRAY_CASE(label, array) \
+        case label: \
+          if (pTemplate[count].pValue) { \
+            if (pTemplate[count].ulValueLen >= sizeof(array)) { \
+              memcpy(pTemplate[count].pValue, array, sizeof(array)); \
+            } else { \
+              pTemplate[count].ulValueLen = CK_UNAVAILABLE_INFORMATION; \
+            } \
+          } else { \
+            pTemplate[count].ulValueLen = sizeof(array); \
+          } \
+          break;
+
+        BYTEARRAY_CASE(CKA_VALUE, certValue)
+        BYTEARRAY_CASE(CKA_SERIAL_NUMBER, certSerial)
+        BYTEARRAY_CASE(CKA_ISSUER, certIssuer)
+
+        default:
+          pTemplate[count].ulValueLen = CK_UNAVAILABLE_INFORMATION;
+          break;
         }
-      } else {
-        pTemplate[count].ulValueLen = (CK_ULONG)-1;
       }
+      return CKR_OK;
+    default:
+      break;
     }
-    return CKR_OK;
   }
   return CKR_FUNCTION_NOT_SUPPORTED;
 }
@@ -345,19 +467,47 @@ CK_RV Test_C_FindObjectsInit(CK_SESSION_HANDLE hSession,
                              CK_ATTRIBUTE_PTR pTemplate, CK_ULONG ulCount) {
   // Slot 4
   if (hSession == 4) {
+    CK_OBJECT_CLASS objectClass = CKO_DATA;
+    CK_BYTE *id = NULL;
+    CK_ULONG idLen = 0;
     for (CK_ULONG count = 0; count < ulCount; count++) {
       CK_ATTRIBUTE attribute = pTemplate[count];
-      if (attribute.type == CKA_CLASS) {
-        assert(attribute.ulValueLen == sizeof(CK_ULONG));
-
-        CK_ULONG value;
-        memcpy(&value, attribute.pValue, attribute.ulValueLen);
-        if (value == CKO_PROFILE) {
-          readingProfile = true;
-          profileIndex = 0;
-          break;
+      switch (attribute.type) {
+      case CKA_CLASS:
+        assert(attribute.ulValueLen == sizeof(CK_OBJECT_CLASS));
+
+        memcpy(&objectClass, attribute.pValue, attribute.ulValueLen);
+        break;
+      case CKA_ID:
+        id = (CK_BYTE *)attribute.pValue;
+        idLen = attribute.ulValueLen;
+        break;
+      default:
+        break;
+      }
+    }
+
+    switch (objectClass) {
+    case CKO_PROFILE:
+      readingProfile = true;
+      profileIndex = 0;
+      break;
+    case CKO_CERTIFICATE:
+      readingCert = true;
+      certIndex = 0;
+      if (id) {
+        certIdGiven = true;
+        for (size_t count = 0; count < sizeof(certs) / sizeof(certs[0]); count++) {
+          if (certs[count].idLen == idLen &&
+              memcmp(certs[count].id, id, idLen) == 0) {
+            certHandle = count + 3;
+            break;
+          }
         }
       }
+      break;
+    default:
+      break;
     }
   }
   return CKR_OK;
@@ -379,6 +529,30 @@ CK_RV Test_C_FindObjects(CK_SESSION_HANDLE hSession,
     }
     profileIndex += count;
     *pulObjectCount = count;
+  } else if (readingCert) {
+    assert(hSession == 4);
+    if (!certIdGiven) {
+      CK_ULONG count = ulMaxObjectCount;
+      size_t remaining = sizeof(certs) / sizeof(certs[0]) - certIndex;
+      if (count > remaining) {
+        count = remaining;
+      }
+      for (CK_ULONG i = 0; i < count; i++) {
+        phObject[i] = i + 3;
+      }
+      *pulObjectCount = count;
+      certIndex += count;
+    } else if (certHandle != CK_INVALID_HANDLE) {
+      if (certIndex == 0 && ulMaxObjectCount > 0) {
+        phObject[0] = certHandle;
+        *pulObjectCount = 1;
+        certIndex = 1;
+      } else {
+        *pulObjectCount = 0;
+      }
+    } else {
+      *pulObjectCount = 0;
+    }
   } else {
     *pulObjectCount = 0;
   }
@@ -387,6 +561,9 @@ CK_RV Test_C_FindObjects(CK_SESSION_HANDLE hSession,
 
 CK_RV Test_C_FindObjectsFinal(CK_SESSION_HANDLE hSession) {
   readingProfile = false;
+  readingCert = false;
+  certHandle = CK_INVALID_HANDLE;
+  certIdGiven = false;
   return CKR_OK;
 }
 
diff --git a/lib/pk11wrap/pk11cert.c b/lib/pk11wrap/pk11cert.c
index 84d830035..fd36e1979 100644
--- a/lib/pk11wrap/pk11cert.c
+++ b/lib/pk11wrap/pk11cert.c
@@ -616,9 +616,8 @@ transfer_uri_certs_to_collection(nssList *certList, PK11URI *uri,
     PRUint32 i, count;
     NSSToken **tokens, **tp;
     PK11SlotInfo *slot;
-    const char *id;
+    const SECItem *id;
 
-    id = PK11URI_GetPathAttribute(uri, PK11URI_PATTR_ID);
     count = nssList_Count(certList);
     if (count == 0) {
         return;
@@ -627,13 +626,14 @@ transfer_uri_certs_to_collection(nssList *certList, PK11URI *uri,
     if (!certs) {
         return;
     }
+    id = PK11URI_GetPathAttributeItem(uri, PK11URI_PATTR_ID);
     nssList_GetArray(certList, (void **)certs, count);
     for (i = 0; i < count; i++) {
         /*
-	 * Filter the subject matched certs based on the
-	 * CKA_ID from the URI
-	 */
-        if (id && (strlen(id) != certs[i]->id.size ||
+         * Filter the subject matched certs based on the
+         * CKA_ID from the URI
+         */
+        if (id && (id->len != certs[i]->id.size ||
                    memcmp(id, certs[i]->id.data, certs[i]->id.size)))
             continue;
         tokens = nssPKIObject_GetTokens(&certs[i]->object, NULL);
@@ -666,6 +666,14 @@ transfer_uri_certs_to_collection(nssList *certList, PK11URI *uri,
                     continue;
                 }
 
+                value = PK11URI_GetPathAttribute(uri, PK11URI_PATTR_SERIAL);
+                if (value &&
+                    !pk11_MatchString(value,
+                                      (char *)slot->tokenInfo.serialNumber,
+                                      sizeof(slot->tokenInfo.serialNumber))) {
+                    continue;
+                }
+
                 nssPKIObjectCollection_AddObject(collection,
                                                  (nssPKIObject *)certs[i]);
                 break;
@@ -683,7 +691,8 @@ find_certs_from_uri(const char *uriString, void *wincx)
     PK11URI *uri = NULL;
     CK_ATTRIBUTE attributes[10];
     CK_ULONG nattributes = 0;
-    const char *label;
+    const SECItem *id;
+    const char *label, *type;
     PK11SlotInfo *slotinfo;
     nssCryptokiObject **instances;
     PRStatus status;
@@ -710,10 +719,16 @@ find_certs_from_uri(const char *uriString, void *wincx)
         goto loser;
     }
 
+    /* if the "type" attribute is specified its value must be "cert" */
+    type = PK11URI_GetPathAttribute(uri, PK11URI_PATTR_TYPE);
+    if (type && strcmp(type, "cert")) {
+        goto loser;
+    }
+
     label = PK11URI_GetPathAttribute(uri, PK11URI_PATTR_OBJECT);
     if (label) {
         (void)nssTrustDomain_GetCertsForNicknameFromCache(defaultTD,
-                                                          (const char *)label,
+                                                          label,
                                                           certList);
     } else {
         (void)nssTrustDomain_GetCertsFromCache(defaultTD, certList);
@@ -739,6 +754,14 @@ find_certs_from_uri(const char *uriString, void *wincx)
         nattributes++;
     }
 
+    id = PK11URI_GetPathAttributeItem(uri, PK11URI_PATTR_ID);
+    if (id) {
+        attributes[nattributes].type = CKA_ID;
+        attributes[nattributes].pValue = (void *)id->data;
+        attributes[nattributes].ulValueLen = id->len;
+        nattributes++;
+    }
+
     tokens = NSSTrustDomain_FindTokensByURI(defaultTD, uri);
     for (tok = tokens; tok && *tok; tok++) {
         if (nssToken_IsPresent(*tok)) {
diff --git a/lib/util/nssutil.def b/lib/util/nssutil.def
index 2d61f53cd..fa0373074 100644
--- a/lib/util/nssutil.def
+++ b/lib/util/nssutil.def
@@ -341,3 +341,10 @@ NSS_LockPolicy;
 ;+    local:
 ;+       *;
 ;+};
+;+NSSUTIL_3.82 {         # NSS Utilities 3.82 release
+;+    global:
+PK11URI_GetPathAttributeItem;
+PK11URI_GetQueryAttributeItem;
+;+    local:
+;+       *;
+;+};
diff --git a/lib/util/pkcs11uri.c b/lib/util/pkcs11uri.c
index c29521080..e0c4077a3 100644
--- a/lib/util/pkcs11uri.c
+++ b/lib/util/pkcs11uri.c
@@ -47,7 +47,7 @@ static const char *qattr_names[] = {
 
 struct PK11URIBufferStr {
     PLArenaPool *arena;
-    char *data;
+    unsigned char *data;
     size_t size;
     size_t allocated;
 };
@@ -55,7 +55,7 @@ typedef struct PK11URIBufferStr PK11URIBuffer;
 
 struct PK11URIAttributeListEntryStr {
     char *name;
-    char *value;
+    SECItem value;
 };
 typedef struct PK11URIAttributeListEntryStr PK11URIAttributeListEntry;
 
@@ -133,11 +133,11 @@ pk11uri_DestroyBuffer(PK11URIBuffer *buffer)
 
 /* URI encoding functions. */
 static char *
-pk11uri_Escape(PLArenaPool *arena, const char *value, size_t length,
+pk11uri_Escape(PLArenaPool *arena, const unsigned char *value, size_t length,
                const char *available)
 {
     PK11URIBuffer buffer;
-    const char *p;
+    const unsigned char *p;
     unsigned char buf[4];
     char *result = NULL;
     SECStatus ret;
@@ -154,7 +154,7 @@ pk11uri_Escape(PLArenaPool *arena, const char *value, size_t length,
                 goto fail;
             }
         } else {
-            ret = pk11uri_AppendBuffer(&buffer, (const unsigned char *)p, 1);
+            ret = pk11uri_AppendBuffer(&buffer, p, 1);
             if (ret != SECSuccess) {
                 goto fail;
             }
@@ -167,7 +167,7 @@ pk11uri_Escape(PLArenaPool *arena, const char *value, size_t length,
     }
 
     /* Steal the memory allocated in buffer. */
-    result = buffer.data;
+    result = (char *)buffer.data;
     buffer.data = NULL;
 
 fail:
@@ -176,18 +176,18 @@ fail:
     return result;
 }
 
-static char *
-pk11uri_Unescape(PLArenaPool *arena, const char *value, size_t length)
+static unsigned char *
+pk11uri_Unescape(PLArenaPool *arena, const char *value, size_t *length)
 {
     PK11URIBuffer buffer;
     const char *p;
     unsigned char buf[1];
-    char *result = NULL;
+    unsigned char *result = NULL;
     SECStatus ret;
 
     pk11uri_InitBuffer(&buffer, arena);
 
-    for (p = value; p < value + length; p++) {
+    for (p = value; p < value + *length; p++) {
         if (*p == '%') {
             int c;
             size_t i;
@@ -218,6 +218,7 @@ pk11uri_Unescape(PLArenaPool *arena, const char *value, size_t length)
             goto fail;
         }
     }
+    *length = buffer.size;
     buf[0] = '\0';
     ret = pk11uri_AppendBuffer(&buffer, buf, 1);
     if (ret != SECSuccess) {
@@ -277,7 +278,7 @@ pk11uri_CompareQueryAttributeName(const char *a, const char *b)
 
 static SECStatus
 pk11uri_InsertToAttributeList(PK11URIAttributeList *attrs,
-                              char *name, char *value,
+                              char *name, unsigned char *value, size_t size,
                               PK11URIAttributeCompareNameFunc compare_name,
                               PRBool allow_duplicate)
 {
@@ -309,7 +310,9 @@ pk11uri_InsertToAttributeList(PK11URIAttributeList *attrs,
     }
 
     attrs->attrs[i].name = name;
-    attrs->attrs[i].value = value;
+    attrs->attrs[i].value.type = siBuffer;
+    attrs->attrs[i].value.data = value;
+    attrs->attrs[i].value.len = size;
 
     attrs->num_attrs++;
 
@@ -323,7 +326,8 @@ pk11uri_InsertToAttributeListEscaped(PK11URIAttributeList *attrs,
                                      PK11URIAttributeCompareNameFunc compare_name,
                                      PRBool allow_duplicate)
 {
-    char *name_copy = NULL, *value_copy = NULL;
+    char *name_copy = NULL;
+    unsigned char *value_copy = NULL;
     SECStatus ret;
 
     if (attrs->arena) {
@@ -337,13 +341,13 @@ pk11uri_InsertToAttributeListEscaped(PK11URIAttributeList *attrs,
     memcpy(name_copy, name, name_size);
     name_copy[name_size] = '\0';
 
-    value_copy = pk11uri_Unescape(attrs->arena, value, value_size);
+    value_copy = pk11uri_Unescape(attrs->arena, value, &value_size);
     if (value_copy == NULL) {
         goto fail;
     }
 
-    ret = pk11uri_InsertToAttributeList(attrs, name_copy, value_copy, compare_name,
-                                        allow_duplicate);
+    ret = pk11uri_InsertToAttributeList(attrs, name_copy, value_copy, value_size,
+                                        compare_name, allow_duplicate);
     if (ret != SECSuccess) {
         goto fail;
     }
@@ -374,7 +378,7 @@ pk11uri_DestroyAttributeList(PK11URIAttributeList *attrs)
 
         for (i = 0; i < attrs->num_attrs; i++) {
             PORT_Free(attrs->attrs[i].name);
-            PORT_Free(attrs->attrs[i].value);
+            PORT_Free(attrs->attrs[i].value.data);
         }
         PORT_Free(attrs->attrs);
     }
@@ -414,7 +418,7 @@ pk11uri_AppendAttributeListToBuffer(PK11URIBuffer *buffer,
             return ret;
         }
 
-        escaped = pk11uri_Escape(buffer->arena, attr->value, strlen(attr->value),
+        escaped = pk11uri_Escape(buffer->arena, attr->value.data, attr->value.len,
                                  unescaped);
         if (escaped == NULL) {
             return ret;
@@ -510,7 +514,9 @@ pk11uri_InsertAttributes(PK11URIAttributeList *dest_attrs,
         if (j < num_attr_names) {
             /* Named attribute. */
             ret = pk11uri_InsertToAttributeList(dest_attrs,
-                                                name, value,
+                                                name,
+                                                (unsigned char *)value,
+                                                strlen(value),
                                                 compare_name,
                                                 allow_duplicate);
             if (ret != SECSuccess) {
@@ -519,7 +525,9 @@ pk11uri_InsertAttributes(PK11URIAttributeList *dest_attrs,
         } else {
             /* Vendor attribute. */
             ret = pk11uri_InsertToAttributeList(dest_vattrs,
-                                                name, value,
+                                                name,
+                                                (unsigned char *)value,
+                                                strlen(value),
                                                 strcmp,
                                                 vendor_allow_duplicate);
             if (ret != SECSuccess) {
@@ -777,7 +785,7 @@ PK11URI_FormatURI(PLArenaPool *arena, PK11URI *uri)
         goto fail;
     }
 
-    result = buffer.data;
+    result = (char *)buffer.data;
     buffer.data = NULL;
 
 fail:
@@ -798,7 +806,7 @@ PK11URI_DestroyURI(PK11URI *uri)
 }
 
 /* Accessors. */
-static const char *
+static const SECItem *
 pk11uri_GetAttribute(PK11URIAttributeList *attrs,
                      PK11URIAttributeList *vattrs,
                      const char *name)
@@ -807,27 +815,53 @@ pk11uri_GetAttribute(PK11URIAttributeList *attrs,
 
     for (i = 0; i < attrs->num_attrs; i++) {
         if (strcmp(name, attrs->attrs[i].name) == 0) {
-            return attrs->attrs[i].value;
+            return &attrs->attrs[i].value;
         }
     }
 
     for (i = 0; i < vattrs->num_attrs; i++) {
         if (strcmp(name, vattrs->attrs[i].name) == 0) {
-            return vattrs->attrs[i].value;
+            return &vattrs->attrs[i].value;
         }
     }
 
     return NULL;
 }
 
+const SECItem *
+PK11URI_GetPathAttributeItem(PK11URI *uri, const char *name)
+{
+    return pk11uri_GetAttribute(&uri->pattrs, &uri->vpattrs, name);
+}
+
 const char *
 PK11URI_GetPathAttribute(PK11URI *uri, const char *name)
 {
-    return pk11uri_GetAttribute(&uri->pattrs, &uri->vpattrs, name);
+    const SECItem *value;
+
+    value = PK11URI_GetPathAttributeItem(uri, name);
+    if (!value) {
+        return NULL;
+    }
+
+    return (const char *)value->data;
+}
+
+const SECItem *
+PK11URI_GetQueryAttributeItem(PK11URI *uri, const char *name)
+{
+    return pk11uri_GetAttribute(&uri->qattrs, &uri->vqattrs, name);
 }
 
 const char *
 PK11URI_GetQueryAttribute(PK11URI *uri, const char *name)
 {
-    return pk11uri_GetAttribute(&uri->qattrs, &uri->vqattrs, name);
+    const SECItem *value;
+
+    value = PK11URI_GetQueryAttributeItem(uri, name);
+    if (!value) {
+        return NULL;
+    }
+
+    return (const char *)value->data;
 }
diff --git a/lib/util/pkcs11uri.h b/lib/util/pkcs11uri.h
index 662c85470..84fb69f6f 100644
--- a/lib/util/pkcs11uri.h
+++ b/lib/util/pkcs11uri.h
@@ -56,12 +56,25 @@ extern char *PK11URI_FormatURI(PLArenaPool *arena, PK11URI *uri);
 /* Destroy a PK11URI object. */
 extern void PK11URI_DestroyURI(PK11URI *uri);
 
-/* Retrieve a path attribute with the given name. */
+/* Retrieve a path attribute with the given name. This function can be used only
+ * when we can assume that the attribute value is a string (such as "label" or
+ * "type"). If it can be a binary blob (such as "id"), use
+ * PK11URI_GetPathAttributeItem.
+ */
 extern const char *PK11URI_GetPathAttribute(PK11URI *uri, const char *name);
 
-/* Retrieve a query attribute with the given name. */
+/* Retrieve a query attribute with the given name. This function can be used
+ * only when we can assume that the attribute value is a string (such as
+ * "module-name").  If it can be a binary blob, use
+ * PK11URI_GetQueryAttributeItem.*/
 extern const char *PK11URI_GetQueryAttribute(PK11URI *uri, const char *name);
 
+/* Retrieve a path attribute with the given name as a SECItem. */
+extern const SECItem *PK11URI_GetPathAttributeItem(PK11URI *uri, const char *name);
+
+/* Retrieve a query attribute with the given name as a SECItem. */
+extern const SECItem *PK11URI_GetQueryAttributeItem(PK11URI *uri, const char *name);
+
 SEC_END_PROTOS
 
 #endif /* _PKCS11URI_H_ */