Closed Bug 838769 Opened 8 years ago Closed 7 years ago

Run ECC SSL tests with SSL2 disabled and with TLS enabled

Categories

(NSS :: Libraries, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wtc, Assigned: wtc)

References

Details

Attachments

(3 files, 2 obsolete files)

Attached patch PatchSplinter Review
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.
Attachment #710871 - Flags: review?(kaie)
Attachment #710871 - Flags: feedback?(rrelyea)
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 :-)
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.
Attachment #710877 - Flags: review?(rrelyea)
Blocks: 838778
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.
Attachment #710871 - Flags: review?(kaie) → review+
I propose to include this testing correctness in 3.14.3
Target Milestone: --- → 3.14.3
Depends on: 839109
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.
Target Milestone: 3.14.3 → ---
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
Attachment #710871 - Flags: checked-in+
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
Attachment #710877 - Flags: review?(agl)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.14.4
Attachment #710877 - Flags: checked-in+
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.
Attachment #710877 - Flags: review?(agl) → review+
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.
Attachment #710877 - Flags: checked-in+
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.
Attachment #714791 - Flags: review?(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.
Attachment #714791 - Attachment description: Change memleak.sh to not test SSL cipher suites and to run strsclnt with SSL2 disabled → Change memleak.sh to not test SSL2 cipher suites and to run strsclnt with SSL2 disabled
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.
Attachment #714791 - Flags: review?(kaie)
Attachment #714791 - Flags: review+
Attachment #714791 - Flags: feedback?(rrelyea)
Kai, to eliminate any concern about the need to test our SSL2 code,
I improved my patch. Thanks.
Attachment #714791 - Attachment is obsolete: true
Attachment #714791 - Flags: feedback?(rrelyea)
Attachment #715235 - Flags: review?
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
Attachment #715235 - Flags: review? → review+
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
Attachment #715235 - Attachment is obsolete: true
Attachment #715264 - Flags: review?(kaie)
Attachment #715264 - Flags: checked-in+
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:
Attachment #715264 - Flags: review?(kaie) → review+
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
Attachment #710877 - Flags: checked-in+
Comment on attachment 710871 [details] [diff] [review]
Patch

r+ rrelyea
Attachment #710871 - Flags: feedback?(rrelyea) → feedback+
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
Attachment #710877 - Flags: review?(rrelyea) → review-
oops, Sorry I took so long to get to the review, but there is more work needed here...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
I read the patch backwards. closing the bug again...
Status: REOPENED → RESOLVED
Closed: 8 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: 3.14.4 → 3.15
You need to log in before you can comment on or make changes to this bug.