475578 - Implement Extended DSA as defined in FIPS 186-3 (DSS)

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Martin von Gagern]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.5) Gecko/2008121814 Gentoo Firefox/3.0.5
Build Identifier: dev-libs/nss-3.12.2_rc1 Gentoo package

I spent the larger part of today debugging some HTTPS host I had set up. Konqueror, Opera and OpenSSL all were happy to connect to it, but FF3 displayed a page stating "Peer's public key is invalid. (Error code: sec_error_bad_key)".

In the end I found the reason: DSA_MAX_P_BITS limits DSA key sizes to 1024 bits. My certificate was (for no particular reason except general paranoia) 2048 bits.

Reproducible: Always

Steps to Reproduce:
1. openssl req -x509 -nodes -newkey rsa:1024 -keyout ca.key -out ca.crt
2. openssl dsaparam -genkey -out dsaparam 2048
3. openssl req -nodes -newkey dsa:dsaparam -keyout demo.key -out demo.csr
4. openssl x509 -CA ca.crt -CAkey ca.key -in demo.csr -out demo.crt \
                -days 30 -sha1 -req -CAserial ca.srl -CAcreateserial
5. openssl s_server -www -key demo.key -cert demo.crt -accept 4433
6. firefox https://localhost:4433/
7. Trust certificate, at least for now

I don't have an IP to spare to demonstrate this issue online.
Actual Results:  
Secure Connection Failed
An error occurred during a connection to localhost:4433.
Peer's public key is invalid.
(Error code: sec_error_bad_key)
The page you are trying to view can not be shown because the authenticity of the received data could not be verified.
Please contact the web site owners to inform them of this problem.

Expected Results:  
Page displayed, just as in other software.

Some useful pointers I found while debugging:

Definition and references of DSA_MAX_P_BITS:
http://mxr.mozilla.org/security/search?string=DSA_MAX_P_BITS

Function enforcing the size constraint:
http://mxr.mozilla.org/security/ident?i=sftk_handlePublicKeyObject

Function transforming the CKR_ATTRIBUTE_VALUE_INVALID from sftk_ConstrainAttribute into a generic SEC_ERROR_BAD_KEY:
http://mxr.mozilla.org/security/ident?i=PK11_Verify

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

NSS implements DSS according to the FIPS standard, FIPS 186-2 Change Notice 1
http://csrc.nist.gov/publications/fips/fips186-2/fips186-2-change1.pdf
That standard defines P as: 
> p = a prime modulus, where 2^(L-1) < p < 2^L for 512 <= L <= 1024 and 
> L (is) a multiple of 64.

The *only* valid lengths of p, as defined in that standard, are 512, 576, 640, 
704, 768, 832, 896, 960, and 1024 bits, and that is exactly what NSS implements.

But I suspect the real intent of this bug report is to request an enhancement,
namely that NSS implement DSS according to FIPS 186-3 (now presently a draft,
AFAIK), which permits use of larger sizes of p, up to 3k bits, when used with
appropriate hash functions (SHA 224 or SHA 256). For a copy of that draft, see 
http://csrc.nist.gov/publications/drafts/fips_186-3/Draft_FIPS-186-3%20_November2008.pdf

So I am changing this bug into an enhancement request, accordingly.  
However, note that NSS already implements ECDSA as specified in that FIPS pub,
and that is clearly the direction in which FIPS compliant users should be 
headed.

Comment 2

[Wan-Teh Chang]

FIPS 186-3 has been officially published.  NIST's DSA validation list
at http://csrc.nist.gov/groups/STM/cavp/documents/dss/dsaval.htm shows
only one algorithm certificate (#375) issued for FIPS 186-3 DSA.
OpenSSL 1.0.0 supports "dsa-with-SHA224 and dsa-with-SHA256 signature
types".

The current PKCS #11 v2.30 draft at
ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-11/v2-30/pkcs-11v2-30m1-d7.pdf
doesn't have new DSA hash-and-sign mechanisms that support SHA-2
yet.  It has only CKM_DSA_SHA1.

One solution is to relax CKM_DSA to support the new private key
sizes.  

I found that there is only one hash-and-sign mechanism for ECDSA
too: CKM_ECDSA_SHA1.  So NSS must be using CKM_ECDSA to generate
ECDSA signatures with SHA-2 hashes.  Bob, Nelson, is that correct?

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

NIST doesn't even recommend DSA any more: "Note that some approved algorithms and key sizes, such as DSA (1024 or 2048), are omitted to enhance interoperability. RSA and ECDSA, which are included in Table 2-1 above, have been widely deployed in PKIs. Therefore, they are recommended for use to enhance interoperability. However, DSA (1024 and 2048) as specified in FIPS 186-3 may be used as long as the required security strength is satisfied."

http://csrc.nist.gov/publications/nistpubs/800-57/sp800-57_PART3_key-management_Dec2009.pdf

Comment 4

[Robert Relyea]

Maybe our .chk files need to move to RSA now that FIPS accepts RSA keys.

Comment 5

[Robert Relyea]

> I found that there is only one hash-and-sign mechanism for ECDSA
> too: CKM_ECDSA_SHA1.  So NSS must be using CKM_ECDSA to generate
> ECDSA signatures with SHA-2 hashes.

Yes, NSS always does it's own hashing and then calls the single primitive signature function.

DSA-2 has raised it's head again here. 186-3 defines the following DSA profiles:

len(p)=1024, len(q)=160
len(p)=2048  len(q)=224
len(p)=2048  len(q)=256
len(p)=3072  len(q)=256

I've looked through most of the DSA code, and generally our interfaces are set to handle multiple types of p and q. 

In freebl we explicitly check lengths against DSA_SUBPRIME_LEN, but the functions have the actual q.len available. The only freebl function that can't easily be get access to the prime length is FIPS186Change_ReduceModQForDSA(), which is an entry point for the fipstest program I suggest a new entry point for FIPS186Change_ReduceModQForDSA2() if necessary (DSA2 may have a different key generation algorithm, so this may not be necessary).

In softoken, the PKCS #11 interface includes the PQG parameters, which fully define the DSA key parameters for everything bye PQG generation. For PQG generatation there already is an attribute CKA_PRIME_BITS defined to set the prime bit value. There is no attribute to define the subprime bits (q). It seams reasonable to reuse the DSA1 mechanisms, determining the difference by the expanded values in ulMaxKeySize. The only issue is what to do about specifying subprime bits. I would suggest that until a CKA_SUBPRIME_BITS is defined by the cryptoki group, using q=256 for p > 1024. We could still generate keys and use keys with q=224, but we couldn't generate PQG params for q=224.

In cryptohigh, There are 2 DSA functions that DER encode a DSA signature. Both functions are exported, as well as versions that already take a length. I suggest just labelling these to functions as DSA1 and use the one that takes a length for ECC and DSA2.

I'm working on a patch with these changes right now.

bob

Comment 6

[Robert Relyea]

I'm about ready to dump my patches up. There are some pretty extensive patches, particularly pqg.c and fipstest.c, the latter being the more critical.

With these patches, we will be able to

 1) continue to generate DSA1 keys, PQG parameters and verify parameters (If you called the old PQG interfaces, you will get DSA1, so existing programs will continue to work).

 2) Generate PQG parameters for all for L & N combos listed in comment 5 using a single fixed hashing algorithm (based on length of N) and FIPS 186-3 Probable Prime generation.

 3) Generate keys with any PQG parameters generate with the L & N combos listed above as well as the old PQG.

 4) Sign and verify signatures will all specified DSA key sizes (both DSA1 and DSA2) and all SHA hashing schemes. If sizeof(q) == hash_length, the hash will be expanded or truncated appropriately (as specified iin FIPS 186-3).

 5) Verify PQG's from all forms of PQG generation: FIPS 186-1, FIPS 186-3 Probable Primes, FIPS 186-3 Provable Primes, FIPS 186-3 Verifiable Canonical G, and Unverifiable Generation of G. We can verify PQ independently from G, we can also verify G without fully validating P and Q (namely we can validate G without the counter).

PKCS #11 already specifies CKA_SUBPRIME_BITS, for use with DH, so I've implemented DSA2 with the existing mechanism for generating PQG, allowing for an optional CKA_SUBPRIME_BITS. See design in the my next post.
--------------------------------------------------------------------------------
Questions that are raised:

1) Should I look at allowing Hash to be specified when we generate PQG? The delta on top of this patch is relatively small, but it requires adding a new Attribute in PKCS #11 (or coopting an existing one), or changing the mechanism parameters for the PQG mechanism (which I would like to avoid).

2) Should I add a mechanism to be able to generate G independent of P and Q? FIPS 186-3 specifies generation of P and Q separately from G. The idea being P and Q are expensive to generate and G is cheaper, so you can use different G's to create different domains. Generation of G takes a user index parameter. This one would be relatively easy to do. I can simply say if G is not specified, generate one when creating a PQG parameter. I would need to add a new freebl and NSS API for this (the former for NSS applications, the latter for softoken). The current patch always generates a G when you generate PQG with index=1 for FIPS 186-3 (random h for FIPS 186-1). There currently is no way to generate a second G given a PQ seed, and index.

3) Should I use provable prime generations? I originally avoided provable primes for 2 reasons: a) I didn't want to implement another algorithm and the probable prime pqg tests were extremely similiar to the DSA1 pqg generation, and b) the verify parameters for provable primes consist of 3 seeds and 3 counts (instead of one seed and one count). It turns out, however, I needed to validate provable primes when validating certain G's in the indexing test. Validating provable primes basically involves regenerating them from their seeds, so most of the code is now down. It also turns out that it's relatively simple to separate out the 3 seeds from a single seed value, so we can carry all three seeds with the single existing seed parameter. Currently the code uses only the seeds to verify, and ignores the count. We could, however carry both counts in the single existing count parameter since counts are guaranteed to be well less then 16 bit (16 bits would support a 2 million bit prime).

3a) If I use provable primes, should I just make it the default for generating DSA-2 keys? No one is validation DSA-2 parameters from NSS yet and even the FIPS 186-3 probable prime generation produced incompatible values with the FIPS 186-1 validation, so there really isn't a compatibility reason not use use DSA-2 parameters.

3b) If I use provable primes, should I be able to select provable versus probable prime? This has the same issues as question 1 with the hashes.


My current take: let's review what is here, adding any of these would be easier as a delta, but we need to make any of these changes before we ship a release that includes these changes because many of these options would require tweaks to the api. 

Of these options, 3a seems the strongest to me and 2 may have some value. I'm not so sure about 1 or 3b.

bob

Comment 7

[Robert Relyea]

Background:

When DSA-1 was defined, There was only one government approved hash algorithm,
and one government approved signing algorithm. Keysizes between 512 bits and
1024 bits were considered more than adequate, and SHA-1 was considered strong
against all attacks.

To prevent certain kinds of attacks, a method for generating the domain
parameters shared by several users (PQG) was devised to make sure we actually
generate primes with the required characteristics, and G which has not short
cuts in generation.

Today, 1024 is considered barely adequate, to a little bit dicey. Sizes less
than 1024 are considered weak. SHA-1 is considered weak against collision
attacks, and there are new government approved hashing algorithms.
Also, the method for generating G was considered vunerable to birthday attacks.

As a result NIST has released an updated FIPS 186 document, which:

1) Deprecates all older keysizes except 1024.
2) Defines new PQG generation mechanisms.
    The new PQG generation which takes L, N, and hash rather than just L in
   previous designs.
    One new PQG Generation generates provable primes for PQ.
    The new PQG generation is split between PQ generation and G generation to
   allow generation of multiple G parameters from the same PQ.
    The new G uses a fixed seed plus and index and hash to generate G rather
   than a completely random value.
