Closed Bug 453234 Opened 16 years ago Closed 15 years ago

Support for SEED Cipher Suites to TLS RFC4010

Categories

(NSS :: Libraries, enhancement, P4)

x86
All
enhancement

Tracking

(Not tracked)

RESOLVED FIXED
3.12.3

People

(Reporter: kang.yeonjung, Assigned: nelson)

References

Details

(Whiteboard: FIPS)

Attachments

(8 files, 7 obsolete files)

113.47 KB, patch
Details | Diff | Splinter Review
66.44 KB, patch
Details | Diff | Splinter Review
78.62 KB, patch
nelson
: superreview-
Details | Diff | Splinter Review
70.17 KB, patch
Details | Diff | Splinter Review
6.74 KB, patch
Details | Diff | Splinter Review
75.74 KB, patch
Details | Diff | Splinter Review
75.67 KB, patch
Details | Diff | Splinter Review
1.22 KB, patch
nelson
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1)
Build Identifier: 

Support for SEED Cipher Suites to TLS RFC4010

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Attached patch patch for NSS-trunk (obsolete) — Splinter Review
It seems like this bug should not be security-sensitive.  Can we un-hide the bug?

Also, I suspect the NSS patch should be in a bug in the NSS product (which may already exist), and the rest should be in a bug in the Core product, Security:PSM component.  (It looks like the NSS patch is included in the other patch, and the non-NSS part of that patch is really rather small.)
Summary: Support for SEED Cipher Suites to TLS RFC4010 → Support for SEED Cipher Suites to TLS RFC4162
Duplicate of bug 307210, although a bug with a patch is better than one without.

This is horribly misfiled as a "localization" bug.
Blocks: 307210
Changing component to product NSS, as most of the patch changes files in NSS.

The only difference between the two patches:
Two small changes to PSM to static data, in order to enable SEED and make it known to the application preferences (about:config).
Assignee: channy → nobody
Status: UNCONFIRMED → NEW
Component: ko / Korean → Libraries
Ever confirmed: true
Product: Mozilla Localizations → NSS
QA Contact: admin → libraries
Version: unspecified → trunk
Yeonjung Kang, unless you object, we will open this bug to the public in a couple of days.
Very happy to open this bug.

Furthermore, SEED will be added to PKCS#11 v2.30 that is under development.
Summary: Support for SEED Cipher Suites to TLS RFC4162 → Support for SEED Cipher Suites to TLS RFC4010
Group: core-security
DO I  know the progress of Bug 453234?

PLEASE LET ME KNOW, I want to do next.
Yeonjung, I looked at your patch briefly, and I am worried about the license headers used in some files. I can see license headers from "copyright KISA" and some from openssl.

All code that lives in Mozilla or NSS must be triple-licensed under MPL/GPL/LGPL.

I am not a lawyer, but if you have copied code from elsewhere, I think we would need explicit approval from copyright holders that re-licensing under the MPL/GPL/LGPL is permitted.
In case of Camellia, all patches were written under MPL/GPL/LGPL license as like Bug #361025. The KISA Licensed code is very non sense in Mozilla code.
Your next steps should be to ask for a review (probably my myself. for the NSS code and kaie for the PSM code).

I suggest you split your patch up accordingly (It looks like you've attached 2 copies of the same patch). The NSS portion is anything under mozilla/security/nss. The NSS portion is the largest. Because of it's size is will take me a while to complete the review.

firefox/PSM portion is everything else.

bob
The license at the top of the first patch, beginning:
"Copyright (c) 2007 KISA(Korea Information Security Agency). All rights reserved."
is fine.

However, including OpenSSL code is not fine - their license is incompatible with the GPL, which is one of the three licenses Mozilla code needs to be compatible with. Such code will need to be removed, replaced or rewritten. Sorry :-|

Neither is it fine, incidentally, to stick a KISA license statement on the top of an OpenSSL-licensed file, which implies that KISA owns the copyright (which is not true) and that the KISA terms apply (which they do not).

Gerv
Here are some more issues with the two patches presently attached.
1) There are two sets of patches for mozilla/security/nss/lib.
There should only be one patch for those files, and it should be for 
the NSS trunk, ONLY.

2) The patches add no code to NSS test programs to test out the new
implementations.  It is not acceptable to contribute new code to NSS
shared libraries but contribute no code to test the new library code.
You need to ensure that the SSL test programs and the blapi test 
programs (bltest) can test the SEED functions, and that the test 
scripts cipher.sh and ssl.sh test them.

Yeonjung, please attach 3 new patches to this bug.  The three patches
should be:
- a patch to all the existing files in the NSS trunk in mozilla/security/nss/. 
  This will include patches for the test programs and test scripts.
  Please request a review of that patch, using my email address as the 
  requested reviewer.
- a patch to all the existing files NOT in mozilla/security/nss/
- a patch with any new files to be added to NSS.

Regarding the entirely new source files, IF KISA is the sole author of 
those files, then KISA, as the sole author, probably has the right to contribute those source files to NSS under the "tri-license" license terms
(which you find at the beginning of most NSS source files), without any 
mention of OpenSSL or any other licenses.  However if KISA is NOT the sole 
author of those files, then it may be very difficult or impossible to get 
all the contributing authors of that code to agree to also relicense that
code to Mozilla under the tri-license terms.
"Copyright (c) 2007 KISA(Korea Information Security Agency)" is no problem.

SEED is a royalty-free.

We wiil be making progress of required patch.

Thank you for your interest in Bug 453234.
Blocks: FIPS2008
Attached patch patch for NSS-trunk v2 (obsolete) — Splinter Review
Thanks for many comment.

Some changes are as follows.
-------------------------------------------------------------------------------
1) There are two sets of patches for mozilla/security/nss/lib.
There should only be one patch for those files, and it should be for
the NSS trunk, ONLY.

     ---> We revewed the NSS-trunk patch file required.
 

2) The patches add no code to NSS test programs to test out the new
implementations.  It is not acceptable to contribute new code to NSS
shared libraries but contribute no code to test the new library code.
You need to ensure that the SSL test programs and the blapi test
programs (bltest) can test the SEED functions, and that the test
scripts cipher.sh and ssl.sh test them.
Yeonjung, please attach 3 new patches to this bug.  The three patches
should be:
- a patch to all the existing files in the NSS trunk in mozilla/security/nss/.
  This will include patches for the test programs and test scripts.
  Please request a review of that patch, using my email address as the
  requested reviewer.
