838769 - Run ECC SSL tests with SSL2 disabled and with TLS enabled

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

Attachment: Patch

The attached patch changes our SSL test scripts to run the ECC SSL tests
with SSL2 disabled and with TLS enabled.

If SSL2 is enabled, we use SSL2-compatible ClientHello, which cannot include
ClientHello extensions.

If TLS is disabled, we don't use ClientHello extensions either.

Since ECC cipher suites require the Supported Elliptic Curves Extension
(extension type elliptic_curves), if we cannot send ClientHello extensions,
we disable all ECC cipher suites *unless* NSS_ECC_MORE_THAN_SUITE_B is
defined. In the NSS_ECC_MORE_THAN_SUITE_B case, we support all curves,
which is why we don't need to use the Supported Elliptic Curves Extension.

In order to run the ECC SSL tests without setting NSS_ECC_MORE_THAN_SUITE_B,
I propose that we only run the tests when we can send ClientHello extensions.

Bob: you don't need to review this patch, but I'd like you to approve this
change.

Comment 1

[Wan-Teh Chang]

Comment on attachment 710871 [details] [diff] [review]
Patch

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

::: mozilla/security/nss/tests/ssl/ssl.sh
@@ +299,4 @@
>        elif [ "`echo $ectype | cut -b 1`" != "#" ] ; then
>            echo "$SCRIPTNAME: running $testname ----------------------------"
>            VMAX="ssl3"
> +          if [ "$testmax" = "TLS10" ]; then

We forgot to make this change when we changed "TLS" to "TLS10" in sslcov.txt.
Because of this mistake, the tests that say "TLS10" are getting VMAX="ssl3",
so TLS 1.0 is actually disabled :-)

Comment 2

[Wan-Teh Chang]

Attachment: Also require the elliptic_curves extension in the NSS_ECC_MORE_THAN_SUITE_B case

Bob, please review this patch.

The effect of this patch is that the "Enhanced ECC" build of NSS (which
supports all defined named curves today) will also disable ECC cipher
suites when it cannot send ClientHello extensions.

In the unlikely event that new named curves are added to the IANA TLS
EC Named Curve Registry, our current behavior will be wrong because we
won't support all the defined named curves.

This patch will only change the behavior of the "Enhanced ECC" build
when an NSS SSL client disables TLS. This is an uninteresting corner
case.

Comment 3

[Kai Engert [:KaiE:]]

Comment on attachment 710871 [details] [diff] [review]
Patch