3) Defines signatures with new hashes for all key sizes.

NIST defines a specific set of key and Q sizes that are supported:

L=1024 N=160 (same as largest DSA1 keys).
L=2048 N=224
L=2048 N=256
L=3072 N=256

We need to support the new PQG generation mechanism as well as the new
key sizes. Without breaking binary compatibility with old applications.

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

The design: Part 1, PKCS #11 level

PKCS #11 already defines Mechanisms for DSA1 which:
   1) Generates PQG parameters with their Verifiers (CKM_DSA_PARAMETER_GEN)
   2) Generates new DSA keys (given PQG). (CKM_DSA_KEYPAIR_GEN);
   3) Signs with a DSA key. (C_Sign with CKM_DSA)
   4) Verifies with a DSA key. (C_Verify with CKM_DSA)

NSS also already has extensions for Verifying the parameters.
   1) Create a CKO_DOMAIN_PARAMETERS object with CKA_NETSCAPE_PQG_COUNTER,
 CKA_NETSCAPE_PQG_SEED, and CKA_NETSCAPE_PQG_H set. If the create succeeds,
 then the parameters are valid.

The Keys size is fully specified for any operation which takes PQG, so no
interface changes are necessary for operations 2, 3, or 4.

In the new PQG generation, FIPS 186-3 specifies L (CKA_PRIME_BITS),
N (CKA_SUBPRIME_BITS), and hash. for the generation of P and Q. FIPS 186-3
also specifies methods for generating P and Q where P and Q are provably prime.
PKCS #11 will currently not generate provably prime PQ's.

It also specifies domain_seed (same as seed used to generate P and Q), index,
P and Q to generate G. FIPS 186-3 also allows the for the old method of
generating G, but calls this method "generating an unverifiable G".

To support these semantics, I have modified the PKCS #11 interface as follows:

For CKM_DSA_PARAMETER_GEN (C_GenerateKey):
1) Softoken accepts values for CKA_PRIME_BITS as follows:
    512-1024 in steps of 64 bits (same as before)
    2048
    3072

2) Softoken accepts a new optional parameter, CKA_SUBPRIME_BITS.
This attribute is already defined in the PKCS #11 spec, for use with
CKM_DH_PKCS_PARAMETER_GEN. If CKA_SUBPRIME_BITS is specified, it becomes N.
if it is not specified N would match the following table:
      L<=1024 N=160
      L=2048 N=224
      L=3072 N=256

3) The hash is chosen to be the smallest permissible hash for the key strength:
    L<1024 Hash=SHA-1
    L=2048 N=224 Hash=SHA-224
    L=2048 N=256 Hash=SHA-256
    L=3072 N=256 Hash=SHA-256
   While softoken will be able to handle other PQG generated from other hash
algs (including verifying them), but it cannot generate PQG parameters with
arbitrary hashes.

4) The method for generating PQG parameters will follow FIPS186-1 if L<1024 and
FIPS186-3 for L>1024. if L=1024, it will follow FIPS186-1 if CKA_SUBPRIME_BITS
is not specified, and FIPS186-3 if it is. This will preserve binary
compatibility as old apps will never set CKA_SUBPRIME_BITS.

4) Softoken will only generate PQG parameters together. No method is provided
to generate just P and Q, nor any method is provided to generate a G given
a P, Q, seed and index.

5) When generating PQG for FIPS 186-3, softoken will use the FIPS 186-3 A.2.3:
"Verifiable Canonical Generation of Generator g". CKA_NETSCAPE_PQG_H (H) will
by the one byte index (which is always 1) rather than a seed for g. In all
other cases softoken will use A.2.1 "Unverifiable Generate of Generator g", and
CKA_NETSCAPE_PQG_H (H) will be the seed used to generate G.

NOT implemented:

 There is no provision for specifiying a different HASH (other then the
smallest acceptable hash), generating PQ with provable primes, generating
PQ separately from G, generating G separatly from PQ.

For Verifying PQG with CKO_DOMAIN_PARAMETERS and C_CreateObject().

1) Softoken will accept the union of PQG sizes specified in FIPS 186-1 and
186-3.

2) Softoken will use CKA_NETSCAPE_PQG_SEED (seed)  and CKA_SUBPRIME (Q) to
automatically select:
   a) whether FIPS 186-1 or 186-3 probable or 186-3 provable was used to
   generate PQG, and
   b) what hash was used to generate PQG.
Softoken will only use FIPS 186-1 for L< 1024 and FIPS 186-3 for L>1024. Either
FIPS 186-1 and FIPS 186-3 would be accepted for L=1024.

3) Softoken will accept a CKO_DOMAIN_PARAMETERS C_CreateObject() even if
CKA_BASE (G) has not been specified. In this case only CKA_PRIME(P) and
CKA_SUBPRIME(Q) are verified.

4) Softoken will accept CKO_DOMAIN_PARAMETERS C_CreateObject() even if
CKA_NETSCAPE_PGP_COUNTER is not specified.  In this case CKA_SUBPRIME(Q) is
verified with CKA_NETSCAPE_PQG_SEED(seed) to find whether FIPS 186-1 was used
and what hash was used. and then G is verified. If the CKA_NETSCAPE_PGP_COUNTER
is not specified, G must be.

5) When verifying CKA_BASE (G). Softoken will use:
     a) FIPS 186-3 A.2.4: "Validation Routine when the Canonical Generation of
 Generator g Routine Was Used" when FIPS 186-3 was used to generate Q and
 the length of CKA_NETSCAPE_PQG_H is 1.
     b) FIPS 186-3 A.2.2 "Assurance of the Validity of Generator g"  when
 CKA_NETSCAPE_PQG_H is not specified.
     c) FIPS 186-3 A.2.2 "Assurance of the Validity of Generator g" plus
 FIPS 186-2 in all other cases.
 Note: In both cases b and c G is partially validated since we cannot preclude
 a exploitable relationship between g and and unknown g'.

NOTE: While softoken does not generate PQ with arbitrary hashes or provable
primes, NSS can validate PQ's generated with arbitrary hashes and provable
primes. Softoken will automatically determine the hash and algorithm based
on q and the seed. When validation PQ's with provable primes, set
CKA_NETSCAPE_PQG_SEED to the concatenation of first_seed, pseed, and qseed.

For CKM_DSA_KEY_PAIR_GEN

1) PQG paramters for DSA2 keys are accepted for key pair generation.

For CKM_DSA (C_Sign and C_Verify).

1) DSA 2 keys are accepted.
2) All forms of SHA-x are accepted as digests. The length of the digest is
adjusted to match the q of the PQG parameters.

Other:

DSA supported key sizes are now between 512 and 3072. Sizes greater than 1024
imply the additional interfaces and semantics described here.

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

The design: Part 2, Freebl level.

A new function, PQG2_ParamGen() is added. This function generates PQG
paramters based on FIPS 186-3. It takes the same arguments as
PQG_ParamGenSeedLen() except it also takes N, the subprime length in bits.
PQG2_ParamGen only accepts FIPS 186-3 L and N values.

PGQ_Verify is modified to provide the semantics explained in "Verifying PQG
with CKO_DOMAIN_PARAMETERS and C_Create". Missing G and H are specified by a
length of '0'. Missing count is specified by the value -1.

All functions are modified to remove dependencies on fixed DSA lengths (namely
Q and signature length).

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

The design: Part 3, NSS.

Like the lower layers, any place in NSS where the fixed length of the DSA1
algorithms were used need to adjusted to allow for variable lengths. Support
for the new hashing algorithms also needs to be added. We need to be able to
select a token that can do DSA2 from among tokens that can only do DSA1.
Finally we need to add a new function to generate PQG parameters a la
PQG2_ParamGen()

PK11_SignatureLen() needs to return the correct length for the given DSA
key.

PK11_PQG2_ParamGen() needs to be added. Unlike PQG2_ParamGen, it doesn't always
use DSA2. It will use DSA2 if 1) the key size is > 1024, or the key size is
equal to 1024 and q size is specified. This means PK11_PQG2_ParamGen() can
be used to replace the other two PK11_PQG functions in existing programs.

PK11_GetBestSlotWithKeySize() gets the best slot that can handle the
requested mechanism at the requested keysize.

PK11_GetBestSlotMultipleWithKeySize() takes an optional array of key sizes.
The key size array matches the mechanism array. If the key size is nonzero,
then the selected token needs to do the requested mechanism at the requested
key size to qualify.

Comment 8

[Robert Relyea]

Attachment: Freebl/softoken patch to add DSA-2 support

----------- lib/freebl -----------------------
blapi.h
  * Add PQG2_ParamGen
blapit.h
  * Remove old DSA_ fixed lengths and make them DSA1_ Add DSA_Max lengths.
dsa.c
  * Replace old DSA_ fixed lengths.
  * add check for supported key sizes in generate key.
  * accept hash sizes where hash length != q length. pad or truncate hash to
  q length.
ldvector.c
  * add PQG2_ParamGen
loader.c
  * add PQG2_ParamGen
loader.h
  * add PQG2_ParamGen
pqg.c
  * update prime test counts according to FIPS 186-3.
  * Add functions to validate keys size selections.
  * Add functions to select the hashing algorithms.
  * update variables, comments and code to match FIPS 186-3.
  * add FIPS 186-3 function to generate Q from seed.
  * add Shawne-Taylor functions to generate provable primes.
  * add function which find the hash and FIPS algorithm used to generate Q
  from seed. This allows us to validate any PQG parameters from the
  given verify parameters without knowing, a priori, what the hash and FIPS
  algorithm was.
  * removed fixed SHA1 hashes and replace with hashes selected by type.
  * add new PQG2_ParamGen
  * allow PQG_VerifyParam to verify parts of the parameters (like P and Q,
  or Q and G).
pqg.h
 *** new file. added some exported helpers for dsa.c to use.
rawhash.c
  * add utility functions to return length and hashbuf for internal freebl use.
shvfy.c
  * add support for other hash algorithms than SHA-1
----------- lib/softoken -----------------------
pkcs11.c
  * Update DSA constraints on subprime.
  * fix constraints for DSA, RSA, and DH (MIN is the minimum acceptable value,
  not the maximum unacceptable value. min DSA/DH base =2)
  * allow parameters to be left out when creating a DSA parameter. This allows
  upper level code to validate pq and g independently.
pkcs11c.c
  * add support for larger keysizes and subprimes other than 160 when
  generating PQG parameters.

Comment 9

[Robert Relyea]

Attachment: Add DSA-2 support to pk11wrap

----------- lib/nss -----------------------
nss.def
 * add PK11_PGQ2_ParamGen which generates DSA2 parameters.
 * Add 2 new PK11_GetBestSlot functions which take into account key size as
 well as mechanisms.
----------- lib/pk11wrap -----------------------
pk11obj.c
 * fix PK11_SignatureLen() to return a variable length for DSA signatures based
 on the length of the DSA subprime.
 * When Verifying a signature and the key is DSA2, look for a slot that can do
 DSA2 (may not necessarily be softoken).
pk11pqg.c
 * add PK11_PGQ2_ParamGen which generates DSA2 parameters.
 * allow partial PQG and Vfy parameters in PQG_Verify so that PQ and G can be
 verified independently
pk11pqg.h
 * add PK11_PGQ2_ParamGen which generates DSA2 parameters.
pk11slot.c
 * Add 2 new PK11_GetBestSlot functions which take into account key size as
 well as mechanisms.
pk11slot.h
 * Add 2 new PK11_GetBestSlot functions which take into account key size as
 well as mechanisms.