- a patch to all the existing files NOT in mozilla/security/nss/
- a patch with any new files to be added to NSS.

     --->  We included patch for the test programs and test scripts.


Regarding the entirely new source files, IF KISA is the sole author of
those files, then KISA, as the sole author, probably has the right to
contribute those source files to NSS under the "tri-license" license terms
(which you find at the beginning of most NSS source files), without any
mention of OpenSSL or any other licenses.  However if KISA is NOT the sole
author of those files, then it may be very difficult or impossible to get
all the contributing authors of that code to agree to also relicense that
code to Mozilla under the tri-license terms.

 
     --->  We added to code to agree to also relicense that code to Mozilla under the tri-license terms.

------------------------------------------------------------------------------------ 

If the patch has other bugs, please comment me.
Attachment #336412 - Attachment is obsolete: true
Priority: -- → P4
Whiteboard: FIPS
Target Milestone: --- → 3.12.7
Comment on attachment 343019 [details] [diff] [review]
patch for NSS-trunk v2

Adding flag to remind me to review this.

NOTE, in the future you may want to request a review or the patch will just sit around.

bob
Attachment #343019 - Flags: review?(rrelyea)
Yeonjung: the new patch says that the code is copyright NTT. Do you work for NTT, or do you have their permission to license it under the tri-licence (MPL/LGPL/GPL)?

Gerv
Comment on attachment 343019 [details] [diff] [review]
patch for NSS-trunk v2

The MPL/GPL/LGPL triple license with NTT copyright is copied from
the Camellia files.  This is wrong.  It is fine to use a BSD license
with KISA copyright.  You should not blindly copy the license/copyright
header from the Camellia files.

I also see #include "openssl/e_os2.h" in this patch.
If this bug is completed by Nov17 2008 it will be included in the FIPS2008 validation otherwise it will be dropped for a later release.
Attached patch patch for NSS-trunk v3 (obsolete) — Splinter Review
버그질라 게시판에 남길 글입니다.


We feel sorry about the updated patch.

We referred to Camellia similar to development environment but there was a mistake during the update.

This patch reflects the required comments as above.

Some changes are as follows.
-------------------------------------------
The MPL/GPL/LGPL triple license with NTT copyright is copied from the Camellia files. 
  --> We modified “BSD license with KISA copyright”.

I also see #include "openssl/e_os2.h" in this patch.
  --> We removed the explanatory notes for #include "openssl/e_os2.h".
----------------------------------------------------

We will pay more attention to this bug to be completed by Nov17 2008.
Attachment #343019 - Attachment is obsolete: true
Attachment #343019 - Flags: review?(rrelyea)
Yeonjung: I'm afraid the new license headers you have inserted are still not suitable. :-( 

I'm not going to tell you what's wrong with them because I think that you need to find someone within your organization with a good command of both English and licensing to review this bug and our licensing documents and put together the correct headers. I would strongly suggest that you consult your company lawyers and make sure a) that you own all this code or have permission to license it in the appropriate way, and b) you know what the correct license headers are.

I don't want to sound unhelpful, but if I just tell you what they "should" say, you will just change them that way and we would still not have confidence that you have the right to license the code. We take licensing very seriously, so it is important that anyone who contributes code does so legally.

Gerv
Comment on attachment 343512 [details] [diff] [review]
patch for NSS-trunk v3

r- for the primary reason the patch appears to be incomplete. 

seedblapi.c implements a correct set of entry points for the seed algorithm, but they do not appear to be hooked up to anything. It appears there is code that is missing in SEED_Encrypt and SEED_Decrypt. I suspect either the patch has not been tested against a seed server, or one of the files in the patch is incomplete.

I will give additional comments on the code that I reviewed in the following comment section.
Attachment #343512 - Flags: review-
General style comments in the freebl seed* files.

1) It would be preferred to colapse all the seed*.c files into a single file like all our other symmetric algorithms.
2) in seed.c, the SS array should be static const rather than static to put the table in the text section.
3) The use of array parameters is not normal in NSS, but it's also not incorrect. If you feel strongly you can leave them in.
4) in seed.h you can use the builtin PRUint32 to get a 32 bit value instead of having to resort to SEED_LONG (set from AES_LONG).
5) in SEED_cbc_crypt(): for block ciphers, NSS will guarrentee it only every gets called integral block lengths, so there is not real need to handle the partial block case. Same with decrypt.
6) SEED_cfb128 and SEED_ofb128 do not seem to have PKCS #11 mechanisms, and are thus unavailable. Other then blapitest, all applications go through PKCS #11 to access given ciphers, so these modes are not accessible and probably should be deleted.
7) I found no ecb mode implementation, though you define one in PKCS #11.
8) seedblapi.c - as meantioned in the review, appears to be commented out. This means at best seed is operating as a NO-op, and at worst, it is just returning random data.
9) In secoid.c, you define both seed_CBC and seed_KEY_WRAP, but only supply the former to the oid table. Also, you added SEED in the middle of the table. The order of this table is critical, NSS won't initialize unless the index to the entry of the table matches the oidTag value.

Please make these change, as well as build an test the result. You can replace your system's mozilla version of NSS with the one you build. Note that I will not be reviewing the SEED implementation for correctness. You will have to make sure it functions correctly against other implementations.

Thanks,

bob
Yeonjung when you get a new patch up, you will need to mark it for me to review. click on the details button and then click the review flag to ? and add rrelyea@redhat.com.

You'll want to get this in early as I have to schedule additional time to review such a large patch (and have my own stuff on the Nov 17th dealine;).

bob
Attached patch patch for NSS-trunk v4 (obsolete) — Splinter Review
Thanks for many comments.

We renewed the license headers correctly.
This patch is as follows.

--------------------------------------------------------------------------------
1) It would be preferred to colapse all the seed*.c files into a single file
like all our other symmetric algorithms.
 ==> file is optimized like as other symmetric algorithms.

2) in seed.c, the SS array should be static const rather than static to put the
table in the text section.
 ==> Modified following as comment.

