Bug 1383824 - set slot->isLoggedIn after setting password, r=ttaubert

Differential Revision: https://nss-review.dev.mozaws.net/D391

3 files changed, 99 insertions, 10 deletions

diff --git a/cpputil/scoped_ptrs.h b/cpputil/scoped_ptrs.h
index 9f224dfc0..b92b8132b 100644
--- a/cpputil/scoped_ptrs.h
+++ b/cpputil/scoped_ptrs.h
@@ -36,6 +36,7 @@ struct ScopedDelete {
   void operator()(PK11URI* uri) { PK11URI_DestroyURI(uri); }
   void operator()(PLArenaPool* arena) { PORT_FreeArena(arena, PR_FALSE); }
   void operator()(PK11Context* context) { PK11_DestroyContext(context, true); }
+  void operator()(PK11GenericObject* obj) { PK11_DestroyGenericObject(obj); }
 };
 
 template <class T>
@@ -66,6 +67,7 @@ SCOPED(SECKEYPrivateKeyList);
 SCOPED(PK11URI);
 SCOPED(PLArenaPool);
 SCOPED(PK11Context);
+SCOPED(PK11GenericObject);
 
 #undef SCOPED
 
diff --git a/gtests/softoken_gtest/softoken_gtest.cc b/gtests/softoken_gtest/softoken_gtest.cc
index 2cddaed50..fc82f4185 100644
--- a/gtests/softoken_gtest/softoken_gtest.cc
+++ b/gtests/softoken_gtest/softoken_gtest.cc
@@ -3,6 +3,7 @@
 #include "nspr.h"
 #include "nss.h"
 #include "pk11pub.h"
+#include "secerr.h"
 
 #include "scoped_ptrs.h"
 