Comment 10

[Robert Relyea]

Attachment: Add DSA-2 support to cryptohi

----------- lib/cryptohi -----------------------
cryptohi.h
  * update comment on why DSAU_EncodeDerSig is needed (originally for ECC, no
  for ECC and DSA2.
dsautil.c
  * remove fixed DSA length selections. Replace old fixed length with DSA1
  specific lengths where appropriate for DSA1 only.
seckey.c
  * add support for DSA with SHA_224 and SHA_256 digests.
  * dsa signature length is now variable
secsign.c
  * add support for DSA with SHA_224 and SHA_256 digests.
secvfy.c
  * add support for DSA with SHA_224 and SHA_256 digests.
NOTE: There are currently no oids for SHA_384 or SHA_512. If those are ever
specified, we sould only need to update seckey.c, secsign.c, secvfy.c,
 secoid.c, and secoidt.h to support them.

----------- lib/util -----------------------
secoid.c
  * add dsa with 224 and dsa with 256 algorithms
secoidt.h
  * add dsa with 224 and dsa with 256 algorithms

Comment 11

[Robert Relyea]

Attachment: DSA-2 changes in ssl (short)

----------- lib/ssl -----------------------
ssl3con.c
 * replace fixed DSA length check.

Comment 12

[Robert Relyea]

Attachment: DSA-2 changes for commands.

----------- cmd/makepqg ---------------------
makepqg.c
  * add support for longer keysizes
  * add support for specifying subprime sizes (q sizes)
  * add support for PK11_PGQ2_ParamGen
----------- cmd/shlibsign.c ---------------------
shlibsign.c
  * add support for 2048/256 PQG.
  * add option to select key size.
  * add code to automatically select key size from softoken.

Comment 13

[Robert Relyea]

Attachment: Add DSA-2 tests to blapi_test

----------- cmd/bltest ---------------------
blapitest.c
 * Add support for longer key sizes.
        - change dsa keysize variable from the index 'j' to a full keysize.
 * Add support for PQG2_ for keysizes > 1024.
--- tests/dsa/*
  Add tests for
    - DSA 1024/160/SHA-1,SHA-224,SHA-256,SHA-384,SHA-512
    - DSA 2048/224/SHA-1,SHA-224,SHA-256,SHA-384,SHA-512
    - DSA 2048/256/SHA-1,SHA-224,SHA-256,SHA-384,SHA-512
    - DSA 3072/256/SHA-1,SHA-224,SHA-256,SHA-384,SHA-512
  On top of the 1 test for DSA 512/160/SHA-1 (20 new tests, 21 in all).
  Tests were taken from the NIST sample .txt file for DSASigGen. The source is
  also includes "dsa_fips.txt"

Comment 14

[Robert Relyea]

Attachment: DSA-2 changes to handle FIPS .req files for DSA-2

----------- cmd/fipstest ---------------------
fipstest.c
  * primarily update fipstest.c to handle the DSA2 samples form NIST,
  specifically
   - replace explicit use of fixed DSA lengths with variable. In some cases
     the tests really only apply to DSA1. In those cases DSA1 specific lengths
     were preserved.
   - add generic hashing support so we can support more than just SHA1. (I've
     also changed non-DSA usage that depends on generic hashing to use this
     generic hashing support.
   - increase dsa buffers to handle longer P, G and Y's.
   - parse both prime bits (L) and subprime bits (N) from the file.
   - detect which DSA2 algorithms to use (if none specified default to DSA1).
   - conform to new DSA2 lables and parameters.
   - add missing init functions.

Comment 15

[Wan-Teh Chang]

Comment on attachment 611087 [details] [diff] [review]
DSA-2 changes in ssl (short)

r=wtc.

Comment 16

[Wan-Teh Chang]

Comment on attachment 611083 [details] [diff] [review]
Freebl/softoken patch to add DSA-2 support

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

r=wtc.  I will attach a file that contains detailed review comments.
Here I only write a few comments.

I found some bugs and suggest some changes.  Please attach an updated
patch before you check it in.  Thanks.

::: lib/freebl/dsa.c
@@ +160,4 @@
>      }
>      if (maxDestLen < qLen) {
>          /* This condition can occur when DSA_SignDigest is passed a group
> +           with a subprime that is larger than dsa_subprime_len. */

Should dsa_subprime_len be DSA_MAX_SUBPRIME_LEN?

@@ +342,4 @@
>      return dsa_NewKeyExtended(params, &seedItem, privKey);
>  }
>  
> +

Nit: delete this blank line.

@@ +449,2 @@
>  cleanup:
> +    PORT_Memset(localDigestData, 0, DSA_MAX_SUBPRIME_LEN);

This doesn't seem necessary because localDigestData doesn't hold secret
data.

::: lib/freebl/loader.c
@@ +918,4 @@
>    (vector->p_PQG_DestroyVerify)(vfy);
>  }
>  
> +

Delete this blank line?

::: lib/softoken/pkcs11.c
@@ +905,4 @@
>  	if (crv != CKR_OK) {
>  	    return crv;
>  	}
> +	crv = sftk_ConstrainAttribute(object, CKA_BASE, 2, DSA_MAX_P_BITS, 0);

Why did you change the minLength argument to these sftk_ConstrainAttribute
calls from 1 to 2?  Is it because CKA_BASE and CKA_VALUE cannot be 0 or 1
(the only 1-bit values)?

Comment 17

[Wan-Teh Chang]

Attachment: Review comments on "Freebl/softoken patch to add DSA-2 support" (attachment 611083)

Here are my detailed review comments on the freebl/softoken patch.

Some of these are bugs.  Also note my comments on blapi.h and blapit.h,
the two exported headers.

Comment 18

[Robert Relyea]

> Should dsa_subprime_len be DSA_MAX_SUBPRIME_LEN?

Yes.

> Why did you change the minLength argument to these sftk_ConstrainAttribute
> calls from 1 to 2?  Is it because CKA_BASE and CKA_VALUE cannot be 0 or 1
> (the only 1-bit values)?

yes. If base is either 1 or 0 then an attack can always calculate A^x mod p without knowing x (because it's always either 1 or zero).

Comment 19

[Robert Relyea]

> This doesn't seem necessary because localDigestData doesn't hold secret
> data.

I believe that's true, but I'd like to keep it as well. Without it the code will preserve data passed by some other function on the stack. If the Sign function is ever used where a CSP itself is signed, we would then be leaking this. Keeping the function means we don't have to review all usages to verify that the digest is never a CSP in this case when we do our next FIPS validation.

Comment 20

[Robert Relyea]

> PQG2_ParamGen: can this function be named PQG_ParamGenV2 or PQG_ParamGenEx
> so that it still uses the PQG_ prefix?

yes.

> Two input lengths are in bits (L and N), while one input length is in
> bytes (seedBytes).  This seems inconsistent.  Can all the input lengths
> be in bytes?

I'd prefer not, simply because everyone talks about L and N in terms of bits. It seems most natural to present that way to the user.

> blapit.h is a public header file, so we can't rename macros defined in
> this header, such as DSA_SIGNATURE_LEN, DSA_SUBPRIME_LEN, and DSA_Q_BITS.

I think I will add them back, but as depricated. Any usage of these values should be examine to see if they preclude the use of DSA2 in the appropriate code. I certainly wanted to expunge them from the NSS main code base.

> Should we increase DSA_MIN_P_BITS to 1024?  I believe that is required
> by FIPS 186-3.

I didn't because of binary compatibility issues. I wouldn't want to do it in this patch because we really should remove a lot of DSA-1 code when we do (the
whole funky 'j' thing would go away since only one valid value of j would remain (1024-512)/64.

> In DSA_NewKey, the PGQ_Check call should be written like this:

or change rv to SECFailure in the current code. Good catch.

> Should the new PQG2_ParamGen function be added in the same order as
loader.h?

It's not a bug, but I feel you are right (unless loader.c has other out of order functions, in which case it should be grouped appropriately).

> I didn't review most of the changes to this file.  Sorry.
> It is a shame that we need to add so much code to implement a
> rarely used standard.

Actually, this is where most of the changes in the DSA-2 spec resides. The only real difference between DSA-1 and DSA-2 is the size of PQG and how they are generated. Most of this is for the Deterministic Prime generation. Actually the deterministic prime generation could be useful in other contexts as well.

Things I didn't do: Add a Lucas probablistic prime check. I instead opted for using many more interations of the Rabin/Miller test.

> makePrimefromPrimesShawneTaylor: who is Shawne Taylor?

Shawne/Taylor. It's the algorithm, and how it's referenced in the DSA2 spec. (same as Rabin/Miller for probalistic prime check).

> The 'obj' parameter is poorly named and definitely needs documenting.

any suggestions?;)

> It is not necessary to test hashObj for null because if hashcx is
> not NULL, hashObj cannot be null.  (hashcx can only be created by
> hashObj->create().)

Just as a reference- unless there is a real need for efficiency I do tend toward redunant safe checks for 2 reasons: 1) when reviewing or modifying the code I (or the next person) doesn't have to figure out the logic on why hashObj can't must be NULL. 2) it makes the code a little more durable to later changes... Sometimes a comment suffices, and In this case I'll take your suggestion with a comment explaining why it is safe.

> Since vfy.counter is an unsigned int, it doesn't make sense to assign
> -1 to it.  Should we assign 0 to vfy.counter instead?

IIRC 0 is a valid value (or had another meaning). I should have made a comment as I knew that vfy.counter was unsigned.

> It would be nice to document what needVfy means.  needVfy is only
> checked in one place, when the seed is missing:

needVfy means we need to create a verify structure. It's a basic way of determining if enough parameters have been specified. (I was trying to preserve the old semantic). In the old code you had to specify either pqg or pqg + chs. Now you can drop peices so you can verify just pq if you want. In all cases in which you are verifying, you need a seedAttr. If one isn't there, then you don't have enough values in your template.



Other comments will be accepted into the new code.

Comment 21

[Robert Relyea]

Attachment: Updated patch for freebl/softoken

Comment 22

[Robert Relyea]

Checking in lib/freebl/blapi.h;
/cvsroot/mozilla/security/nss/lib/freebl/blapi.h,v  <--  blapi.h
new revision: 1.47; previous revision: 1.46
done
Checking in lib/freebl/blapit.h;
/cvsroot/mozilla/security/nss/lib/freebl/blapit.h,v  <--  blapit.h
new revision: 1.28; previous revision: 1.27
done
Checking in lib/freebl/dsa.c;
/cvsroot/mozilla/security/nss/lib/freebl/dsa.c,v  <--  dsa.c
new revision: 1.23; previous revision: 1.22
done
Checking in lib/freebl/ldvector.c;
/cvsroot/mozilla/security/nss/lib/freebl/ldvector.c,v  <--  ldvector.c
new revision: 1.31; previous revision: 1.30
done
Checking in lib/freebl/loader.c;
/cvsroot/mozilla/security/nss/lib/freebl/loader.c,v  <--  loader.c
new revision: 1.56; previous revision: 1.55
done
Checking in lib/freebl/loader.h;
/cvsroot/mozilla/security/nss/lib/freebl/loader.h,v  <--  loader.h
new revision: 1.37; previous revision: 1.36
done
Checking in lib/freebl/pqg.c;
/cvsroot/mozilla/security/nss/lib/freebl/pqg.c,v  <--  pqg.c
new revision: 1.19; previous revision: 1.18
done
RCS file: /cvsroot/mozilla/security/nss/lib/freebl/pqg.h,v
done
Checking in lib/freebl/pqg.h;
/cvsroot/mozilla/security/nss/lib/freebl/pqg.h,v  <--  pqg.h
initial revision: 1.1
done
Checking in lib/freebl/rawhash.c;
/cvsroot/mozilla/security/nss/lib/freebl/rawhash.c,v  <--  rawhash.c
new revision: 1.8; previous revision: 1.7
done
Checking in lib/freebl/shvfy.c;
/cvsroot/mozilla/security/nss/lib/freebl/shvfy.c,v  <--  shvfy.c
new revision: 1.17; previous revision: 1.16
done
Checking in lib/softoken/pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.177; previous revision: 1.176
done
Checking in lib/softoken/pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.125; previous revision: 1.124
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.184; previous revision: 1.183
done