3) The use of array parameters is not normal in NSS, but it's also not
incorrect. If you feel strongly you can leave them in.
 ==> We feel strongly, so we leave them in.

4) in seed.h you can use the builtin PRUint32 to get a 32 bit value instead of
having to resort to SEED_LONG (set from AES_LONG).
 ==> Modified following as comment.

5) in SEED_cbc_crypt(): for block ciphers, NSS will guarrentee it only every
gets called integral block lengths, so there is not real need to handle the
partial block case. Same with decrypt.
 ==> We can't find out SEED_cbc_crypt function, please advice more detailed information Line number of Patchfile.

6) SEED_cfb128 and SEED_ofb128 do not seem to have PKCS #11 mechanisms, and are
thus unavailable. Other then blapitest, all applications go through PKCS #11 to
access given ciphers, so these modes are not accessible and probably should be
deleted.
 ==> They are deleted following as comment.

7) I found no ecb mode implementation, though you define one in PKCS #11.
 ==> NSS_SEED references ECB Mode. Please check agagin seed.c file.
 ============================================================================
 void SEED_ecb_encrypt(const unsigned char *in, unsigned char *out, const SEED_KEY_SCHEDULE *ks, int enc)
 {
 if (enc)
 out, ks);
 else
 SEED_decrypt(in, out, ks);
 }
 =============================================================================

8) seedblapi.c - as meantioned in the review, appears to be commented out. This
means at best seed is operating as a NO-op, and at worst, it is just returning
random data.
 ==> Modified to correct implementation

9) In secoid.c, you define both seed_CBC and seed_KEY_WRAP, but only supply the
former to the oid table. Also, you added SEED in the middle of the table. The
order of this table is critical, NSS won't initialize unless the index to the
entry of the table matches the oidTag value.
 ==> Modified following as comment.
--------------------------------------------------------------------------------


Yeonjung
Attachment #343512 - Attachment is obsolete: true
Attachment #346196 - Flags: review?(rrelyea)
Attachment #336411 - Flags: review?(rrelyea)
Comment on attachment 346196 [details] [diff] [review]
patch for NSS-trunk v4

r-

This patch is *VERY* close. Thank you for fixing all the previous problems. I only see 2 real issues and a few nits.

1. The first real issue is the order of new functions in ldvector.c and loader.h. These are both have binary compatibility issues, so all new functions must be added at the end. (that is why the DES_AllocateContext is separated from the other DES_ functions). When adding new functions to freebl, you need to bump the freebl version number in loader.h to 0x030B and mark the new end of 'version 3.011 in both loader.h and ldvector.c.

2. The second issue I'd like nelson to review. That is the position of the SEED cipher in the SSLCipherSuiteInfo table in sslinfo.c. I believe seed should go after AES_256 since it's a 128 bit cipher. I'm ambivalent about whether it should go before or after Camellia, since the likelihood that both ciphers are turned on in a given server is pretty small. Having it before RC4-128 seems correct, since Seed servers would likely have RC4-128 as a fallback. The general rule for the order of this table is key strength followed by algorithm speed, modified by practical considerations (like allowing Seed to be selected with an RC4 fallback).

NITS: 
1) it would be best to get a CVS diff -Nup for a patch, rather than a gendiff. gendiff does not work well with mozilla tools, which means I can't use interdiff to see the differences you made from one patch to the next (which means I have to review the entire patch over again).

2) You define SEED_BLOCK_SIZE in 2 different places. You probably want to keep the definition in blapi.h, which is already included by seed.h, where the other definition is.


2. I would have liked to see seed.c and seedblapi.c merged to a single file. This one isn't as critical as the other issues.
Attachment #346196 - Flags: review?(rrelyea) → review-
Comment on attachment 346196 [details] [diff] [review]
patch for NSS-trunk v4

Nelson, no actual review (r+/r-) is necessary, but I would like to get your opinion of the SSLCipherInfo position off the seed cipher. 

Thanks,

bob
Attachment #346196 - Flags: superreview?(nelson)
Attachment #346196 - Flags: superreview?(nelson) → superreview-
Comment on attachment 346196 [details] [diff] [review]
patch for NSS-trunk v4

I am not doing a complete review of this patch at this time, but
would like a chance to do a review of this patch once Bob's issues
are addressed.  I completely agree with Bob's concerns about binary 
compatibility of ldvector and loader in Freebl.

In direct answer to Bob's questions in the preceeding bug comments, 
I agree that this patch injects the Seed cipher suites in the wrong
place in certain tables, such as 
  SSL_ImplementedCiphers in sslenum.c,
  suiteInfo              in sslinfo.c,
  cipherSuites           in ssl3con.c, 
  cipher_suite_defs      in ssl3con.c, (maybe)


I have some questions for Bob.  

1) How is the array named PK11_DefaultArray in pk11slot.c supposed to be 
   ordered?
- Alphabetically?
- By the values of the FLAG bits in increasing order?  (or decreasing?)
- By algorithm type (e.g. public key, then stream ciphers, then block 
  ciphers, then hashes, then ...  
- Random?
Is there any need to preserve the existing order of that list?
Should new items be added only at the end?
(There should be a comment answering these questions in the code.)

2) The same questions apply to the following other tables/arrays:
   "mechanisms"            array in pkcs11.c
   secmod_argSlotFlagTable       in pk11pars.h
   suiteInfo                     in sslinfo.c
   cipher_suite_defs             in ssl3con.c

3) Are the new values in pkcs11t.h officially allocated by the cryptoki WG?  
Or should they be in the vendor-defined space?

Other review comments.
- in secoid.c, I want to see a comment documenting the components of the
SEED_OID and their meanings, just as was done for Camerllia at line 171.

- Regarding the following lines from the patch to ssl3con.c, it's time 
for this test to become a switch statement.

>      if (calg == ssl_calg_des || calg == ssl_calg_3des || calg == ssl_calg_aes
> -	|| calg == ssl_calg_camellia) {
> +	|| calg == ssl_calg_camellia || calg == ssl_calg_seed) {
>  	/* For block ciphers, if the server is encrypting, then the client
>  	 * is decrypting, and vice versa.
>  	 */
>  	optArg1 = !optArg1;
>      }
In the preceding comment, I wrote:

