Bug 1471985, make SECKEY_Get{Public,Private}KeyType consistent on RSA-PSS, r=rrelyea,fkiefer

Summary:
In bug 1413596, we changed SECKEY_GetPrivateKeyType() to return
rsaPssKey, if the private key is restricted to RSA-PSS when importing.
Although the intention of this change was to extend the certutil
output to provide more information about key types, it introduced
inconsistency with the existing code, as SECKEY_GetPublicKeyType()
still returns rsaKey.

This patch partially revert the change and determine the actual
(restricted) key type in a different way, using CERT_GetCertKeyType()
and PK11_GetCertFromPrivateKey().

Reviewers: rrelyea, franziskus

Reviewed By: franziskus

Subscribers: franziskus

Bug #: 1471985

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

7 files changed, 113 insertions, 24 deletions

diff --git a/cmd/certutil/certutil.c b/cmd/certutil/certutil.c
index dbb93c922..058bb6238 100644
--- a/cmd/certutil/certutil.c
+++ b/cmd/certutil/certutil.c
@@ -856,7 +856,7 @@ SECItemToHex(const SECItem *item, char *dst)
 }
 
 static const char *const keyTypeName[] = {
-    "null", "rsa", "dsa", "fortezza", "dh", "kea", "ec", "rsaPss"
+    "null", "rsa", "dsa", "fortezza", "dh", "kea", "ec", "rsaPss", "rsaOaep"
 };
 
 #define MAX_CKA_ID_BIN_LEN 20
@@ -869,6 +869,8 @@ PrintKey(PRFileDesc *out, const char *nickName, int count,
 {
     SECItem *ckaID;
     char ckaIDbuf[MAX_CKA_ID_STR_LEN + 4];
+    CERTCertificate *cert;
+    KeyType keyType;
 
     pwarg = NULL;
     ckaID = PK11_GetLowLevelKeyIDForPrivateKey(key);
@@ -888,8 +890,15 @@ PrintKey(PRFileDesc *out, const char *nickName, int count,
         SECItemToHex(&idItem, ckaIDbuf);
     }
 
+    cert = PK11_GetCertFromPrivateKey(key);
+    if (cert) {
+        keyType = CERT_GetCertKeyType(&cert->subjectPublicKeyInfo);
+        CERT_DestroyCertificate(cert);
+    } else {
+        keyType = key->keyType;
+    }
     PR_fprintf(out, "<%2d> %-8.8s %-42.42s %s\n", count,
-               keyTypeName[key->keyType], ckaIDbuf, nickName);
+               keyTypeName[keyType], ckaIDbuf, nickName);
     SECITEM_ZfreeItem(ckaID, PR_TRUE);
 
     return SECSuccess;
diff --git a/lib/nss/nss.def b/lib/nss/nss.def
index 4f0ade4d0..8a9b3b030 100644
--- a/lib/nss/nss.def
+++ b/lib/nss/nss.def
@@ -1133,3 +1133,9 @@ SEC_CreateSignatureAlgorithmParameters;
 ;+    local:
 ;+       *;
 ;+};
