Closed Bug 1082959 Opened 10 years ago Closed 8 years ago

Improve TLS version handling test coverage

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: mt, Assigned: mt)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(2 files, 4 obsolete files)

:keeler identified some shortcomings in our test coverage.  I now have tests that provide coverage of our pref and version handling and am working on improving these further to cover version intolerance fallback.
Depends on: 1084669
You asked for tests, here are some tests.  Let me know if you want the two patches squashed (it might be easier to review that way).
Attachment #8507280 - Flags: feedback?(dkeeler)
Continuation...

I haven't tested that the fallback limit actually applies, but this is a start on getting the whole version intolerance testing bootstrapped.
Attachment #8507281 - Flags: feedback?(dkeeler)
Comment on attachment 8507280 [details] [diff] [review]
0001-Bug-1082959-Adding-tests-for-version-compatibility.patch

Review of attachment 8507280 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good in general. I think the refactoring of head_psm.js should be in a separate bug.

::: security/manager/ssl/tests/unit/head_psm.js
@@ +219,5 @@
>  
>  */
>  function add_tls_server_setup(serverBinName) {
>    add_test(function() {
> +    start_tls_server(serverBinName).then(process => {

Refactoring this infrastructure should probably be moved to a separate bug.

@@ +235,5 @@
>  
>    function Connection(aHost) {
>      this.host = aHost;
>      let threadManager = Cc["@mozilla.org/thread-manager;1"]
> +      .getService(Ci.nsIThreadManager);

nit: keep these as they were

@@ +240,4 @@
>      this.thread = threadManager.currentThread;
>      this.defer = Promise.defer();
>      let sts = Cc["@mozilla.org/network/socket-transport-service;1"]
> +      .getService(Ci.nsISocketTransportService);

nit: same here

::: security/manager/ssl/tests/unit/test_versions.js
@@ +18,5 @@
> +  Services.prefs.setIntPref("security.tls.version.max", vmax);
> +  Services.prefs.setIntPref("security.tls.version.fallback-limit", vfb);
> +}
> +
> +var serverProcess;

nit: let, not var

@@ +26,5 @@
> +                 .getService(Ci.nsIEnvironment);
> +  envSvc.set("TLS_SERVER_VERSION_MIN", vmin.toString(10));
> +  envSvc.set("TLS_SERVER_VERSION_MAX", vmax.toString(10));
> +  let serverStarted = start_tls_server("BadCertServer");
> +  addCertFromFile(certdb, "tlsserver/other-test-ca.der", "CTu,u,u");

Why other-test-ca and not the default? (which should already be trusted by the test infrastructure, if I recall correctly...)

@@ +47,5 @@
> +
> +  // connect to a known-good host
> +  return connect_to_host("good.include-subdomains.pinning.example.com",
> +                             expectSuccess ? Cr.NS_OK :
> +                             getXPCOMStatusFromNSS(SSL_ERROR_NO_CYPHER_OVERLAP))

nit: you can break these lines like so:

expectSuccess
? Cr.NS_OK
: getXPCOMStatusFromNSS(SSL_ERROR_NO_CYPHER_OVERLAP))

::: security/manager/ssl/tests/unit/tlsserver/lib/TLSServer.cpp
@@ +5,5 @@
>  #include "TLSServer.h"
>  
> +#include <algorithm>
> +#include <cstdlib>
> +#include <cstdio>

nit: I think cstdio would sort before cstdlib

@@ +269,5 @@
>  
> +static uint16_t
> +GetEnvVersion(const char* envName, uint16_t defVersion)
> +{
> +  char* val = getenv(envName);

So far this file has used PR_GetEnv

@@ +274,5 @@
> +  if (!val) {
> +    return defVersion;
> +  }
> +  char* endptr;
> +  unsigned long int envValue = strtoul(val, &endptr, 10);

We could do this, or we could just do string/char comparison on "0", "1", "2", "3", etc. (not a big deal either way, though)

@@ +284,5 @@
> +}
> +
> +static SECStatus
> +ConfigureTLSVersion() {
> +  uint16_t minVersion = GetEnvVersion("TLS_SERVER_VERSION_MIN",

nit: let's call this "MOZ_TLS_SERVER_TLS_VERSION_MIN/MAX"

::: security/manager/ssl/tests/unit/xpcshell.ini
@@ +18,5 @@
>    test_ocsp_fetch_method/**
>    test_keysize/**
>    test_pinning_dynamic/**
>  
> +[test_versions.js]

nit: maybe "test_tls_versions.js"
Attachment #8507280 - Flags: feedback?(dkeeler) → feedback+
Comment on attachment 8507281 [details] [diff] [review]
0002-Bug-1083058-Adding-TLS-version-intolerance-tests.patch

Review of attachment 8507281 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good in general. Just a few comments.

::: security/manager/ssl/tests/unit/head_psm.js
@@ +63,5 @@
>  const SSL_ERROR_BAD_CERT_ALERT                          = SSL_ERROR_BASE +  17;
> +const SSL_ERROR_HANDSHAKE_FAILURE_ALERT                 = SSL_ERROR_BASE +  61;
> +const SSL_ERROR_INAPPROPRIATE_FALLBACK_ALERT            = SSL_ERROR_BASE +  131;
> +
> +const NS_ERROR_NET_RESET                                = 0x804B0014;

Doesn't this already exist in Components.results?

::: security/manager/ssl/tests/unit/head_versions.js
@@ +1,1 @@
> +// -*- indent-tabs-mode: nil; js-indent-level: 2 -*-

This can probably go in head_psm.js.

::: security/manager/ssl/tests/unit/tlsserver/cmd/IntolerantServer.cpp
@@ +50,5 @@
> +}
> +
> +int32_t
> +BeVersionIntolerant(PRFileDesc *aFd, const SECItem *aSrvNameArr,
> +                  uint32_t aSrvNameArrSize, void *aArg)

nit: *s to the left

@@ +71,5 @@
> +    // This is going to seriously mess up the state machine, intentionally.
> +    // We do that by injecting an alert at the TCP layer.
> +    PRFileDesc* removedLayer =  PR_PopIOLayer(aFd, PR_TOP_IO_LAYER);
> +    PR_Write(aFd, message, sizeof(message));
> +    PR_PushIOLayer(aFd, PR_TOP_IO_LAYER, removedLayer);

I'm concerned this will cause assertion failures/crashes in the tlsserver. Is there another way to do this?

::: security/manager/ssl/tests/unit/tlsserver/lib/TLSServer.cpp
@@ +269,2 @@
>  int
> +InitServer(const char *nssCertDBDir)

nit: const char* nssCertDBDir

@@ +306,3 @@
>  
> +int
> +StartServer(SSLSNISocketConfig sniSocketConfig, void *sniSocketConfigArg)

nit: void* sniSocketConfigArg

::: security/manager/ssl/tests/unit/tlsserver/lib/TLSServer.h
@@ +42,5 @@
>                                  /*optional*/ ScopedCERTCertificate *cert,
>                                  /*optional*/ SSLKEAType *kea);
>  
>  int
> +InitServer(const char *nssCertDBDir);

Why does InitServer need to be a separate step?

::: security/manager/ssl/tests/unit/xpcshell.ini
@@ +21,5 @@
>  
>  [test_versions.js]
> +head = head_psm.js head_versions.js
> +[test_intolerance.js]
> +head = head_psm.js head_versions.js

Forgot to mention - tests that use the tlsserver infrastructure generally can't be run in parallel. Also, they don't run on android.
Attachment #8507281 - Flags: feedback?(dkeeler) → feedback+
(In reply to David Keeler (:keeler) [use needinfo?] from comment #4)
> Looks good in general. Just a few comments.

Yeah, I'll see what I can do about the nits.

> ::: security/manager/ssl/tests/unit/head_versions.js
> This can probably go in head_psm.js.

I didn't want to load it down (left to me, I'd be splitting it into more pieces), but I can do that for you.

> @@ +71,5 @@
> > +    // This is going to seriously mess up the state machine, intentionally.
> > +    // We do that by injecting an alert at the TCP layer.
> > +    PRFileDesc* removedLayer =  PR_PopIOLayer(aFd, PR_TOP_IO_LAYER);
> > +    PR_Write(aFd, message, sizeof(message));
> > +    PR_PushIOLayer(aFd, PR_TOP_IO_LAYER, removedLayer);
> 
> I'm concerned this will cause assertion failures/crashes in the tlsserver.
> Is there another way to do this?

Yeah, this is crazy.  But it's hard to force NSS to act badly like this (design for test is a concept that is a little newer than NSS).  

I couldn't work out how to do anything better (if you have ideas, I'm open to suggestions).  And I'm not sure that it's going to work properly anyway.  While this works on Linux, it looks like it's triggering different results on my mac.

> Why does InitServer need to be a separate step?

Because IntolerantServer.cpp needs to configure NSS after the DB init stuff, but before the server socket is opened.
OK, I had a few issues with the tests, particularly with the landing of bug 1088915, but I think we're good to go now: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2fd7d19065cb
Attached patch Refactor head_psm.js (obsolete) — Splinter Review
I've move the refactoring here.  I can move it to another bug if you like, but I figure a separate commit is much easier (I don't have to individually verify the outcome, for one).
Attachment #8507280 - Attachment is obsolete: true
Attachment #8517620 - Flags: review?(dkeeler)
Attachment #8507281 - Attachment is obsolete: true
Attachment #8517621 - Flags: review?(dkeeler)
Attachment #8517620 - Attachment description: 425eed5db281.patch → Refactor head_psm.js
Martin, just to let you know, I don't think I'm going to get to these this week. I'll review them next week as soon as I can.
I appreciate you spending the time.  These are low priority in the sense that nothing burns if they are left to lie idle for a while.
Comment on attachment 8517620 [details] [diff] [review]
Refactor head_psm.js

Review of attachment 8517620 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/tests/unit/head_psm.js
@@ +385,5 @@
>    let serverBin = _getBinaryUtil(serverBinName);
>    let process = Cc["@mozilla.org/process/util;1"].createInstance(Ci.nsIProcess);
>    process.init(serverBin);
> +  do_register_cleanup(function() {
> +    if (process.isRunning) {

I have a vague recollection that process.isRunning doesn't actually work. I don't remember the details, though. In any case, it looks like the tests passed fine, so maybe this isn't an issue anymore.

@@ +396,5 @@
>    do_check_true(certDir.exists());
>    // Using "sql:" causes the SQL DB to be used so we can run tests on Android.
>    process.run(false, [ "sql:" + certDir.path ], 1);
>  
> +  return serverStarted.then(() => process);

I guess 'serverStarted' is coming from the other patch. I'd just prefer if they were folded together.
Attachment #8517620 - Flags: review?(dkeeler) → review+
(In reply to David Keeler (:keeler) [use needinfo?] from comment #11)
> Comment on attachment 8517620 [details] [diff] [review]
> Refactor head_psm.js
> 
> Review of attachment 8517620 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: security/manager/ssl/tests/unit/head_psm.js
> @@ +385,5 @@
> >    let serverBin = _getBinaryUtil(serverBinName);
> >    let process = Cc["@mozilla.org/process/util;1"].createInstance(Ci.nsIProcess);
> >    process.init(serverBin);
> > +  do_register_cleanup(function() {
> > +    if (process.isRunning) {
> 
> I have a vague recollection that process.isRunning doesn't actually work. I
> don't remember the details, though. In any case, it looks like the tests
> passed fine, so maybe this isn't an issue anymore.

In this case, it doesn't matter if the process is running or not.  What matters is whether it is safe to call stop() on the process.  The problem that came up here was that calling stop() twice causes an unwanted explosion.

> I guess 'serverStarted' is coming from the other patch. I'd just prefer if
> they were folded together.

Happy to do that.
Comment on attachment 8517621 [details] [diff] [review]
TLS version handling for xpcshell

Review of attachment 8517621 [details] [diff] [review]:
-----------------------------------------------------------------

Ok - looks good with comments addressed. Basically, it would be good to have more documentation for the new tests. Also, I feel fairly strongly about not splitting out StartServer into two functions. We should always be able to call ConfigureTLSVersion, so there's no reason to add complexity just so the other servers can avoid it.

::: security/manager/ssl/tests/unit/head_psm.js
@@ +226,5 @@
>    });
>  }
>  
> +
> +/* Returns a promise to connect to aHost that resolves to the result of that

nit: use //-style comments

::: security/manager/ssl/tests/unit/head_versions.js
@@ +9,5 @@
> +"use strict";
> +
> +do_get_profile(); // must be called before getting nsIX509CertDB
> +const certdb = Cc["@mozilla.org/security/x509certdb;1"]
> +                  .getService(Ci.nsIX509CertDB);

nit: indent one less space here

@@ +28,5 @@
> +    serverProcess.kill();
> +  }
> +}
> +
> +function run_server_with_version(vmin, vmax, vintolerant, sendScsv) {

nit: some documentation would be appreciated (e.g. "returns a promise that...")

@@ +41,5 @@
> +    envSvc.set(intolerantPref, "");
> +  }
> +  stop_server();
> +  let serverStarted = start_tls_server("IntolerantServer");
> +//  addCertFromFile(certdb, "tlsserver/other-test-ca.der", "CTu,u,u");

nit: remove commented-out code

::: security/manager/ssl/tests/unit/test_tls_version_intolerance.js
@@ +10,5 @@
> +
> +function do_xhr(aHost) {
> +  const PORT = 8443;
> +  let req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +    .createInstance(Ci.nsIXMLHttpRequest);

nit: indent so the '.' and '[' are aligned

@@ +24,5 @@
> +  req.send();
> +  return promise;
> +}
> +
> +function check_server(name, expected, vmin, vmax, vfb) {

less abbreviation of parameters/variables, please. "vfb" is a bit opaque - this is the value for the version fallback limit, right? "fallbackLimit" seems like a fine name.

::: security/manager/ssl/tests/unit/tlsserver/cmd/ClientAuthServer.cpp
@@ +111,5 @@
>      fprintf(stderr, "usage: %s <NSS DB directory>\n", argv[0]);
>      return 1;
>    }
>  
> +  if (InitServer(argv[1])) {

I still think we can get away with not splitting this out if we have StartServer call ConfigureTLSVersion and do "the right thing" if the environment variables aren't set.

::: security/manager/ssl/tests/unit/tlsserver/cmd/IntolerantServer.cpp
@@ +8,5 @@
> +// If all is good, the client then sends one encrypted byte and receives that
> +// same byte back.
> +// This server also has the ability to "call back" another process waiting on
> +// it. That is, when the server is all set up and ready to receive connections,
> +// it will connect to a specified port and issue a simple HTTP request.

nit: update documentation here

@@ +39,5 @@
> +  return SSL_LIBRARY_VERSION_3_0 + static_cast<uint16_t>(envValue);
> +}
> +
> +void
> +ConfigureTLSVersion() {

This is always safe to call, even if the environment variables aren't set, right? So why don't we just always call it? That way, we don't have to modify the other servers (this code can just go in TLSServer.cpp).

@@ +70,5 @@
> +    fprintf(stderr, "Simulating intolerance\n");
> +
> +    // This is going to seriously mess up the state machine, intentionally.
> +    // We do that by injecting an alert at the TCP layer.
> +    PRFileDesc* removedLayer =  PR_PopIOLayer(aFd, PR_TOP_IO_LAYER);

nit: unnecessary extra space after the '='

@@ +97,5 @@
> +  if (argc != 2) {
> +    fprintf(stderr, "usage: %s <NSS DB directory>\n", argv[0]);
> +    return 1;
> +  }
> +  static uint16_t intolerant_version =

nit: intolerantVersion

::: security/manager/ssl/tests/unit/tlsserver/lib/TLSServer.h
@@ +38,4 @@
>  // Pass DEFAULT_CERT_NICKNAME as certName unless you need a specific
>  // certificate.
>  SECStatus
>  ConfigSecureServerWithNamedCert(PRFileDesc *fd, const char *certName,

Might as well update the placement of the '*' in this file as well.
Attachment #8517621 - Flags: review?(dkeeler) → review+
Rebased on top of the new version of bug 1084669.
Attachment #8572044 - Flags: review+
Attachment #8517620 - Attachment is obsolete: true
Attachment #8517621 - Attachment is obsolete: true
:emk, this isn't landing until NSS bug 1084669 lands in a version of gecko, but you should probably be aware of its existence, since you are doing lots of work in this area.  If there is anything that I can do here to make your work easier, let me know and I'll fix it up.
Assignee: nobody → martin.thomson
I imagine these patches have bitrotted some and our fallback situation has changed somewhat in the past year, but there might still be something relevant here. What do you think, Martin?
Flags: needinfo?(martin.thomson)
Whiteboard: [psm-assigned]
Yeah, I think that too much has changed to make these worthwhile.  I looked at these a few months ago and concluded I didn't want to sink more time into them.  If you see something specific...
Flags: needinfo?(martin.thomson)
OBE
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.