@@ -25,24 +26,24 @@ namespace nss_test {
 // this implementation.
 class ScopedUniqueDirectory {
  public:
-  explicit ScopedUniqueDirectory(const std::string& prefix);
+  explicit ScopedUniqueDirectory(const std::string &prefix);
 
   // NB: the directory must be empty upon destruction
   ~ScopedUniqueDirectory() { assert(rmdir(mPath.c_str()) == 0); }
 
-  const std::string& GetPath() { return mPath; }
+  const std::string &GetPath() { return mPath; }
 
  private:
   static const int RETRY_LIMIT = 5;
-  static void GenerateRandomName(/*in/out*/ std::string& prefix);
-  static bool TryMakingDirectory(/*in/out*/ std::string& prefix);
+  static void GenerateRandomName(/*in/out*/ std::string &prefix);
+  static bool TryMakingDirectory(/*in/out*/ std::string &prefix);
 
   std::string mPath;
 };
 
-ScopedUniqueDirectory::ScopedUniqueDirectory(const std::string& prefix) {
+ScopedUniqueDirectory::ScopedUniqueDirectory(const std::string &prefix) {
   std::string path;
-  const char* workingDirectory = PR_GetEnvSecure("NSS_GTEST_WORKDIR");
+  const char *workingDirectory = PR_GetEnvSecure("NSS_GTEST_WORKDIR");
   if (workingDirectory) {
     path.assign(workingDirectory);
   }
@@ -59,7 +60,7 @@ ScopedUniqueDirectory::ScopedUniqueDirectory(const std::string& prefix) {
   assert(mPath.length() > 0);
 }
 
-void ScopedUniqueDirectory::GenerateRandomName(std::string& prefix) {
+void ScopedUniqueDirectory::GenerateRandomName(std::string &prefix) {
   std::stringstream ss;
   ss << prefix;
   // RAND_MAX is at least 32767.
@@ -69,7 +70,7 @@ void ScopedUniqueDirectory::GenerateRandomName(std::string& prefix) {
   ss >> prefix;
 }
 
-bool ScopedUniqueDirectory::TryMakingDirectory(std::string& prefix) {
+bool ScopedUniqueDirectory::TryMakingDirectory(std::string &prefix) {
   GenerateRandomName(prefix);
 #if defined(_WIN32)
   return _mkdir(prefix.c_str()) == 0;
@@ -91,7 +92,7 @@ class SoftokenTest : public ::testing::Test {
 
   virtual void TearDown() {
     ASSERT_EQ(SECSuccess, NSS_Shutdown());
-    const std::string& nssDBDirPath = mNSSDBDir.GetPath();
+    const std::string &nssDBDirPath = mNSSDBDir.GetPath();
     ASSERT_EQ(0, unlink((nssDBDirPath + "/cert9.db").c_str()));
     ASSERT_EQ(0, unlink((nssDBDirPath + "/key4.db").c_str()));
     ASSERT_EQ(0, unlink((nssDBDirPath + "/pkcs11.txt").c_str()));
@@ -116,9 +117,92 @@ TEST_F(SoftokenTest, ResetSoftokenNonEmptyPassword) {
   EXPECT_EQ(SECSuccess, PK11_InitPin(slot.get(), nullptr, "password2"));
 }
 
+// Test certificate to use in the CreateObject tests.
+static const CK_OBJECT_CLASS cko_nss_trust = CKO_NSS_TRUST;
+static const CK_BBOOL ck_false = CK_FALSE;
+static const CK_BBOOL ck_true = CK_TRUE;
+static const CK_TRUST ckt_nss_must_verify_trust = CKT_NSS_MUST_VERIFY_TRUST;
+static const CK_TRUST ckt_nss_trusted_delegator = CKT_NSS_TRUSTED_DELEGATOR;
+static const CK_ATTRIBUTE attributes[] = {
+    {CKA_CLASS, (void *)&cko_nss_trust, (PRUint32)sizeof(CK_OBJECT_CLASS)},
+    {CKA_TOKEN, (void *)&ck_true, (PRUint32)sizeof(CK_BBOOL)},
+    {CKA_PRIVATE, (void *)&ck_false, (PRUint32)sizeof(CK_BBOOL)},
+    {CKA_MODIFIABLE, (void *)&ck_false, (PRUint32)sizeof(CK_BBOOL)},
+    {CKA_LABEL,
+     (void *)"Symantec Class 2 Public Primary Certification Authority - G4",
+     (PRUint32)61},
+    {CKA_CERT_SHA1_HASH,
+     (void *)"\147\044\220\056\110\001\260\042\226\100\020\106\264\261\147\054"
+             "\251\165\375\053",
+     (PRUint32)20},
+    {CKA_CERT_MD5_HASH,
+     (void *)"\160\325\060\361\332\224\227\324\327\164\337\276\355\150\336\226",
+     (PRUint32)16},
+    {CKA_ISSUER,
+     (void *)"\060\201\224\061\013\060\011\006\003\125\004\006\023\002\125\123"
+             "\061\035\060\033\006\003\125\004\012\023\024\123\171\155\141\156"
+             "\164\145\143\040\103\157\162\160\157\162\141\164\151\157\156\061"
+             "\037\060\035\006\003\125\004\013\023\026\123\171\155\141\156\164"
+             "\145\143\040\124\162\165\163\164\040\116\145\164\167\157\162\153"
+             "\061\105\060\103\006\003\125\004\003\023\074\123\171\155\141\156"
+             "\164\145\143\040\103\154\141\163\163\040\062\040\120\165\142\154"
+             "\151\143\040\120\162\151\155\141\162\171\040\103\145\162\164\151"
+             "\146\151\143\141\164\151\157\156\040\101\165\164\150\157\162\151"
+             "\164\171\040\055\040\107\064",
+     (PRUint32)151},
+    {CKA_SERIAL_NUMBER,
+     (void *)"\002\020\064\027\145\022\100\073\267\126\200\055\200\313\171\125"
+             "\246\036",
+     (PRUint32)18},
+    {CKA_TRUST_SERVER_AUTH, (void *)&ckt_nss_must_verify_trust,
+     (PRUint32)sizeof(CK_TRUST)},
+    {CKA_TRUST_EMAIL_PROTECTION, (void *)&ckt_nss_trusted_delegator,
+     (PRUint32)sizeof(CK_TRUST)},
+    {CKA_TRUST_CODE_SIGNING, (void *)&ckt_nss_must_verify_trust,
+     (PRUint32)sizeof(CK_TRUST)},
+    {CKA_TRUST_STEP_UP_APPROVED, (void *)&ck_false,
+     (PRUint32)sizeof(CK_BBOOL)}};
+
+TEST_F(SoftokenTest, CreateObjectNonEmptyPassword) {
+  ScopedPK11SlotInfo slot(PK11_GetInternalKeySlot());
+  ASSERT_TRUE(slot);
+  EXPECT_EQ(SECSuccess, PK11_InitPin(slot.get(), nullptr, "password"));
+  EXPECT_EQ(SECSuccess, PK11_Logout(slot.get()));
+  ScopedPK11GenericObject obj(PK11_CreateGenericObject(
+      slot.get(), attributes, PR_ARRAY_SIZE(attributes), true));
+  EXPECT_EQ(nullptr, obj);
+}
+
+TEST_F(SoftokenTest, CreateObjectChangePassword) {
+  ScopedPK11SlotInfo slot(PK11_GetInternalKeySlot());
+  ASSERT_TRUE(slot);
+  EXPECT_EQ(SECSuccess, PK11_InitPin(slot.get(), nullptr, nullptr));
+  EXPECT_EQ(SECSuccess, PK11_ChangePW(slot.get(), "", "password"));
+  EXPECT_EQ(SECSuccess, PK11_Logout(slot.get()));
+  ScopedPK11GenericObject obj(PK11_CreateGenericObject(
+      slot.get(), attributes, PR_ARRAY_SIZE(attributes), true));
+  EXPECT_EQ(nullptr, obj);
+}
+
+TEST_F(SoftokenTest, CreateObjectChangeToEmptyPassword) {
+  ScopedPK11SlotInfo slot(PK11_GetInternalKeySlot());
+  ASSERT_TRUE(slot);
+  EXPECT_EQ(SECSuccess, PK11_InitPin(slot.get(), nullptr, "password"));
+  EXPECT_EQ(SECSuccess, PK11_ChangePW(slot.get(), "password", ""));
+  // PK11_Logout returnes an error and SEC_ERROR_TOKEN_NOT_LOGGED_IN if the user
+  // is not "logged in".
+  EXPECT_EQ(SECFailure, PK11_Logout(slot.get()));
+  EXPECT_EQ(SEC_ERROR_TOKEN_NOT_LOGGED_IN, PORT_GetError());
+  ScopedPK11GenericObject obj(PK11_CreateGenericObject(
+      slot.get(), attributes, PR_ARRAY_SIZE(attributes), true));
+  // Because there's no password we can't logout and the operation should have
+  // succeeded.
+  EXPECT_NE(nullptr, obj);
+}
+
 }  // namespace nss_test
 
-int main(int argc, char** argv) {
+int main(int argc, char **argv) {
   ::testing::InitGoogleTest(&argc, argv);
 
   return RUN_ALL_TESTS();
diff --git a/lib/softoken/pkcs11.c b/lib/softoken/pkcs11.c
index 4e940990e..c6d2f79df 100644
--- a/lib/softoken/pkcs11.c
+++ b/lib/softoken/pkcs11.c
@@ -3788,7 +3788,10 @@ NSC_SetPIN(CK_SESSION_HANDLE hSession, CK_CHAR_PTR pOldPin,
 
     /* Now update our local copy of the pin */
     if (rv == SECSuccess) {
+        PZ_Lock(slot->slotLock);
         slot->needLogin = (PRBool)(ulNewLen != 0);
+        slot->isLoggedIn = (PRBool)(sftkdb_PWCached(handle) == SECSuccess);
+        PZ_Unlock(slot->slotLock);
         /* Reset login flags. */
         if (ulNewLen == 0) {
             PRBool tokenRemoved = PR_FALSE;