Bug 1592007 - land NSS d64102b76a43 UPGRADE_NSS_RELEASE, r=kjacobs
authorJ.C. Jones <jc@mozilla.com>
Mon, 25 Nov 2019 23:48:46 +0000
changeset 503791 8a2358762789286d20bdd549796094d9d18d2aa1
parent 503790 383a117b7df0244a8ff5191c0cda6ab7d6faa83d
child 503792 e2b25d4ceb3fead5cd8695741553fb8c426a26b3
push id36847
push userncsoregi@mozilla.com
push dateTue, 26 Nov 2019 09:34:48 +0000
treeherdermozilla-central@7bc0df30a8a9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskjacobs
bugs1592007, 1590001, 1596450, 1455952, 1522203, 1592557
milestone72.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1592007 - land NSS d64102b76a43 UPGRADE_NSS_RELEASE, r=kjacobs 2019-11-20 Kevin Jacobs <kjacobs@mozilla.com> * lib/ssl/ssl3con.c, lib/ssl/tls13con.c: Bug 1590001 - Prevent negotiation of versions lower than 1.3 after HelloRetryRequest. r=mt This patch prevents negotiation of TLS versions lower than 1.3 after an HRR has been sent. [d64102b76a43] [tip] 2019-11-22 J.C. Jones <jjones@mozilla.com> * lib/softoken/pkcs11u.c: Bug 1596450 - Fixup, coverity CID 1455952 r=kjacobs [46b1355d8765] * lib/pk11wrap/pk11slot.c: Bug 1522203 - Remove Pentium Pro workaround for PK11_GetAllTokens r=kjacobs The comment indicated the wasted effort was to work around a cache issue on the Pentium Pro. I think it has served its purpose. [27d9fb4ac69b] 2019-11-21 Franziskus Kiefer <franziskuskiefer@gmail.com> * tests/gtests/gtests.sh: Bug 1592557 - fix prng kat tests, r=jcj fix for prng kat tests [474334bb790b] 2019-11-20 Robert Relyea <rrelyea@redhat.com> * lib/softoken/pkcs11c.c, lib/softoken/pkcs11i.h, lib/softoken/sftkhmac.c: Bug 1596450 - softoken: unified MAC implementation patch by Alex Scheel review by rrelyea [3147585149f0] Differential Revision: https://phabricator.services.mozilla.com/D54637
security/nss/TAG-INFO
security/nss/coreconf/coreconf.dep
security/nss/lib/pk11wrap/pk11slot.c
security/nss/lib/softoken/pkcs11c.c
security/nss/lib/softoken/pkcs11i.h
security/nss/lib/softoken/pkcs11u.c
security/nss/lib/softoken/sftkhmac.c
security/nss/lib/ssl/ssl3con.c
security/nss/lib/ssl/tls13con.c
security/nss/tests/gtests/gtests.sh
--- a/security/nss/TAG-INFO
+++ b/security/nss/TAG-INFO
@@ -1,1 +1,1 @@
-1e22a0c93afe
\ No newline at end of file
+d64102b76a43
\ No newline at end of file
--- a/security/nss/coreconf/coreconf.dep
+++ b/security/nss/coreconf/coreconf.dep
@@ -5,8 +5,9 @@
 
 /*
  * A dummy header file that is a dependency for all the object files.
  * Used to force a full recompilation of NSS in Mozilla's Tinderbox
  * depend builds.  See comments in rules.mk.
  */
 
 #error "Do not include this header file."
