Bug 1252849 - Remove export-grade cipher suites

Bug 1252849 - Remove export-grade cipher suites

[mt, 2016-03-02 19:37:30]

This is pretty good.  A few lapses with public header files, but other than that
it looks OK.  There's a chance that there are bits you missed, but I couldn't
see anything.  We can remove any unused stuff later.

https://codereview.appspot.com/290970043/diff/1/lib/softoken/pkcs11c.c
File lib/softoken/pkcs11c.c (left):

https://codereview.appspot.com/290970043/diff/1/lib/softoken/pkcs11c.c#oldcod...
lib/softoken/pkcs11c.c:6471: ((!ssl3_keys->bIsExport) * IVSize));
I think that we should fail here if bIsExport is true.  In case someone is
setting this.  I looked for a way that this function was invoked, but couldn't
see it, is there a public API that leads here?

https://codereview.appspot.com/290970043/diff/1/lib/ssl/dtlscon.c
File lib/ssl/dtlscon.c (left):

https://codereview.appspot.com/290970043/diff/1/lib/ssl/dtlscon.c#oldcode32
lib/ssl/dtlscon.c:32: static const ssl3CipherSuite nonDTLSSuites[] = {
I wonder if we can amend this to be a check for !RC4.

https://codereview.appspot.com/290970043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (left):

https://codereview.appspot.com/290970043/diff/1/lib/ssl/ssl3con.c#oldcode4138
lib/ssl/ssl3con.c:4138: key_material_params.bIsExport =
(CK_BBOOL)(kea_def->is_limited);
I think that this should be set to PR_FALSE.

https://codereview.appspot.com/290970043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/290970043/diff/1/lib/ssl/ssl3con.c#newcode297
lib/ssl/ssl3con.c:297: {kea_rsa_fips,       kt_rsa,  ssl_sign_rsa,   PR_TRUE, 
PR_FALSE, SEC_OID_TLS_RSA},
This one is next in the firing line.

https://codereview.appspot.com/290970043/diff/1/lib/ssl/ssl3con.c#newcode9468
lib/ssl/ssl3con.c:9468: PORT_SetError(SEC_ERROR_UNSUPPORTED_KEYALG);
Isn't this LIBRARY_FAILURE?

https://codereview.appspot.com/290970043/diff/1/lib/ssl/sslinfo.c
File lib/ssl/sslinfo.c (right):

https://codereview.appspot.com/290970043/diff/1/lib/ssl/sslinfo.c#newcode139
lib/ssl/sslinfo.c:139: #define C_RC2 "RC2", calg_rc2
unused now, I think

https://codereview.appspot.com/290970043/diff/1/lib/ssl/sslinfo.c#newcode187
lib/ssl/sslinfo.c:187: {0,CS(TLS_RSA_WITH_SEED_CBC_SHA),             S_RSA,
K_RSA, C_SEED,B_128, M_SHA, 1, 0, 0 },
It might pay to go through these and remove those we don't want or need.

https://codereview.appspot.com/290970043/diff/1/lib/ssl/sslt.h
File lib/ssl/sslt.h (left):

https://codereview.appspot.com/290970043/diff/1/lib/ssl/sslt.h#oldcode212
lib/ssl/sslt.h:212: PRUintn isExportable : 1;
Can't remove this, sorry.

https://codereview.appspot.com/290970043/diff/1/lib/util/pkcs11t.h
File lib/util/pkcs11t.h (left):

https://codereview.appspot.com/290970043/diff/1/lib/util/pkcs11t.h#oldcode1640
lib/util/pkcs11t.h:1640: CK_BBOOL                bIsExport;
Can't remove this these.  Just annotate this and the others and make sure that a
value of PR_TRUE causes an error.  (Ignoring it might also be OK, but it means
that apps using this will get a surprising answer).

[ttaubert, 2016-03-03 10:56:49]

https://codereview.appspot.com/290970043/diff/1/lib/softoken/pkcs11c.c
File lib/softoken/pkcs11c.c (left):

https://codereview.appspot.com/290970043/diff/1/lib/softoken/pkcs11c.c#oldcod...
lib/softoken/pkcs11c.c:6471: ((!ssl3_keys->bIsExport) * IVSize));
On 2016/03/02 19:37:31, mt wrote:
> I think that we should fail here if bIsExport is true.  In case someone is
> setting this.  I looked for a way that this function was invoked, but couldn't
> see it, is there a public API that leads here?