Comment 23

[Wan-Teh Chang]

Comment on attachment 611090 [details] [diff] [review]
Add DSA-2 tests to blapi_test

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

r=wtc.

It's not clear how we use the new file dsa_fips.txt.

The cmd/bltest/tests/dsa/numtests file is changed to contain 21, but the
the test files are numbered 1 to 20.  Is 21 off by one?

::: cmd/bltest/blapitest.c
@@ +1460,4 @@
>      return SECSuccess;
>  }
>  
> +blapi_pqg_param_gen(unsigned int keysize, PQGParams **pqg, PQGVerify **vfy)

Can PQG2_ParamGen handle keysize < 1024?  I am just wondering
whether you want test coverage for PQG_ParamGen or PQG_ParamGen
is needed to handle keysize < 1024.

Comment 24

[Robert Relyea]

> It's not clear how we use the new file dsa_fips.txt.

I included it for documentation about where tests 1-20 came from.

> The cmd/bltest/tests/dsa/numtests file is changed to contain 21, but the
> the test files are numbered 1 to 20.  Is 21 off by one?

The new tests are numbered 1 to 20. There is an old dsa test numbered 0, which I retained so the total is 21.

> Can PQG2_ParamGen handle keysize < 1024? 

Yes. I didn't include any tests because PQG_ParamGen already handled keysize < 1024 and had only one test.

bob

Comment 25

[Robert Relyea]

The following was checked in to fix windows breakage in tinderbox...


bobslaptop.local(71) cvs diff -u pqg.c
Index: pqg.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/lib/freebl/pqg.c,v
retrieving revision 1.19
diff -u -r1.19 pqg.c
--- pqg.c	12 Jun 2012 16:39:00 -0000	1.19
+++ pqg.c	13 Jun 2012 01:06:47 -0000
@@ -463,6 +463,8 @@
     unsigned char x[DSA_MAX_P_BITS/8+HASH_LENGTH_MAX];
     mp_err err = MP_OKAY;
     int i;
+    int iterations;
+    int old_counter;
 
     MP_DIGITS(&c) = 0;
     MP_DIGITS(&c0_2) = 0;
@@ -477,8 +479,6 @@
     CHECK_MPI_OK( mp_init(&z) );
     CHECK_MPI_OK( mp_init(&two_length_minus_1) );
 
