A memory leak was reported and confirmed in this bug. However,
during the "manual" analysis of the flow, another possible leak was
found. I created a patch for both leaks, added gtests for unexpected
keySizes and adjusted the general syntax of the gtest file.

[7ef8d2604494]

2019-11-11 Tom Prince <[email protected]>

automation/taskcluster/graph/src/extend.js,

automation/taskcluster/windows/setup.sh:
Bug 1594891 - Use tc-proxy for nss tooltool; r=dustin,jcj

[c33b214b2ec8]

2019-11-08 Daiki Ueno <[email protected]>

gtests/ssl_gtest/ssl_dhe_unittest.cc,

gtests/ssl_gtest/ssl_ecdh_unittest.cc,
gtests/ssl_gtest/tls_connect.h, lib/ssl/ssl3con.c:
Bug 1566131, check policy against hash algorithms used for
ServerKeyExchange, r=mt

Summary: This adds necessary policy checks in
ssl3_ComputeCommonKeyHash(), right before calculating hashes. Note
that it currently doesn't check MD5 as it still needs to be allowed
in TLS 1.1 or earlier and many tests fail if we change that.

Reviewers: mt

Reviewed By: mt

Bug #: 1566131

[c08947c6af57]

2019-11-08 Kai Engert <[email protected]>

coreconf/coreconf.dep:

Dummy change, trigger a build to test latest NSPR commits.
[e766899c72a5]

automation/taskcluster/graph/src/extend.js:

Bug 1579836 - Execute NSPR tests as part of NSS continuous
integration. r=jcj
[46bfbabf7e75]

2019-11-08 Dustin J. Mitchell <[email protected]>

automation/taskcluster/graph/npm-shrinkwrap.json,

automation/taskcluster/graph/package.json,
automation/taskcluster/graph/src/image_builder.js,
automation/taskcluster/graph/src/queue.js,
automation/taskcluster/scripts/tools.sh,
automation/taskcluster/windows/gen_certs.sh,
automation/taskcluster/windows/run_tests.sh:
Bug 1594891 - Updates to run correctly on the new TC deployment
r=jcj

Update the Taskcluster client used in the decision task to one

that understands Taskcluster rootUrls.

Update scripts that fetch content to use the TASKCLUSTER_ROOT_URL
- the absence of this variale signals an "old" worker so we use an

"old" URL

[67d630e7cb7c]

2019-11-07 Tom Prince <[email protected]>

.taskcluster.yml, automation/taskcluster/graph/src/extend.js,

automation/taskcluster/graph/src/queue.js:
Bug 1591275: Switch workers to use AWS Provder; r=kjacobs

[a2bebaad41dd]

2019-11-06 Daiki Ueno <[email protected]>

gtests/pk11_gtest/pk11_module_unittest.cc:

Bug 1577803, clang-format, a=bustage
[c9014b2892d5]

gtests/pk11_gtest/pk11_module_unittest.cc,

gtests/pkcs11testmodule/pkcs11testmodule.cpp,
lib/pk11wrap/debug_module.c, lib/pk11wrap/pk11obj.c,
lib/pk11wrap/pk11slot.c, lib/pk11wrap/secmodti.h,
lib/util/pkcs11t.h:
Bug 1577803, pk11wrap: set friendly flag if token implements
CKP_PUBLIC_CERTIFICATES_TOKEN, r=rrelyea

Summary: This makes NSS look for CKO_PROFILE object at token
initialization time to check if it implements the [[ https://docs
.oasis-open.org/pkcs11/pkcs11-profiles/v3.0/pkcs11-profiles-v3.0.pdf

Public Certificates Token profile ]] as defined in PKCS #11 v3.0.

If it is found, the token is automatically marked as friendly so no
authentication attempts will be made when accessing certificates.

Reviewers: rrelyea

Reviewed By: rrelyea

Subscribers: reviewbot

Bug #: 1577803

[b39c8eeabe6a]

2019-11-06 Martin Thomson <[email protected]>

lib/freebl/blinit.c, lib/freebl/gcm-ppc.c:

Bug 1566126 - clang-format, a=bustage
[6125200fbc88]

2019-11-06 Lauri Kasanen <[email protected]>

lib/freebl/Makefile, lib/freebl/altivec-types.h,