+
--- a/security/nss/lib/pk11wrap/pk11slot.c
+++ b/security/nss/lib/pk11wrap/pk11slot.c
@@ -2172,20 +2172,16 @@ PK11_GetAllTokens(CK_MECHANISM_TYPE type
 {
     PK11SlotList *list;
     PK11SlotList *loginList;
     PK11SlotList *friendlyList;
     SECMODModuleList *mlp;
     SECMODModuleList *modules;
     SECMODListLock *moduleLock;
     int i;
-#if defined(XP_WIN32)
-    int j = 0;
-    PRInt32 waste[16];
-#endif
 
     moduleLock = SECMOD_GetDefaultModuleListLock();
     if (!moduleLock) {
         PORT_SetError(SEC_ERROR_NOT_INITIALIZED);
         return NULL;
     }
 
     list = PK11_NewSlotList();
@@ -2200,28 +2196,16 @@ PK11_GetAllTokens(CK_MECHANISM_TYPE type
             PK11_FreeSlotList(friendlyList);
         return NULL;
     }
 
     SECMOD_GetReadLock(moduleLock);
 
     modules = SECMOD_GetDefaultModuleList();
     for (mlp = modules; mlp != NULL; mlp = mlp->next) {
-
-#if defined(XP_WIN32)
-        /* This is works around some horrible cache/page thrashing problems
-        ** on Win32.  Without this, this loop can take up to 6 seconds at
-        ** 100% CPU on a Pentium-Pro 200.  The thing this changes is to
-        ** increase the size of the stack frame and modify it.
-        ** Moving the loop code itself seems to have no effect.
-        ** Dunno why this combination makes a difference, but it does.
-        */
-        waste[j & 0xf] = j++;
-#endif
-
         for (i = 0; i < mlp->module->slotCount; i++) {
             PK11SlotInfo *slot = mlp->module->slots[i];
 
             if (pk11_IsPresentCertLoad(slot, loadCerts)) {
                 if (needRW && slot->readOnly)
                     continue;
                 if ((type == CKM_INVALID_MECHANISM) || PK11_DoesMechanism(slot, type)) {
                     if (pk11_LoginStillRequired(slot, wincx)) {
--- a/security/nss/lib/softoken/pkcs11c.c
+++ b/security/nss/lib/softoken/pkcs11c.c
@@ -1999,141 +1999,64 @@ sftk_SignCopy(
 static SECStatus
 sftk_HMACCmp(CK_ULONG *copyLen, unsigned char *sig, unsigned int sigLen,
              unsigned char *hash, unsigned int hashLen)
 {
     return (NSS_SecureMemcmp(sig, hash, *copyLen) == 0) ? SECSuccess : SECFailure;
 }
 
 /*
- * common HMAC initalization routine
+ * common HMAC + CMAC initialization routine
  */
 static CK_RV
-sftk_doHMACInit(SFTKSessionContext *context, HASH_HashType hash,
-                SFTKObject *key, CK_ULONG mac_size)
+sftk_doMACInit(CK_MECHANISM_TYPE mech, SFTKSessionContext *session,
+               SFTKObject *key, CK_ULONG mac_size)
 {
-    SFTKAttribute *keyval;
-    HMACContext *HMACcontext;
+    CK_RV crv;
+    sftk_MACCtx *context;
     CK_ULONG *intpointer;
-    const SECHashObject *hashObj = HASH_GetRawHashObject(hash);
     PRBool isFIPS = (key->slot->slotID == FIPS_SLOT_ID);
 
-    /* required by FIPS 198 Section 4 */
-    if (isFIPS && (mac_size < 4 || mac_size < hashObj->length / 2)) {
+    /* Set up the initial context. */
+    crv = sftk_MAC_Create(mech, key, &context);
+    if (crv != CKR_OK) {
+        return crv;
+    }
+
+    session->hashInfo = context;
+    session->multi = PR_TRUE;
+
+    /* Required by FIPS 198 Section 4. Delay this check until after the MAC
+     * has been initialized to steal the output size of the MAC. */
+    if (isFIPS && (mac_size < 4 || mac_size < context->mac_size / 2)) {
+        sftk_MAC_Destroy(context, PR_TRUE);
         return CKR_BUFFER_TOO_SMALL;
     }
 
-    keyval = sftk_FindAttribute(key, CKA_VALUE);
-    if (keyval == NULL)
-        return CKR_KEY_SIZE_RANGE;
-
-    HMACcontext = HMAC_Create(hashObj,
-                              (const unsigned char *)keyval->attrib.pValue,
-                              keyval->attrib.ulValueLen, isFIPS);
-    context->hashInfo = HMACcontext;
-    context->multi = PR_TRUE;
-    sftk_FreeAttribute(keyval);
-    if (context->hashInfo == NULL) {
-        if (PORT_GetError() == SEC_ERROR_INVALID_ARGS) {
-            return CKR_KEY_SIZE_RANGE;
-        }
-        return CKR_HOST_MEMORY;
-    }
-    context->hashUpdate = (SFTKHash)HMAC_Update;
-    context->end = (SFTKEnd)HMAC_Finish;
-
-    context->hashdestroy = (SFTKDestroy)HMAC_Destroy;
-    intpointer = PORT_New(CK_ULONG);
-    if (intpointer == NULL) {
-        return CKR_HOST_MEMORY;
-    }
-    *intpointer = mac_size;
-    context->cipherInfo = intpointer;
-    context->destroy = (SFTKDestroy)sftk_Space;
-    context->update = (SFTKCipher)sftk_SignCopy;
-    context->verify = (SFTKVerify)sftk_HMACCmp;
-    context->maxLen = hashObj->length;
-    HMAC_Begin(HMACcontext);
-    return CKR_OK;
-}
-
-/*
- * common CMAC initialization routine
- */
-static CK_RV
-sftk_doCMACInit(SFTKSessionContext *session, CMACCipher type,
-                SFTKObject *key, CK_ULONG mac_size)
-{
-    SFTKAttribute *keyval;
-    CMACContext *cmacContext;
-    CK_ULONG *intpointer;
-
-    /* Unlike HMAC, CMAC doesn't need to check key sizes as the underlying
-     * block cipher does this for us: block ciphers support only a single
-     * key size per variant.
-     *
-     * To introduce support for a CMAC based on a new block cipher, first add
-     * support for the relevant block cipher to CMAC in the freebl layer. Then
-     * update the switch statement at the end of this function. Also remember
-     * to update the switch statement in NSC_SignInit with the PKCS#11
-     * mechanism constants.
-     */
-
-    keyval = sftk_FindAttribute(key, CKA_VALUE);
-    if (keyval == NULL) {
-        return CKR_KEY_SIZE_RANGE;
-    }
-
-    /* Create the underlying CMACContext and associate it with the
-     * SFTKSessionContext's hashInfo field */
-    cmacContext = CMAC_Create(type,
-                              (const unsigned char *)keyval->attrib.pValue,
-                              keyval->attrib.ulValueLen);
-    sftk_FreeAttribute(keyval);
-
-    if (cmacContext == NULL) {
-        if (PORT_GetError() == SEC_ERROR_INVALID_ARGS) {
-            return CKR_KEY_SIZE_RANGE;
-        }
-
-        return CKR_HOST_MEMORY;
-    }
-    session->hashInfo = cmacContext;
-
-    /* MACs all behave roughly the same. However, CMAC can fail because
-     * the underlying cipher can fail. In practice, this shouldn't occur
-     * because we're not using any chaining modes, letting us safely ignore
-     * the return value. */
-    session->multi = PR_TRUE;
-    session->hashUpdate = (SFTKHash)CMAC_Update;
-    session->end = (SFTKEnd)CMAC_Finish;
-    session->hashdestroy = (SFTKDestroy)CMAC_Destroy;
+    /* Configure our helper functions appropriately. Note that these casts
+     * ignore the return values. */
+    session->hashUpdate = (SFTKHash)sftk_MAC_Update;
+    session->end = (SFTKEnd)sftk_MAC_Finish;
+    session->hashdestroy = (SFTKDestroy)sftk_MAC_Destroy;
 
     intpointer = PORT_New(CK_ULONG);
     if (intpointer == NULL) {
+        sftk_MAC_Destroy(context, PR_TRUE);
         return CKR_HOST_MEMORY;
     }
     *intpointer = mac_size;
     session->cipherInfo = intpointer;
 
     /* Since we're only "hashing", copy the result from session->end to the
      * caller using sftk_SignCopy. */
     session->update = (SFTKCipher)sftk_SignCopy;
     session->verify = (SFTKVerify)sftk_HMACCmp;
     session->destroy = (SFTKDestroy)sftk_Space;
 
-    /* Will need to be updated for additional block ciphers in the future. */
-    switch (type) {
-        case CMAC_AES:
-            session->maxLen = AES_BLOCK_SIZE;
-            break;
-        default:
-            PORT_Assert(0);
-            return CKR_KEY_SIZE_RANGE;
-    }
+    session->maxLen = context->mac_size;
 
     return CKR_OK;
 }
 
 /*
  *  SSL Macing support. SSL Macs are inited, then update with the base
  * hashing algorithm, then finalized in sign and verify
  */
@@ -2874,59 +2797,60 @@ NSC_SignInit(CK_SESSION_HANDLE hSession,
             }
             context->cipherInfo = privKey;
             context->update = (SFTKCipher)nsc_ECDSASignStub;
             context->destroy = (privKey == key->objectInfo) ? (SFTKDestroy)sftk_Null : (SFTKDestroy)sftk_FreePrivKey;
             context->maxLen = MAX_ECKEY_LEN * 2;
 
             break;
 
-#define INIT_HMAC_MECH(mmm)                                               \
-    case CKM_##mmm##_HMAC_GENERAL:                                        \
-        PORT_Assert(pMechanism->pParameter);                              \
-        if (!pMechanism->pParameter) {                                    \
-            crv = CKR_MECHANISM_PARAM_INVALID;                            \
-            break;                                                        \
-        }                                                                 \
-        crv = sftk_doHMACInit(context, HASH_Alg##mmm, key,                \
-                              *(CK_ULONG *)pMechanism->pParameter);       \
-        break;                                                            \
-    case CKM_##mmm##_HMAC:                                                \
-        crv = sftk_doHMACInit(context, HASH_Alg##mmm, key, mmm##_LENGTH); \
+#define INIT_HMAC_MECH(mmm)                                        \
+    case CKM_##mmm##_HMAC_GENERAL:                                 \
+        PORT_Assert(pMechanism->pParameter);                       \
+        if (!pMechanism->pParameter) {                             \
+            crv = CKR_MECHANISM_PARAM_INVALID;                     \
+            break;                                                 \
+        }                                                          \
+        crv = sftk_doMACInit(pMechanism->mechanism, context, key,  \
+                             *(CK_ULONG *)pMechanism->pParameter); \
+        break;                                                     \
+    case CKM_##mmm##_HMAC:                                         \
+        crv = sftk_doMACInit(pMechanism->mechanism, context, key,  \
+                             mmm##_LENGTH);                        \
         break;
 
             INIT_HMAC_MECH(MD2)
             INIT_HMAC_MECH(MD5)
             INIT_HMAC_MECH(SHA224)
             INIT_HMAC_MECH(SHA256)
             INIT_HMAC_MECH(SHA384)
             INIT_HMAC_MECH(SHA512)
 
         case CKM_SHA_1_HMAC_GENERAL:
             PORT_Assert(pMechanism->pParameter);
             if (!pMechanism->pParameter || pMechanism->ulParameterLen != sizeof(CK_MAC_GENERAL_PARAMS)) {
                 crv = CKR_MECHANISM_PARAM_INVALID;
                 break;
             }
-            crv = sftk_doHMACInit(context, HASH_AlgSHA1, key,
-                                  *(CK_ULONG *)pMechanism->pParameter);
+            crv = sftk_doMACInit(pMechanism->mechanism, context, key,
+                                 *(CK_ULONG *)pMechanism->pParameter);
             break;
         case CKM_SHA_1_HMAC:
-            crv = sftk_doHMACInit(context, HASH_AlgSHA1, key, SHA1_LENGTH);
+            crv = sftk_doMACInit(pMechanism->mechanism, context, key, SHA1_LENGTH);
             break;
         case CKM_AES_CMAC_GENERAL:
             PORT_Assert(pMechanism->pParameter);
             if (!pMechanism->pParameter || pMechanism->ulParameterLen != sizeof(CK_MAC_GENERAL_PARAMS)) {
                 crv = CKR_MECHANISM_PARAM_INVALID;
                 break;
             }
-            crv = sftk_doCMACInit(context, CMAC_AES, key, *(CK_ULONG *)pMechanism->pParameter);
+            crv = sftk_doMACInit(pMechanism->mechanism, context, key, *(CK_ULONG *)pMechanism->pParameter);
             break;
         case CKM_AES_CMAC:
-            crv = sftk_doCMACInit(context, CMAC_AES, key, AES_BLOCK_SIZE);
+            crv = sftk_doMACInit(pMechanism->mechanism, context, key, AES_BLOCK_SIZE);
             break;
         case CKM_SSL3_MD5_MAC:
             PORT_Assert(pMechanism->pParameter);
             if (!pMechanism->pParameter) {
                 crv = CKR_MECHANISM_PARAM_INVALID;
                 break;
             }
             crv = sftk_doSSLMACInit(context, SEC_OID_MD5, key,
@@ -3591,21 +3515,21 @@ NSC_VerifyInit(CK_SESSION_HANDLE hSessio
             INIT_HMAC_MECH(SHA512)
 
         case CKM_SHA_1_HMAC_GENERAL:
             PORT_Assert(pMechanism->pParameter);
             if (!pMechanism->pParameter) {
                 crv = CKR_MECHANISM_PARAM_INVALID;
                 break;
             }
-            crv = sftk_doHMACInit(context, HASH_AlgSHA1, key,
-                                  *(CK_ULONG *)pMechanism->pParameter);
+            crv = sftk_doMACInit(pMechanism->mechanism, context, key,
+                                 *(CK_ULONG *)pMechanism->pParameter);
             break;
         case CKM_SHA_1_HMAC:
-            crv = sftk_doHMACInit(context, HASH_AlgSHA1, key, SHA1_LENGTH);
+            crv = sftk_doMACInit(pMechanism->mechanism, context, key, SHA1_LENGTH);
             break;
 
         case CKM_SSL3_MD5_MAC:
             PORT_Assert(pMechanism->pParameter);
             if (!pMechanism->pParameter) {
                 crv = CKR_MECHANISM_PARAM_INVALID;
                 break;
             }
--- a/security/nss/lib/softoken/pkcs11i.h
+++ b/security/nss/lib/softoken/pkcs11i.h
@@ -12,16 +12,19 @@
 #include "secoidt.h"
 #include "lowkeyti.h"
 #include "pkcs11t.h"
 
 #include "sftkdbt.h"
 #include "chacha20poly1305.h"
 #include "hasht.h"
 
+#include "alghmac.h"
+#include "cmac.h"
+
 /*
  * Configuration Defines
  *
  * The following defines affect the space verse speed trade offs of
  * the PKCS #11 module. For the most part the current settings are optimized
  * for web servers, where we want faster speed and lower lock contention at
  * the expense of space.
  */
@@ -590,16 +593,83 @@ typedef struct sftk_parametersStr {
     sftk_token_parameters *tokens;
     int token_count;
 } sftk_parameters;
 
 /* path stuff (was machine dependent) used by dbinit.c and pk11db.c */
 #define CERT_DB_FMT "%scert%s.db"
 #define KEY_DB_FMT "%skey%s.db"
 
+struct sftk_MACConstantTimeCtxStr {
+    const SECHashObject *hash;
+    unsigned char mac[64];
+    unsigned char secret[64];
+    unsigned int headerLength;
+    unsigned int secretLength;
+    unsigned int totalLength;
+    unsigned char header[75];
+};
+typedef struct sftk_MACConstantTimeCtxStr sftk_MACConstantTimeCtx;
+
+struct sftk_MACCtxStr {
+    /* This is a common MAC context that supports both HMAC and CMAC
+     * operations. This also presents a unified set of semantics:
+     *
+     *  - Everything except Destroy returns a CK_RV, indicating success
+     *    or failure. (This handles the difference between HMAC's and CMAC's
+     *    interfaces, since the underlying AES _might_ fail with CMAC).
+     *
+     *  - The underlying MAC is started on Init(...), so Update(...) can
+     *    called right away. (This handles the difference between HMAC and
+     *    CMAC in their *_Init(...) functions).
+     *
+     *  - Calling semantics:
+     *
+     *      - One of sftk_MAC_{Create,Init,InitRaw}(...) to set up the MAC
+     *        context, checking the return code.
+     *      - sftk_MAC_Update(...) as many times as necessary to process
+     *        input data, checking the return code.
+     *      - sftk_MAC_Finish(...) to get the output of the MAC; result_len
+     *        may be NULL if the caller knows the expected output length,
+     *        checking the return code. If result_len is NULL, this will
+     *        PR_ASSERT(...) that the actual returned length was equal to
+     *        max_result_len.
+     *
+     *        Note: unlike HMAC_Finish(...), this allows the caller to specify
+     *        a return value less than return length, to align with
+     *        CMAC_Finish(...)'s semantics. This will force an additional
+     *        stack allocation of size SFTK_MAX_MAC_LENGTH.
+     *      - sftk_MAC_Reset(...) if the caller wishes to compute a new MAC
+     *        with the same key, checking the return code.
+     *      - sftk_MAC_Destroy(...) when the caller frees its associated
+     *        memory, passing PR_TRUE if sftk_MAC_Create(...) was called,
+     *        and PR_FALSE otherwise.
+     */
+
+    CK_MECHANISM_TYPE mech;
+    unsigned int mac_size;
+
+    union {
+        HMACContext *hmac;
+        CMACContext *cmac;
+
+        /* Functions to update when adding a new MAC or a new hash:
+         *
+         *  - sftk_MAC_Init
+         *  - sftk_MAC_Update
+         *  - sftk_MAC_Finish
+         *  - sftk_MAC_Reset
+         */
+        void *raw;
+    } mac;
+
+    void (*destroy_func)(void *ctx, PRBool free_it);
+};
+typedef struct sftk_MACCtxStr sftk_MACCtx;
+
 SEC_BEGIN_PROTOS
 
 /* shared functions between pkcs11.c and fipstokn.c */
 extern PRBool nsf_init;
 extern CK_RV nsc_CommonInitialize(CK_VOID_PTR pReserved, PRBool isFIPS);
 extern CK_RV nsc_CommonFinalize(CK_VOID_PTR pReserved, PRBool isFIPS);
 extern PRBool sftk_ForkReset(CK_VOID_PTR pReserved, CK_RV *crv);
 extern CK_RV nsc_CommonGetSlotList(CK_BBOOL tokPresent,
@@ -761,27 +831,16 @@ extern CK_RV jpake_Round1(HASH_HashType 
 extern CK_RV jpake_Round2(HASH_HashType hashType,
                           CK_NSS_JPAKERound2Params *params,
                           SFTKObject *sourceKey, SFTKObject *key);
 extern CK_RV jpake_Final(HASH_HashType hashType,
                          const CK_NSS_JPAKEFinalParams *params,
                          SFTKObject *sourceKey, SFTKObject *key);
 
 /* Constant time MAC functions (hmacct.c) */
-
-struct sftk_MACConstantTimeCtxStr {
-    const SECHashObject *hash;
-    unsigned char mac[64];
-    unsigned char secret[64];
-    unsigned int headerLength;
-    unsigned int secretLength;
-    unsigned int totalLength;
-    unsigned char header[75];
-};
-typedef struct sftk_MACConstantTimeCtxStr sftk_MACConstantTimeCtx;
 sftk_MACConstantTimeCtx *sftk_HMACConstantTime_New(
     CK_MECHANISM_PTR mech, SFTKObject *key);
 sftk_MACConstantTimeCtx *sftk_SSLv3MACConstantTime_New(
     CK_MECHANISM_PTR mech, SFTKObject *key);
 void sftk_HMACConstantTime_Update(void *pctx, const void *data, unsigned int len);
 void sftk_SSLv3MACConstantTime_Update(void *pctx, const void *data, unsigned int len);
 void sftk_MACConstantTime_EndHash(
     void *pctx, void *out, unsigned int *outLength, unsigned int maxLength);
@@ -793,11 +852,21 @@ void sftk_MACConstantTime_DestroyContext
 
 extern CK_RV
 sftk_TLSPRFInit(SFTKSessionContext *context,
                 SFTKObject *key,
                 CK_KEY_TYPE key_type,
                 HASH_HashType hash_alg,
                 unsigned int out_len);
 
+/* PKCS#11 MAC implementation. See sftk_MACCtxStr declaration above for
+ * calling semantics for these functions. */
+CK_RV sftk_MAC_Create(CK_MECHANISM_TYPE mech, SFTKObject *key, sftk_MACCtx **ret_ctx);
+CK_RV sftk_MAC_Init(sftk_MACCtx *ctx, CK_MECHANISM_TYPE mech, SFTKObject *key);
+CK_RV sftk_MAC_InitRaw(sftk_MACCtx *ctx, CK_MECHANISM_TYPE mech, const unsigned char *key, unsigned int key_len, PRBool isFIPS);
+CK_RV sftk_MAC_Update(sftk_MACCtx *ctx, CK_BYTE_PTR data, unsigned int data_len);
+CK_RV sftk_MAC_Finish(sftk_MACCtx *ctx, CK_BYTE_PTR result, unsigned int *result_len, unsigned int max_result_len);
+CK_RV sftk_MAC_Reset(sftk_MACCtx *ctx);
+void sftk_MAC_Destroy(sftk_MACCtx *ctx, PRBool free_it);
+
 SEC_END_PROTOS
 
 #endif /* _PKCS11I_H_ */
--- a/security/nss/lib/softoken/pkcs11u.c
+++ b/security/nss/lib/softoken/pkcs11u.c
@@ -136,17 +136,17 @@ sftk_DestroyAttribute(SFTKAttribute *att
 }
 
 /*
  * release a reference to an attribute structure
  */
 void
 sftk_FreeAttribute(SFTKAttribute *attribute)
 {
-    if (attribute->freeAttr) {
+    if (attribute && attribute->freeAttr) {
         sftk_DestroyAttribute(attribute);
         return;
     }
 }
 
 static SFTKAttribute *
 sftk_FindTokenAttribute(SFTKTokenObject *object, CK_ATTRIBUTE_TYPE type)
 {
--- a/security/nss/lib/softoken/sftkhmac.c
+++ b/security/nss/lib/softoken/sftkhmac.c
@@ -4,22 +4,24 @@
 
 #include "seccomon.h"
 #include "secerr.h"
 #include "blapi.h"
 #include "pkcs11i.h"
 #include "softoken.h"
 #include "hmacct.h"
 
-/* MACMechanismToHash converts a PKCS#11 MAC mechanism into a freebl hash
+/* HMACMechanismToHash converts a PKCS#11 MAC mechanism into a freebl hash
  * type. */
 static HASH_HashType
-MACMechanismToHash(CK_MECHANISM_TYPE mech)
+HMACMechanismToHash(CK_MECHANISM_TYPE mech)
 {
     switch (mech) {
+        case CKM_MD2_HMAC:
+            return HASH_AlgMD2;
         case CKM_MD5_HMAC:
         case CKM_SSL3_MD5_MAC:
             return HASH_AlgMD5;
         case CKM_SHA_1_HMAC:
         case CKM_SSL3_SHA1_MAC:
             return HASH_AlgSHA1;
         case CKM_SHA224_HMAC:
             return HASH_AlgSHA224;
@@ -43,17 +45,17 @@ SetupMAC(CK_MECHANISM_PTR mech, SFTKObje
     SFTKAttribute *keyval;
     unsigned char secret[sizeof(ctx->secret)];
     unsigned int secretLength;
 
     if (mech->ulParameterLen != sizeof(CK_NSS_MAC_CONSTANT_TIME_PARAMS)) {
         return NULL;
     }
 
-    alg = MACMechanismToHash(params->macAlg);
+    alg = HMACMechanismToHash(params->macAlg);
     if (alg == HASH_AlgNULL) {
         return NULL;
     }
 
     keyval = sftk_FindAttribute(key, CKA_VALUE);
     if (keyval == NULL) {
         return NULL;
     }
@@ -183,8 +185,289 @@ sftk_MACConstantTime_EndHash(void *pctx,
     }
 }
 
 void
 sftk_MACConstantTime_DestroyContext(void *pctx, PRBool free)
 {
     PORT_Free(pctx);
 }
+
+CK_RV
+sftk_MAC_Create(CK_MECHANISM_TYPE mech, SFTKObject *key, sftk_MACCtx **ret_ctx)
+{
+    CK_RV ret;
+
+    if (ret_ctx == NULL || key == NULL) {
+        return CKR_HOST_MEMORY;
+    }
+
+    *ret_ctx = PORT_New(sftk_MACCtx);
+    if (*ret_ctx == NULL) {
+        return CKR_HOST_MEMORY;
+    }
+
+    ret = sftk_MAC_Init(*ret_ctx, mech, key);
+    if (ret != CKR_OK) {
+        sftk_MAC_Destroy(*ret_ctx, PR_TRUE);
+    }
+
+    return ret;
+}
+
+CK_RV
+sftk_MAC_Init(sftk_MACCtx *ctx, CK_MECHANISM_TYPE mech, SFTKObject *key)
+{
+    SFTKAttribute *keyval = NULL;
+    PRBool isFIPS = (key->slot->slotID == FIPS_SLOT_ID);
+    CK_RV ret = CKR_OK;
+
+    /* Find the actual value of the key. */
+    keyval = sftk_FindAttribute(key, CKA_VALUE);
+    if (keyval == NULL) {
+        ret = CKR_KEY_SIZE_RANGE;
+        goto done;
+    }
+
+    ret = sftk_MAC_InitRaw(ctx, mech,
+                           (const unsigned char *)keyval->attrib.pValue,
+                           keyval->attrib.ulValueLen, isFIPS);
+
+done:
+    sftk_FreeAttribute(keyval);
+    return ret;
+}
+
+CK_RV
+sftk_MAC_InitRaw(sftk_MACCtx *ctx, CK_MECHANISM_TYPE mech, const unsigned char *key, unsigned int key_len, PRBool isFIPS)
+{
+    const SECHashObject *hashObj = NULL;
+    CK_RV ret = CKR_OK;
+
+    if (ctx == NULL) {
+        return CKR_HOST_MEMORY;
+    }
+
+    /* Clear the context before use. */
+    PORT_Memset(ctx, 0, sizeof(*ctx));
+
+    /* Save the mech. */
+    ctx->mech = mech;
+
+    /* Initialize the correct MAC context. */
+    switch (mech) {
+        case CKM_MD2_HMAC:
+        case CKM_MD5_HMAC:
+        case CKM_SHA_1_HMAC:
+        case CKM_SHA224_HMAC:
+        case CKM_SHA256_HMAC:
+        case CKM_SHA384_HMAC:
+        case CKM_SHA512_HMAC:
+            hashObj = HASH_GetRawHashObject(HMACMechanismToHash(mech));
+
+            /* Because we condition above only on hashes we know to be valid,
+             * hashObj should never be NULL. This assert is only useful when
+             * adding a new hash function (for which only partial support has
+             * been added); thus there is no need to turn it into an if and
+             * avoid the NULL dereference on the following line. */
+            PR_ASSERT(hashObj != NULL);
+            ctx->mac_size = hashObj->length;
+
+            goto hmac;
+        case CKM_AES_CMAC:
+            ctx->mac.cmac = CMAC_Create(CMAC_AES, key, key_len);
+            ctx->destroy_func = (void (*)(void *, PRBool))(&CMAC_Destroy);
+
+            /* Copy the behavior of sftk_doCMACInit here. */
+            if (ctx->mac.cmac == NULL) {
+                if (PORT_GetError() == SEC_ERROR_INVALID_ARGS) {
+                    ret = CKR_KEY_SIZE_RANGE;
+                    goto done;
+                }
+
+                ret = CKR_HOST_MEMORY;
+                goto done;
+            }
+
+            ctx->mac_size = AES_BLOCK_SIZE;
+
+            goto done;
+        default:
+            ret = CKR_MECHANISM_PARAM_INVALID;
+            goto done;
+    }
+
+hmac:
+    ctx->mac.hmac = HMAC_Create(hashObj, key, key_len, isFIPS);
+    ctx->destroy_func = (void (*)(void *, PRBool))(&HMAC_Destroy);
+
+    /* Copy the behavior of sftk_doHMACInit here. */
+    if (ctx->mac.hmac == NULL) {
+        if (PORT_GetError() == SEC_ERROR_INVALID_ARGS) {
+            ret = CKR_KEY_SIZE_RANGE;
+            goto done;
+        }
+        ret = CKR_HOST_MEMORY;
+        goto done;
+    }
+
+    /* Semantics: HMAC and CMAC should behave the same. Begin HMAC now. */
+    HMAC_Begin(ctx->mac.hmac);
+
+done:
+    /* Handle a failure: ctx->mac.raw should be NULL, but make sure
+     * destroy_func isn't set. */
+    if (ret != CKR_OK) {
+        ctx->destroy_func = NULL;
+    }
+
+    return ret;
+}
+
+CK_RV
+sftk_MAC_Reset(sftk_MACCtx *ctx)
+{
+    /* Useful for resetting the state of MAC prior to calling update again
+     *
+     * This lets the caller keep a single MAC instance and re-use it as long
+     * as the key stays the same. */
+    switch (ctx->mech) {
+        case CKM_MD2_HMAC:
+        case CKM_MD5_HMAC:
+        case CKM_SHA_1_HMAC:
+        case CKM_SHA224_HMAC:
+        case CKM_SHA256_HMAC:
+        case CKM_SHA384_HMAC:
+        case CKM_SHA512_HMAC:
+            HMAC_Begin(ctx->mac.hmac);
+            break;
+        case CKM_AES_CMAC:
+            if (CMAC_Begin(ctx->mac.cmac) != SECSuccess) {
+                return CKR_FUNCTION_FAILED;
+            }
+            break;
+        default:
+            /* This shouldn't happen -- asserting indicates partial support
+             * for a new MAC type. */
+            PR_ASSERT(PR_FALSE);
+            return CKR_FUNCTION_FAILED;
+    }
+
+    return CKR_OK;
+}
+
+CK_RV
+sftk_MAC_Update(sftk_MACCtx *ctx, CK_BYTE_PTR data, unsigned int data_len)
+{
+    switch (ctx->mech) {
+        case CKM_MD2_HMAC:
+        case CKM_MD5_HMAC:
+        case CKM_SHA_1_HMAC:
+        case CKM_SHA224_HMAC:
+        case CKM_SHA256_HMAC:
+        case CKM_SHA384_HMAC:
+        case CKM_SHA512_HMAC:
+            /* HMAC doesn't indicate failure in the return code. */
+            HMAC_Update(ctx->mac.hmac, data, data_len);
+            break;
+        case CKM_AES_CMAC:
+            /* CMAC indicates failure in the return code, however this is
+             * unlikely to occur. */
+            if (CMAC_Update(ctx->mac.cmac, data, data_len) != SECSuccess) {
+                return CKR_FUNCTION_FAILED;
+            }
+            break;
+        default:
+            /* This shouldn't happen -- asserting indicates partial support
+             * for a new MAC type. */
+            PR_ASSERT(PR_FALSE);
+            return CKR_FUNCTION_FAILED;
+    }
+    return CKR_OK;
+}
+
+CK_RV
+sftk_MAC_Finish(sftk_MACCtx *ctx, CK_BYTE_PTR result, unsigned int *result_len, unsigned int max_result_len)
+{
+    unsigned int actual_result_len;
+
+    switch (ctx->mech) {
+        case CKM_MD2_HMAC:
+        case CKM_MD5_HMAC:
+        case CKM_SHA_1_HMAC:
+        case CKM_SHA224_HMAC:
+        case CKM_SHA256_HMAC:
+        case CKM_SHA384_HMAC:
+        case CKM_SHA512_HMAC:
+            /* HMAC doesn't indicate failure in the return code. Additionally,
+             * unlike CMAC, it doesn't support partial results. This means that we
+             * need to allocate a buffer if max_result_len < ctx->mac_size. */
+            if (max_result_len >= ctx->mac_size) {
+                /* Split this into two calls to avoid an unnecessary stack
+                 * allocation and memcpy when possible. */
+                HMAC_Finish(ctx->mac.hmac, result, &actual_result_len, max_result_len);
+            } else {
+                uint8_t tmp_buffer[SFTK_MAX_MAC_LENGTH];
+
+                /* Assumption: buffer is large enough to hold this HMAC's
+                 * output. */
+                PR_ASSERT(SFTK_MAX_MAC_LENGTH >= ctx->mac_size);
+
+                HMAC_Finish(ctx->mac.hmac, tmp_buffer, &actual_result_len, SFTK_MAX_MAC_LENGTH);
+
+                if (actual_result_len > max_result_len) {
+                    /* This should always be true since:
+                     *
+                     *   (SFTK_MAX_MAC_LENGTH >= ctx->mac_size =
+                     *       actual_result_len) > max_result_len,
+                     *
+                     * but guard this truncation just in case. */
+                    actual_result_len = max_result_len;
+                }
+
+                PORT_Memcpy(result, tmp_buffer, actual_result_len);
+            }
+            break;
+        case CKM_AES_CMAC:
+            /* CMAC indicates failure in the return code, however this is
+             * unlikely to occur. */
+            if (CMAC_Finish(ctx->mac.cmac, result, &actual_result_len, max_result_len) != SECSuccess) {
+                return CKR_FUNCTION_FAILED;
+            }
+            break;
+        default:
+            /* This shouldn't happen -- asserting indicates partial support
+             * for a new MAC type. */
+            PR_ASSERT(PR_FALSE);
+            return CKR_FUNCTION_FAILED;
+    }
+
+    if (result_len) {
+        /* When result length is passed, inform the caller of its value. */
+        *result_len = actual_result_len;
+    } else if (max_result_len == ctx->mac_size) {
+        /* Validate that the amount requested was what was actually given; the
+         * caller assumes that what they passed was the output size of the
+         * underlying MAC and that they got all the bytes the asked for. */
+        PR_ASSERT(actual_result_len == max_result_len);
+    }
+
+    return CKR_OK;
+}
+
+void
+sftk_MAC_Destroy(sftk_MACCtx *ctx, PRBool free_it)
+{
+    if (ctx == NULL) {
+        return;
+    }
+
+    if (ctx->mac.raw != NULL && ctx->destroy_func != NULL) {
+        ctx->destroy_func(ctx->mac.raw, PR_TRUE);
+    }
+
+    /* Clean up the struct so we don't double free accidentally. */
+    PORT_Memset(ctx, 0, sizeof(sftk_MACCtx));
+
+    if (free_it == PR_TRUE) {
+        PORT_Free(ctx);
+    }
+}
--- a/security/nss/lib/ssl/ssl3con.c
+++ b/security/nss/lib/ssl/ssl3con.c
@@ -1097,16 +1097,22 @@ ssl_SetSpecVersions(sslSocket *ss, ssl3C
  * enabled version rather than the peer's selected version.
  */
 SECStatus
 ssl3_NegotiateVersion(sslSocket *ss, SSL3ProtocolVersion peerVersion,
                       PRBool allowLargerPeerVersion)
 {
     SSL3ProtocolVersion negotiated;
 
+    /* Prevent negotiating to a lower version in response to a TLS 1.3 HRR. */
+    if (ss->ssl3.hs.helloRetry) {
+        PORT_SetError(SSL_ERROR_UNSUPPORTED_VERSION);
+        return SECFailure;
+    }
+
     if (SSL_ALL_VERSIONS_DISABLED(&ss->vrange)) {
         PORT_SetError(SSL_ERROR_SSL_DISABLED);
         return SECFailure;
     }
 
     if (peerVersion < ss->vrange.min ||
         (peerVersion > ss->vrange.max && !allowLargerPeerVersion)) {
         PORT_SetError(SSL_ERROR_UNSUPPORTED_VERSION);
@@ -8667,56 +8673,79 @@ ssl3_HandleClientHello(sslSocket *ss, PR
                                    PR_TRUE);
         if (rv != SECSuccess) {
             desc = (version > SSL_LIBRARY_VERSION_3_0) ? protocol_version
                                                        : handshake_failure;
             errCode = SSL_ERROR_UNSUPPORTED_VERSION;
             goto alert_loser;
         }
     }
-
-    if (ss->firstHsDone && ss->version >= SSL_LIBRARY_VERSION_TLS_1_3) {
-        desc = unexpected_message;
-        errCode = SSL_ERROR_RENEGOTIATION_NOT_ALLOWED;
-        goto alert_loser;
-    }
-
-    isTLS13 = ss->version >= SSL_LIBRARY_VERSION_TLS_1_3;
     ss->ssl3.hs.preliminaryInfo |= ssl_preinfo_version;
+
     /* Update the write spec to match the selected version. */
     if (!ss->firstHsDone) {
         ssl_GetSpecWriteLock(ss);
         ssl_SetSpecVersions(ss, ss->ssl3.cwSpec);
         ssl_ReleaseSpecWriteLock(ss);
     }
 
-    if (isTLS13 && sidBytes.len > 0 && !IS_DTLS(ss)) {
-        SECITEM_FreeItem(&ss->ssl3.hs.fakeSid, PR_FALSE);
-        rv = SECITEM_CopyItem(NULL, &ss->ssl3.hs.fakeSid, &sidBytes);
-        if (rv != SECSuccess) {
-            desc = internal_error;
-            errCode = PORT_GetError();
+    isTLS13 = ss->version >= SSL_LIBRARY_VERSION_TLS_1_3;
+    if (isTLS13) {
+        if (ss->firstHsDone) {
+            desc = unexpected_message;
+            errCode = SSL_ERROR_RENEGOTIATION_NOT_ALLOWED;
             goto alert_loser;
         }
-    }
-
-    /* If there is a cookie, then this is a second ClientHello (TLS 1.3). */
-    if (ssl3_FindExtension(ss, ssl_tls13_cookie_xtn)) {
-        ss->ssl3.hs.helloRetry = PR_TRUE;
-    }
-
-    if (ss->ssl3.hs.receivedCcs) {
-        /* This is only valid if we sent HelloRetryRequest, so we should have
-         * negotiated TLS 1.3 and there should be a cookie extension. */
-        if (ss->version < SSL_LIBRARY_VERSION_TLS_1_3 ||
-            !ss->ssl3.hs.helloRetry) {
+
+        if (sidBytes.len > 0 && !IS_DTLS(ss)) {
+            SECITEM_FreeItem(&ss->ssl3.hs.fakeSid, PR_FALSE);
+            rv = SECITEM_CopyItem(NULL, &ss->ssl3.hs.fakeSid, &sidBytes);
+            if (rv != SECSuccess) {
+                desc = internal_error;
+                errCode = PORT_GetError();
+                goto alert_loser;
+            }
+        }
+
+        /* TLS 1.3 requires that compression include only null. */
+        if (comps.len != 1 || comps.data[0] != ssl_compression_null) {
+            goto alert_loser;
+        }
+
+        /* If there is a cookie, then this is a second ClientHello (TLS 1.3). */
+        if (ssl3_FindExtension(ss, ssl_tls13_cookie_xtn)) {
+            ss->ssl3.hs.helloRetry = PR_TRUE;
+        }
+
+        /* receivedCcs is only valid if we sent an HRR. */
+        if (ss->ssl3.hs.receivedCcs && !ss->ssl3.hs.helloRetry) {
             desc = unexpected_message;
             errCode = SSL_ERROR_RX_UNEXPECTED_CHANGE_CIPHER;
             goto alert_loser;
         }
+    } else {
+        /* HRR is TLS1.3-only. We ignore the Cookie extension here. */
+        if (ss->ssl3.hs.helloRetry) {
+            desc = protocol_version;
+            errCode = SSL_ERROR_UNSUPPORTED_VERSION;
+            goto alert_loser;
+        }
+
+        /* receivedCcs is only valid if we sent an HRR. */
+        if (ss->ssl3.hs.receivedCcs) {
+            desc = unexpected_message;
+            errCode = SSL_ERROR_RX_UNEXPECTED_CHANGE_CIPHER;
+            goto alert_loser;
+        }
+
+        /* TLS versions prior to 1.3 must include null somewhere. */
+        if (comps.len < 1 ||
+            !memchr(comps.data, ssl_compression_null, comps.len)) {
+            goto alert_loser;
+        }
     }
 
     /* Now parse the rest of the extensions. */
     rv = ssl3_HandleParsedExtensions(ss, ssl_hs_client_hello);
     ssl3_DestroyRemoteExtensions(&ss->ssl3.hs.remoteExtensions);
     if (rv != SECSuccess) {
         if (PORT_GetError() == SSL_ERROR_UNSUPPORTED_SIGNATURE_ALGORITHM) {
             errCode = SSL_ERROR_UNSUPPORTED_SIGNATURE_ALGORITHM;
@@ -8732,29 +8761,16 @@ ssl3_HandleClientHello(sslSocket *ss, PR
             if (suite_i != TLS_FALLBACK_SCSV)
                 continue;
             desc = inappropriate_fallback;
             errCode = SSL_ERROR_INAPPROPRIATE_FALLBACK_ALERT;
             goto alert_loser;
         }
     }
 
-    /* TLS 1.3 requires that compression only include null. */
-    if (isTLS13) {
-        if (comps.len != 1 || comps.data[0] != ssl_compression_null) {
-            goto alert_loser;
-        }
-    } else {
-        /* Other versions need to include null somewhere. */
-        if (comps.len < 1 ||
-            !memchr(comps.data, ssl_compression_null, comps.len)) {
-            goto alert_loser;
-        }
-    }
-
     if (!ssl3_ExtensionNegotiated(ss, ssl_renegotiation_info_xtn)) {
         /* If we didn't receive an RI extension, look for the SCSV,
          * and if found, treat it just like an empty RI extension
          * by processing a local copy of an empty RI extension.
          */
         for (i = 0; i + 1 < suites.len; i += 2) {
             PRUint16 suite_i = (suites.data[i] << 8) | suites.data[i + 1];
             if (suite_i == TLS_EMPTY_RENEGOTIATION_INFO_SCSV) {
@@ -8762,17 +8778,17 @@ ssl3_HandleClientHello(sslSocket *ss, PR
                 PRUint32 L2 = sizeof emptyRIext;
                 (void)ssl3_HandleExtensions(ss, &b2, &L2, ssl_hs_client_hello);
                 break;
             }
         }
     }
 
     /* The check for renegotiation in TLS 1.3 is earlier. */
-    if (ss->version < SSL_LIBRARY_VERSION_TLS_1_3) {
+    if (!isTLS13) {
         if (ss->firstHsDone &&
             (ss->opt.enableRenegotiation == SSL_RENEGOTIATE_REQUIRES_XTN ||
              ss->opt.enableRenegotiation == SSL_RENEGOTIATE_TRANSITIONAL) &&
             !ssl3_ExtensionNegotiated(ss, ssl_renegotiation_info_xtn)) {
             desc = no_renegotiation;
             level = alert_warning;
             errCode = SSL_ERROR_RENEGOTIATION_NOT_ALLOWED;
             goto alert_loser;
@@ -8787,17 +8803,17 @@ ssl3_HandleClientHello(sslSocket *ss, PR
     }
 
     /* We do stateful resumes only if we are in TLS < 1.3 and
      * either of the following conditions are satisfied:
      * (1) the client does not support the session ticket extension, or
      * (2) the client support the session ticket extension, but sent an
      * empty ticket.
      */
-    if ((ss->version < SSL_LIBRARY_VERSION_TLS_1_3) &&
+    if (!isTLS13 &&
         (!ssl3_ExtensionNegotiated(ss, ssl_session_ticket_xtn) ||
          ss->xtnData.emptySessionTicket)) {
         if (sidBytes.len > 0 && !ss->opt.noCache) {
             SSL_TRC(7, ("%d: SSL3[%d]: server, lookup client session-id for 0x%08x%08x%08x%08x",
                         SSL_GETPID(), ss->fd, ss->sec.ci.peer.pr_s6_addr32[0],
                         ss->sec.ci.peer.pr_s6_addr32[1],
                         ss->sec.ci.peer.pr_s6_addr32[2],
                         ss->sec.ci.peer.pr_s6_addr32[3]));
@@ -8854,17 +8870,17 @@ ssl3_HandleClientHello(sslSocket *ss, PR
         }
     }
 
     if (IS_DTLS(ss)) {
         ssl3_DisableNonDTLSSuites(ss);
         dtls_ReceivedFirstMessageInFlight(ss);
     }
 
-    if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_3) {
+    if (isTLS13) {
         rv = tls13_HandleClientHelloPart2(ss, &suites, sid, savedMsg, savedLen);
     } else {
         rv = ssl3_HandleClientHelloPart2(ss, &suites, sid,
                                          savedMsg, savedLen);
     }
     if (rv != SECSuccess) {
         errCode = PORT_GetError();
         goto loser;
--- a/security/nss/lib/ssl/tls13con.c
+++ b/security/nss/lib/ssl/tls13con.c
@@ -5827,16 +5827,25 @@ tls13_NegotiateVersion(sslSocket *ss, co
     if (rv != SECSuccess) {
         return SECFailure;
     }
     if (data.len || !versions.len || (versions.len & 1)) {
         FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_CLIENT_HELLO, illegal_parameter);
         return SECFailure;
     }
     for (version = ss->vrange.max; version >= ss->vrange.min; --version) {
+        if (ss->ssl3.hs.helloRetry && version < SSL_LIBRARY_VERSION_TLS_1_3) {
+            /* Prevent negotiating to a lower version in response to a TLS 1.3 HRR.
+             * Since we check in descending (local) order, this will only fail if
+             * our vrange has changed or the client didn't offer 1.3 in response. */
+            PORT_SetError(SSL_ERROR_UNSUPPORTED_VERSION);
+            FATAL_ERROR(ss, SSL_ERROR_UNSUPPORTED_VERSION, protocol_version);
+            return SECFailure;
+        }
+
         PRUint16 wire = tls13_EncodeDraftVersion(version, ss->protocolVariant);
         unsigned long offset;
 
         for (offset = 0; offset < versions.len; offset += 2) {
             PRUint16 supported =
                 (versions.data[offset] << 8) | versions.data[offset + 1];
             if (supported == wire) {
                 ss->version = version;
--- a/security/nss/tests/gtests/gtests.sh
+++ b/security/nss/tests/gtests/gtests.sh
@@ -19,17 +19,16 @@
 
 ############################## gtest_init ##############################
 # local shell function to initialize this script
 ########################################################################
 gtest_init()
 {
   cd "$(dirname "$1")"
   pwd
-  SOURCE_DIR="$PWD"/../..
   if [ -z "${INIT_SOURCED}" -o "${INIT_SOURCED}" != "TRUE" ]; then
       cd ../common
       . ./init.sh
   fi
 
   SCRIPTNAME=gtests.sh
   . "${QADIR}"/common/certsetup.sh
 
@@ -95,11 +94,12 @@ gtest_start()
 gtest_cleanup()
 {
   html "</TABLE><BR>"
   . "${QADIR}"/common/cleanup.sh
 }
 
 ################## main #################################################
 GTESTS="${GTESTS:-prng_gtest certhigh_gtest certdb_gtest der_gtest pk11_gtest util_gtest freebl_gtest softoken_gtest sysinit_gtest blake2b_gtest smime_gtest mozpkix_gtest}"
+SOURCE_DIR="$PWD"/../..
 gtest_init "$0"
 gtest_start
 gtest_cleanup