-    int iterations;
-    int old_counter;
 
     /*
     ** There is a slight mapping of variable names depending on which
bobslaptop.local(72) cvs commit pqg.c
Checking in pqg.c;
/cvsroot/mozilla/security/nss/lib/freebl/pqg.c,v  <--  pqg.c
new revision: 1.20; previous revision: 1.19
done

Comment 26

[Robert Relyea]

Attachment: Update tests to blapit_test

Same as previous patch, except the name change of PQG2_ParamGen to PQG_ParamGenV2 as requested by wtc in his freebl review.

Comment 27

[Robert Relyea]

bltest checkin log:
Checking in cmd/bltest/tests/dsa/key7;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/key7,v  <--  key7
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/key8,v
done
Checking in cmd/bltest/tests/dsa/key8;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/key8,v  <--  key8
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/key9,v
done
Checking in cmd/bltest/tests/dsa/key9;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/key9,v  <--  key9
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed1,v
done
Checking in cmd/bltest/tests/dsa/keyseed1;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed1,v  <--  keyseed1
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed10,v
done
Checking in cmd/bltest/tests/dsa/keyseed10;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed10,v  <--  keyseed10
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed11,v
done
Checking in cmd/bltest/tests/dsa/keyseed11;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed11,v  <--  keyseed11
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed12,v
done
Checking in cmd/bltest/tests/dsa/keyseed12;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed12,v  <--  keyseed12
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed13,v
done
Checking in cmd/bltest/tests/dsa/keyseed13;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed13,v  <--  keyseed13
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed14,v
done
Checking in cmd/bltest/tests/dsa/keyseed14;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed14,v  <--  keyseed14
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed15,v
done
Checking in cmd/bltest/tests/dsa/keyseed15;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed15,v  <--  keyseed15
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed16,v
done
Checking in cmd/bltest/tests/dsa/keyseed16;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed16,v  <--  keyseed16
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed17,v
done
Checking in cmd/bltest/tests/dsa/keyseed17;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed17,v  <--  keyseed17
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed18,v
done
Checking in cmd/bltest/tests/dsa/keyseed18;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed18,v  <--  keyseed18
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed19,v
done
Checking in cmd/bltest/tests/dsa/keyseed19;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed19,v  <--  keyseed19
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed2,v
done
Checking in cmd/bltest/tests/dsa/keyseed2;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed2,v  <--  keyseed2
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed20,v
done
Checking in cmd/bltest/tests/dsa/keyseed20;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed20,v  <--  keyseed20
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed3,v
done
Checking in cmd/bltest/tests/dsa/keyseed3;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed3,v  <--  keyseed3
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed4,v
done
Checking in cmd/bltest/tests/dsa/keyseed4;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed4,v  <--  keyseed4
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed5,v
done
Checking in cmd/bltest/tests/dsa/keyseed5;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed5,v  <--  keyseed5
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed6,v
done
Checking in cmd/bltest/tests/dsa/keyseed6;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed6,v  <--  keyseed6
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed7,v
done
Checking in cmd/bltest/tests/dsa/keyseed7;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed7,v  <--  keyseed7
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed8,v
done
Checking in cmd/bltest/tests/dsa/keyseed8;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed8,v  <--  keyseed8
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed9,v
done
Checking in cmd/bltest/tests/dsa/keyseed9;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/keyseed9,v  <--  keyseed9
initial revision: 1.1
done
Checking in cmd/bltest/tests/dsa/numtests;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/numtests,v  <--  numtests
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext1,v
done
Checking in cmd/bltest/tests/dsa/plaintext1;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext1,v  <--  plaintext1
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext10,v
done
Checking in cmd/bltest/tests/dsa/plaintext10;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext10,v  <--  plaintext10
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext11,v
done
Checking in cmd/bltest/tests/dsa/plaintext11;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext11,v  <--  plaintext11
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext12,v
done
Checking in cmd/bltest/tests/dsa/plaintext12;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext12,v  <--  plaintext12
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext13,v
done
Checking in cmd/bltest/tests/dsa/plaintext13;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext13,v  <--  plaintext13
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext14,v
done
Checking in cmd/bltest/tests/dsa/plaintext14;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext14,v  <--  plaintext14
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext15,v
done
Checking in cmd/bltest/tests/dsa/plaintext15;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext15,v  <--  plaintext15
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext16,v
done
Checking in cmd/bltest/tests/dsa/plaintext16;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext16,v  <--  plaintext16
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext17,v
done
Checking in cmd/bltest/tests/dsa/plaintext17;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext17,v  <--  plaintext17
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext18,v
done
Checking in cmd/bltest/tests/dsa/plaintext18;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext18,v  <--  plaintext18
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext19,v
done
Checking in cmd/bltest/tests/dsa/plaintext19;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext19,v  <--  plaintext19
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext2,v
done
Checking in cmd/bltest/tests/dsa/plaintext2;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext2,v  <--  plaintext2
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext20,v
done
Checking in cmd/bltest/tests/dsa/plaintext20;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext20,v  <--  plaintext20
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext3,v
done
Checking in cmd/bltest/tests/dsa/plaintext3;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext3,v  <--  plaintext3
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext4,v
done
Checking in cmd/bltest/tests/dsa/plaintext4;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext4,v  <--  plaintext4
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext5,v
done
Checking in cmd/bltest/tests/dsa/plaintext5;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext5,v  <--  plaintext5
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext6,v
done
Checking in cmd/bltest/tests/dsa/plaintext6;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext6,v  <--  plaintext6
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext7,v
done
Checking in cmd/bltest/tests/dsa/plaintext7;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext7,v  <--  plaintext7
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext8,v
done
Checking in cmd/bltest/tests/dsa/plaintext8;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext8,v  <--  plaintext8
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext9,v
done
Checking in cmd/bltest/tests/dsa/plaintext9;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/plaintext9,v  <--  plaintext9
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg1,v
done
Checking in cmd/bltest/tests/dsa/pqg1;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg1,v  <--  pqg1
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg10,v
done
Checking in cmd/bltest/tests/dsa/pqg10;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg10,v  <--  pqg10
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg11,v
done
Checking in cmd/bltest/tests/dsa/pqg11;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg11,v  <--  pqg11
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg12,v
done
Checking in cmd/bltest/tests/dsa/pqg12;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg12,v  <--  pqg12
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg13,v
done
Checking in cmd/bltest/tests/dsa/pqg13;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg13,v  <--  pqg13
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg14,v
done
Checking in cmd/bltest/tests/dsa/pqg14;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg14,v  <--  pqg14
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg15,v
done
Checking in cmd/bltest/tests/dsa/pqg15;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg15,v  <--  pqg15
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg16,v
done
Checking in cmd/bltest/tests/dsa/pqg16;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg16,v  <--  pqg16
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg17,v
done
Checking in cmd/bltest/tests/dsa/pqg17;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg17,v  <--  pqg17
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg18,v
done
Checking in cmd/bltest/tests/dsa/pqg18;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg18,v  <--  pqg18
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg19,v
done
Checking in cmd/bltest/tests/dsa/pqg19;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg19,v  <--  pqg19
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg2,v
done
Checking in cmd/bltest/tests/dsa/pqg2;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg2,v  <--  pqg2
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg20,v
done
Checking in cmd/bltest/tests/dsa/pqg20;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg20,v  <--  pqg20
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg3,v
done
Checking in cmd/bltest/tests/dsa/pqg3;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg3,v  <--  pqg3
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg4,v
done
Checking in cmd/bltest/tests/dsa/pqg4;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg4,v  <--  pqg4
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg5,v
done
Checking in cmd/bltest/tests/dsa/pqg5;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg5,v  <--  pqg5
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg6,v
done
Checking in cmd/bltest/tests/dsa/pqg6;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg6,v  <--  pqg6
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg7,v
done
Checking in cmd/bltest/tests/dsa/pqg7;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg7,v  <--  pqg7
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg8,v
done
Checking in cmd/bltest/tests/dsa/pqg8;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg8,v  <--  pqg8
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg9,v
done
Checking in cmd/bltest/tests/dsa/pqg9;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/pqg9,v  <--  pqg9
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed1,v
done
Checking in cmd/bltest/tests/dsa/sigseed1;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed1,v  <--  sigseed1
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed10,v
done
Checking in cmd/bltest/tests/dsa/sigseed10;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed10,v  <--  sigseed10
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed11,v
done
Checking in cmd/bltest/tests/dsa/sigseed11;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed11,v  <--  sigseed11
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed12,v
done
Checking in cmd/bltest/tests/dsa/sigseed12;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed12,v  <--  sigseed12
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed13,v
done
Checking in cmd/bltest/tests/dsa/sigseed13;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed13,v  <--  sigseed13
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed14,v
done
Checking in cmd/bltest/tests/dsa/sigseed14;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed14,v  <--  sigseed14
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed15,v
done
Checking in cmd/bltest/tests/dsa/sigseed15;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed15,v  <--  sigseed15
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed16,v
done
Checking in cmd/bltest/tests/dsa/sigseed16;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed16,v  <--  sigseed16
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed17,v
done
Checking in cmd/bltest/tests/dsa/sigseed17;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed17,v  <--  sigseed17
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed18,v
done
Checking in cmd/bltest/tests/dsa/sigseed18;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed18,v  <--  sigseed18
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed19,v
done
Checking in cmd/bltest/tests/dsa/sigseed19;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed19,v  <--  sigseed19
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed2,v
done
Checking in cmd/bltest/tests/dsa/sigseed2;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed2,v  <--  sigseed2
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed20,v
done
Checking in cmd/bltest/tests/dsa/sigseed20;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed20,v  <--  sigseed20
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed3,v
done
Checking in cmd/bltest/tests/dsa/sigseed3;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed3,v  <--  sigseed3
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed4,v
done
Checking in cmd/bltest/tests/dsa/sigseed4;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed4,v  <--  sigseed4
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed5,v
done
Checking in cmd/bltest/tests/dsa/sigseed5;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed5,v  <--  sigseed5
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed6,v
done
Checking in cmd/bltest/tests/dsa/sigseed6;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed6,v  <--  sigseed6
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed7,v
done
Checking in cmd/bltest/tests/dsa/sigseed7;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed7,v  <--  sigseed7
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed8,v
done
Checking in cmd/bltest/tests/dsa/sigseed8;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed8,v  <--  sigseed8
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed9,v
done
Checking in cmd/bltest/tests/dsa/sigseed9;
/cvsroot/mozilla/security/nss/cmd/bltest/tests/dsa/sigseed9,v  <--  sigseed9
initial revision: 1.1
done

Comment 28

[Wan-Teh Chang]

(In reply to Robert Relyea from comment #24)
> > It's not clear how we use the new file dsa_fips.txt.
> 
> I included it for documentation about where tests 1-20 came from.

Could you add a README file in cmd/bltest/tests/dsa to explain
this?  Thanks.  (We already have cmd/bltest/tests/README, but
I think cmd/bltest/tests/dsa/README would be clearer.)

Comment 29

[Wan-Teh Chang]

Comment on attachment 611085 [details] [diff] [review]
Add DSA-2 support to pk11wrap

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

r=wtc.  This patch needs work, but I'm giving it review+ in advance
because I won't be able to review it again in the next two weeks.
You can check it in after making the suggested changes.  Please
attach the new version of the patch that you check in.  Thanks.

High-Level Comments

I suggest renaming the three exported functions.

I suggest using 'unsigned int' instead of CK_ULONG for the parameters
of the exported functions.

I found two bugs (marked with "BUG" in my comments).

Other comments are all about nits.

::: lib/nss/nss.def
@@ +1034,4 @@
>  ;+    local:
>  ;+       *;
>  ;+};
> +;+NSS_3.13.4 { 	# NSS 3.13.4 release

Change this to NSS 3.14.

::: lib/pk11wrap/pk11obj.c
@@ +556,1 @@
>      SECItem params = {siBuffer, NULL, 0};

Nit: this SECItem was named 'params' because it holds the value of
CKA_EC_PARAMS.  In the new code, it may also hold the value of
CKA_SUBPRIME, so a more neutral name such as 'attribute', 'attrVal',
or even 'item' would be better.

@@ +572,5 @@
> +        rv = PK11_ReadAttribute(key->pkcs11Slot, key->pkcs11ID, CKA_SUBPRIME, 
> +				NULL, &params);
> +        if (rv == SECSuccess) {
> +	    length = params.len;
> +	    if (params.data[0] == 0) {

Unless params.len cannot possibly be 0, it is better to check if params.len > 0
before reading params.data[0]:
            if (length > 0 && params.data[0] == 0) {
                length--;
            }

@@ +591,5 @@
>  		length = ((length + 7)/8) * 2;
>  		return length;
>  	    }
>  	}
> +	return pk11_backupGetSignLength(key);

NOTE: there could be a subtle change of behavior here.  In the original code, if
we can't read the CKA_EC_PARAMS attribute of the EC key, we break out of the switch
statement and return 0.  In the new code, we return pk11_backupGetSignLength(key) in
this case.

Please confirm that this change of behavior is deliberate or doesn't matter.

@@ +711,5 @@
> +				 * positive sign */
> +				(key->u.dsa.params.prime.len > 129)) {
> +	    /* we need to get a slot that not only can do DSA, but can do DSA2
> +	     * key lengths */
> +	    int length = key->u.dsa.params.prime.len;

I think 'length' should be 'unsigned int'.

::: lib/pk11wrap/pk11pqg.c
@@ +47,5 @@
>  
>  
>  /* Generate PQGParams and PQGVerify structs.
> + * Length of P specified by primeBits.
> + *   if P is greater than 1023 then the resulting verify parameters will be

Do you mean 1023 or 1024 here?

In the rest of this comment block, 'P' and 'Q' are incorrectly used to refer
to the lengths of P and Q.  So "if P is greater than 1023" really means
"if P is longer than 1023 bits" or "if primeBits is greater than 1023".

Note that PQG_ParamGenV2 in freebl names these two parameters 'L' and 'N'.
That would also be a good solution.

@@ +68,5 @@
> + *   P=3072   Q=0 or 256
> + * if P <= 1024
> + *   seedBbytes must be in the range [20..256].
> + * if P >= 1024
> + *   seedBbytes must be in the range [20..P/16].

After you fix this comment block, please copy it to pk11pqg.h because the
comment block in pk11pqg.h has the same problems.

@@ +73,3 @@
>   */
>  extern SECStatus
> +PK11_PQG2_ParamGen( CK_ULONG primeBits, CK_ULONG subPrimeBits,

PK11_PQG2_ParamGen => PK11_PQG_ParamGenV2 to match the PQG_ParamGenV2 function in
freebl.

I strongly recommend using "unsigned int' as the type of primeBits and subPrimeBits.

@@ +132,5 @@
> +
> +	crv = PK11_GETTAB(slot)->C_GetMechanismInfo(slot->slotID,
> +			CKM_DSA_PARAMETER_GEN, &mechanism_info);
> +	/* a bug in the old softoken left CKM_DSA_PARAMETER_GEN off of the
> +	 * mechanism List. If we get a failure asking for this value, we know

Nit: List => list

@@ +142,5 @@
> +	    if (slot == NULL) {
> +		PORT_SetError(SEC_ERROR_NO_TOKEN); /* can happen */
> +		goto loser;
> +	    }
> +	    /* ditch seedbits in this case, they are nescape specific and at

Nit: seedbits => seedBits
     nescape => netscape (or Netscape)

@@ +254,5 @@
> +extern SECStatus
> +PK11_PQG_ParamGenSeedLen( unsigned int j, unsigned int seedBytes,
> +				 PQGParams **pParams, PQGVerify **pVfy)
> +{
> +    CK_ULONG primeBits = PQG_INDEX_TO_PBITS(j);

CK_ULONG => unsigned int

@@ +265,5 @@
>   */
>  extern SECStatus
>  PK11_PQG_ParamGen(unsigned int j, PQGParams **pParams, PQGVerify **pVfy)
>  {
> +    CK_ULONG primeBits = PQG_INDEX_TO_PBITS(j);

CK_ULONG => unsigned int

But actually, this function doesn't need to be changed.  The original code
still works:
    return PK11_PQG_ParamGenSeedLen(j, 0, pParams, pVfy);

@@ +321,3 @@
>      PK11_SETATTRS(attrs, CKA_TOKEN, &ckfalse, sizeof(ckfalse)); attrs++;
>      if (vfy) {
> +	if (vfy->counter >= 0) {

BUG: this may be a bug.

vfy->counter is an unsigned int, so vfy->counter >= 0 is always true.  Did you
mean to say "if (vfy->counter > 0) {" ?

::: lib/pk11wrap/pk11pqg.h
@@ +100,5 @@
>   *       SECFailure: PQGParams are invalid.
>   *
>   * Verify the following 12 facts about PQG counter SEED g and h
> + * These tests are specified in FIPS 186-3 Appendix A.1.1.1, A.1.1.3, and A.2.2
> + * PQG_VerifyParams will automatically choose the appropriate test.

PQG_VerifyParams => PK11_PQG_VerifyParams

::: lib/pk11wrap/pk11slot.c
@@ +1994,5 @@
> + * In normal cases this should grab the first slot on the list with no fuss.
> + * The size array is presumed to match one for one with the mechanism type 
> + * array, which allows you to specify the required key size for each
> + * mechanism in the list. Whether key size is in bits or bytes is mechanism
> + * dependent. Typically

"Typically" is an incomplete sentence...

@@ +1999,3 @@
>   */
>  PK11SlotInfo *
> +PK11_GetBestSlotMultipleKeySize(CK_MECHANISM_TYPE *type, CK_ULONG *size,

The function name is misleading because it seems to suggest the function
returns a key size.  How about PK11_GetBestSlotMultipleWithKeySize or
PK11_GetBestSlotWithKeySizeMultiple?

CK_ULONG *size => const unsigned int *keySize  (note the renaming)

I also recommend changing "int mech_count" to "unsigned int mech_count".
"unsigned int" is more appropriate, but it'll create an inconsistency
with the older PK11_GetBestSlotMultiple function.

@@ +2055,5 @@
> +    		    CK_RV crv = PK11_GETTAB(le->slot)->C_GetMechanismInfo(
> +				slot->slotID, type[i], &mechanism_info);
> +		    if (crv != CKR_OK 
> +			|| (mechanism_info.ulMinKeySize < size[i])
> +			|| (mechanism_info.ulMaxKeySize > size[i])) {

BUG: I believe this should be
    mechanism_info.ulMinKeySize > size[i]
and
    mechanism_info.ulMaxKeySize < size[i]

@@ +2098,5 @@
> +    return PK11_GetBestSlotMultipleKeySize(&type, NULL, 1, wincx);
> +}
> +
> +PK11SlotInfo *
> +PK11_GetBestSlotKeySize(CK_MECHANISM_TYPE type, CK_ULONG keySize, 

The function name is misleading because it seems to suggest the function
returns a key size.  How about PK11_GetBestSlotWithKeySize or
PK11_GetBestSlotByKeySize?

CK_ULONG keySize => unsigned int keySize

Comment 30

[Wan-Teh Chang]

Comment on attachment 632291 [details] [diff] [review]
Updated patch for freebl/softoken

>> makePrimefromPrimesShawneTaylor(

ShawneTaylor is a typo. It should be ShaweTaylor :-)

Comment 31

[Wan-Teh Chang]

Comment on attachment 611086 [details] [diff] [review]
Add DSA-2 support to cryptohi

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

r=wtc.  Please attach the new version of the patch before you
check this in.

All of my comments below are about nits, except two bugs, which I
marked with "BUG".

::: lib/cryptohi/cryptohi.h
@@ +60,4 @@
>  ** DER encode/decode (EC)DSA signatures
>  */
>  
> +/* ANSI X9.57 defines DSA1 signatures as DER encoded data.  Our DSA code (and

Nit: should we also change "DSA code" to "DSA1 code"?

::: lib/cryptohi/dsautil.c
@@ +39,5 @@
>  #include "secitem.h"
>  #include "prerr.h"
>  
> +#ifndef DSA1_SUBPRIME_LEN
> +#define DSA1_SUBPRIME_LEN 20	/* bytes */

Should we include "blapit.h" and remove the local definition of
the DSA1_SUBPRIME_LEN macro?  Is it bad for a cryptohi file to
include "blapit.h"?

::: lib/cryptohi/seckey.c
@@ +364,2 @@
>  
>  	if ( (tag != SEC_OID_ANSIX9_DSA_SIGNATURE) &&

Above this code there is a comment
    Check if cert has a DSA public key.

Should we change it to
    Check if cert has a DSA or EC public key.
?

BUG: I don't understand why we are testing for SEC_OID_ANSIX962_EC_PUBLIC_KEY
here.  EC public keys don't have PQG parameters.  Is this a bug?

@@ +408,2 @@
>  
>  	if ( (tag != SEC_OID_ANSIX9_DSA_SIGNATURE) &&

Similarly, above this code there is a comment
    Check if issuer cert has a DSA public key.

Should we change that comment, too?

@@ +1038,5 @@
>      	b0 = pubk->u.rsa.modulus.data[0];
>      	return b0 ? pubk->u.rsa.modulus.len : pubk->u.rsa.modulus.len - 1;
>      case dsaKey:
> +	size = pubk->u.dsa.params.subPrime.len;
> +    	return size*2;

Nit: add spaces around '*'.

I suggest we just say
    return pubk->u.dsa.params.subPrime.len * 2;

::: lib/cryptohi/secsign.c
@@ +359,5 @@
>  	    algID = SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION;
>  	    break;
>  	  case dsaKey:
> +	    /* get q_len and work from there */
> +	    len = PK11_SignatureLen(pk);

Nit: the "q_len" in the comment is confusing because PK11_SignatureLen
is 2 * q_len.

BUG: 'len' is an input argument (the length of the input argument 'buf').
It seems wrong to overwrite 'len' here.

::: lib/cryptohi/secvfy.c
@@ +268,5 @@
> +        *hashalg = SEC_OID_SHA224;
> +	break;
> +      case SEC_OID_NIST_DSA_SIGNATURE_WITH_SHA256_DIGEST:
> +        *hashalg = SEC_OID_SHA256;
> +	break;

Nit: we can simply add SEC_OID_NIST_DSA_SIGNATURE_WITH_SHA224_DIGEST
and SEC_OID_NIST_DSA_SIGNATURE_WITH_SHA256_DIGEST to the existing lists
of cases for SEC_OID_SHA224 and SEC_OID_SHA256 above.

::: lib/util/secoid.c
@@ +77,4 @@
>  #define NISTALGS    USGOV, 3, 4
>  #define AES         NISTALGS, 1
>  #define SHAXXX      NISTALGS, 2
> +#define DSA2        NISTALGS, 3

NIST calls this OID arc 'sigAlgs', so there could be non-DSA
NIST signature algorithms under this arc in the future.  I
guess we should name this arc NISTSIGALGS, but that's a bit
too long.  So I am fine with naming this arc DSA2 until that
happens.

@@ +1664,5 @@
> +	"DSA2 with SHA-224 Signature",
> +	CKM_INVALID_MECHANISM /* not yet defined */, INVALID_CERT_EXTENSION),
> +    OD( nistDSASignaturewithSHA256Digest,
> +	SEC_OID_NIST_DSA_SIGNATURE_WITH_SHA256_DIGEST,
> +	"DSA2 with SHA-256 Signature",

Nit: "DSA2" is not the official name used by NIST although I have seen it
used informally.  I think in the text messages we should use "DSA" instead
of "DSA2".  Cf: the names of the OIDs are
id-dsa-with-sha224 ::= { sigAlgs 1 }
id-dsa-with-sha256 ::= { sigAlgs 2 }

Comment 32

[Robert Relyea]

-- pk11 wrap...

Thanks for the review wtc. If I didn't comment, then I'll accept your recommendation wholesale.

> I suggest using 'unsigned int' instead of CK_ULONG for the parameters
> of the exported functions.

If they match PKCS #11 values, probably unsigned long. Particularly if the are lengths.

> Please confirm that this change of behavior is deliberate or doesn't matter.

It's deliberate. The new behavior is more robust.

> Do you mean 1023 or 1024 here?

I meant less than equal to 1023 (or less than 1024). The latter is probably clearer.

> CK_ULONG => unsigned int

I'll make this change, but it does mean I need to make an extra copy in PK11_PQG_ParamGenV2. The values in the templates really need to be CK_ULONG. On some platforms CK_ULONG and unsigned int are different sizes.

> BUG: this may be a bug.

yup.

> The function name is misleading 

I'll change the name. It will probably change again do to bug 613496. Rather than add 3x2 new functions I'd rather add 1x2 new functions. Where the new function handles both size and Attribute contraints.


> I also recommend changing "int mech_count" to "unsigned int mech_count".
> "unsigned int" is more appropriate, but it'll create an inconsistency
> with the older PK11_GetBestSlotMultiple function.

I prefer consistency, though I'm not against changing this to unsigned int in all occurances. It doesn't change the size, so it's not an ABI issue. It may cause some compiler warnings in older applications.

> BUG: I believe this should be
yes.

> CK_ULONG keySize => unsigned int keySize

I'll likely make this unsigned long.

Comment 33

[Robert Relyea]

-- cryptohi...

Thanks for the review wtc. If I didn't comment, then I'll accept your recommendation wholesale.

> Nit: should we also change "DSA code" to "DSA1 code"?

We should change it to "ANSI X9.57 defines DSA signatures as DER encoded data.  Our DSA1 code (and"

I believe X9.57 applies to all forms of DSA (including ECC and "DSA-2").

> Should we include "blapit.h" and remove the local definition of the DSA1_SUBPRIME_LEN macro?

I seem to remember trying to avoid it, but I can't remember why.

> Should we change it to
>    Check if cert has a DSA or EC public key?

Most likely.

> BUG: 'len' is an input argument (the length of the input argument 'buf').
> It seems wrong to overwrite 'len' here.

Ouch this one should have been caught by our tests. It looks like we don't call SEC_DerSignData in the nsstests. In this case len is almost certainly signatureLen/2 in the normal case.

Comment 34

[Robert Relyea]

Attachment: Add DSA-2 support to pk11wrap - with wtc comments

Comment 35

[Robert Relyea]

Checking in lib/pk11wrap/pk11obj.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11obj.c,v  <--  pk11obj.c
new revision: 1.26; previous revision: 1.25
done
Checking in lib/pk11wrap/pk11pqg.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pqg.c,v  <--  pk11pqg.c
new revision: 1.16; previous revision: 1.15
done
Checking in lib/pk11wrap/pk11pqg.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pqg.h,v  <--  pk11pqg.h
new revision: 1.8; previous revision: 1.7
done
Checking in lib/pk11wrap/pk11pub.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pub.h,v  <--  pk11pub.h
new revision: 1.39; previous revision: 1.38
done
Checking in lib/pk11wrap/pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.109; previous revision: 1.108
done
Checking in lib/pk11wrap/pk11util.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11util.c,v  <--  pk11util.c
new revision: 1.62; previous revision: 1.61
done
Checking in lib/nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.219; previous revision: 1.218
done
Checking in lib/freebl/pqg.c;
/cvsroot/mozilla/security/nss/lib/freebl/pqg.c,v  <--  pqg.c
new revision: 1.21; previous revision: 1.20
done

Comment 36

[Robert Relyea]

Checking in lib/cryptohi/cryptohi.h;
/cvsroot/mozilla/security/nss/lib/cryptohi/cryptohi.h,v  <--  cryptohi.h
new revision: 1.17; previous revision: 1.16
done
Checking in lib/cryptohi/dsautil.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/dsautil.c,v  <--  dsautil.c
new revision: 1.10; previous revision: 1.9
done
Checking in lib/cryptohi/seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.68; previous revision: 1.67
done
Checking in lib/cryptohi/secsign.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/secsign.c,v  <--  secsign.c
new revision: 1.29; previous revision: 1.28
done
Checking in lib/cryptohi/secvfy.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/secvfy.c,v  <--  secvfy.c
new revision: 1.30; previous revision: 1.29
done
Checking in lib/pk11wrap/pk11pub.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pub.h,v  <--  pk11pub.h
new revision: 1.40; previous revision: 1.39
done
Checking in lib/pk11wrap/pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.110; previous revision: 1.109
done
Checking in lib/util/secoid.c;
/cvsroot/mozilla/security/nss/lib/util/secoid.c,v  <--  secoid.c
new revision: 1.66; previous revision: 1.65
done
Checking in lib/util/secoidt.h;
/cvsroot/mozilla/security/nss/lib/util/secoidt.h,v  <--  secoidt.h
new revision: 1.36; previous revision: 1.35
done

Comment 37

[Robert Relyea]

Attachment: Add DSA-2 support to cryptohi with wtc comments

Comment 38

[Elio Maldonado]

Comment on attachment 611091 [details] [diff] [review]
DSA-2 changes to handle FIPS .req files for DSA-2

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

r- because it needs synching up with latest renaming done in the library part. Once PQG2_ParamGen is changed to PQG_ParamGenV2 it will. Other comments below.

::: cmd/fipstest/fipstest.c
@@ +3247,5 @@
> +    default:
> +	break;
> +    }
> +    return rv;
> +}

Please change mess to msg as it's used elsewere.

Now, I prefer an arrangement like this

SECStatus
fips_hashBuf(HASH_HashType type, unsigned char *hashBuf,
unsigned char *nsg, int len)
{
    switch (type) {
    case HASH_AlgSHA1:   return SHA1_HashBuf(hashBuf, msg, len);
    case HASH_AlgSHA224: return SHA224_HashBuf(hashBuf, msg, len);
    case HASH_AlgSHA256: return SHA256_HashBuf(hashBuf, msg, len);
    case HASH_AlgSHA384: return SHA384_HashBuf(hashBuf, mgs, len);
    case HASH_AlgSHA512: return SHA512_HashBuf(hashBuf, msg, len);
    default: return SECFailure;
    }
}
being compact I find that it reads easier. Just a suggestion.
Similarly for the next functions.

@@ +3274,5 @@
> +    default:
> +	break;
> +    }
> +    return len;
> +}

similar compactness suggestion as before

{
    switch (type) {
    case HASH_AlgSHA1:   return SHA1_LENGTH;
    case HASH_AlgSHA224: return SHA224_LENGTH;
    case HASH_AlgSHA256: return SHA256_LENGTH;
    case HASH_AlgSHA384: return SHA384_LENGTH;
    case HASH_AlgSHA512: return SHA512_LENGTH;
    default: return 0;
    }
}

@@ +3301,5 @@
> +    default:
> +	break;
> +    }
> +    return oid;
> +}

similar suggestion as above regarding simpler code, srictly optional.

@@ +3329,5 @@
> +    default:
> +	break;
> +    }
> +    return hashType;
> +}

