1328318 - Allow configuration of SSL/TLS version ranges to silenty succeed if a reduced range is selected because of policies or limitations

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert (:KaiE:)]

As of today, if an application requests a specific range of TLS versions
  (example: from TLS 1.0 to TLS 1.2)
but NSS is unable to comply, for example on a system, where the systemwide crypto policy requires the use of TLS 1.2 or later,
NSS will return a failure from the configuration call.

This may cause unexpected failures. For example, Firefox assumes that NSS init has failed, and malfunctions/aborts.

The following is a suggestion to change the behavior of existing APIs:

(a) If there is no overlap at all, e.g. if the application requested TLS 1.0 (only), and the system requires/supports only TLS 1.2 or newer, then the SSL_VersionRangeSet* APIs should fail (as it is today).

(b) If there is overlap, and at least one SSL/TLS version can be enabled, then the API should return success. (It returns failure today.)

This means, because of (b), applications will no longer be able to rely on the calls to SSL_VersionRangeSet* to configure exactly what the application has requested.

Instead, if an application needs to know what range was configured, the application must perform a call to an SSL_VersionRangeGet* API to learn this detail.

This change is required to prevent Firefox (and possibly other applications) from crashing/exiting on systems where the policy is stricter than the application's default configuration.

Comment 1

[Kai Engert (:KaiE:)]

In the previous comment, I was referring to these APIs:
- SSL_VersionRangeSetDefault
- SSL_VersionRangeSet
- SSL_VersionRangeGetDefault
- SSL_VersionRangeGet

For both of the above "Get" functions, and also the additional SSL_VersionRangeGetSupported, we must also ensure the following:

(c) The information returned by all "Get" functions must return the intersection between the original support provided by NSS, and the policy limitations.

Comment 2

[Kai Engert (:KaiE:)]

Attachment: 1328318-v1.patch

Comment 3

[Kai Engert (:KaiE:)]

test build:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=0e21b6cc16b66903e8e23bc529ff72bd3c960a6d

Comment 4

[Kai Engert (:KaiE:)]

The NSS try build looks good.

My testing on Fedora looks good.

Comment 5

[Robert Relyea]

Comment on attachment 8850099 [details] [diff] [review]
1328318-v1.patch

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

r-, 
but only for a very minor problem in SSL_VersionRangeGetSupported(), where setting the stream range by policy to <ssl3.0,ssl3.0> will fail completely, and setting it to <ssl3.0,tls1.0> or <ssl3.0, tls2.0> will incorrectly produce ranges of <tls 1.0,tls1.0> and <tls 2.0, tls 2.0>. It is possible that SSL_LIBRARY_VERION_MAX_SUPPORTED is actually set to TLS 2.0, in which case the bug would not manifest, but would suddenly show up when the MAX_SUPPORTED version is bumped up to TLS 1.3.

The rest looks good.

::: lib/ssl/sslsock.c
@@ +2331,5 @@
>  
>      switch (protocolVariant) {
>          case ssl_variant_stream:
>              vrange->min = SSL_LIBRARY_VERSION_3_0;
>              vrange->max = SSL_LIBRARY_VERSION_MAX_SUPPORTED;

We may want to constrain the range here as well. If SSL_LIBRARY_VERSION_MAX_SUPPORTED is TLS 1.3, but we contrain the range to SSL 3 then the next step will up the min range to TLS 1.0. The final constrain below will then set the LIBRARY_VERSION to NONE and fail.

We still need your current ssl3_ConstrainVariantRangeByPolicy at line 2350 because if NSS and policy supports TLS 1.3, but policy constrains the range to TLS 1.2-TLS 1.3 then the lower test will  bump the range down to TLS 1.0, which would be incorrect (we could also just call ssl3ConstrainVarientRangeByPolicy once here and only bump the min up if the max is == TLS 1.3 and the min == SSL 3.0. In that case we would still need a separate call in the ssl_variant_datagram block.)

@@ +2443,5 @@
> +
> +    if (!vrange) {
> +        PORT_SetError(SEC_ERROR_INVALID_ARGS);
> +        return SECFailure;
> +    }

I would have put this test after the ssl_FindSocket(), but that's just bikeshedding (where it's at is actually better since ssl_FindSocket() is a non-trivial function call).

Comment 6

[Eric Rescorla (:ekr)]

Is there some way to add an automated test for this?

Comment 7

[Kai Engert (:KaiE:)]

Bob, thanks a lot for your review and identifying the bug.

I think the issue can be addressed, by applying the policy first, and to only afterwards look at the overlap.

Only if TLS 1.3 is still allowed in the resulting overlap, only then we need to check that the minimum is at least TLS 1.0


delta patch:
diff --git a/lib/ssl/sslsock.c b/lib/ssl/sslsock.c
--- a/lib/ssl/sslsock.c
+++ b/lib/ssl/sslsock.c
@@ -2328,36 +2328,43 @@ SSL_VersionRangeGetSupported(SSLProtocol
         PORT_SetError(SEC_ERROR_INVALID_ARGS);
         return SECFailure;
     }
 
     switch (protocolVariant) {
         case ssl_variant_stream:
             vrange->min = SSL_LIBRARY_VERSION_3_0;
             vrange->max = SSL_LIBRARY_VERSION_MAX_SUPPORTED;
-            // We don't allow SSLv3 and TLSv1.3 together.
-            if (vrange->max == SSL_LIBRARY_VERSION_TLS_1_3) {
-                vrange->min = SSL_LIBRARY_VERSION_TLS_1_0;
-            }
+            /* We don't allow SSLv3 and TLSv1.3 together.
+             * However, don't check yet, apply the policy first.
+             * Because if the effective supported range doesn't use TLS 1.3,
+             * then we don't need to increase the minimum. */
             break;
         case ssl_variant_datagram:
             vrange->min = SSL_LIBRARY_VERSION_TLS_1_1;
             vrange->max = SSL_LIBRARY_VERSION_MAX_SUPPORTED;
             break;
         default:
             PORT_SetError(SEC_ERROR_INVALID_ARGS);
             return SECFailure;
     }
 
     ssl3_ConstrainVariantRangeByPolicy(protocolVariant, vrange);
     if (vrange->min == SSL_LIBRARY_VERSION_NONE) {
         /* Library default and policy don't overlap. */
         return SECFailure;
     }
 