+;+NSS_3.39 { 	# NSS 3.39 release
+;+    global:
+CERT_GetCertKeyType;
+;+    local:
+;+       *;
+;+};
diff --git a/lib/pk11wrap/pk11akey.c b/lib/pk11wrap/pk11akey.c
index 346e473a9..c45901ec3 100644
--- a/lib/pk11wrap/pk11akey.c
+++ b/lib/pk11wrap/pk11akey.c
@@ -804,30 +804,12 @@ PK11_MakePrivKey(PK11SlotInfo *slot, KeyType keyType,
     /* don't know? look it up */
     if (keyType == nullKey) {
         CK_KEY_TYPE pk11Type = CKK_RSA;
-        SECItem info;
 
         pk11Type = PK11_ReadULongAttribute(slot, privID, CKA_KEY_TYPE);
         isTemp = (PRBool)!PK11_HasAttributeSet(slot, privID, CKA_TOKEN, PR_FALSE);
         switch (pk11Type) {
             case CKK_RSA:
                 keyType = rsaKey;
-                /* determine RSA key type from the CKA_PUBLIC_KEY_INFO if present */
-                rv = PK11_ReadAttribute(slot, privID, CKA_PUBLIC_KEY_INFO, NULL, &info);
-                if (rv == SECSuccess) {
-                    CERTSubjectPublicKeyInfo *spki;
-
-                    spki = SECKEY_DecodeDERSubjectPublicKeyInfo(&info);
-                    if (spki) {
-                        SECOidTag tag;
-
-                        tag = SECOID_GetAlgorithmTag(&spki->algorithm);
-                        if (tag == SEC_OID_PKCS1_RSA_PSS_SIGNATURE)
-                            keyType = rsaPssKey;
-                        SECKEY_DestroySubjectPublicKeyInfo(spki);
-                    }
-                    SECITEM_FreeItem(&info, PR_FALSE);
-                }
-
                 break;
             case CKK_DSA:
                 keyType = dsaKey;
diff --git a/tests/cert/TestUser-rsa-pss-interop.p12 b/tests/cert/TestUser-rsa-pss-interop.p12
new file mode 100644
index 000000000..f0e8d24d6
--- /dev/null
+++ b/tests/cert/TestUser-rsa-pss-interop.p12
diff --git a/tests/cert/cert.sh b/tests/cert/cert.sh
index 34006efd1..1fa3e7483 100755
--- a/tests/cert/cert.sh
+++ b/tests/cert/cert.sh
@@ -448,6 +448,27 @@ cert_add_cert()
 	fi
 	cert_log "SUCCESS: $CERTNAME's mixed EC Cert Created"
 
+	echo "Importing RSA-PSS server certificate"
+	pk12u -i ${QADIR}/cert/TestUser-rsa-pss-interop.p12 -k ${R_PWFILE} -w ${R_PWFILE} -d ${PROFILEDIR}
+	# Let's get the key ID of the imported private key.
+	KEYID=`${BINDIR}/certutil -d ${PROFILEDIR} -K -f ${R_PWFILE} | \
+		grep 'TestUser-rsa-pss-interop$' | sed -n 's/^<.*> [^ ]\{1,\} *\([^ ]\{1,\}\).*/\1/p'`
+
+	CU_ACTION="Generate RSA-PSS Cert Request for $CERTNAME"
+	CU_SUBJECT="CN=$CERTNAME, E=${CERTNAME}-rsa-pss@bogus.com, O=BOGUS NSS, L=Mountain View, ST=California, C=US"
+	certu -R -d "${PROFILEDIR}" -k ${KEYID} -f "${R_PWFILE}" \
+	-z "${R_NOISE_FILE}" -o req 2>&1
+
+	CU_ACTION="Sign ${CERTNAME}'s RSA-PSS Request"
+	NEWSERIAL=`expr ${CERTSERIAL} + 30000`
+	certu -C -c "TestCA" -m "$NEWSERIAL" -v 60 -d "${P_R_CADIR}" \
+	      -i req -o "${CERTNAME}-rsa-pss.cert" -f "${R_PWFILE}" "$1" 2>&1
+
+	CU_ACTION="Import $CERTNAME's RSA-PSS Cert -t u,u,u"
+	certu -A -n "$CERTNAME-rsa-pss" -t "u,u,u" -d "${PROFILEDIR}" -f "${R_PWFILE}" \
+	      -i "${CERTNAME}-rsa-pss.cert" 2>&1
+	cert_log "SUCCESS: $CERTNAME's RSA-PSS Cert Created"
+
     return 0
 }
 
diff --git a/tests/ssl/ssl.sh b/tests/ssl/ssl.sh
index 9a63bd997..13e7b80b5 100755
--- a/tests/ssl/ssl.sh
+++ b/tests/ssl/ssl.sh
@@ -211,22 +211,27 @@ start_selfserv()
       echo "$SCRIPTNAME: $testname ----"
   fi
   sparam=`echo $sparam | sed -e 's;_; ;g'`
-  if [ -z "$NO_ECC_CERTS" -o "$NO_ECC_CERTS" != "1"  ] ; then
+  if [ -z "$NO_ECC_CERTS" -o "$NO_ECC_CERTS" != "1" ] ; then
       ECC_OPTIONS="-e ${HOSTADDR}-ecmixed -e ${HOSTADDR}-ec"
   else
       ECC_OPTIONS=""
   fi
+  if [ -z "$RSA_PSS_CERT" -o "$RSA_PSS_CERT" != "1" ] ; then
+      RSA_OPTIONS="-n ${HOSTADDR}"
+  else
+      RSA_OPTIONS="-n ${HOSTADDR}-rsa-pss"
+  fi
   echo "selfserv starting at `date`"
-  echo "selfserv -D -p ${PORT} -d ${P_R_SERVERDIR} -n ${HOSTADDR} ${SERVER_OPTIONS} \\"
+  echo "selfserv -D -p ${PORT} -d ${P_R_SERVERDIR} ${RSA_OPTIONS} ${SERVER_OPTIONS} \\"
   echo "         ${ECC_OPTIONS} -S ${HOSTADDR}-dsa -w nss ${sparam} -i ${R_SERVERPID}\\"
   echo "         -V ssl3:tls1.2 $verbose -H 1 &"
   if [ ${fileout} -eq 1 ]; then
-      ${PROFTOOL} ${BINDIR}/selfserv -D -p ${PORT} -d ${P_R_SERVERDIR} -n ${HOSTADDR} ${SERVER_OPTIONS} \
+      ${PROFTOOL} ${BINDIR}/selfserv -D -p ${PORT} -d ${P_R_SERVERDIR} ${RSA_OPTIONS} ${SERVER_OPTIONS} \
                ${ECC_OPTIONS} -S ${HOSTADDR}-dsa -w nss ${sparam} -i ${R_SERVERPID} -V ssl3:tls1.2 $verbose -H 1 \
                > ${SERVEROUTFILE} 2>&1 &
       RET=$?
   else
-      ${PROFTOOL} ${BINDIR}/selfserv -D -p ${PORT} -d ${P_R_SERVERDIR} -n ${HOSTADDR} ${SERVER_OPTIONS} \
+      ${PROFTOOL} ${BINDIR}/selfserv -D -p ${PORT} -d ${P_R_SERVERDIR} ${RSA_OPTIONS} ${SERVER_OPTIONS} \
                ${ECC_OPTIONS} -S ${HOSTADDR}-dsa -w nss ${sparam} -i ${R_SERVERPID} -V ssl3:tls1.2 $verbose -H 1 &
       RET=$?
   fi
@@ -283,6 +288,13 @@ ssl_cov()
       echo "${testname}" | grep "EXPORT" > /dev/null
       EXP=$?
 
+      # RSA-PSS tests are handled in a separate function
+      case $testname in
+	*RSA-PSS)
+	  continue
+          ;;
+      esac
+
       echo "$SCRIPTNAME: running $testname ----------------------------"
       VMAX="ssl3"
       if [ "$testmax" = "TLS10" ]; then
@@ -313,6 +325,59 @@ ssl_cov()
   html "</TABLE><BR>"
 }
 