lib/freebl/blapii.h, lib/freebl/blinit.c, lib/freebl/freebl.gyp,
lib/freebl/gcm-ppc.c, lib/freebl/gcm.c, lib/freebl/gcm.h:
Bug 1566126 - freebl: POWER GHASH Vector Acceleration, r=mt

Implementation for POWER8 adapted from the ARM paper:
https://conradoplg.cryptoland.net/files/2010/12/gcm14.pdf

Benchmark of `bltest -E -m aes_gcm -i tests/aes_gcm/plaintext10 \
-v tests/aes_gcm/iv10 -k tests/aes_gcm/key10 -5 10` on POWER8 3.3GHz.

NSS_DISABLE_HW_CRYPTO=1 mode in symmkey opreps cxreps context op
time(sec) thrgput aes_gcm_e 309Mb 192 5M 0 0.000 10000.000 10.001
30Mb

	 mode in symmkey opreps cxreps context op time(sec) thrgput

aes_gcm_e 829Mb 192 14M 0 0.000 10000.000 10.001 82Mb

Notable operf results, sw: samples % image name symbol name 226033
59.3991 libfreeblpriv3.so bmul 80606 21.1824 libfreeblpriv3.so
rijndael_encryptBlock128 28851 7.5817 libfreeblpriv3.so
gcm_HashMult_sftw

hw: 213899 56.2037 libfreeblpriv3.so rijndael_encryptBlock128 45233
11.8853 libfreeblpriv3.so gcm_HashMult_hw

So the ghash part is ~5.6x faster.

Signed-off-by: Lauri Kasanen <[email protected]>
[3d7e509d6d20]

2019-11-05 Marcus Burghardt <[email protected]>

lib/certdb/certdb.c, lib/util/secport.h:

Bug 1589073 - Use of new PR_ASSERT_ARG in certdb.c. r=mt

Bug 1588015 introduced in NSPR a new way to ASSERT values where the
arguments are always used avoiding "unused variable" errors. This
was implemented in NSS, at certdb.c.

[73c28cad3dbb]

2019-11-05 Daiki Ueno <[email protected]>

cpputil/nss_scoped_ptrs.h, gtests/manifest.mn,

gtests/pk11_gtest/manifest.mn, gtests/pk11_gtest/pk11_gtest.gyp,
gtests/pk11_gtest/pk11_module_unittest.cc,
gtests/pkcs11testmodule/Makefile, gtests/pkcs11testmodule/config.mk,
gtests/pkcs11testmodule/manifest.mn,
gtests/pkcs11testmodule/pkcs11testmodule.cpp,
gtests/pkcs11testmodule/pkcs11testmodule.def,
gtests/pkcs11testmodule/pkcs11testmodule.gyp,
gtests/pkcs11testmodule/pkcs11testmodule.rc, nss.gyp:
Bug 1577803, gtests: import pkcs11testmodule from Firefox, r=rrelyea

Summary: This adds a mock PKCS #11 module from Firefox and add basic
tests around it. This is needed for proper testing of PKCS #11 v3.0
profile objects (D45669).

Reviewers: rrelyea

Reviewed By: rrelyea

Subscribers: reviewbot

Bug #: 1577803

[0a86945adf74]

Test Plan

https://treeherder.mozilla.org/#/jobs?repo=try&revision=77b18bc38fcfb26cbd1df38bc67ec840399a87e0

Diff Detail

Repository: rMOZILLACENTRAL mozilla-central

Event Timeline

• jcj created this revision.Nov 13 2019, 12:01 AM

Herald added a project: secure-revision. · View Herald TranscriptNov 13 2019, 12:01 AM

Harbormaster completed remote builds in B134399: Diff 190970.Nov 13 2019, 12:01 AM

phab-bot requested review of this revision.Nov 13 2019, 12:02 AM

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".

phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".

phab-bot removed a project: secure-revision.

• jcj edited the test plan for this revision. (Show Details)Nov 13 2019, 12:03 AM

• jcj edited reviewers, added: keeler; removed: kjacobs.

Code analysis found 1 defect in the diff 190970:

1 defect found by Coverity

If you see a problem in this automated review, please report it here.

