Bug 1303224 - Remove the PKCS#11 bypass #15

ttaubert · 2016-09-18T10:16:32Z

martinthomson · 2016-09-19T01:10:31Z

automation/taskcluster/scripts/run_scan_build.sh

@@ -23,7 +23,7 @@ cd nss && make nss_build_all
 # we run scan-build on these folders
 declare -a scan=("lib/ssl" "lib/freebl")
 # corresponds to the number of errors that are expected in the |scan| folder
-declare -a ignore=(1 0)
+declare -a ignore=(0 0)


Since you are no longer ignoring, maybe you can instead remove the ability to ignore entirely.

We should keep the ignores. The plan is to enable scan-build on other folders where we'll have the same problem.

martinthomson · 2016-09-19T01:12:30Z

cmd/selfserv/selfserv.c

-    ** XXX: 'E' and 'x' were used in the past, please leave some
-    **      time before resuing those. */
+    ** XXX: 'B', 'E', 'q', and 'x' were used in the past, please
+    **      leave some time before resuing those. */


Include the release in which these were removed so that it is easier to decide when to overwrite them.

martinthomson · 2016-09-19T01:13:20Z

cmd/strsclnt/strsclnt.c

@@ -1323,13 +1314,9 @@ main(int argc, char **argv)
    progName = progName ? progName + 1 : tmp;

    optstate = PL_CreateOptState(argc, argv,
-                                 "BC:DNP:TUV:W:a:c:d:f:gin:op:qst:uvw:z");
+                                 "C:DNP:TUV:W:a:c:d:f:gin:op:qst:uvw:z");


Add a comment here too.

martinthomson · 2016-09-19T01:13:27Z

cmd/tstclnt/tstclnt.c

@@ -960,7 +957,7 @@ main(int argc, char **argv)
    SSL_VersionRangeGetSupported(ssl_variant_stream, &enabledVersions);

    optstate = PL_CreateOptState(argc, argv,
-                                 "46BCDFGHKM:OR:STUV:W:Ya:bc:d:fgh:m:n:op:qr:st:uvw:z");
+                                 "46CDFGHKM:OR:STUV:W:Ya:bc:d:fgh:m:n:op:qr:st:uvw:z");


martinthomson · 2016-09-19T01:15:28Z

lib/ssl/ssl.h