> this patch injects the Seed cipher suite in the wrong
> place in certain tables, such as 
>   SSL_ImplementedCiphers in sslenum.c,
>   suiteInfo              in sslinfo.c,
>   cipherSuites           in ssl3con.c, 
>   cipher_suite_defs      in ssl3con.c, (maybe)

But I didn't say where the new suite SHOULD be. 
Since it is a 128-bit cipher suite, it should consistently
come either immediately before or immediately after the 
corresponding CAMELLIA_128_CBC cipher suites in those tables.
It's probably in the right place in cipher_suite_defs.
We ask bob a question.

It seems that we can add the next patch after you answer to Nelson's question.

When you answer to Nelson's question as soon as possible?
> 1) How is the array named PK11_DefaultArray in pk11slot.c supposed to be 
>   ordered?
> - Random?

Random. Actually, the FLAGS bits were created in the order of the algorithm's occurrence in the array, so in effect it was orded according the the bit position in the default flags, but there is no code requirement that affects the order.

Ideally new items should probably be added to the end, but that's a nicety not a requirement.

The array is strictly internal, so we can reorder that array any time we want in the future.
I

> 2) The same questions apply to the following other tables/arrays:
>    "mechanisms"            array in pkcs11.c
There is no required order, though I tend to organize it in terms of algorithm types (asymentric together, symetric together, hash togther, etc.).

This array is visible, but it regularly added to the middle. Code using this array is expecting the array to have not fixed order (other PKCS #11 modules may order their mechanisms differently), so we can reorder this any type we want. Locating the algorithm with other symmetric algorithms is most correct.

>   secmod_argSlotFlagTable       in pk11pars.h

Same answer as PK11_Default array. Added at the end is preferred.

>   suiteInfo                     in sslinfo.c
>   cipher_suite_defs             in ssl3con.c

These are only visible to the SSL code.  I don't think these affect real symantics, but I would have to differ to the SSL expert on these:).

> 3) Are the new values in pkcs11t.h officially allocated by the cryptoki WG?  
> Or should they be in the vendor-defined space?

I thought SEED had gone through PKCS #11, but I see that that a proposal has been made, but no movement has happend on it. As far as I can see there are are several proposals sort of treading water. BTW the link to the SEED proposal listed on the PKCS #11 group no longer works...:(
Attached patch patch for NSS-trunk v5 (obsolete) — Splinter Review
We added the new patch except for the comment #32.
We will reflect the comment #32 as soon as possible.
Please review this patch.
This patch is as follows.

--------------------------------------------------------------------
1. The first real issue is the order of new functions in ldvector.c and
loader.h. 
These are both have binary compatibility issues, so all new functions must be added at the end. (that is why the DES_AllocateContext is separated from the other DES_ functions). 
When adding new functions to freebl, you need to bump the freebl version number in loader.h to 0x030B and mark the new end of
'version 3.011 in both loader.h and ldvector.c.
==> loader.h and ldvector.c files are modified following as the comment.

2. The second issue I'd like nelson to review.
 That is the position of the SEED cipher in the SSLCipherSuiteInfo table in sslinfo.c. I believe seed should go after AES_256 since it's a 128 bit cipher. I'm ambivalent about whether it should go before or after Camellia, 
since the likelihood that both ciphers are turned on in a given server is pretty small. 
Having it before RC4-128 seems correct, since Seed servers would likely have RC4-128 as a fallback. 
The general rule for the order of this table is key strength followed by algorithm speed, modified by practical considerations 
(like allowing Seed to be selected with an RC4 fallback).
==> sslinfo.c file is modified following as the comment.

NITS: 

1) it would be best to get a CVS diff -Nup for a patch, rather than a gendiff. gendiff does not work well with mozilla tools, which means I can't use
interdiff to see the differences you made from one patch to the next (which means I have to review the entire patch over again).
==> I generated patch file by using your recommendation.

2) You define SEED_BLOCK_SIZE in 2 different places. You probably want to keep the definition in blapi.h, which is already included by seed.h, where the other
definition is.
==> SEED_BLOCK_SIZE is removed from seed.h and it is included in blapit.h file as I refer to other algorithm implementation. 

2. I would have liked to see seed.c and seedblapi.c merged to a single file.
This one isn't as critical as the other issues.
==> seed.c and seedblapi.c files are merged to seed.c file

Other review comments.
- in secoid.c, I want to see a comment documenting the components of the
SEED_OID and their meanings, just as was done for Camellia at line 171.
==> Modified OID information and commented in the source code.
 
- Regarding the following lines from the patch to ssl3con.c, it's time 
for this test to become a switch statement.
=> Modified source code to switch statement
>      if (calg == ssl_calg_des || calg == ssl_calg_3des || calg == ssl_calg_aes
> - || calg == ssl_calg_camellia) {
> + || calg == ssl_calg_camellia || calg == ssl_calg_seed) {
>   /* For block ciphers, if the server is encrypting, then the client
>    * is decrypting, and vice versa.
>    */
>   optArg1 = !optArg1;
>      }
--------------------------------------------------------------------
Attachment #346196 - Attachment is obsolete: true
Attachment #347414 - Flags: superreview?(nelson)
Attachment #347414 - Flags: review?(rrelyea)
We don't feel the modification of the patch file following as bob's comments.
SEED will be added to PKCS#11 v2.30 that is under development.
Please let me know, if you need any modification.
Yeonjung, is there a draft of PKCS #11 v2.30 you can point to, or failing that a draft of the SEED proposal that you can point to in this bug (the pointer I found on the mailing list doesn't seem to work).

Also a pointer to the SSL cipher suite number for SEED probably should be in the bug as well.

(Neither should require modification of the patch;).

bob
Bob, I e-mailed a draft of PKCS #11 v2.30 to you.
Please see the e-mail for details.
Let me know what to do before Nov17 2008 deadline.

yeonjung
We are concerned about the notice;
you have tentatively scheduled an upgrade to our Bugzilla website on Tuesday, November 18.

Doesn't Bug 453234 plan on the upgrade this time or later?

We want to add to this plan.