+    if (protocolVariant == ssl_variant_stream) {
+        /* We don't allow SSLv3 and TLSv1.3 together */
+        if (vrange->max == SSL_LIBRARY_VERSION_TLS_1_3) {
+            vrange->min = PR_MAX(vrange->min, SSL_LIBRARY_VERSION_TLS_1_0);
+        }
+    }
+
     return SECSuccess;
 }
 
 SECStatus
 SSL_VersionRangeGetDefault(SSLProtocolVariant protocolVariant,
                            SSLVersionRange *vrange)
 {
     if ((protocolVariant != ssl_variant_stream &&

Comment 8

[Kai Engert (:KaiE:)]

Attachment: 1328318-v2.patch

Comment 9

[Robert Relyea]

Comment on attachment 8850560 [details] [diff] [review]
1328318-v2.patch

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

Almost, You still have the second case that I pointed out (though probably not clearly enough).

::: lib/ssl/sslsock.c
@@ +2354,5 @@
> +    }
> +
> +    if (protocolVariant == ssl_variant_stream) {
> +        /* We don't allow SSLv3 and TLSv1.3 together */
> +        if (vrange->max == SSL_LIBRARY_VERSION_TLS_1_3) {

Almost, you also need to check if vrange-min == SSL_LIBRARY_VERION_SSL_3_0. So the entire if should be:

if ((vrange->max == SSL_LIBRARY_VERSION_TLS_1_3) && (vrange.min == SSL_LIBRARY_VERSION_SSL_3_0)) {

If policy constrained <TLS1.2,TLS1.3> or <TLS1.3,TLS1.3> then this code would still return <TLS1.0,TLS1.3> which would be incorrect. You should only drop SSL 3.0 in this case if SSL 3.0 is in the range.

Comment 10

[Kai Engert (:KaiE:)]

Bob, I think you didn't notice that I changed how the minimum variable is assigned.
Please look again.

It's no longer a simpler assigned to tls1.0

Instead, it's taking the PR_MAX of the effective range and tls1.0

    if (protocolVariant == ssl_variant_stream) {
        /* We don't allow SSLv3 and TLSv1.3 together */
        if (vrange->max == SSL_LIBRARY_VERSION_TLS_1_3) {
            vrange->min = PR_MAX(vrange->min, SSL_LIBRARY_VERSION_TLS_1_0);

This means, if vrange->min is tls1.2 or tls1.3, it will remain.

Only if vrange->min is smaller than tls1.0 (e.g. ssl3), then PR_MAX will cause it to be set to tls1.0

Comment 11

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie) from comment #10)
> 
> It's no longer a simpler assigned to tls1.0

It's no longer a "simple assignment" to tls1.0

Comment 12

[Kai Engert (:KaiE:)]

Attachment: alternative 1328318-v3.patch

Although I believe that patch v2 already addresses the issue, I'm providing a patch v3 with the following logic, if you prefer it for better readability.

    if (protocolVariant == ssl_variant_stream) {
        /* We don't allow SSLv3 and TLSv1.3 together */
        if (vrange->max == SSL_LIBRARY_VERSION_TLS_1_3 &&
            vrange->min == SSL_LIBRARY_VERSION_3_0) {
            vrange->min = SSL_LIBRARY_VERSION_TLS_1_0;
        }
    }

Comment 13

[Kai Engert (:KaiE:)]

(In reply to Eric Rescorla (:ekr) from comment #6)
> Is there some way to add an automated test for this?

I'll try to create something. Given that it relies on policy files being installed, it seems tricky to have that in a single binary like gtest. It might be easier to execute a tool every time with each policy, like the existing ssl.sh and sslpolicy.txt tests are created.

Maybe we could have tstclnt always call SSL_VersionRangeGetSupported() as a sanity check, prior to doing anything else, and existing with failure, if there isn't any overlap.

In addition, we could potentially have a new parameter for tstclnt, like --expected-V, that will attempt to set the given -V, and then query using SSL_VersionRangeGet if it's the expected range provided with --expected-V. This could be a test-only mode, that always exits without further action, and either returning "0" (expectation was matched) or a failure code, if the expectation wasn't matched. Then we could execute this a couple of times with various policies and -V ranges.

Comment 14

[Robert Relyea]

Comment on attachment 8850560 [details] [diff] [review]
1328318-v2.patch

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

r+ You are totally right Kai. My single focus from my previous review missed your use of PR_MAX.

Your code is more robust than my suggestion. 

bob

::: lib/ssl/sslsock.c
@@ +2356,5 @@
> +    if (protocolVariant == ssl_variant_stream) {
> +        /* We don't allow SSLv3 and TLSv1.3 together */
> +        if (vrange->max == SSL_LIBRARY_VERSION_TLS_1_3) {
> +            vrange->min = PR_MAX(vrange->min, SSL_LIBRARY_VERSION_TLS_1_0);
> +        }

Ah Kai pointed out I missed the PR_MAX. This solves the issue.

Comment 15

[Martin Thomson [:mt:]]

Kai, add long as the policy file ultimately sets a variable or calls a function, then you can use those to override the policy for testing this. Look at libssl_internals.c for all the things we tweak for testing. I think that you will find it easier and faster to create tests that use that method rather than add to the already burdened tstclnt interface.

Comment 16

[Kai Engert (:KaiE:)]

Thanks Martin for your suggestion, that's helpful. I'm looking into it.

Comment 17

[Kai Engert (:KaiE:)]

Independent of adding new tests for policy influenced version ranges:

I'd like to suggest, that our test suite shall assume, that the host system executing the test suite isn't constrained by a crypto policy, or if present, that the system's crypto allows to use the full range of SSL/TLS versions that NSS supports. Otherwise the test results might be surprising, or only test an incomplete subset.

In order to test that test execution isn't constrained, I suggest that we change the existing tests, which call the SSL_VersionRangeSet* APIs. In addition to the *Set* API, I suggest we always follow it with a call to a *Get* API, and asserts that the expected range was activated.

Comment 18

[Martin Thomson [:mt:]]

That sounds like an excellent plan Kai.  I believe that our current suite of gtests assumes that TLS 1.0 through 1.3 is enabled (unless 1.3 is disabled at compilation time).

Asserting that Get produces the same range as was Set will ensure that this is true.  e.g.,

EXPECT_EQ(set_range.max, get_range.max) << "System policy can't disallow a maximum version of " << set_range.max;

Comment 19

[Kai Engert (:KaiE:)]

Comment on attachment 8850560 [details] [diff] [review]
1328318-v2.patch

Testing revealed that patch v2 doesn't work as expected.

There was an unexpected side effect caused by function ssl3_VersionRangeIsValid(), (which is called by SSL_VersionRangeSet+Default),
because it calls ssl3_VersionIsSupported(), which calls ssl_VersionIsSupportedByPolicy().

As a result, input that isn't allowed by policy caused the function to exit with failure, prior to attempts to adjust the input.

I'll split the non-policy subset of ssl3_VersionIsSupported into a new function ssl3_VersionIsSupportedByCode(), and use that instead from the SSL_VersionRangeSet+Default as the sanity check.

Function ssl3_VersionRangeIsValid() isn't used by anyone else, so I changed it to use ssl3_VersionIsSupportedByCode(), excluding the policy check.

Functions SSL_VersionRangeSet+Default shared some common code, to avoid redundancy, I've move that to new function ssl3_CheckRangeValidAndConstrainByPolicy.

Comment 20

[Kai Engert (:KaiE:)]

Attachment: 1328318-v7-logic.patch

Updated core logic. You may use interdiff against obsolete patch v2.

Comment 21

[Kai Engert (:KaiE:)]

Attachment: 1328318-v7-system.patch

This is the addition discussed in comment 17 and comment 18, to ensure that system policy doesn't restrict test execution.

Comment 22

[Kai Engert (:KaiE:)]

Attachment: 1328318-v7-test.patch

And here are new tests.

Comment 23

[Kai Engert (:KaiE:)]

https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=f6a00f4d6ce21fe7d4dc74d0638fdd6851e49836

Comment 24

[Kai Engert (:KaiE:)]

I'll attach v8 patches. Only change is source code formatting.

Comment 25

[Kai Engert (:KaiE:)]

Attachment: 1328318-v8-logic.patch

Comment 26

[Kai Engert (:KaiE:)]

Attachment: 1328318-v8-system.patch

Comment 27

[Kai Engert (:KaiE:)]

Attachment: 1328318-v8-test.patch

Comment 28

[Martin Thomson [:mt:]]

Comment on attachment 8852465 [details] [diff] [review]
1328318-v8-test.patch

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

Thanks for putting the tests together Kai.  I have a few suggestions that I hope will improve this.

::: gtests/ssl_gtest/manifest.mn
@@ +38,5 @@
>        ssl_skip_unittest.cc \
>        ssl_staticrsa_unittest.cc \
>        ssl_v2_client_hello_unittest.cc \
>        ssl_version_unittest.cc \
> +      ssl_versionpolicy_unittest.cc \

You also need to hit the ssl_gtest.gyp file as well.

::: gtests/ssl_gtest/ssl_versionpolicy_unittest.cc
@@ +17,5 @@
> +#include "tls_filter.h"
> +#include "tls_parser.h"
> +
> +static bool SSLVersionRangesAreEqual(SSLVersionRange &vr1,
> +                                     SSLVersionRange &vr2) {

const SSLVersionRange&

@@ +25,5 @@
> +namespace nss_test {
> +
> +class TestVersionRangePolicy : public ::testing::Test {
> + protected:
> +  PRInt32 savedMinTLS;

We generally use cstdint typedefs instead of the NSPR ones for the gtests, even though the API uses NSPR: int32_t, uint32_t, etc...

@@ +32,5 @@
> +  PRInt32 savedMaxDTLS;
> +  PRUint32 savedAlgorithmPolicy;
> +
> + public:
> +  void SaveOriginalPolicy() {

You can add this to SetUp() so that you avoid having to call this function every time.

@@ +42,5 @@
> +  }
> +  void SetUsePolicyInSSL() {
> +    NSS_SetAlgorithmPolicy(SEC_OID_APPLY_SSL_POLICY, NSS_USE_POLICY_IN_SSL, 0);
> +  }
> +  void RestoreOriginalPolicy() {

This can go in TearDown.

@@ +79,5 @@
> +        return "NONE";
> +    }
> +    return "undefined???";
> +  }
> +  std::string info_str(const SSLVersionRange &policy,

whitespace between methods please.

@@ +163,5 @@
> +    SetUsePolicyInSSL();
> +
> +#ifndef NSS_DISABLE_TLS_1_3
> +    SSLVersionRange range3to13{SSL_LIBRARY_VERSION_3_0,
> +                               SSL_LIBRARY_VERSION_TLS_1_3};

It looks like you are trying to run a huge number of permutations here.  gtests have a way of parameterizing tests that you should probably take advantage of.

Also, for these inputs, rather than writing this out long-hand, a loop would be much cleaner.

Comment 29

[Kai Engert (:KaiE:)]

I had considered to use a loop for running the many permutations.
But the tricky points are:
- some expected results are unusual, how would you separate straightforward expectations from unusual expectations,
  if you want to handle everything in a loop?
- what if there's a bug in the logic that handles the above complex loop (to handle both straightforward and unsual
  expectations).

It seemed that explicitly spelling out the various expectations avoids the risk of introducing a bug into the test loop, and might be more readable.

Comment 30

[Robert Relyea]

Comment on attachment 8852463 [details] [diff] [review]
1328318-v8-logic.patch

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

r+ rrelyea

Comment 31

[Kai Engert (:KaiE:)]

Attachment: 1328318-v9-test.patch

This addresses most of Martin's requests from comment 28,
except the one about permutations and parameterizing.

I'd like to avoid that if possible. I've spent a considerable amount of time trying to come up with a good strategy for looping, but I couldn't find anything that I was happy with. I think these explicitly spelled out tests are in logic groups, so future reads will easily see the consistency in each of those groups.

Comment 32

[Martin Thomson [:mt:]]

Comment on attachment 8853032 [details] [diff] [review]
1328318-v9-test.patch

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

Kai, I realize that this gtest framework is new, but the parameterization is pretty useful here.

If you take the arguments that you pass to the main test function and turn that into a tuple in the form:

std::tuple<SSLVersionRange {policy}, SSLVersionRange {set}, SSLVersionRange {expected}, std::string or int or bool {mode}>

You can use the test permutation with very little change to what you have.  The advantage of using that method is that you can compress some of the looping by using the combining operations that are supported in gtests.  Also, you can select individual tests to run.

Take a look at how we parameterize some of the other tests for an example.

::: gtests/ssl_gtest/ssl_versionpolicy_unittest.cc
@@ +17,5 @@
> +#include "tls_filter.h"
> +#include "tls_parser.h"
> +
> +static bool SSLVersionRangesAreEqual(const SSLVersionRange &vr1,
> +                                     const SSLVersionRange &vr2) {

You can create a comparison operator

bool operator==(const SSLVersionRange& lvalue, const SSLVersionRange& rvalue) {

@@ +29,5 @@
> +  int32_t savedMinTLS;
> +  int32_t savedMaxTLS;
> +  int32_t savedMinDTLS;
> +  int32_t savedMaxDTLS;
> +  uint32_t savedAlgorithmPolicy;

I believe that the coding style here is to have methods before members, then public before protected before private.

@@ +49,5 @@
> +    NSS_OptionSet(NSS_TLS_VERSION_MIN_POLICY, savedMinTLS);
> +    NSS_OptionSet(NSS_TLS_VERSION_MAX_POLICY, savedMaxTLS);
> +    NSS_OptionSet(NSS_DTLS_VERSION_MIN_POLICY, savedMinDTLS);
> +    NSS_OptionSet(NSS_DTLS_VERSION_MAX_POLICY, savedMaxDTLS);
> +    /* If it wasn't set initially, clear the bit that we set. */

convention is // comments

@@ +88,5 @@
> +
> +  std::string info_str(const SSLVersionRange &policy,
> +                       const SSLVersionRange &vrange,
> +                       const SSLVersionRange *expectation,
> +                       const SSLVersionRange *result, bool testDTLS) {

You can create an ostream << operator for the version ranges to make this easier, which helps the tests in a number of ways.  If you take my suggestion about the parameterized testing, and add the right operators, then this will look after itself.

@@ +111,5 @@
> +                                  SSLVersionRange &expectation, bool testDTLS) {
> +    SECStatus rv;
> +
> +    SetTLSPolicy(policy);
> +    rv = SSL_VersionRangeSetDefault(ssl_variant_stream, &vrange);

Are you only testing the SetDefault variants?

@@ +140,5 @@
> +      EXPECT_EQ(SECSuccess, rv)
> +          << "expected successful return from SSL_VersionRangeGetDefault: "
> +          << info_str(policy, vrange, &expectation, NULL, true);
> +
> +      EXPECT_EQ(true, SSLVersionRangesAreEqual(result, expectation))

EXPECT_TRUE

Comment 33

[Kai Engert (:KaiE:)]

IIUC, this request:

(In reply to Martin Thomson [:mt:] from comment #32)
> 
> You can use the test permutation with very little change to what you have. 
> The advantage of using that method is that you can compress some of the
> looping by using the combining operations that are supported in gtests. 
> Also, you can select individual tests to run.


contradicts your earlier request:

> You can add this to SetUp() so that you avoid having to call this function every time.


IIUC the parameterization will cause the setup/teardown to be executed every time, while with the current code it's not.

You're requesting me to change the implementation approach to the style that you're preferring. It's true that this would allow to test individual combinations separately. I'm not convinced that small benefit is worth the rewrite of the test.

Comment 34

[Kai Engert (:KaiE:)]

I believe you have suggested that I combine looping with paramerized types.

It seems that creating parameterized types requires very verbose statements that involve INSTANTIATE_TEST_CASE_P.

INSTANTIATE_TEST_CASE_P seems to create types at compile time.

It seems that each of my current lines
    TestPolicyRangeExpectation(range3to13, range3to3, range3to3, false);

would have to be transformed into a multiple lines statement, where each one would be at least 5 lines.

I don't get the benefit of this proposal. My condensed code seems to be more readable to me.

Also, I don't understand how one would create a dynamic loop at runtime over multiple different test instantiations, because each variant would require a different INSTANTIATE_TEST_CASE_P type to be created at compile time?

Comment 35

[Kai Engert (:KaiE:)]

(In reply to Martin Thomson [:mt:] from comment #32)
> 
> @@ +111,5 @@
> > +                                  SSLVersionRange &expectation, bool testDTLS) {
> > +    SECStatus rv;
> > +
> > +    SetTLSPolicy(policy);
> > +    rv = SSL_VersionRangeSetDefault(ssl_variant_stream, &vrange);
> 
> Are you only testing the SetDefault variants?

Yes. The intention is to test that the combination of policy+input produces the expected output. Both variants Set+SetDefault call ssl3_CheckRangeValidAndConstrainByPolicy(), which covers 100% of that logic.

Comment 36

[Kai Engert (:KaiE:)]

Attachment: 1328318-v10-test.patch

Comment 37

[Kai Engert (:KaiE:)]

Comment on attachment 8856564 [details] [diff] [review]
1328318-v10-test.patch

Eric has given me some hints in email, so I'll give it another try to improve it.

Comment 38

[Kai Engert (:KaiE:)]

While extending the set of tests, and trying to test invalid policy configuration (like min > max), I found that NSS ignores the policy in that case, but the patches here don't handle these scenarios well.

After some discussions today, we concluded that NSS shouldn't ignore a contradicting policy. Instead, it should disable TLS altogether, to give the system administrator/user a chance to immediately discover the broken system configuration.

Below are some expectations, which I think NSS should implement, please comment if you disagree:

if policy.max and policy.min are in contradiction, disable TLS completly
if policy.min > policy.max, disable TLS completly

if policy.max < supported.min, disable TLS completly
if policy.min > supported.max, disable TLS completly

If the policy is invalid and TLS is disabled, GetSupported returns a range {none,none}
and TLS connectivity should fail.

if policy.min < supported.min => no problem, treat as policy.min == supported.min
if policy.max > supported.max => no problem, treat as policy.max == supported.max

Comment 39

[Kai Engert (:KaiE:)]

Attachment: 1328318-v12-logic

This updated patch handles invalid system policies better.

Comment 40

[Kai Engert (:KaiE:)]

Attachment: 1328318-v12-test

This patch adds around 5000 tests in total, that tests all SSL versions, plus invalid version numberts, that are smaller or larger than what we support today. It doesn't use known-input-known-output. Instead, it uses code to derive the expected test results.

Comment 41

[Kai Engert (:KaiE:)]

try build:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=87ea2d2312fdfc2ac95eacaeeab8dae96373904f

I'll attach updated patches for clang-format.

Also, I have attempted to submit it for review here (already includes clang-format fixes):
  https://nss-review.dev.mozaws.net/D286

Comment 42

[Kai Engert (:KaiE:)]

Attachment: 1328318-v12-logic-plus-clang-format

Comment 43

[Kai Engert (:KaiE:)]

Attachment: 1328318-v12-test-plus-clang-format

Comment 44

[Eric Rescorla (:ekr)]

Comment on attachment 8857618 [details] [diff] [review]
1328318-v12-test-plus-clang-format

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

I took a pass through this. I think it's going in the right direction but could use some revision,

::: gtests/ssl_gtest/ssl_versionpolicy_unittest.cc
@@ +38,5 @@
> +  }
> +  if (v < SSL_LIBRARY_VERSION_3_0)
> +    return "undefined-too-low";
> +  else
> +    return "undefined-too-high";

You do not need an else here.

@@ +41,5 @@
> +  else
> +    return "undefined-too-high";
> +}
> +
> +inline std::ostream &operator<<(std::ostream &stream,

& attaches to the type in this code

@@ +49,5 @@
> +}
> +
> +inline bool operator==(const SSLVersionRange &vr1, const SSLVersionRange &vr2) {
> +  return vr1.min == vr2.min && vr1.max == vr2.max;
> +}

Why don't you define this in tls_agent.h and then you can use it in both places.

@@ +52,5 @@
> +  return vr1.min == vr2.min && vr1.max == vr2.max;
> +}
> +
> +class VersionRangeWithLabel {
> + public:

This code uses Google style:

- arguments do not take aFoo
- member variables are suffixed with _

@@ +63,5 @@
> +
> +inline std::ostream &operator<<(std::ostream &stream,
> +                                const VersionRangeWithLabel &vrwl) {
> +  return stream << vrwl.label << " " << vrwl.vr;
> +}

You should be able to make this a class function and then mark the members private.

@@ +77,5 @@
> +    : public ::testing::Test,
> +      public ::testing::WithParamInterface<PolicyVersionRangeInput> {
> + public:
> +  TestPolicyVersionRange()
> +      : variant((SSLProtocolVariant)(std::get<0>(GetParam()))),

Please use C++-style casts in this code.

@@ +88,5 @@
> +    NSS_OptionGet(NSS_TLS_VERSION_MIN_POLICY, &savedMinTLS);
> +    NSS_OptionGet(NSS_TLS_VERSION_MAX_POLICY, &savedMaxTLS);
> +    NSS_OptionGet(NSS_DTLS_VERSION_MIN_POLICY, &savedMinDTLS);
> +    NSS_OptionGet(NSS_DTLS_VERSION_MAX_POLICY, &savedMaxDTLS);
> +    NSS_GetAlgorithmPolicy(SEC_OID_APPLY_SSL_POLICY, &savedAlgorithmPolicy);

Every function that returns SECStatus needs a return value check

@@ +104,5 @@
> +    // If it wasn't set initially, clear the bit that we set.
> +    if (!(savedAlgorithmPolicy & NSS_USE_POLICY_IN_SSL)) {
> +      NSS_SetAlgorithmPolicy(SEC_OID_APPLY_SSL_POLICY, 0,
> +                             NSS_USE_POLICY_IN_SSL);
> +    }

Why not just unconditionally restore the value?

@@ +119,5 @@
> +    NSS_OptionSet(NSS_DTLS_VERSION_MAX_POLICY, policy.max);
> +  }
> +
> +  void GetOverlap(const SSLVersionRange &r1, const SSLVersionRange &r2,
> +                  SSLVersionRange &overlap) {

In this code, all reference passing must be const. Out-vars must be pointers. https://google.github.io/styleguide/cppguide.html#Reference_Arguments. Here and below.

@@ +164,5 @@
> +  SSLProtocolVariant variant;
> +  uint16_t policyMin;
> +  uint16_t policyMax;
> +  uint16_t inputMin;
> +  uint16_t inputMax;

local and member variables use snake case, not camelcase, here and throughout the file.

@@ +174,5 @@
> +    SSL_LIBRARY_VERSION_TLS_1_2,
> +#ifndef NSS_DISABLE_TLS_1_3
> +    SSL_LIBRARY_VERSION_TLS_1_3,
> +#endif
> +    SSL_LIBRARY_VERSION_MAX_SUPPORTED + 1};

Can you put these in single column please.

@@ +182,5 @@
> +static ::testing::internal::ParamGenerator<uint16_t> kVariants =
> +    ::testing::ValuesIn(kVariantsArr);
> +
> +static ::testing::internal::ParamGenerator<uint16_t> kAllVersions =
> +    ::testing::ValuesIn(kAllVersionsArr);

Either put this right after versions (so the arrays and the generators interlace) or
have the generators together and in the same order.

@@ +184,5 @@
> +
> +static ::testing::internal::ParamGenerator<uint16_t> kAllVersions =
> +    ::testing::ValuesIn(kAllVersionsArr);
> +
> +TEST_P(TestPolicyVersionRange, Whatever2) {

This is perhaps not an awesome name

@@ +186,5 @@
> +    ::testing::ValuesIn(kAllVersionsArr);
> +
> +TEST_P(TestPolicyVersionRange, Whatever2) {
> +  SECStatus rv;
> +  std::string variantInfo;

Please define these closer to use.

@@ +189,5 @@
> +  SECStatus rv;
> +  std::string variantInfo;
> +
> +  SSLVersionRange policyRange = {policyMin, policyMax};
> +  VersionRangeWithLabel policyInfo(" policy:", policyRange);

All of these seem to start with " " so why not put that in the ctor?

@@ +206,5 @@
> +    variantInfo = "DTLS";
> +    librarySupport.min = SSL_LIBRARY_VERSION_MIN_SUPPORTED_DATAGRAM;
> +    SetDTLSPolicy(policyRange);
> +  }
> +  VersionRangeWithLabel librarySupportInfo(" library-support:", librarySupport);

Info is confusing because the different infos are of different types. Maybe
"variantName" or just "variant"?

@@ +211,5 @@
> +
> +  std::cerr << "testing: " << variantInfo << policyInfo << inputInfo
> +            << std::endl;
> +  ASSERT_TRUE(librarySupport.min != SSL_LIBRARY_VERSION_NONE)
> +      << "testing unsupported ssl variant";

This assert would be easier as an else clause after the else if above

@@ +229,5 @@
> +
> +    SSLVersionRange enabledRange;
> +    rv = SSL_VersionRangeGetDefault(variant, &enabledRange);
> +
> +    EXPECT_EQ(SECFailure, rv)

Extra whitespace above this.

@@ +233,5 @@
> +    EXPECT_EQ(SECFailure, rv)
> +        << "expected SSL_VersionRangeGetDefault to fail with invalid policy, "
> +        << variantInfo << policyInfo << supportedInfo;
> +
> +    // TODO: test that no TLS connections are possible

You probably want to do this TODO

@@ +252,5 @@
> +    bool haveOverlap = (overlapRange.min != SSL_LIBRARY_VERSION_NONE);
> +
> +    ASSERT_TRUE(haveOverlap) << "expected overlap between valid policy and "
> +                                "library supported versions "
> +                             << variantInfo << policyInfo << librarySupportInfo;

You don't need a temporary here if you are going to use ASSERT_TRUE. Also, maybe you could have GetOverlap() return bool

@@ +262,5 @@
> +        << "expected range from GetSupported to be identical with calculated "
> +           "overlap "
> +        << variantInfo << policyInfo << supportedInfo << overlapInfo;
> +
> +    // TODO: test that TLS connections are possible

This TODO as well

@@ +267,5 @@
> +  }
> +
> +  //----------------------------------------------------------------------------
> +
> +  rv = SSL_VersionRangeSetDefault(variant, &inputRange);

Can this stuff here be a separate test case.

@@ +309,5 @@
> +    EXPECT_EQ(SECFailure, rv)
> +        << "expected failure return from SSL_VersionRangeSetDefault with: "
> +        << variantInfo << policyInfo << inputInfo;
> +    return;
> +  }

ASSERT_EQ() would work here.

@@ +327,5 @@
> +
> +  EXPECT_TRUE(overlapWithLibAndPolicy == effective)
> +      << "range returned by SSL_VersionRangeGetDefault doesn't match "
> +         "expectation: "
> +      << variantInfo << policyInfo << inputInfo << overlapInfo << effectiveInfo;

What about SSL_VersionRangeSet()

::: gtests/ssl_gtest/tls_agent.cc
@@ +27,5 @@
>  
> +static bool SSLVersionRangesAreEqual(SSLVersionRange& vr1,
> +                                     SSLVersionRange& vr2) {
> +  return vr1.min == vr2.min && vr1.max == vr2.max;
> +}

I agree with MT that it would be better to do operator overloading for ==. In any case, this function must take const SSLVersionRange&

@@ +437,5 @@
> +    rv = SSL_VersionRangeGet(ssl_fd(), &verify_vrange);
> +    EXPECT_EQ(SECSuccess, rv);
> +    bool ranges_are_equal = SSLVersionRangesAreEqual(vrange_, verify_vrange);
> +    EXPECT_TRUE(ranges_are_equal)
> +        << "System policy must not restrict the allowed min/max SSL/TLS range";

Seems like this code appears twice, so maybe you could factor it into a method?

Comment 45

[Robert Relyea]

Kai, I will be out for the next couple of weeks. If you can wait that long, I'll be happy to review this, but if you need it sooner, try to get another reviewer.

bob

Comment 46

[Kai Engert (:KaiE:)]

(In reply to Eric Rescorla (:ekr) from comment #44)
> @@ +41,5 @@
> > +  else
> > +    return "undefined-too-high";
> > +}
> > +
> > +inline std::ostream &operator<<(std::ostream &stream,
> 
> & attaches to the type in this code

Sorry I don't understand, can you please clarify your request?

Are you requesting that I write std::ostream& ?

This is code that passed through clang-format.


> @@ +63,5 @@
> > +
> > +inline std::ostream &operator<<(std::ostream &stream,
> > +                                const VersionRangeWithLabel &vrwl) {
> > +  return stream << vrwl.label << " " << vrwl.vr;
> > +}
> 
> You should be able to make this a class function and then mark the members
> private.

You had not used that approach in your own past commit:
  https://hg.mozilla.org/projects/nss/rev/bff5daa96e2b#l15.163

Probably because it doesn't work:
  http://stackoverflow.com/questions/9814345/cant-overload-operator-as-member-function

Although I think it's overkill to introduce a separate WriteStream class for a trivial wrapper helper class like this one, I'll do as you did.


> @@ +104,5 @@
> > +    // If it wasn't set initially, clear the bit that we set.
> > +    if (!(savedAlgorithmPolicy & NSS_USE_POLICY_IN_SSL)) {
> > +      NSS_SetAlgorithmPolicy(SEC_OID_APPLY_SSL_POLICY, 0,
> > +                             NSS_USE_POLICY_IN_SSL);
> > +    }
> 
> Why not just unconditionally restore the value?

This is a bitmask. Why should I touch the bits that I don't need to touch?

Function signature:
  NSS_SetAlgorithmPolicy(SECOidTag tag, PRUint32 setBits, PRUint32 clearBits);

If I want to restore all bits, I'd have to construct the correct negative value
that covers all bits, then clear all bits, and then restore all.

I don't know if the function signature supports overlapping bits in both
parameters, so I'd have to call the function twice, once to clear everything,
then set the saved value.

Isn't it cleaner to just restore the bit that I've touched, if needed?

> @@ +164,5 @@
> > +  SSLProtocolVariant variant;
> > +  uint16_t policyMin;
> > +  uint16_t policyMax;
> > +  uint16_t inputMin;
> > +  uint16_t inputMax;
> 
> local and member variables use snake case, not camelcase, here and
> throughout the file.

It's painful if I have to rewrite my whole patch for that, it's not clear to me why we cannot have a little bit of flexibility here?

It's confusing that for function names it's apparently expected to use camelCase, but for variables it's forbidden?


> @@ +174,5 @@
> > +    SSL_LIBRARY_VERSION_TLS_1_2,
> > +#ifndef NSS_DISABLE_TLS_1_3
> > +    SSL_LIBRARY_VERSION_TLS_1_3,
> > +#endif
> > +    SSL_LIBRARY_VERSION_MAX_SUPPORTED + 1};
> 
> Can you put these in single column please.

I wanted them all in a single column, but that's how clang-format changed it. (sigh)
I conclude you want me to use clang-format off.

Comment 47

[Kai Engert (:KaiE:)]

> @@ +211,5 @@
> > +
> > +  std::cerr << "testing: " << variantInfo << policyInfo << inputInfo
> > +            << std::endl;
> > +  ASSERT_TRUE(librarySupport.min != SSL_LIBRARY_VERSION_NONE)
> > +      << "testing unsupported ssl variant";
> 
> This assert would be easier as an else clause after the else if above

My intention was that the "testing: " lines is printed first, because otherwise, how do you know which variant has produced the assertion?

I need that block above to produce the labels.
The "testing: " must appear after that block.
And if I move the assert further up, it will stand alone, without having the information in the logfile.

Comment 48

[Kai Engert (:KaiE:)]

> @@ +229,5 @@
> > +
> > +    SSLVersionRange enabledRange;
> > +    rv = SSL_VersionRangeGetDefault(variant, &enabledRange);
> > +
> > +    EXPECT_EQ(SECFailure, rv)
> 
> Extra whitespace above this.

Your request isn't clear.

Are you asking me to remove the empty line above EXPECT_EQ ?

Comment 49

[Kai Engert (:KaiE:)]

> @@ +233,5 @@
> > +    EXPECT_EQ(SECFailure, rv)
> > +        << "expected SSL_VersionRangeGetDefault to fail with invalid policy, "
> > +        << variantInfo << policyInfo << supportedInfo;
> > +
> > +    // TODO: test that no TLS connections are possible
> 
> You probably want to do this TODO

> @@ +262,5 @@
> > +        << "expected range from GetSupported to be identical with calculated "
> > +           "overlap "
> > +        << variantInfo << policyInfo << supportedInfo << overlapInfo;
> > +
> > +    // TODO: test that TLS connections are possible
> 
> This TODO as well

I was hoping to do these in a follow-up iteration.

Comment 50

[Eric Rescorla (:ekr)]

(In reply to Kai Engert (:kaie) from comment #46)
> (In reply to Eric Rescorla (:ekr) from comment #44)
> > @@ +41,5 @@
> > > +  else
> > > +    return "undefined-too-high";
> > > +}
> > > +
> > > +inline std::ostream &operator<<(std::ostream &stream,
> > 
> > & attaches to the type in this code
> 
> Sorry I don't understand, can you please clarify your request?
> 
> Are you requesting that I write std::ostream& ?

Yes. That's the style.


> This is code that passed through clang-format.

OK, but that's a tool to enforce the style, not the last word on style.


> > @@ +63,5 @@
> > > +
> > > +inline std::ostream &operator<<(std::ostream &stream,
> > > +                                const VersionRangeWithLabel &vrwl) {
> > > +  return stream << vrwl.label << " " << vrwl.vr;
> > > +}
> > 
> > You should be able to make this a class function and then mark the members
> > private.
> 
> You had not used that approach in your own past commit:
>   https://hg.mozilla.org/projects/nss/rev/bff5daa96e2b#l15.163
> 
> Probably because it doesn't work:
>  
> http://stackoverflow.com/questions/9814345/cant-overload-operator-as-member-
> function
> 
> Although I think it's overkill to introduce a separate WriteStream class for
> a trivial wrapper helper class like this one, I'll do as you did.


I didn't mean as a member function but as a class function. perhaps that doesn't work
either, thought.



> > @@ +104,5 @@
> > > +    // If it wasn't set initially, clear the bit that we set.
> > > +    if (!(savedAlgorithmPolicy & NSS_USE_POLICY_IN_SSL)) {
> > > +      NSS_SetAlgorithmPolicy(SEC_OID_APPLY_SSL_POLICY, 0,
> > > +                             NSS_USE_POLICY_IN_SSL);
> > > +    }
> > 
> > Why not just unconditionally restore the value?
> 
> This is a bitmask. Why should I touch the bits that I don't need to touch?
> 
> Function signature:
>   NSS_SetAlgorithmPolicy(SECOidTag tag, PRUint32 setBits, PRUint32
> clearBits);
> 
> If I want to restore all bits, I'd have to construct the correct negative
> value
> that covers all bits, then clear all bits, and then restore all.

Yes.


> I don't know if the function signature supports overlapping bits in both
> parameters,

Well, that seems like it should be determinable as an API invariant.


> so I'd have to call the function twice, once to clear everything,
> then set the saved value.
> 
> Isn't it cleaner to just restore the bit that I've touched, if needed?

No, I don't. If you feel strongly about it, I'm willing to let this go.


> > @@ +164,5 @@
> > > +  SSLProtocolVariant variant;
> > > +  uint16_t policyMin;
> > > +  uint16_t policyMax;
> > > +  uint16_t inputMin;
> > > +  uint16_t inputMax;
> > 
> > local and member variables use snake case, not camelcase, here and
> > throughout the file.
> 
> It's painful if I have to rewrite my whole patch for that, it's not clear to
> me why we cannot have a little bit of flexibility here?
> 
> It's confusing that for function names it's apparently expected to use
> camelCase, but for variables it's forbidden?

We're trying to have a consistent style for this code and this is the style.


> > @@ +174,5 @@
> > > +    SSL_LIBRARY_VERSION_TLS_1_2,
> > > +#ifndef NSS_DISABLE_TLS_1_3
> > > +    SSL_LIBRARY_VERSION_TLS_1_3,
> > > +#endif
> > > +    SSL_LIBRARY_VERSION_MAX_SUPPORTED + 1};
> > 
> > Can you put these in single column please.
> 
> I wanted them all in a single column, but that's how clang-format changed
> it. (sigh)
> I conclude you want me to use clang-format off.

No, if clang-format changes it I'm willing to live with it.

Comment 51

[Kai Engert (:KaiE:)]

> Also, maybe you could have GetOverlap() return bool

If I did that, I could no longer use ASSERT_ macros inside GetOverlap, because apparently those only work inside void functions.


> @@ +327,5 @@
> > +
> > +  EXPECT_TRUE(overlapWithLibAndPolicy == effective)
> > +      << "range returned by SSL_VersionRangeGetDefault doesn't match "
> > +         "expectation: "
> > +      << variantInfo << policyInfo << inputInfo << overlapInfo << effectiveInfo;
> 
> What about SSL_VersionRangeSet()

I had replied to Martin's same question in comment 35:
> The intention is to test that the combination of policy+input produces the expected output. 
> Both variants Set+SetDefault call ssl3_CheckRangeValidAndConstrainByPolicy(), which covers 100% of that logic.

Is that convincing?

Comment 52

[Kai Engert (:KaiE:)]

(In reply to Eric Rescorla (:ekr) from comment #44)
> @@ +309,5 @@
> > +    EXPECT_EQ(SECFailure, rv)
> > +        << "expected failure return from SSL_VersionRangeSetDefault with: "
> > +        << variantInfo << policyInfo << inputInfo;
> > +    return;
> > +  }
> 
> ASSERT_EQ() would work here.

No, this doesn't match my intention.

If the input is invalid, I want to stop testing, and always return. All I want to test here is that the SetDefault function failed as expected. The remainder of the function assumes correct input values, and will fail if we don't return.

Comment 53

[Kai Engert (:KaiE:)]

Attachment: 1328318-v13-test.patch

This is just a backup, still work in progress, still haven't addresses all reviewer requests.

Comment 54

[Eric Rescorla (:ekr)]

(In reply to Kai Engert (:kaie) from comment #51)
> > Also, maybe you could have GetOverlap() return bool
> 
> If I did that, I could no longer use ASSERT_ macros inside GetOverlap,
> because apparently those only work inside void functions.

Correct, you could use EXPECT_EQ() followed by return false.

>
> 
> > @@ +327,5 @@
> > > +
> > > +  EXPECT_TRUE(overlapWithLibAndPolicy == effective)
> > > +      << "range returned by SSL_VersionRangeGetDefault doesn't match "
> > > +         "expectation: "
> > > +      << variantInfo << policyInfo << inputInfo << overlapInfo << effectiveInfo;
> > 
> > What about SSL_VersionRangeSet()
> 
> I had replied to Martin's same question in comment 35:
> > The intention is to test that the combination of policy+input produces the expected output. 
> > Both variants Set+SetDefault call ssl3_CheckRangeValidAndConstrainByPolicy(), which covers 100% of that logic.
> 
> Is that convincing?

ISTM that the objective should be to test the API surface, and while the implementation might currently be as you say, that might not always be true.

Comment 55

[Eric Rescorla (:ekr)]

(In reply to Kai Engert (:kaie) from comment #48)
> > @@ +229,5 @@
> > > +
> > > +    SSLVersionRange enabledRange;
> > > +    rv = SSL_VersionRangeGetDefault(variant, &enabledRange);
> > > +
> > > +    EXPECT_EQ(SECFailure, rv)
> > 
> > Extra whitespace above this.
> 
> Your request isn't clear.
> 
> Are you asking me to remove the empty line above EXPECT_EQ ?

Yes.

Comment 56

[Martin Thomson [:mt:]]

(In reply to Eric Rescorla (:ekr) from comment #54)
> (In reply to Kai Engert (:kaie) from comment #51)
> > I had replied to Martin's same question in comment 35:
> > > The intention is to test that the combination of policy+input produces the expected output. 
> > > Both variants Set+SetDefault call ssl3_CheckRangeValidAndConstrainByPolicy(), which covers 100% of that logic.
> > 
> > Is that convincing?
> 
> ISTM that the objective should be to test the API surface, and while the
> implementation might currently be as you say, that might not always be true.

BTW, you can test both with the same code if you provide the test function with a callback.  A closure is probably easiest to use for that.

Comment 57

[Kai Engert (:KaiE:)]

While adding the socket specific tests, I discovered, if the application doesn't make any calls to set the desired version range, then SSL_VersionRangeGet() returns the set configured by the library, ignoring any policy restrictions.

(I verified using a manual test, that the library enforces the policy restriction, regardless of the configured version range for the socket.)

I suggest that SSL_VersionRangeGet() should return the effective range, after applying the policy.
If effectively no versions are available, it should return a failure, like we do in SSL_VersionRangeGetDefault()

Comment 58

[Kai Engert (:KaiE:)]

Attachment: 1328318-v14-logic

Comment 59

[Kai Engert (:KaiE:)]

Attachment: 1328318-v14-test

Comment 60

[Kai Engert (:KaiE:)]

I found another problem.

If a new socket is created, but the application doesn't make any calls to set the preferred version range, then the socket is configured with the library default.

If the policy restricts the range to forbit the highest version, then we run into an assertion failure in ssl3_NegotiateVersion():
    ss->version = PR_MIN(peerVersion, ss->vrange.max);
    PORT_Assert(ssl3_VersionIsSupported(ss->protocolVariant, ss->version));

To fix this, I suggest that when creating a socket, in ssl_NewSocket, and ssl_DupSocket, we call ssl3_CreateOverlapWithPolicy, to ensure that ss->vrange contains the effectively available range.

Comment 61

[Kai Engert (:KaiE:)]

Attachment: 1328318-v17-logic.patch

Comment 62

[Kai Engert (:KaiE:)]

Attachment: 1328318-v17-test.patch

Comment 63

[Kai Engert (:KaiE:)]

Could you please review v17 ? I've also submitted to phabricator.

Comment 64

[Kai Engert (:KaiE:)]

Attachment: 1328318-v18-logic.patch

Comment 65

[Kai Engert (:KaiE:)]

Attachment: 1328318-v18-test.patch

Comment 66

[Kai Engert (:KaiE:)]

Attachment: 1328318-v19-logic.patch

Comment 67

[Kai Engert (:KaiE:)]

Attachment: 1328318-v19-test.patch

Comment 68

[Kai Engert (:KaiE:)]

Attachment: sslsock-v17-to-v18.patch

Patch v18/v19 have gotten r+ by Martin in https://nss-review.dev.mozaws.net/D286

I was about to commit the changes, but a try run showed that we have a test regression in the selfserv based tests.

v17 still worked, v18 is broken.
I'm attaching the diff to sslsock.c between those two revisions.

Comment 69

[Kai Engert (:KaiE:)]

This new failure was caused by my change to ssl3_ConstrainRangeByPolicy, where I returned failures from ssl3_CreateOverlapWithPolicy, which caused ssl_Init to fail.

We must allow the empty overlap to be created, without failing NSS init, because the library default might not overlap with system policy, and this shouldn't prevent NSS applications to start.

I'll revert that function to the original state, and add a comment, so I don't think it needs another review.

Comment 70

[Kai Engert (:KaiE:)]

Attachment: 1328318-v20-logic.patch

Comment 71

[Kai Engert (:KaiE:)]

Both patches checked in.
https://hg.mozilla.org/projects/nss/rev/0aefc8ab1bbb
https://hg.mozilla.org/projects/nss/rev/4955a4c9b8b1

Comment 72

[Martin Thomson [:mt:]]

Just so that I understand this, Kai did you back out ALL the changes from that patch?  Or did you just ignore the error return at the appropriate place?

I see that ssl3_ConstrainRangeByPolicy(void) could be a void function now:
https://searchfox.org/nss/rev/d67e90fec8e8d8aeb45e214ed80de77289e3fd11/lib/ssl/sslsock.c#2277

I'd recommend at least that change as a follow-up.

There are too many changes in these patches for me to work out what you changed before landing.

Comment 73

[Kai Engert (:KaiE:)]

Martin, after you had looked at the patch last, I made the following changes:

- I addressed several of your requests, I commited them to phabricator, and I attached them here as patch v19

- because of the test regression, I changed ssl3_ConstrainRangeByPolicy to ignore the return values of ssl3_CreateOverlapWithPolicy.

That's all.