+ssl_cov_rsa_pss()
+{
+  #verbose="-v"
+  html_head "SSL Cipher Coverage (RSA-PSS) $NORM_EXT - server $SERVER_MODE/client $CLIENT_MODE"
+
+  testname=""
+  sparam="$CIPHER_SUITES"
+
+  if [ "$NORM_EXT" = "Extended Test" ] ; then
+      echo "$SCRIPTNAME: skipping SSL Cipher Coverage (RSA-PSS) for $NORM_EXT"
+      return 0
+  fi
+
+  RSA_PSS_CERT=1
+  NO_ECC_CERTS=1
+  start_selfserv # Launch the server
+  RSA_PSS_CERT=0
+  NO_ECC_CERTS=0
+
+  VMIN="tls1.2"
+  VMAX="tls1.2"
+
+  ignore_blank_lines ${SSLCOV} | \
+  while read ectype testmax param testname
+  do
+      case $testname in
+	*RSA-PSS)
+          ;;
+	*)
+	  continue
+	  ;;
+      esac
+
+      echo "$SCRIPTNAME: running $testname (RSA-PSS) ----------------------------"
+
+      echo "tstclnt -4 -p ${PORT} -h ${HOSTADDR} -c ${param} -V ${VMIN}:${VMAX} ${CLIENT_OPTIONS} \\"
+      echo "        -f -d ${P_R_CLIENTDIR} $verbose -w nss < ${REQUEST_FILE}"
+
+      rm ${TMP}/$HOST.tmp.$$ 2>/dev/null
+      ${PROFTOOL} ${BINDIR}/tstclnt -4 -p ${PORT} -h ${HOSTADDR} -c ${param} -V ${VMIN}:${VMAX} ${CLIENT_OPTIONS} -f \
+              -d ${P_R_CLIENTDIR} $verbose -w nss < ${REQUEST_FILE} \
+              >${TMP}/$HOST.tmp.$$  2>&1
+      ret=$?
+      cat ${TMP}/$HOST.tmp.$$
+      rm ${TMP}/$HOST.tmp.$$ 2>/dev/null
+      html_msg $ret 0 "${testname}" \
+               "produced a returncode of $ret, expected is 0"
+  done
+
+  kill_selfserv
+  html "</TABLE><BR>"
+}
+
 ############################## ssl_auth ################################
 # local shell function to perform SSL  Client Authentication tests
 ########################################################################
@@ -1152,6 +1217,7 @@ ssl_run()
             ;;
         "cov")
             ssl_cov
+            ssl_cov_rsa_pss
             ;;
         "auth")
             ssl_auth
diff --git a/tests/ssl/sslcov.txt b/tests/ssl/sslcov.txt
index 1eb7f47de..93f247b96 100644
--- a/tests/ssl/sslcov.txt
+++ b/tests/ssl/sslcov.txt
@@ -141,3 +141,8 @@
    ECC   TLS12  :C030 TLS12_ECDHE_RSA_WITH_AES_256_GCM_SHA384
    ECC   TLS12  :CCA8 TLS12_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
    ECC   TLS12  :CCA9 TLS12_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
+#
+# Test against server with RSA-PSS server certificate
+#
+   ECC   TLS12  :C02F TLS12_ECDHE_RSA_WITH_AES_128_GCM_SHA256 - RSA-PSS
+   ECC   TLS12  :C030 TLS12_ECDHE_RSA_WITH_AES_256_GCM_SHA384 - RSA-PSS