Please let us know your opinion upon it.
Yeonjung Kang, the new patch v5 is only 2/3 the size of the previous v4.
Why is it so much smaller?  Is it complete?  Or is some part missing?
Yeonjung Kang: the notice is about an upgrade to the "Bugzilla" software which runs this bug-tracking system (bugzilla.mozilla.org). It appears at the top of all 450,000+ bugs in the database - it is not specific to this bug here. It will have no effect on the SEED cipher suites one way or the other. Don't worry :-)

Gerv
Thank you for comments.
We were concerned about the notice : )

The new patch v5 is only 2/3 the size of the previous v4 because it optimizes the source code as requested.
We think it is complete.

Will this Bug be included in the FIPS2008 validation?
The patch looks good, it's missing the r+ because I'm waiting on verification of the actual PKCS #11 cipher numbers.

Please get me a copy of either the SEED PKCS #11 draft proposal or a copy of the PKCS #11 v2.30 draft. Thanks.

bob
Comment on attachment 347414 [details] [diff] [review]
patch for NSS-trunk v5

The patch named "patch for NSS-trunk v5" appears to be two patches 
generated by different methods, put together in the same file.  
It does not appear to be the output of the single command 
  cvs diff -Npu mozilla/security

I attempted to apply this patch, so that I could regenerate it as the 
output of that command, and I found that this patch does not apply 
cleanly for two existing files, which are:

mozilla/security/nss/lib/softoken/pkcs11.c
mozilla/security/nss/lib/softoken/pkcs11c.c

Therefore, I am not sure that the patched files I have are the same 
as the files produced by the contributor Yeonjung Kang. 

Shortly I will attach another patch which is merely what I get from 
applying "patch for NSS-trunk v5" and then doing cvs add commands
for the new files, and then doing the cvs diff command given above.
I am going to ask Yeonjung Kang to confirm that the contents of that
patch are as he intended, especially for new files and the two pkcs11
files named above.  If so, then I will review that patch.
I question whether we want to take the implementation and tests of the two 
feedback modes, cipher feedback (cfb) and output feedback (ofb).  
Although these modes are defined for any and all block ciphers, NSS doesn't
implement them for any other block cipher, and none of the protocols 
implemented in NSS (such as TLS or CMS) uses them.  Unless a clear use for 
these can be found, I'd rather not increase the maintenance burden unnecessarily.
Attached patch patch for NSS-trunk v5 redux (obsolete) — Splinter Review
This patch is intended to be the same as the previous v5 patch, except:
a) it is the output of cvs diff -Npu, and
b) it does not include the test files for CFB or OFB.

As I mentioned in an earlier comment, I want the contributor to review
this patch and make sure that it matches/produces the expected content
for all files, especially the new files and the pkcs11* files.
My previous attempt to convert this patch to be a cvs diff caused the 
files seed.c and seed.h to be included twice.  This one corrects that.
This is just for my review purposes.
Attachment #348605 - Attachment is obsolete: true
Comment on attachment 347414 [details] [diff] [review]
patch for NSS-trunk v5

I am going to provide my review comments as two separate comments 
to this bug.  First I will give comments for all the files except
those in nss/lib/ssl, then in a separate bug comment to follow, 
I will review the files in nss/lib/ssl.  

Most of the files were fine as they are.  I only had issues with a
few.  Most of the issues I found were issues with conformance to 
the NSS coding standards.  But I did find a few minor issues of a 
slightly more substantive nature.  Here are the comments by file:

***** nss/lib/softoken/pkcs11c.c ******
- at about line 2904 (in the patched source) we see this new code:

    case CKM_SEED_KEY_GEN:
	*key_type = CKK_SEED;
	*key_length = 16;
	if (*key_length == 0) crv = CKR_TEMPLATE_INCOMPLETE;
	break;

That if statement is dead code because it can never be true, right?
Please remove it.

***** nss/lib/freebl/blapi.h *****
- Lines are too long and need to be wrapped to not exceed 80 columns. 

***** nss/lib/freebl/seed.c  and nss/lib/freebl/seed.h ****
- Lines are too long and need to be wrapped to not exceed 80 columns. 

- Lots of unnecessary uses of "& 0xff" or "& 0xffffffff"
Given that type "seed_word" is a PRUint32, which is always a 32-bit 
unsigned type on every platform without exception, expressions such 
as  (some_seed_word & 0xffffffff) which do an "and" with a 32-bit
mask of all one bits are never necessary and do nothing.  
So, I'd like to see all those unnecessary "& 0xffffffff" removed.
Similarly, expressions such as  
  (unsigned char)((l)>>16) & 0xff
are unnecessary and actually do not do what is intended.  That 
expression is actually of type unsigned int, not unsigned char,
because it is a logical bitwise operation between two rvals,
one of type unsigned char, and one of type int.  By contrast,
the expression
   (unsigned char)(((l)>>16) & 0xff) 
is of type unsigned char, but the &0xff is totally unnecessary there
because the cast implies/ensures that it will be done if necessary.
So (unsigned char)((l)>>16)  is preferred for that type.

- File exhibits a mixture of several different styles of indentation
and bracing.  A new file in NSS should have one style and use it 
consistently.  I would like to see it follow the most predominate
NSS coding style, which I believe is commonly called the "k&r style",
where the first and last braces of a function are in column 1,
and withing the function the opening brace of a block comes at 
the end of a line, the corresponding closing brace begins a 
line at the same level of indentation as the line that opened the 
block, and all lines within the block are indented by 4 columns, 
else on same line as closing brace of preceding block, 
e.g. 