@@ -1199,7 +1199,10 @@ SSL_IMPORT SECStatus SSL_ExportKeyingMaterial(PRFileDesc *fd,
 */
 SSL_IMPORT CERTCertificate *SSL_LocalCertificate(PRFileDesc *fd);

-/* Test an SSL configuration to see if  SSL_BYPASS_PKCS11 can be turned on.
+/* DEPRECATED: The PKCS#11 bypass has been removed. This function will now
+**             always return false.


Remove the rest of the comment and the other comments on the #defines.

martinthomson · 2016-09-19T01:16:31Z

lib/ssl/ssl3con.c

-        spec->destroy(spec->encodeContext, freeit);
-        spec->destroy(spec->decodeContext, freeit);
+        spec->destroy(spec->encodeContext, PR_TRUE);
+        spec->destroy(spec->decodeContext, PR_TRUE);


Note to self: check if we actually need/use spec->destroy.

Looks like we don't! Added another commit to get rid of it.

martinthomson · 2016-09-19T05:43:36Z

lib/ssl/ssl3con.c

-    if (spec->bypassCiphers) {
-        /* This function doesn't support PKCS#11 bypass. We fallback on the
-         * non-constant time version. */
-        goto fallback;


You didn't remove the trailing fallback block at the end of this function.

We still need it in case PK11_SignWithSymKey() fails.

If there is only one case that calls that code, move it inline and remove the goto.

Yeah, good idea.

martinthomson · 2016-09-19T05:50:12Z

lib/ssl/ssl3con.c

-        }
-        hashesForVerify = hashes;
-    } else {
+    if (!hashes) {


If this is the right change (and I think that it is), then I think that we can simply add a PORT_Assert on line 9657.

I fought with scan-build about this change here, as I started by doing what you suggested. The problem is that _HandleCertificateVerify() can definitely be called when seeing a certificate_verify message but ssl3.hs.ws != wait_certificate_verify. In that case we should bail out before checking for (!hashes) because that will be true in those cases.

Not sure that I understand given that the ssl3.hs.ws check is above this, but it's a nit only, so do as you see fit.

I was trying to say that ssl3_HandleCertificateVerify() will be called when we receive a certificate_verify message but don't expect any. In that case hashes == NULL. So we'd better assert after we checked that we actually expect a certificate_verify message, otherwise we'll throw a library error when we shouldn't.

martinthomson · 2016-09-19T05:54:40Z

tests/ssl/ssl.sh

@@ -1223,7 +1223,7 @@ ssl_run_tests()
                SERVER_OPTIONS=
                ;;
            "bypass")


can we just remove the option entirely? I mean, it will disable tests without locks on, but those should be fine, modulo any threading errors they might expose (and we're not sure that the locks will guard against that anyhow).

Yeah, let's remove them.

franziskuskiefer

lgtm modulo mt's comments. There might be more but we can do that in follow ups.

franziskuskiefer · 2016-09-19T06:49:08Z

automation/taskcluster/scripts/run_scan_build.sh

@@ -23,7 +23,7 @@ cd nss && make nss_build_all
 # we run scan-build on these folders
 declare -a scan=("lib/ssl" "lib/freebl")
 # corresponds to the number of errors that are expected in the |scan| folder
-declare -a ignore=(1 0)
+declare -a ignore=(0 0)


We should keep the ignores. The plan is to enable scan-build on other folders where we'll have the same problem.

franziskuskiefer · 2016-09-19T06:53:16Z

lib/ssl/derive.c

@@ -1,716 +0,0 @@
-/*


this removes the file? strange way to display the diff....

How would you show it? Just showing the filename and "deleted" means that you can't see what was deleted, which might be important.

franziskuskiefer · 2016-09-20T07:30:33Z

lib/ssl/derive.c

-                if (requiredECCbits > signatureKeyStrength)
-                    requiredECCbits = signatureKeyStrength;
-
-                ecGroup = ssl_GetECGroupWithStrength(NULL, requiredECCbits);


you can update ssl_GetECGroupWithStrength to assert the socket now. This is the only call that doesn't have a socket.

More cleanup, yay, thanks!

ttaubert · 2016-09-20T12:27:00Z

Rebased. r? @franziskuskiefer @martinthomson

franziskuskiefer · 2016-09-20T14:20:50Z

lib/ssl/ssl3ecc.c

        if (!group || group->type != group_type_ec ||
            group->bits < requiredECCbits) {
            continue;
        }
-        if (!ss || ssl_NamedGroupEnabled(ss, group)) {


maybe assert the ss, just in case

martinthomson · 2016-09-23T01:54:13Z

lib/ssl/ssl3con.c

-    if (spec->bypassCiphers) {
-        /* This function doesn't support PKCS#11 bypass. We fallback on the
-         * non-constant time version. */
-        goto fallback;


If there is only one case that calls that code, move it inline and remove the goto.

martinthomson · 2016-09-23T01:55:40Z

lib/ssl/ssl3con.c

-        }
-        hashesForVerify = hashes;
-    } else {
+    if (!hashes) {


Not sure that I understand given that the ssl3.hs.ws check is above this, but it's a nit only, so do as you see fit.

martinthomson reviewed Sep 19, 2016

View reviewed changes

franziskuskiefer approved these changes Sep 19, 2016

View reviewed changes

franziskuskiefer reviewed Sep 20, 2016

View reviewed changes

ttaubert force-pushed the bug/1303224-remove-pk11-bypass branch from 4b3199e to de76a5b Compare September 20, 2016 12:25

ttaubert force-pushed the bug/1303224-remove-pk11-bypass branch from aa5bea4 to 51489b7 Compare September 20, 2016 12:58

franziskuskiefer approved these changes Sep 20, 2016

View reviewed changes

ttaubert force-pushed the bug/1303224-remove-pk11-bypass branch from b85434a to 6bd6e62 Compare September 22, 2016 12:39

martinthomson approved these changes Sep 23, 2016

View reviewed changes

Bug 1303224 - Remove the PKCS#11 bypass

f8ea10b

ttaubert force-pushed the bug/1303224-remove-pk11-bypass branch from 6bd6e62 to f8ea10b Compare September 23, 2016 22:13

ttaubert closed this Sep 24, 2016

ttaubert deleted the bug/1303224-remove-pk11-bypass branch September 24, 2016 09:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1303224 - Remove the PKCS#11 bypass #15

Bug 1303224 - Remove the PKCS#11 bypass #15

ttaubert commented Sep 18, 2016

martinthomson Sep 19, 2016

franziskuskiefer Sep 19, 2016

ttaubert Sep 19, 2016

martinthomson Sep 19, 2016 •

edited

Loading

ttaubert Sep 19, 2016

martinthomson Sep 19, 2016

ttaubert Sep 19, 2016

martinthomson Sep 19, 2016

ttaubert Sep 19, 2016

martinthomson Sep 19, 2016

ttaubert Sep 19, 2016 •

edited

Loading

martinthomson Sep 19, 2016

ttaubert Sep 20, 2016

martinthomson Sep 19, 2016

ttaubert Sep 19, 2016

martinthomson Sep 23, 2016

ttaubert Sep 23, 2016

martinthomson Sep 19, 2016

ttaubert Sep 19, 2016

martinthomson Sep 23, 2016

ttaubert Sep 23, 2016

martinthomson Sep 19, 2016 •

edited

Loading

ttaubert Sep 19, 2016

franziskuskiefer left a comment

franziskuskiefer Sep 19, 2016

franziskuskiefer Sep 19, 2016

martinthomson Sep 19, 2016

franziskuskiefer Sep 20, 2016

ttaubert Sep 20, 2016

ttaubert commented Sep 20, 2016

franziskuskiefer Sep 20, 2016 •

edited

Loading

ttaubert Sep 20, 2016

martinthomson Sep 23, 2016

martinthomson Sep 23, 2016

Bug 1303224 - Remove the PKCS#11 bypass #15

Bug 1303224 - Remove the PKCS#11 bypass #15

Conversation

ttaubert commented Sep 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinthomson Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttaubert Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinthomson Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franziskuskiefer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttaubert commented Sep 20, 2016

franziskuskiefer Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinthomson Sep 19, 2016 •

edited

Loading

ttaubert Sep 19, 2016 •

edited

Loading

martinthomson Sep 19, 2016 •

edited

Loading

franziskuskiefer Sep 20, 2016 •

edited

Loading