security/nss/lib/softoken/pkcs11c.c
4406	Checker reliability is low, meaning that the false positive ratio is high. Calling "nsspkcs5_DestroyPBEParameter" frees pointer "params->poolp" which has already been freed. The path that leads to this defect is: security/nss/lib/softoken/pkcs11c.c:4317: `path: Condition "oid == NULL", taking false branch.`. security/nss/lib/softoken/pkcs11c.c:4321: `path: Condition "pMechanism->mechanism == 944", taking true branch.`. security/nss/lib/softoken/pkcs11c.c:4322: `path: Condition "!pMechanism->pParameter", taking false branch.`. security/nss/lib/softoken/pkcs11c.c:4322: `path: Condition "pMechanism->ulParameterLen < 72UL /* sizeof (CK_PKCS5_PBKD2_PARAMS) /", taking false branch.`. security/nss/lib/softoken/pkcs11c.c:4326: `path: Switch case value "1UL".`. security/nss/lib/softoken/pkcs11c.c:4329: `path: Breaking from switch.`. security/nss/lib/softoken/pkcs11c.c:4345: `path: Condition "pbkd2_params->saltSource != 1", taking false branch.`. security/nss/lib/softoken/pkcs11c.c:4351: `path: Falling through to end of if statement.`. security/nss/lib/softoken/pkcs11c.c:4361: `path: Condition "params == NULL", taking false branch.`. security/nss/lib/softoken/pkcs11c.c:4365: `path: Switch case default.`. security/nss/lib/softoken/pkcs11c.c:4401: `freed_arg: "nsspkcs5_DestroyPBEParameter" frees "params->poolp".`. security/nss/lib/softoken/pkcs11c.c:4402: `path: Breaking from switch.`. security/nss/lib/softoken/pkcs11c.c:4404: `path: Condition "crv == 0", taking false branch.`. security/nss/lib/softoken/pkcs11c.c:4407*: `double_free: Calling "nsspkcs5_DestroyPBEParameter" frees pointer "params->poolp" which has already been freed.`.

Running the nss uplift command results in the same diff for me, so r+. That said, maybe we should confirm/deny that potential double-free before landing?

This revision is now accepted and ready to land.Nov 13 2019, 12:37 AM

Revision updated.

• jcj retitled this revision from Bug 1592007 - land NSS d244c7287908 UPGRADE_NSS_RELEASE, r=kjacobs to Bug 1592007 - land NSS 87f35ba4c82f UPGRADE_NSS_RELEASE, r=keeler.Nov 13 2019, 7:44 PM

• jcj edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B134743: Diff 191444.Nov 13 2019, 7:44 PM

Closed by commit rMOZILLACENTRALcbd4aa02eba9: Bug 1592007 - land NSS 87f35ba4c82f UPGRADE_NSS_RELEASE, r=keeler (authored by • jcj). · Explain WhyNov 13 2019, 8:21 PM

This revision was automatically updated to reflect the committed changes.

• jcj added a commit: rMOZILLACENTRALcbd4aa02eba9: Bug 1592007 - land NSS 87f35ba4c82f UPGRADE_NSS_RELEASE, r=keeler.

In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:

Should they have tests, or are they dead code ?

You can file a bug blocking Bug 1415824 for untested files that should be tested.
You can file a bug blocking Bug 1415819 for untested files that should be removed.

If you see a problem in this automated review, please report it here.

Csoregi Natalia <[email protected]> reopened this revision.Nov 14 2019, 2:30 AM

This revision is now accepted and ready to land.Nov 14 2019, 2:30 AM

Csoregi Natalia <[email protected]> added a reverting change: rMOZILLACENTRAL3cf4cf89e8ea: Backed out changeset cbd4aa02eba9 (bug 1592007) for failures on….Nov 14 2019, 2:30 AM

• jcj added a child revision: D53050: Bug 1596430 - Permit an additional stat to the NSS certDB metadata files r?florian.Nov 14 2019, 5:32 PM

Closed by commit rMOZILLACENTRAL9ba180fee075: Bug 1592007 - land NSS 87f35ba4c82f UPGRADE_NSS_RELEASE, r=keeler (authored by • jcj). · Explain WhyNov 14 2019, 8:15 PM

This revision was automatically updated to reflect the committed changes.

• jcj added a commit: rMOZILLACENTRAL9ba180fee075: Bug 1592007 - land NSS 87f35ba4c82f UPGRADE_NSS_RELEASE, r=keeler.