{
    if (foo)
        bar();
    else if (xxx)
        yyy();

    if (foo) {
        bar();
    } else if (xxx) {
        yyy();
    } else {
        zzz();
    }

    switch (aaa) {
    case bbb:
        bar();
        break;
    default:
    }

Stay tuned for comments on the patch to nss/lib/ssl
Attachment #347414 - Flags: superreview?(nelson) → superreview-
Comment on attachment 347414 [details] [diff] [review]
patch for NSS-trunk v5

Only two issues in the ssl code.

1. In comments 29-30, I wrote

> I agree that this patch injects the Seed cipher suites in the wrong
> place in certain tables, such as 
>   SSL_ImplementedCiphers in sslenum.c,
>   suiteInfo              in sslinfo.c,
>   cipherSuites           in ssl3con.c, 
>   cipher_suite_defs      in ssl3con.c, (maybe)

> Since it is a 128-bit cipher suite, it should consistently
> come either immediately before or immediately after the 
> corresponding CAMELLIA_128_CBC cipher suites in those tables.

These issues appear to have not been addressed in this patch.
In particular, the 128-bit SEED suite still appears at the top of 
the tables SSL_ImplementedCiphers and cipherSuites.

2. In ssl3con.c,  There's a new switch statement that is indented 
more than is normal in NSS.   The case labels should be at the 
same level of indentation as the switch statement itself, and the
code for those case labels should be indented only 4 more spaces.
There should be a space after the word "switch".

>+	switch(calg) {
>+		case ssl_calg_des:
>+		case ssl_calg_3des:
>+		case ssl_calg_aes:
>+		case ssl_calg_camellia:
>+		case ssl_calg_seed:
>+		/* For block ciphers, if the server is encrypting, then the client
>+		 * is decrypting, and vice versa.
>+		 */
>+		optArg1 = !optArg1;
>     }
Thank you for many comments.

We modified the new patch for the comments.

This patch is as follows.
================================================================================
I question whether we want to take the implementation and tests of the two 
feedback modes, cipher feedback (cfb) and output feedback (ofb).  
Although these modes are defined for any and all block ciphers, NSS doesn't
implement them for any other block cipher, and none of the protocols 
implemented in NSS (such as TLS or CMS) uses them.  Unless a clear use for 
these can be found, I'd rather not increase the maintenance burden
unnecessarily.
==> CFB and OFB mode are deleted from source code because it is not included in PKCS #11.


This patch is intended to be the same as the previous v5 patch, except:
a) it is the output of cvs diff -Npu, and
==> Patch file is created by using "-Npu" option.

b) it does not include the test files for CFB or OFB.
==> CFB and OFB mode are deleted from source code because it is not included in PKCS #11.


***** nss/lib/softoken/pkcs11c.c ******
- at about line 2904 (in the patched source) we see this new code:

    case CKM_SEED_KEY_GEN:
    *key_type = CKK_SEED;
    *key_length = 16;
    if (*key_length == 0) crv = CKR_TEMPLATE_INCOMPLETE;
    break;

That if statement is dead code because it can never be true, right?
Please remove it.
==> I removed it following as your comment.

***** nss/lib/freebl/blapi.h *****
- Lines are too long and need to be wrapped to not exceed 80 columns. 
==> I modified the source file following as your comment.

***** nss/lib/freebl/seed.c  and nss/lib/freebl/seed.h ****
- Lines are too long and need to be wrapped to not exceed 80 columns. 
==> I modified the source file following as your comment.

- Lots of unnecessary uses of "& 0xff" or "& 0xffffffff"
Given that type "seed_word" is a PRUint32, which is always a 32-bit 
unsigned type on every platform without exception, expressions such 
as  (some_seed_word & 0xffffffff) which do an "and" with a 32-bit
mask of all one bits are never necessary and do nothing.  
So, I'd like to see all those unnecessary "& 0xffffffff" removed.
==> I modified the source file following as your comment.

Similarly, expressions such as  
  (unsigned char)((l)>>16) & 0xff
are unnecessary and actually do not do what is intended.  That 
expression is actually of type unsigned int, not unsigned char,
because it is a logical bitwise operation between two rvals,
one of type unsigned char, and one of type int.  By contrast,
the expression
   (unsigned char)(((l)>>16) & 0xff) 
is of type unsigned char, but the &0xff is totally unnecessary there
because the cast implies/ensures that it will be done if necessary.
So (unsigned char)((l)>>16)  is preferred for that type.
==> I don't want modify it. The modification makes big impact SEED algorithm standard.