Done.

We do have PK11_DeriveWithTemplate(), PK11_PubDerive(), and
PK11_PubDeriveWithKDF() that all lead here, I think it should be possible to set
bIsExport=true. Is it better to just ignore it in this case or fail?

https://codereview.appspot.com/290970043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (left):

https://codereview.appspot.com/290970043/diff/1/lib/ssl/ssl3con.c#oldcode4138
lib/ssl/ssl3con.c:4138: key_material_params.bIsExport =
(CK_BBOOL)(kea_def->is_limited);
On 2016/03/02 19:37:31, mt wrote:
> I think that this should be set to PR_FALSE.

Done.

https://codereview.appspot.com/290970043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/290970043/diff/1/lib/ssl/ssl3con.c#newcode9468
lib/ssl/ssl3con.c:9468: PORT_SetError(SEC_ERROR_UNSUPPORTED_KEYALG);
On 2016/03/02 19:37:31, mt wrote:
> Isn't this LIBRARY_FAILURE?

Yeah, you're right.

https://codereview.appspot.com/290970043/diff/1/lib/ssl/sslinfo.c
File lib/ssl/sslinfo.c (right):

https://codereview.appspot.com/290970043/diff/1/lib/ssl/sslinfo.c#newcode139
lib/ssl/sslinfo.c:139: #define C_RC2 "RC2", calg_rc2
On 2016/03/02 19:37:31, mt wrote:
> unused now, I think

Indeed, I chose to leave it in for now and refresh bug 1240529 after this lands.

https://codereview.appspot.com/290970043/diff/1/lib/ssl/sslinfo.c#newcode187
lib/ssl/sslinfo.c:187: {0,CS(TLS_RSA_WITH_SEED_CBC_SHA),             S_RSA,
K_RSA, C_SEED,B_128, M_SHA, 1, 0, 0 },
On 2016/03/02 19:37:31, mt wrote:
> It might pay to go through these and remove those we don't want or need.

Agreed, but we should probably do that in a follow-up?

https://codereview.appspot.com/290970043/diff/1/lib/ssl/sslt.h
File lib/ssl/sslt.h (left):

https://codereview.appspot.com/290970043/diff/1/lib/ssl/sslt.h#oldcode212
lib/ssl/sslt.h:212: PRUintn isExportable : 1;
On 2016/03/02 19:37:31, mt wrote:
> Can't remove this, sorry.

Ok, reverted.

https://codereview.appspot.com/290970043/diff/1/lib/util/pkcs11t.h
File lib/util/pkcs11t.h (left):

https://codereview.appspot.com/290970043/diff/1/lib/util/pkcs11t.h#oldcode1640
lib/util/pkcs11t.h:1640: CK_BBOOL                bIsExport;
On 2016/03/02 19:37:31, mt wrote:
> Can't remove this these.  Just annotate this and the others and make sure that
a
> value of PR_TRUE causes an error.  (Ignoring it might also be OK, but it means
> that apps using this will get a surprising answer).

Done.

[mt, 2016-03-03 21:28:38]

https://codereview.appspot.com/290970043/diff/1/lib/softoken/pkcs11c.c
File lib/softoken/pkcs11c.c (left):

https://codereview.appspot.com/290970043/diff/1/lib/softoken/pkcs11c.c#oldcod...
lib/softoken/pkcs11c.c:6471: ((!ssl3_keys->bIsExport) * IVSize));
On 2016/03/03 10:56:48, ttaubert wrote:
> On 2016/03/02 19:37:31, mt wrote:
> > I think that we should fail here if bIsExport is true.  In case someone is
> > setting this.  I looked for a way that this function was invoked, but
couldn't
> > see it, is there a public API that leads here?
> 
> Done.
> 
> We do have PK11_DeriveWithTemplate(), PK11_PubDerive(), and
> PK11_PubDeriveWithKDF() that all lead here, I think it should be possible to
set
> bIsExport=true. Is it better to just ignore it in this case or fail?

I think that failing is cleanest.  If someone is passing this in with true, then
they probably want to learn that there is a problem quickly.