(In reply to Wan-Teh Chang from comment #1)
> > +          if [ "$testmax" = "TLS10" ]; then
> 
> We forgot to make this change when we changed "TLS" to "TLS10" in sslcov.txt.

Thank you for finding this mistake.

The remainder of your proposal sounds good to me.

Comment 4

[Kai Engert [:KaiE:]]

I propose to include this testing correctness in 3.14.3

Comment 5

[Kai Engert [:KaiE:]]

We already fixed this important part in bug 839109.
-          if [ "$testmax" = "TLS" ]; then
+          if [ "$testmax" = "TLS10" ]; then

The remainder of this bug is probably less urgent; removing the target milestone.

Comment 6

[Wan-Teh Chang]

Comment on attachment 710871 [details] [diff] [review]
Patch

The rest of this patch checked in on the NSS trunk (NSS 3.14.4).

Checking in sslcov.txt;
/cvsroot/mozilla/security/nss/tests/ssl/sslcov.txt,v  <--  sslcov.txt
new revision: 1.18; previous revision: 1.17
done
Checking in sslstress.txt;
/cvsroot/mozilla/security/nss/tests/ssl/sslstress.txt,v  <--  sslstress.txt
new revision: 1.22; previous revision: 1.21
done

Comment 7

[Wan-Teh Chang]

Comment on attachment 710877 [details] [diff] [review]
Also require the elliptic_curves extension in the NSS_ECC_MORE_THAN_SUITE_B case

I described this change to Bob Relyea and Adam Langley. Both of them
agreed with this change. (They both pointed out that new curves may
be added.) So I checked in this patch before their official r+.

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.205; previous revision: 1.204
done
Checking in sslcon.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslcon.c,v  <--  sslcon.c
new revision: 1.53; previous revision: 1.52
done

Comment 8

[Adam Langley]

Comment on attachment 710877 [details] [diff] [review]
Also require the elliptic_curves extension in the NSS_ECC_MORE_THAN_SUITE_B case

Agreed. I think that NSS_ECC_MORE_THAN_SUITE_B should be enabling more curves and so shouldn't be used in libssl at all, except to advertise/gate support for those curves.

Comment 9

[Wan-Teh Chang]

Comment on attachment 710877 [details] [diff] [review]
Also require the elliptic_curves extension in the NSS_ECC_MORE_THAN_SUITE_B case

I had to revert this patch. It broke the ECC SSL tests
on the "memleak" tinderboxes because tests/memleak/memleak.sh
runs strsclnt with SSL2 enabled. I need to fix memleak.sh
first.

Comment 10

[Wan-Teh Chang]

Attachment: Change memleak.sh to not test SSL2 cipher suites and to run strsclnt with SSL2 disabled

With more work, I can preserve the SSL2 cipher suite testing
in memleak.sh, but I think it's not worthwhile. So I removed
the six SSL2 cipher suites (capital letters A-F).

I set the minimum version to "ssl3" to disable SSL2.  The
maximum version (VMAX) is either unbounded or tls1.0, depending
on the cipher suite.

Comment 11

[Kai Engert [:KaiE:]]

Comment on attachment 714791 [details] [diff] [review]
Change memleak.sh to not test SSL2 cipher suites and to run strsclnt with SSL2 disabled

I'm fixing the description of this patch to say "not test SSL2 cipher suites", I think that was your intention.

Comment 12

[Kai Engert [:KaiE:]]

Comment on attachment 714791 [details] [diff] [review]
Change memleak.sh to not test SSL2 cipher suites and to run strsclnt with SSL2 disabled

Could future changes to the SSL/TLS protocol code introduce new memory leaks in the SSL2 code? If yes, then we won't detect them after this chance.

I can't tell if that's acceptable. It might, because we have discouraged the use of SSL2.

I'm giving r=kaie to this patch, because it correctly does what it describes.

But I'm asking Bob to approve the idea of no longer memory-leak-testing the SSL2 code.

Comment 13

[Wan-Teh Chang]

Attachment: Change memleak.sh to run strsclnt with SSL2 enabled only when testing the six SSL2 cipher suites

Kai, to eliminate any concern about the need to test our SSL2 code,
I improved my patch. Thanks.

Comment 14

[Kai Engert [:KaiE:]]

Comment on attachment 715235 [details] [diff] [review]
Change memleak.sh to run strsclnt with SSL2 enabled only when testing the six SSL2 cipher suites

Thank you, this patch looks great. r=kaie

Comment 15

[Wan-Teh Chang]

Attachment: Change memleak.sh to run strsclnt with SSL2 enabled only when testing the six SSL2 cipher suites, v3

Kai, I found that I can further improve the code by converting
the existing if statement for export cipher suites to a case
in the case statement. If you don't like it, I'll change it back.

Patch checked in on the NSS trunk (NSS 3.14.4).

Checking in memleak.sh;
/cvsroot/mozilla/security/nss/tests/memleak/memleak.sh,v  <--  memleak.sh
new revision: 1.43; previous revision: 1.42
done

Comment 16

[Wan-Teh Chang]

Comment on attachment 715264 [details] [diff] [review]
Change memleak.sh to run strsclnt with SSL2 enabled only when testing the six SSL2 cipher suites, v3

For reference, here is the strsclnt options for |cipher| in

cipher_list="A B C D E F :C001 :C002 :C003 :C004 :C005 :C006 :C0
07 :C008 :C009 :C00A :C010 :C011 :C012 :C013 :C014 c d e f g i j k l m n v y z"

one |cipher| per line:

 -C A -V ssl2:
 -C B -V ssl2:
 -C C -V ssl2:
 -C D -V ssl2:
 -C E -V ssl2:
 -C F -V ssl2:
 -C :C001 -V ssl3:
 -C :C002 -V ssl3:
 -C :C003 -V ssl3:
 -C :C004 -V ssl3:
 -C :C005 -V ssl3:
 -C :C006 -V ssl3:
 -C :C007 -V ssl3:
 -C :C008 -V ssl3:
 -C :C009 -V ssl3:
 -C :C00A -V ssl3:
 -C :C010 -V ssl3:
 -C :C011 -V ssl3:
 -C :C012 -V ssl3:
 -C :C013 -V ssl3:
 -C :C014 -V ssl3:
 -C c -V ssl3:
 -C d -V ssl3:
 -C e -V ssl3:
 -C f -V ssl3:tls1.0
 -C g -V ssl3:tls1.0
 -C i -V ssl3:
 -C j -V ssl3:
 -C k -V ssl3:
 -C l -V ssl3:
 -C m -V ssl3:
 -C n -V ssl3:
 -C v -V ssl3:
 -C y -V ssl3:
 -C z -V ssl3:

Comment 17

[Wan-Teh Chang]

Comment on attachment 710877 [details] [diff] [review]
Also require the elliptic_curves extension in the NSS_ECC_MORE_THAN_SUITE_B case

I checked in this patch again after fixing tests/memleak/memleak.sh.

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.207; previous revision: 1.206
done
Checking in sslcon.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslcon.c,v  <--  sslcon.c
new revision: 1.55; previous revision: 1.54
done

Comment 18

[Robert Relyea]

Comment on attachment 710871 [details] [diff] [review]
Patch

r+ rrelyea

Comment 19

[Robert Relyea]

Comment on attachment 710877 [details] [diff] [review]
Also require the elliptic_curves extension in the NSS_ECC_MORE_THAN_SUITE_B case

r- Arg. Unforunately This is not the case.

Red Hat build NSS with MORE_THAN_SUITE_B on, but does not have an ECC implementation. You will see runtime code that determines is we really can support all the curves. You will need to do the runtime checks if MORE_THAN_SUITE_B is set.

bob

Comment 20

[Robert Relyea]

oops, Sorry I took so long to get to the review, but there is more work needed here...

Comment 21

[Wan-Teh Chang]

Comment on attachment 710877 [details] [diff] [review]
Also require the elliptic_curves extension in the NSS_ECC_MORE_THAN_SUITE_B case

Bob, you may have misunderstood what this patch does.

This patch says: if a client can't send ClientHello extensions,
for example, because only SSL 3.0 is enabled, then it should
disable all ECC cipher suites, whether NSS_ECC_MORE_THAN_SUITE_B
is defined or not.

Do you still think this patch is wrong?

Comment 22

[Robert Relyea]

I read the patch backwards. closing the bug again...