- File exhibits a mixture of several different styles of indentation
and bracing.  A new file in NSS should have one style and use it 
consistently.  I would like to see it follow the most predominate
NSS coding style, which I believe is commonly called the "k&r style",
where the first and last braces of a function are in column 1,
and withing the function the opening brace of a block comes at 
the end of a line, the corresponding closing brace begins a 
line at the same level of indentation as the line that opened the 
block, and all lines within the block are indented by 4 columns, 
else on same line as closing brace of preceding block, 
e.g.
{
    if (foo)
        bar();
    else if (xxx)
        yyy();

    if (foo) {
        bar();
    } else if (xxx) {
        yyy();
    } else {
        zzz();
    }

    switch (aaa) {
    case bbb:
        bar();
        break;
    default:
    }
==> I modified the source file following as your comment.

1. In comments 29-30, I wrote

> I agree that this patch injects the Seed cipher suites in the wrong
> place in certain tables, such as 
>   SSL_ImplementedCiphers in sslenum.c,
>   suiteInfo              in sslinfo.c,
>   cipherSuites           in ssl3con.c, 
>   cipher_suite_defs      in ssl3con.c, (maybe)

> Since it is a 128-bit cipher suite, it should consistently
> come either immediately before or immediately after the 
> corresponding CAMELLIA_128_CBC cipher suites in those tables.

These issues appear to have not been addressed in this patch.
In particular, the 128-bit SEED suite still appears at the top of 
the tables SSL_ImplementedCiphers and cipherSuites.
==> I modified the source file following as your comment.

2. In ssl3con.c,  There's a new switch statement that is indented 
more than is normal in NSS.   The case labels should be at the 
same level of indentation as the switch statement itself, and the
code for those case labels should be indented only 4 more spaces.
There should be a space after the word "switch".

>+       switch(calg) {
>+                  case ssl_calg_des:
>+                  case ssl_calg_3des:
>+                  case ssl_calg_aes:
>+                  case ssl_calg_camellia:
>+                  case ssl_calg_seed:
>+                  /* For block ciphers, if the server is encrypting, then the client
>+                  * is decrypting, and vice versa.
>+                  */
>+                  optArg1 = !optArg1;
>     }
==> I modified the source file following as your comment.
================================================================================

Please let me know, if you need any modification.

Yeonjung
Attachment #347414 - Attachment is obsolete: true
Attachment #349086 - Flags: superreview?(nelson)
Attachment #349086 - Flags: review?(rrelyea)
Attachment #347414 - Flags: review?(rrelyea)
Something is strange about this latest patch.  It's very different than the 
patch I get when I use cvs diff -Npu.  In my patch, every single file, 
whether a new file or a modification to an existing file, is preceded by a header that looks something like this:

Index: <filename>
===================================================================
RCS file: <filename>
diff -N <filename>

Bugzilla's patch analysis tools expect the files to look like that.

But in the latest path, only the modifications to existing files have
that header, and the new files do not.  This really confuses bugzilla's
patch analysis tools.  It will be necessary for me to take this latest
patch and insert those headers for every new file to get the tools to
work on this patch correctly.  

Yeonjung Kang, if you can get the patch to look as expected by bugzilla,
that will save me a bunch of work.  

Regarding this comment:
> ==> I don't want modify it. The modification makes big impact SEED 
> algorithm standard.
Are you saying that the algorithm's behavior changes, and it becomes 
incorrect, when that change is made?  If so, that concerns me.
I have attempted to take the preceding patch and convert it to standard
form so that bugzilla will diff it against the previous version.
Patch v6 modified 3 more files than did v5, so the patches are not comparable
by bmo.  So I have split v6 into two parts.  This first part should patch
the same set of files as v5, and thus be comparable to it.
Attachment #349618 - Attachment is obsolete: true
This is the part of patch v6 that modifies files that were not modified in
patch v5.
Comment on attachment 349086 [details] [diff] [review]
patch for NSS-trunk v6

I've got bad news about this patch.  It does not compile.  With this patch applied, I get compile time errors in several files.  

This really makes me wonder. 
Surely you are actually building this code before submitting it for a review, yes? 
If so, why does the patch not match the code that compiles? 

I am going to try to fix the problems and produce a patch that does compile.  But I have to think that if this code does not compile, then it cannot have been tested.
Attachment #349086 - Flags: superreview?(nelson)
Attachment #349086 - Flags: superreview-
Attachment #349086 - Flags: review?(rrelyea)
This patch builds, but all the self tests for SEED fail.
For comparison purposes, diff this patch against the patch named
 patch for NSS-trunk v6 redux (norlmal CVS diff)
Attachment #349625 - Attachment is patch: true
Just for grins, I went back to the seed.c and seed.h files in patch v5
and tried them, so see if the seed failures were due to the changes that
I had requested (removal of unnecessary &0xffffffff).  But the seed code
from patch v5 also fails all the cipher.sh tests.  So, I'm afraid this 
seed patch is not ready yet.  Sorry.
This patch builds *AND* passes the cipher.sh self test. 
Next step is to run all.sh.  

Here's a request for the Good KISA people who contributed.
Please add an attachment here with some URLs for different
https servers on the internet that support SEED, with which 
I can test this modified SSL for interoperability.
All.sh won't do much good.  This patch does not include changes to ssl.sh 
and its associated files to test with the new SEED SSL cipher suites.  
It also includes no changes to tstclnt, strsclnt and selfserv, although 
these may not be strictly necessary.  It may be possible to test without 
change those programs, since those programs now accept cipher suite numbers 
in hex.
Yeonjung,  The next step in getting this code into NSS is to get SSL tests
working.  Your patch did not include any changes to the SSL test scripts 
to test SEED.  

I am willing to write that addition to the test scripts, but first I would 
like to see this code successfully interoperate with some known good SSL 
servers that implement SEED.  Please provide https URLs for servers that
implement SEED, with which I can test this code.
Previously, comment #20 said:
> If this bug is completed by Nov17 2008 it will be included in the FIPS2008
> validation otherwise it will be dropped for a later release.

That date has obviously come and gone, but we're still working on this.
If we can get it tested in the next 7-10 days, it has a chance of getting 
in still.  Right now, there are only 2 things standing between this going 
into NSS 3.12 and not going in.  They are:
- lack of testing.  
- still haven't found the PKCS#11 amdendment!

So, find some https servers on the internet that implement the SEED ciphers
and put their URLs here.  Once I have URLs of working servers, I can do 
some testing.  Without them, all this work will come to nothing.
(In reply to comment #59)

Thank you for great help.
We read your sincere comments.

1.
PKCS#11 editors are starting to revise PKCS#11 to create a version 2.3.
They had proposed the conference call for December 3rd but, due to conflicts, they have postponed the call to 9am on December 10.

2.
We are going to provide you with SSL server that implement SEED
We must know your static IP address because our server can connect IP based network..
Do you use a static IP address?
Please let us e-mail if so.
Comment on attachment 349632 [details] [diff] [review]
patch for NSS-trunk v8 (normal CVS diff) . Checked in.

We can take the freebl subset (lib/freebl and cmd/bltest)
of this patch first, and take the softoken and SSL portions
when we have the PKCS #11 v2.30 header definitions and SSL
unit tests.  It doesn't have to be all or nothing.  This
also makes code review easier -- we don't need to review
the good parts of this large patch again.
In repy to comment 61, yes, we could do that, but adding code to freebl
that cannot be invoked through softoken is just adding bloat.
Nelson,
We will provide you with SSL server that implemented SEED.
Only permitted IP can be connected to the server, so we must know your static IP.
Please let us know if you have the static IP.

Yeonjung
Comment on attachment 336411 [details] [diff] [review]
patch for firefox-trunk

r- This is a combined patch, The FF trunk portion should be split out as the NSS portion has already been. It's probably best to have the FF trunk portion in a different bug that depends on this one.
Attachment #336411 - Flags: review?(rrelyea)
I am pleased to report that today I conducted a test between NSS tstclnt 
and an Apache web server that is also using SEED.  The test successfully 
connected and fetched a web page using SEED.  

So the remaining issue now concerns the acceptance of the proposed PKCS#11
mechanisms for SEED by the PKCS#11 working group.  That working group met 
by telephone today.  To my surprise, no represetnative of KISA participated 
in the call to champion the inclusion of SEED in version 2.30, so I 
volunteered to write a draft proposal to add SEED to PKCS#11, and have sent 
that draft out to the PKCS#11 working group mailing list today.
Thank you very much for your assistance.

I am really very pleased to hear the test that successfully connected and fetched a web page using SEED.

I'm so sorry you volunteered to write a draft proposal to add SEED to PKCS#11.
As soon as you sent that draft out to the PKCS#11 working group mailing list, I responded to your e-mail.

Sincere,


Yeonjung
I committed the patch v8 above, as is.  IF we decide that the PKCS#11
constants are in doubt, we can make another patch to change them to be
vendor-defined constants.  I will prepare a patch to do that, just in case.

cmd/bltest/tests/seed_cbc/ciphertext0; initial revision: 1.1
cmd/bltest/tests/seed_cbc/iv0;         initial revision: 1.1
cmd/bltest/tests/seed_cbc/key0;        initial revision: 1.1
cmd/bltest/tests/seed_cbc/numtests;    initial revision: 1.1
cmd/bltest/tests/seed_cbc/plaintext0;  initial revision: 1.1
cmd/bltest/tests/seed_ecb/ciphertext0; initial revision: 1.1
cmd/bltest/tests/seed_ecb/iv0;         initial revision: 1.1
cmd/bltest/tests/seed_ecb/key0;        initial revision: 1.1
cmd/bltest/tests/seed_ecb/numtests;    initial revision: 1.1
cmd/bltest/tests/seed_ecb/plaintext0;  initial revision: 1.1
lib/freebl/seed.c;                     initial revision: 1.1
lib/freebl/seed.h;                     initial revision: 1.1

cmd/bltest/blapitest.c;   new revision: 1.55;  previous: 1.54
lib/freebl/blapi.h;       new revision: 1.28;  previous: 1.27
lib/freebl/blapit.h;      new revision: 1.22;  previous: 1.21
lib/freebl/ldvector.c;    new revision: 1.19;  previous: 1.18
lib/freebl/loader.c;      new revision: 1.40;  previous: 1.39
lib/freebl/loader.h;      new revision: 1.22;  previous: 1.21
lib/freebl/manifest.mn;   new revision: 1.54;  previous: 1.53
lib/pk11wrap/pk11mech.c;  new revision: 1.13;  previous: 1.12
lib/pk11wrap/pk11slot.c;  new revision: 1.97;  previous: 1.96
lib/pk11wrap/secmod.h;    new revision: 1.26;  previous: 1.25
lib/pk11wrap/secmodt.h;   new revision: 1.39;  previous: 1.38
lib/softoken/pk11pars.h;  new revision: 1.23;  previous: 1.22
lib/softoken/pkcs11.c;    new revision: 1.156; previous: 1.155
lib/softoken/pkcs11c.c;   new revision: 1.105; previous: 1.104
lib/softoken/pkcs11t.h;   new revision: 1.20;  previous: 1.19
lib/ssl/ssl3con.c;        new revision: 1.114; previous: 1.113
lib/ssl/sslenum.c;        new revision: 1.16;  previous: 1.15
lib/ssl/sslimpl.h;        new revision: 1.66;  previous: 1.65
lib/ssl/sslinfo.c;        new revision: 1.18;  previous: 1.17
lib/ssl/sslproto.h;       new revision: 1.13;  previous: 1.12
lib/ssl/sslsock.c;        new revision: 1.56;  previous: 1.55
lib/ssl/sslt.h;           new revision: 1.12;  previous: 1.11
lib/util/secoid.c;        new revision: 1.45;  previous: 1.44
lib/util/secoidt.h;       new revision: 1.28;  previous: 1.27
tests/cipher/cipher.txt;  new revision: 1.5;   previous: 1.4
tests/cipher/symmkey.txt; new revision: 1.4;   previous: 1.3
Status: NEW → ASSIGNED
Comment on attachment 349632 [details] [diff] [review]
patch for NSS-trunk v8 (normal CVS diff) . Checked in.

Fixed a compiler error that MSVC totally ignored!  
Checking in pkcs11c.c; new revision: 1.106; previous revision: 1.105

The patch was trivial:

@@ -717,9 +717,9 @@ finish_des:
        context->cipherInfo = SEED_CreateContext(
            (unsigned char*)att->attrib.pValue,
            (unsigned char*)pMechanism->pParameter,
            pMechanism->mechanism == CKM_SEED_ECB ? NSS_SEED : NSS_SEED_CBC,
-           isEncrypt, att->attrib.ulValueLen, 16);
+           isEncrypt);
After SEED was committed to CVS, JSS tests started to fail:

JSS does not support ciphersuite: 96
ERROR: NSS has implemented ciphersuites that JSS does not support!

see http://mxr.mozilla.org/security/source/security/nss/lib/ssl/sslproto.h
Update org/mozilla/jss/ssl/SSLSocket.java
Update org/mozilla/jss/tests/Constants.java
NSS implemented Ciphersuites missing from JSS
JSS needs to update the ciphersuites!
Created bug 469989 (Add SEED support to JSS.)
(In reply to comment #70)
> Created bug 469989 (Add SEED support to JSS.)

thanks, I will update JSS to support the SEED ciphersuites.
Attachment #349632 - Attachment description: patch for NSS-trunk v8 (normal CVS diff) → patch for NSS-trunk v8 (normal CVS diff) . Checked in.
What work remains to be done in this bug ?
I declare victory.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: 3.12.7 → 3.12.3
The variable 'num' in SEED_Encrypt and SEED_Decrypt is unused.
I checked in this patch on the NSS trunk (NSS 3.12.3).

Checking in seed.c;
/cvsroot/mozilla/security/nss/lib/freebl/seed.c,v  <--  seed.c
new revision: 1.3; previous revision: 1.2
done
Attachment #362532 - Flags: review?(glen.beasley)
Attachment #362532 - Flags: review?(glen.beasley) → review+
(In reply to comment #74)

Thank you for comment.

Your patch is no problem after we check in this patch.

You can remove the variable 'num' that is unused as your comment.
Assignee: nobody → nelson
Mozilla QA (quality assurance) would like to be able to test that SEED is working correctly in Mozilla.

(requested in bug 478839 comment 26)

If you know of a *public* server, that supports SEED and that can be reached from any Internet IP address, using a simple https://www style address, please let us know. Thank you.
We are not operating a public server for SEED in the present situation. However, SEED algorithm has been 
supported from a version 0.9.8f for openssl. So, when we developed the patch, 
we built a server using openssl and apache, and tested an encryption 
communication.
You need to log in before you can comment on or make changes to this bug.