similar suggestion as above regarding simpler code, scrictly optional.

@@ +3346,2 @@
>  
> +    return fips_hashBuf(hashType, MD, msg, msgLen);

return fips_hashBuf(sha_get_hashType(MDLen*BITS_PER_BYTE), MD, msg, msgLen);
saves you a local variable that's used only once.

@@ +3820,5 @@
> +				"ERROR: Unable to generate PQG parameters");
> +                    goto loser;
> +                }
> +	    } else {
> +                if (PQG2_ParamGen(L, N, N, &pqg, &vfy) != SECSuccess) {

Change PQG2_ParamGen to PQG_ParamGenV2 so it compiles. The function was renamed in response to wtc's review.

@@ +3934,5 @@
> +	    } else {
> +		fprintf(stderr, "Unknown dsa ver test %s\n", &buf[1]);
> +		exit(1);
> +	    }
> +		

white space

@@ +4309,5 @@
> +                if (type == FIPS186_1) {
> +                    rv = PQG_ParamGenSeedLen(keySizeIndex, PQG_TEST_SEED_BYTES,
> +                         &pqg, &vfy);
> +                } else {
> +                    rv = PQG2_ParamGen(L, N, N, &pqg, &vfy);

Change PQG2_ParamGen to PQG_ParamGenV2 so it compiles. Function  renamed in response to wtc's review.

@@ +4380,5 @@
>                           * max for Msg = ....
>                           */
>      FILE *dsareq;     /* input stream from the REQUEST file */
>      FILE *dsaresp;    /* output stream to the RESPONSE file */
>      int modulus;          

nuke trailing whitespace.

@@ +4451,5 @@
> +                            "ERROR: Unable to generate PQG parameters");
> +                    goto loser;
> +                }
> +            } else {
> +                if (PQG2_ParamGen(L, N, N, &pqg, &vfy) != SECSuccess) {

Another one, Change PQG2_ParamGen to PQG_ParamGenV2.

@@ +4470,4 @@
>                  fprintf(dsaresp, "ERROR: Unable to generate DSA key");
>                  goto loser;
>              }
> +                

trailing white space.

Comment 39

[Wan-Teh Chang]

Attachment: lib/freebl/pqg.c should not include lib/cryptohi/sechash.h

In bug 753116, sechash.h was moved to lib/crypto.
So files in lib/freebl cannot include sechash.h any
more. Also, the HASH_ResultLen and HASH_HashBuf functions
in lib/freebl/rawhash.c conflict with the same-named
functions in lib/crypto/sechash.c in NSS static library
builds.

This patch is one way to solve this problem.

Another solution is to rename the HASH_ResultLen and
HASH_HashBuf functions (to HASH_RawResultLen and
HASH_RawHashBuf?) and declare them in blapi.h, right
after the declaration of HASH_GetRawHashObject. But
this seems to imply they need to be added to the
FREEBLVector. That seems bad.

Yes another solution could be to use the NSSLOWHASH_
functions. That'll be

Comment 40

[Wan-Teh Chang]

Attachment: Fix a compiler warning and style nits in lib/freebl

Comment 41

[Wan-Teh Chang]

Comment on attachment 663704 [details] [diff] [review]
Fix a compiler warning and style nits in lib/freebl

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

Checking in pqg.c;
/cvsroot/mozilla/security/nss/lib/freebl/pqg.c,v  <--  pqg.c
new revision: 1.22; previous revision: 1.21
done
Checking in pqg.h;
/cvsroot/mozilla/security/nss/lib/freebl/pqg.h,v  <--  pqg.h
new revision: 1.2; previous revision: 1.1
done
Checking in shvfy.c;
/cvsroot/mozilla/security/nss/lib/freebl/shvfy.c,v  <--  shvfy.c
new revision: 1.18; previous revision: 1.17
done

Comment 42

[Robert Relyea]

Comment on attachment 663703 [details] [diff] [review]
lib/freebl/pqg.c should not include lib/cryptohi/sechash.h

r+ rrelyea

Comment 43

[Robert Relyea]

> I prefer an arrangement like this
> 
> SECStatus
> fips_hashBuf(HASH_HashType type, unsigned char *hashBuf, unsigned char *nsg, int len)
> {
>    switch (type) {
>    case HASH_AlgSHA1:   return SHA1_HashBuf(hashBuf, msg, len);
>    case HASH_AlgSHA224: return SHA224_HashBuf(hashBuf, msg, len);
>    case HASH_AlgSHA256: return SHA256_HashBuf(hashBuf, msg, len);
>    case HASH_AlgSHA384: return SHA384_HashBuf(hashBuf, mgs, len);
>    case HASH_AlgSHA512: return SHA512_HashBuf(hashBuf, msg, len);
>    default: return SECFailure;
>    }
> }
>
> being compact I find that it reads easier. Just a suggestion.

I'll decline your suggestion as I found switch statements that have no path that falls through hard to read document, etc, and thus I very purposefully avoid them...

> return fips_hashBuf(sha_get_hashType(MDLen*BITS_PER_BYTE), MD, msg, msgLen);
> saves you a local variable that's used only once.

At the cost of readability. If your compilier can't figure out to optimize away the variable, you need a new compilier:).


The PQG2_ was already changed in my tree, and along with the rest of your comments I've included it in a new patch.

bob

Comment 44

[Robert Relyea]

Attachment: include Elio's comments for DSA-2 changes to handle FIPS .req files for DSA-2

Comment 45

[Elio Maldonado]

Comment on attachment 664676 [details] [diff] [review]
include Elio's comments for DSA-2 changes to handle FIPS .req files for DSA-2

r+ with a small request on something small I missed the previous time.

The dsa_pqg_type enum has an unwanted comma after the last
constant.

typedef enum {
  << snip >>
    A_2_4,   /* Verify Verifiable G */
         ^
} dsa_pqg_type;

Could you remove it? Thanks.

Comment 46

[Wan-Teh Chang]

Comment on attachment 663703 [details] [diff] [review]
lib/freebl/pqg.c should not include lib/cryptohi/sechash.h

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

Checking in pqg.c;
/cvsroot/mozilla/security/nss/lib/freebl/pqg.c,v  <--  pqg.c
new revision: 1.23; previous revision: 1.22
done
Checking in rawhash.c;
/cvsroot/mozilla/security/nss/lib/freebl/rawhash.c,v  <--  rawhash.c
new revision: 1.11; previous revision: 1.10
done

Comment 47

[Wan-Teh Chang]

Attachment: Add comments to explain the *2 in DSA and ECDSA signature lengths

Ryan, please review the comments.

Bob, I have a question for you. Ryan asked about this code:

     case dsaKey:
         rv = PK11_ReadAttribute(key->pkcs11Slot, key->pkcs11ID, CKA_SUBPRIME, 
 				NULL, &attributeItem);
         if (rv == SECSuccess) {
 	    length = attributeItem.len;
 	    if ((length > 0) && attributeItem.data[0] == 0) {
 		length--;
 	    }

CKA_SUBPRIME is a PKCS #11 Big Integer, which is defined as
follows:

Big integer
    a string of CK_BYTEs representing an unsigned
    integer of arbitrary size, most-significant byte first
    (e.g., the integer 32768 is represented as the 2-byte
    string 0x80 0x00)

An unsigned integer type does not need a leading zero
byte if the most significant bit is 1. Also note the
example is represented as 0x80 0x00 rather than
0x00 0x80 0x00.

So the code to skip the leading zero byte should be
unnecessary. Did you add that code unintentionally?
Or did you know some PKCS #11 modules store Big Integers
with leading zero bytes incorrect?

Comment 48

[Robert Relyea]

> So the code to skip the leading zero byte should be
> unnecessary. Did you add that code unintentionally?

No, it's protection. Not all uses of PKCS #11 (most notably softoken) have always implemented the unsigned nature of PKCS #11 Big Integers correctly. You will find this kind of code scattered throughout the wrapper layer.

bob

Comment 49

[Ryan Sleevi]

Comment on attachment 664727 [details] [diff] [review]
Add comments to explain the *2 in DSA and ECDSA signature lengths

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

::: pk11obj.c
@@ +542,5 @@
>  	    if ((length > 0) && attributeItem.data[0] == 0) {
>  		length--;
>  	    }
>  	    PORT_Free(attributeItem.data);
> +	    /* A DSA signature is the concatenation of r and s. */

My comment about surprise was more of the CKA_SUBPRIME and the .data[0] == 0 check, which I still think may be deserving a comment (eg: Bob's explanation that this is defensive against bad implementations), to be clear what's a necessary part of the spec and what are either known or expected rough edges.


That said, a possible slight improvement would be
/* A DSA signature is the concatenation of r and s, which are
 * both |length| (the size in bytes of q, the subprime) bytes long
 */

/* An ECDSA signature is the concatenation of r and s, which
 * are both |length| (the size in bits of n, the EC base point order) bits long
 */

Comment 51

[Robert Relyea]

Attachment: Fix problem when generating pqg < 512. Switch to Shawe/Taylor pqg generation

This patch makes several small changes:

1) in bltest, we need to look at the actual signature length returned by the sign operation. In the old days, it was simply the max signature length for DSA2. Now it depends on q. The underlying code sets the length properly, we just need to copy that length to our buffer.

2) bltest passes in '0' for Q and seedLen when it just wants the defaults. This mirrors the pk11wrap interface which does the same thing. We need to actually pick defaults if we pass '0' in.

3) add code block to handle generating pqg from shawne/taylor, and select shawn/taylor