A test for this might be a good idea.

https://codereview.appspot.com/290970043/diff/1/lib/ssl/sslinfo.c
File lib/ssl/sslinfo.c (right):

https://codereview.appspot.com/290970043/diff/1/lib/ssl/sslinfo.c#newcode187
lib/ssl/sslinfo.c:187: {0,CS(TLS_RSA_WITH_SEED_CBC_SHA),             S_RSA,
K_RSA, C_SEED,B_128, M_SHA, 1, 0, 0 },
On 2016/03/03 10:56:48, ttaubert wrote:
> On 2016/03/02 19:37:31, mt wrote:
> > It might pay to go through these and remove those we don't want or need.
> 
> Agreed, but we should probably do that in a follow-up?

Definitely.

[ttaubert, 2016-03-09 13:41:23]

https://codereview.appspot.com/290970043/diff/1/lib/softoken/pkcs11c.c
File lib/softoken/pkcs11c.c (left):

https://codereview.appspot.com/290970043/diff/1/lib/softoken/pkcs11c.c#oldcod...
lib/softoken/pkcs11c.c:6471: ((!ssl3_keys->bIsExport) * IVSize));
On 2016/03/03 21:28:37, mt wrote:
> On 2016/03/03 10:56:48, ttaubert wrote:
> > On 2016/03/02 19:37:31, mt wrote:
> > > I think that we should fail here if bIsExport is true.  In case someone is
> > > setting this.  I looked for a way that this function was invoked, but
> couldn't
> > > see it, is there a public API that leads here?
> > 
> > Done.
> > 
> > We do have PK11_DeriveWithTemplate(), PK11_PubDerive(), and
> > PK11_PubDeriveWithKDF() that all lead here, I think it should be possible to
> set
> > bIsExport=true. Is it better to just ignore it in this case or fail?
> 
> I think that failing is cleanest.  If someone is passing this in with true,
then
> they probably want to learn that there is a problem quickly.
> 
> A test for this might be a good idea.

Added a test.

https://codereview.appspot.com/290970043/diff/1/lib/ssl/sslinfo.c
File lib/ssl/sslinfo.c (right):

https://codereview.appspot.com/290970043/diff/1/lib/ssl/sslinfo.c#newcode187
lib/ssl/sslinfo.c:187: {0,CS(TLS_RSA_WITH_SEED_CBC_SHA),             S_RSA,
K_RSA, C_SEED,B_128, M_SHA, 1, 0, 0 },
On 2016/03/03 21:28:37, mt wrote:
> On 2016/03/03 10:56:48, ttaubert wrote:
> > On 2016/03/02 19:37:31, mt wrote:
> > > It might pay to go through these and remove those we don't want or need.
> > 
> > Agreed, but we should probably do that in a follow-up?
> 
> Definitely.

Filed bug 1254513.

[mt, 2016-03-09 17:35:36]

https://codereview.appspot.com/290970043/diff/40001/external_tests/pk11_gtest...
File external_tests/pk11_gtest/pk11_export_unittest.cc (right):

https://codereview.appspot.com/290970043/diff/40001/external_tests/pk11_gtest...
external_tests/pk11_gtest/pk11_export_unittest.cc:29: &keyItem, nullptr));
Am I reading this correctly?  Is this importing a key from uninitialized memory?

[ttaubert, 2016-03-09 19:35:14]

On 2016/03/09 17:35:36, mt wrote:
>
https://codereview.appspot.com/290970043/diff/40001/external_tests/pk11_gtest...
> File external_tests/pk11_gtest/pk11_export_unittest.cc (right):
> 
>
https://codereview.appspot.com/290970043/diff/40001/external_tests/pk11_gtest...
> external_tests/pk11_gtest/pk11_export_unittest.cc:29: &keyItem, nullptr));
> Am I reading this correctly?  Is this importing a key from uninitialized
memory?

Yeah, it's like a really bad random value. We don't really care about the values
itself here as I don't have a test vector for this. The test just ensures it's
working (or not) with bIsExport=true/false.

If you don't like this I could also initialize to zero.

[mt, 2016-03-09 19:42:18]

On 2016/03/09 19:35:14, ttaubert wrote:
> If you don't like this I could also initialize to zero.

Initialize to something, anything.