4) performance.sh has seemed to have atrophied. We need to remove the previous output file, or the new one may simply overwrite the old one (blapitest does not open these files O_TRUNC).

Comment 52

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 669298 [details] [diff] [review]
Fix problem when generating pqg < 512. Switch to Shawe/Taylor pqg generation

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

(This comment is best viewed in Splinter.)

Bob, I am not done with the review yet, but I have some questions. Also, I believe I found a bug, in addition to the normal minor nits.

::: lib/freebl/pqg.c
@@ +73,4 @@
>   * this gives us one place to go if we need to bump the requirements in the
>   * future.
>   */
> +static SECStatus 

Trailing whitespace. View your patch in the splinter interface to see the other trailing whitespace lines highlighted.

@@ +103,5 @@
>      return SECSuccess;
>  }
>  
> +static unsigned int
> +pqg_get_default_N(unsigned int L)

IMO, it is confusing to compute a default for the caller. Is it necessary? Can't we just require the caller to always supply an explicit value of N?

It is especially confusing between the spec offers two choices for L==2048: 224 or 256. (See below.)

@@ +111,5 @@
> +    case 1024:
> +	N = DSA1_Q_BITS;
> +	break;
> +    case 2048:
> +	N = 224;

Section 4.2, "Selection of Parameter Sizes and Hash Functions for DSA" allows both N == 256 and N == 224 for L == 2048. So, why not use N == 256 instead of 224, since N == 256 should be more secure?

@@ +1245,5 @@
>      PRBool passed;
>      SECItem hit = { 0, 0, 0 };
> +    SECItem firstseed = { 0, 0, 0};
> +    SECItem qseed = {0 , 0 , 0};
> +    SECItem pseed = {0 , 0 , 0};

Add a space to these three lines: "0};" -> "0 };" to match the line above them.

@@ +1368,5 @@
> +	}
> +	PORT_Memcpy(seed->data, firstseed.data, firstseed.len);
> +	PORT_Memcpy(seed->data+firstseed.len, pseed.data, pseed.len);
> +	PORT_Memcpy(seed->data+firstseed.len+pseed.len, qseed.data, qseed.len);
> +	counter = (qgen_counter << 16) || pgen_counter;

Replace logical (||) OR with bitwise OR (|).

@@ +1552,5 @@
>  PQG_ParamGenV2(unsigned int L, unsigned int N, unsigned int seedBytes,
>                      PQGParams **pParams, PQGVerify **pVfy)
>  {
> +    if (N == 0) {
> +	N = pqg_get_default_N(L);

Please explain why it is important to have this feature of choosing N based on L.

Comment 53

[Wan-Teh Chang]

Comment on attachment 669298 [details] [diff] [review]
Fix problem when generating pqg < 512. Switch to Shawe/Taylor pqg generation

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

r=wtc.

::: cmd/bltest/blapitest.c
@@ +2185,4 @@
>              }
>              TIMEFINISH(cipherInfo->optime, 1.0);
>          }
> +	cipherInfo->output.buf.len = cipherInfo->output.pBuf.len;

Why is this change necessary? Is it because cipherInfo->output.buf.len
was too long? cipherInfo->output.buf seems to be allocated in cipherInit():

    case bltestDSA:
        SECITEM_AllocItem(cipherInfo->arena, &cipherInfo->output.buf,
                          DSA_MAX_SIGNATURE_LEN);
        return bltest_dsa_init(cipherInfo, encrypt);
        break;

Does ecdsaOp() need the same change?

::: lib/freebl/pqg.c
@@ +118,5 @@
> +	N = 256;
> +	break;
> +    default:
> +	PORT_SetError(SEC_ERROR_INVALID_ARGS);
> +	return 0;

You can just use a "break" statement here because you
initialized N to 0.

@@ +1245,5 @@
>      PRBool passed;
>      SECItem hit = { 0, 0, 0 };
> +    SECItem firstseed = { 0, 0, 0};
> +    SECItem qseed = {0 , 0 , 0};
> +    SECItem pseed = {0 , 0 , 0};

Nit: it's better to use NULL as the second components of these
SECItem initializers.

@@ +1343,4 @@
>      */
>      if (type == FIPS186_1_TYPE) {
>  	CHECK_SEC_OK( makeQfromSeed(seedlen, seed, &Q) );
> +    } else if (type == FIPS186_3_TYPE) {

You should update the comment block before this to reflect
the third way to compute Q.

@@ +1368,5 @@
> +	}
> +	PORT_Memcpy(seed->data, firstseed.data, firstseed.len);
> +	PORT_Memcpy(seed->data+firstseed.len, pseed.data, pseed.len);
> +	PORT_Memcpy(seed->data+firstseed.len+pseed.len, qseed.data, qseed.len);
> +	counter = (qgen_counter << 16) || pgen_counter;

We should find out why the code still works (or appears to work)
in spite of this bug.

@@ +1552,1 @@
>  PQG_ParamGenV2(unsigned int L, unsigned int N, unsigned int seedBytes,

Please update the comments in blapi.h for PQG_ParamGenV2 to
document the meaning of N=0 and seedBytes=0.

@@ +1555,5 @@
> +    if (N == 0) {
> +	N = pqg_get_default_N(L);
> +    }
> +    if (seedBytes == 0) {
> +	/* seedBytes == L for probable primes, N for Shawe-Taylor Primes */

I don't understand this comment. It mentions L and N as the
possible values for seedBytes, but the code sets seedBytes to
N/8. Should the code set seedBytes to N instead?

@@ +1558,5 @@
> +    if (seedBytes == 0) {
> +	/* seedBytes == L for probable primes, N for Shawe-Taylor Primes */
> +	seedBytes = N/8;
> +    }
> +    if ((pqg_validate_dsa2(L,N) != SECSuccess)) {

Remove the extra pair of parentheses you added.

::: tests/cipher/performance.sh
@@ +104,4 @@
>  	${BINDIR}/bltest -N -m $mode -b $bufsize -g $keysize -u $cxreps >> ${DSAPERFOUT}
>  	mv "tmp.in.0" "$mode.in"
>  	mv tmp.key $mode.key
> +	rm -f $mode.out

The lines

    mv "tmp.in.0" "$mode.in"
    mv tmp.key $mode.key

are in three while loops in this file.  Should we also add

    rm -f $mode.out

to the other two while loops?

Comment 54

[Robert Relyea]

> Why is this change necessary? Is it because cipherInfo->output.buf.len
> was too long? cipherInfo->output.buf seems to be allocated in cipherInit():

Set the actual signature length is dependent on the PQG parameters.

> We should find out why the code still works (or appears to work)
> in spite of this bug.

which bug? (I think there's a critical comment from you that I'm missing).

> I don't understand this comment. It mentions L and N as the
> possible values for seedBytes, but the code sets seedBytes to
> N/8. Should the code set seedBytes to N instead?

N is in bits, seedBytes is in bytes.

> Should we also add
>    rm -f $mode.out
> to the other two while loops?

Possibly, out of paranoia. I think the other cases didn't have test cases where the output actually shrank rather than grew...

Comment 55

[Robert Relyea]

> IMO, it is confusing to compute a default for the caller. Is it necessary? Can't we just require > the caller to always supply an explicit value of N?

In the general case, I prefer the caller not have to worry about N. The ability to set N is more supporting the corner case where the caller may actually want to set to other than the default.

>  So, why not use N == 256 instead of 224, since N == 256 should be more secure?

Because 224 matches the security of 2048 keys. 256 uses a stronger than necessary q.

> Replace logical (||) OR with bitwise OR (|).

Ah that's the bug wtc was refering to... currently we don't check the count on verify for Shawe-Taylor (it's an optional step). I think for now I'll just return '0', since returning the count is also optional.

Comment 56

[Wan-Teh Chang]

Comment on attachment 669298 [details] [diff] [review]
Fix problem when generating pqg < 512. Switch to Shawe/Taylor pqg generation

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

::: cmd/bltest/blapitest.c
@@ +2185,4 @@
>              }
>              TIMEFINISH(cipherInfo->optime, 1.0);
>          }
> +	cipherInfo->output.buf.len = cipherInfo->output.pBuf.len;

Do you know if we need this change for ECDSA?

::: lib/freebl/pqg.c
@@ +1555,5 @@
> +    if (N == 0) {
> +	N = pqg_get_default_N(L);
> +    }
> +    if (seedBytes == 0) {
> +	/* seedBytes == L for probable primes, N for Shawe-Taylor Primes */

Since L and N are in bits and seedBytes is in bytes, we should use
L/8 and N/8 in this comment.

::: tests/cipher/performance.sh
@@ +104,4 @@
>  	${BINDIR}/bltest -N -m $mode -b $bufsize -g $keysize -u $cxreps >> ${DSAPERFOUT}
>  	mv "tmp.in.0" "$mode.in"
>  	mv tmp.key $mode.key
> +	rm -f $mode.out

Thank you for the explanation. I think the proper fix is
to change blapitest.c to open the output file with PR_TRUNCATE.
The overwrite behavior seems like an unintended bug.

Comment 57

[Robert Relyea]

Checking in cmd/bltest/blapitest.c;
/cvsroot/mozilla/security/nss/cmd/bltest/blapitest.c,v  <--  blapitest.c
new revision: 1.70; previous revision: 1.69
done
Checking in lib/freebl/blapi.h;
/cvsroot/mozilla/security/nss/lib/freebl/blapi.h,v  <--  blapi.h
new revision: 1.49; previous revision: 1.48
done
Checking in lib/freebl/pqg.c;
/cvsroot/mozilla/security/nss/lib/freebl/pqg.c,v  <--  pqg.c
new revision: 1.24; previous revision: 1.23
done
Checking in tests/cipher/performance.sh;
/cvsroot/mozilla/security/nss/tests/cipher/performance.sh,v  <--  performance.sh
new revision: 1.9; previous revision: 1.8
done

Comment 58

[Robert Relyea]

> Do you know if we need this change for ECDSA?

Possibly. I don't think we run performance tests on ECDSA, which is where I ran into the issue.

> Thank you for the explanation. I think the proper fix is
> to change blapitest.c to open the output file with PR_TRUNCATE.
> The overwrite behavior seems like an unintended bug.

I'm pretty sure you are right. I'm a little worried about the BLIO layer in bltest. It's getting a little unwieldy with little comments as to why it does what it does...

bob

Comment 59

[Robert Relyea]

Attachment: ST_pqg gen patch as checked in...

This patch incorporates most of the comments by Brian and wtc.

wtc I missed your last comment since I saw it after I had already checked in.

Comment 60

[Robert Relyea]

Handle wtc's last comment (comment 56 )

Diff is an incremental on top of the previous checkin.

bobslaptop.local(435) !?diff
cvs diff pqg.c > patch3
!?commit
bobslaptop.local(436) !?commit
cvs commit pqg.c
Checking in pqg.c;
/cvsroot/mozilla/security/nss/lib/freebl/pqg.c,v  <--  pqg.c
new revision: 1.25; previous revision: 1.24
done
bobslaptop.local(437) cat patch3
Index: pqg.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/lib/freebl/pqg.c,v
retrieving revision 1.24
diff -r1.24 pqg.c
1568c1568
< 	/* seedBytes == L for probable primes, N for Shawe-Taylor Primes */
---
> 	/* seedBytes == L/8 for probable primes, N/8 for Shawe-Taylor Primes */
bobslaptop.local(438)

Comment 61

[Wan-Teh Chang]

Marked the bug fixed because NSS 3.14 has been released.

If there are any remaining patches, please open a new bug for them. Thanks.

Comment 62

[Robert Relyea]

Nope, this one is done (thankfully).

bob

Comment 63

[Wan-Teh Chang]

Comment on attachment 636377 [details] [diff] [review]
Add DSA-2 support to pk11wrap - with wtc comments

Bob: could you explain your changes to pk11util.c in this patch?

http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11util.c&rev=HEAD&mark=1.62

They are not in the earlier version of this patch that I reviewed
(attachment 611085 [details] [diff] [review]), and I don't remember suggesting these changes.
Thanks.

Comment 64

[Robert Relyea]

I recognize the patch, but it shouldn't have been part of the DSA2 patch.

The patch is allowing for modules that have no slots to show up as supporting removable slots (since the module has no slots, then the only slots that it could possible have in the future would be removable ones). This allows certain pkcs #11 modules to function properly even if there are no supported readers for the module plugged in.

I remember working on this, but it may have been an RH bug, not a mozilla one.

I think we want the code, but we should document it in somewhere other than this bug. (it was probably a patch in the same tree I had the DSA work that I missed when I checked the code in).

bob

Comment 65

[Rick Andrews]

Can anyone tell me the state of this work? Is it officially in NSS? If so, what Firefox release might include it?

Comment 66

[Robert Relyea]

It went into NSS 3.14. I don't know which FF release that maps to. Certainly after FF 17.

bob

Comment 67

[Rick Andrews]

I hope someone can tell me when Firefox will include NSS 3.14. And when will Chrome include it?

Comment 68

[Gervase Markham [:gerv]]

Mozilla Central currently uses NSS 3.14.3 RTM:
https://mxr.mozilla.org/mozilla-central/source/security/nss/TAG-INFO

mozilla-central upgraded to NSS 3.14 BETA 1 on 2012-10-01:
http://hg.mozilla.org/mozilla-central/filelog/293498096b28/security/nss/TAG-INFO

Assuming BETA 1 had the fix in question, then it would have been first shipped in Firefox 19:
http://en.wikipedia.org/wiki/Firefox_release_history

That's the latest release of Firefox, released on Feb 19th.

Gerv

Comment 69

[Rick Andrews]

Using FF 20.0.1, I get errors trying to hit our test DSA sites:

https://ssltest37.ssl.symclab.com/
      Peer's certificate has an invalid signature.
      (Error code: sec_error_bad_signature)
https://ssltest41.ssl.symclab.com/
https://ssltest43.ssl.symclab.com/
      Received incorrect handshakes hash values from peer.
      (Error code: ssl_error_bad_handshake_hash_value)   

The first site (ssltest37) is chained through an intermediate CA, the others are test certs signed by the root. None of the roots are in NSS yet, but I would expect a different error for that.

Another data point: All three sites work fine in Opera 12.15.