158750 - [meta] Support RSAPSS

Status: NEW

(NSS :: Libraries, enhancement, P2)

Description

[Wan-Teh Chang]

We should implement RSAPSS (PKCS#1 v2.1).

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Clearing priority & target milestone for P1 bugs with bogus target milestones.

Comment 2

[Hanno Boeck]

Attachment: add OIDs from RFC 4055 for PSS and OAEP

This adds all OIDs from RFC 4055, which is everything needed for PSS and OAEP plus the rsawithsha224 algorithm.

Comment 3

[Wan-Teh Chang]

Attachment: add OIDs from RFC 4055 for PSS and OAEP, by Hanno Boeck (checked in)

Hanno, thanks for the patch.

I edited your patch as follows.
1. I added a space before an equal sign (=) in secoidt.h.
(I know you were trying to align all the equal signs.)
2. I wrapped a long line in secoid.c.  In NSS, all lines
must be <= 80 characters long.

Next time please generate your patches using "cvs diff -pu8".

Question: in the description for the RSA-PSS OID, should
we say "RSA-PSS" instead of "RSASSA-PSS"?

I checked in this patch on the NSS trunk (NSS 3.12.7).

Checking in secoid.c;
/cvsroot/mozilla/security/nss/lib/util/secoid.c,v  <--  secoid.c
new revision: 1.56; previous revision: 1.55
done
Checking in secoidt.h;
/cvsroot/mozilla/security/nss/lib/util/secoidt.h,v  <--  secoidt.h
new revision: 1.31; previous revision: 1.30
done

Comment 4

[Hanno Boeck]

Attachment: 0001-rename-oids-rsaes-oape-rsassa-pss.diff

As talked yesterday, rename the constants to the more correct names RSASSA-PSS and RSAES-OAEP.

Comment 5

[Hanno Boeck]

Attachment: 0002-add-key-constants-and-logic-v3.diff

Adds new key types for pss and oaep.

Comment 6

[Hanno Boeck]

Attachment: 0002-add-key-constants-and-logic-v4.diff

Comment 7

[Hanno Boeck]

Attachment: 0003-add-mgf.diff

This adds code for the mask generation function MGF1 as described in RFC 3447 and RFC 4055.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Hanno,
Are you submitting these patches for review?
If so, please request review on each one

Comment 9

[Wan-Teh Chang]

Comment on attachment 451601 [details] [diff] [review]
0003-add-mgf.diff

Hanno: thanks for the patch.  In addition to the review comments I gave
to you over Chat, I think that the MGF1 function does not need to be
part of the blapi.h API.  I think the MGF1 function can be an internal
function in rsawrapr.c, because it should be needed only in that file.

Comment 10

[Hanno Boeck]

Attachment: 0003-add-mgf-v2.diff

Comment 11

[Wan-Teh Chang]

Attachment: Use the less precise but more readable "RSA-PSS" (checked in)

I think we should use "RSA-PSS".  Few people know what "RSASSA" means unless they
look it up in PKCS #1.

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

Checking in secoid.c;
/cvsroot/mozilla/security/nss/lib/util/secoid.c,v  <--  secoid.c
new revision: 1.58; previous revision: 1.57
done

Comment 12

[Wan-Teh Chang]

Attachment: add KeyType constants rsaPssKey and rsaOaepKey, by Hanno Boeck (checked in)

I edited Hanno's patch 0002-add-key-constants-and-logic-v4.diff (mostly
coding style issues) and checked it in on the NSS trunk (NSS 3.12.7).

Checking in keythi.h;
/cvsroot/mozilla/security/nss/lib/cryptohi/keythi.h,v  <--  keythi.h
new revision: 1.13; previous revision: 1.12
done
Checking in seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.54; previous revision: 1.53
done
Checking in secvfy.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/secvfy.c,v  <--  secvfy.c
new revision: 1.24; previous revision: 1.23
done

Comment 13

[Wan-Teh Chang]

Comment on attachment 452479 [details] [diff] [review]
0003-add-mgf-v2.diff

Hanno: thanks for the patch.  As we discussed over Chat, this patch is
correct.

I'd like to suggest two improvements.

1. Instead of adding a new file just for the MGF1 function, add it to
mozilla/security/nss/lib/softoken/rsawrapr.c, where the OAEP code
is.

2. This patch allocates a buffer to hold
        mgfSeed || C
You can avoid that by calling the SHAx_Update function twice,
with mgfSeed and C as input, respectively.

In addition, you can use the SHAx_Clone function to copy the
hash context after you have called SHAx_Update with mgfSeed,
to avoid hashing mgfSeed repeatedly.

This means you should use the SECRawHashObjects array
declared in http://mxr.mozilla.org/security/source/security/nss/lib/freebl/hasht.h
to get the function pointers and the hash length, as I suggested
over Chat.  This will replace the switch statement in this patch.

Comment 14

[Hanno Boeck]

Attachment: 0004-pss-verification-v1.diff

This is a first try on implementing a verification operation, which works on certificates with standard parameters. It needs some cleanup still and some things are solved quite hacky, but the basic things are in.

This does not yet support any non-standard parameters - which is quite tricky, as these have to be passed through pretty much all layers and the current api has no way of doing that.

Comment 15

[Hanno Boeck]

Attachment: 0004-pss-verification-v2.diff

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 455950 [details] [diff] [review]
0004-pss-verification-v2.diff

r- 
Even though you did not request review on this patch, I began to 
review it.  Perhaps it is a work in progress.  I hope so. I in that
case, there is still time for you to change aspects of this 
implementation before you go much further.  

My review comments will not be in the same order as the the code 
appears in your patch.

1) The patch to RSA_CheckSignRecover.  
Q) Does PSS guarantee us that no PSS signature block will ever 
begin with the bytes 0x00 0x01 ?  Or is it possible that, after
performing the RSA public key operation on a PSS signature, 
the result could indeed begin 0x00 0x01, even if only one time 
in 64K on average?  

If it is the case that PSS signatures can EVER correctly produce
a signature block in which the first two bytes are 0x00 0x01,
then this patch's approach is incorrect, because it will treat 
such a signature block as a PKCS#1 v1.5 block, not as PSS.  
In that case, the decision of whether the signature is PSS or v1.5
must come from external input, rather than from the decrypted 
signature block.  Perhaps a separate function is needed or some 
input value to this function must signify that PSS is intended.

2) New but unused typedef placed in public header file.
The patch addes to freebl/hasht.h the following typedef:

+typedef enum {
+    MASK_AlgNULL   = 0,
+    MASK_AlgMGF1   = 1,
+    MASK_AlgTOTAL
+} MASK_MGFType;

but it is never used.  It probably should be used ... or else not
be defined.

3) new obsolete-at-birth function?
Your patch adds to file pk11wrap/pk11obj.c a new function defined as:

 /*
+ * verify a PSS signature
+ * needs to be extended later with hash algorithm, mask algorithm and maskhash algorithm parameters
+ */
+SECStatus
+PK11_VerifyPSS(SECKEYPublicKey *key, SECItem *sig, SECItem *hash, void *wincx)

The comment is too wide (exceeds 80 characters), but more importantly, it tells 
us that this API is going to require replacement.  We should NOT introduce a 
new public PK11_ function whose signature that is known to be in sufficient at 
the outset.  MUST FIX.

If you wish to add a function (ilke this one) IN ADDITION TO the generalized
API, from the outset, a function whose name impliest a certain set of values 
for parameters of the more generalized function, I think that's probably OK, 
but the name should make it obvious to the reader that this is a subset of 
the generalized function, and which subset it is.  Perhaps this function 
might be named PK11_VerifySHA1PSS.  But PK11_VerifyPSS should be a function
that can handle all hashes defined now or later for masking.

4) Missing break in new case.
Your patch to function sec_DecodeSigAlg adds a new case 
SEC_OID_PKCS1_RSA_PSS_SIGNATURE which "steals" the break from the preceding case, but does not separate it from that preceding case with a new break statement.  This will break the preceding case. 

Also, here again, this defines a new symbol whose name generically covers 
all mask hash types, but is actually defined for only one hash type,  
namely SHA1.  IMO, The new symbol name should make it clear to the reader 
that this symbol is only for the SHA1 mask type, e.g. 
SEC_OID_PKCS1_RSA_SHA1_PSS_SIGNATURE

5) IINM, Patch to function vfy_CreateContext implements key type rsaPssKey 
such that it assumes the mask hash SHA1.  Perhaps that is unavoidable given
that the signature of function vfy_CreateContext doesn't provide a parameter
with which to specify the mask hash type.  

IF we are going to let this key type serve double duty, to specify BOTH that 
it is an RSA key AND that it is to be used for PSS with hash SHA1, then the 
name should make that absolutely clear, e.g. rsaSha1PssKey.  

But I think it is very undesirable to go in that direction.  A Key type should
be just that, a Key type.  We don't have separate key type enumerations for
EC keys that are used for encryption, vs those that are used for signing. 
The same RSA key can be used for v1.5 and for PSS.  So, I think we do not 
want to go in the direction of using the key type enumeration as a way to 
convey information about other algorithms that do not fundamentally require 
different types of keys.  

Given that vfy_CreateContext is a static function, I think we can give it 
additional parameters for this purpose.

6) Suggested optimizations to emsa_pss_verify

6a) Replace 
+    zeroBits=8;
+    for (i=0;i<8;i++) {
+      if ( (em[0]>>i) & 1 ) zeroBits=7-i;
+    }

with 

+    zeroBits = 8;
+    for (i = em[0]; i; --zeroBits) {
+        i >>= 1;
+    }

6b) Replace

+    for (i=0;i<(emLen - hash->length - 1);i++) {
+        db[i]^=em[i];
+    }

with something like 

+    tmpi = emLen - hash->length - 1;
+    for (i = 0; i < tmpi; i++) {
+        db[i] ^= em[i];
+    }

or (even more efficiently)

+    i = emLen - hash->length - 1;
+    if (i--) do {
+        db[i] ^= em[i];
+    } while (i--);

6c) Replace 

+    db[0]= (0xff>>zeroBits)&db[0];

with 

+    db[0] &= 0xff >> zeroBits;

Nits:
N1) indentation of new case rsaPssKey in function VFY_EndWithSignature is 
    inconsistent with itself and with rest of file.
N2) indentation of new goto statement in NSC_VerifyInit is inconsistent.
N3) inconsistent use of white space in new function emsa_pss_verify, and
    some of the new lines are too wide.  Some lines use WS liberally, some 
    sparingly, some mix the two.  Here one example (there are several):

+    MGF1(db, emLen - hash->length - 1, &em[emLen-hash->length-1], hash->length, hashAlg);

The line is far too wide.
The second argument to MGF1 surrounds the '-' operator with spaces. 
The third  argument to MGF1 does not. 

I suggest you use local automatic variables to hold the values of certain
expressions that get re-evaluated frequently, such as:
emLen - hash->length - 1 
emLen - hash->length - sLen - 2

This will make the code more readable, and reduce the number of too-wide lines.

Comment 17

[Hanno Boeck]

Attachment: 0003-add-mgf-v4.diff

mgf1-patch which uses hash object from rawhash.c/hasht.h. It does not clone the hash, as the clone functions are not implemented yet. As we already discussed per mail, I think this should live in freebl, as it's more like a cryptographic primitive and similar to a hash-function.

I think it is in a state now that it could be merged.

(this patch was not created with cvs diff as it adds a new file and the cvsdo-method described in the mozilla wiki didn't work for me for unknown reasons)

Comment 18

[Hanno Boeck]

Nelson, thanks for your review, it'll help me to improve the code, I'm aware that the pss-verification-patch is not in a state to be included into the nss-code. It is a limited work-in-progress and needs a lot of restructuring and I mainly posted it here so others can see my progress - I should probably indicate that more clearly on further attachments.

Comment 19

[Hanno Boeck]

Attachment: 0005-prettyprint-pss-params-v2.diff

Adds pss parameters to the output of the pp tool. This is mainly to have a working asn1 template which I'll also need in other places (secvfy.c/seckey.c) for pss parameter decoding.
Probably the template and the struct should be put into some central place, though I'm not sure yet where it would fit.

Comment 20

[Hanno Boeck]

Attachment: 0005-prettyprint-pss-params-v3.diff

Comment 21

[Hanno Boeck]

Attachment: 0005-prettyprint-pss-params-v4.diff

Now with template exported to the public API. Please review.

Comment 22

[Hanno Boeck]

Attachment: 0006-add-mgf-and-sha224-constants-to-hasht.diff

This adds constants for MGF-functions (for now it's only one, but others may follow) and a type for MGF-functions. This is rather pointless at the moment as we only have MGF1, but I'd like to have the type ready as a function parameter, if the future gives us more mask generation function. I wasn't sure if hasht.h is the right place for this to live, but it seemed to me the best I found. This also adds a constant for sha224.

I think this can be merged, please review.

Comment 23

[Konstantin Andreev]

Sorry for stupid question, but whether creating two new hi-level KeyType's (rsaPssKey, rsaOaepKey) is unavoidable ? They are both RSA keys, aren't they ?

I dislike that new KeyType's are created, which differ from existing one only by their usages.

Comment 24

[Hanno Boeck]

Attachment: 0004-pss-verification-v5.diff (not for review)

This is still very much work-in-progress and I have not yet worked on all issues mentioned by nelson in the comment above. Requires all other patches (0005-, 0006-) and the sha224-oid attached to #356713 to be applied also.

Comment 25

[Hanno Boeck]

Konstantin: No, in fact there are new keytypes which are only for PSS and OAEP. They differ that they can have specified parameters (like hash function) bound to them. Though it's also possible to use "normal" RSA keys to create PSS signatures and do OAEP encryption, so the issue is a bit more complex. I'm not sure if my approach is the best, but imho it fit's best in the current structure of the code in secvfy.c. But I'm open for arguments how to solve it better (maybe the mailing list would be a better place to discuss this).

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 456781 [details] [diff] [review]
0005-prettyprint-pss-params-v4.diff

I think this is very close.  IMO, it is structured correctly. 
There are just a bunch of minot details (and perhaps one major 
detail to work out.

Specific comments:
1. Line lengths.  Lots of lines exceed 80 columns wide.  Please fold them.

2. When referencing a data symbol (as opposed to a code symbol) in a 
different shared library than the executable or shared library making tha 
reference, it is necessary to use certain macros to ensure that it will
work on all platforms.  This is particularly relevant when passing addresses
of template arrays to decoders and encoders.  

In your new code for cmd/lib/util, you correctly used 
SEC_ASN1_GET( to reference SEC_RSAPSSParamsTemplate) in your first call to 
SEC_QuickDERDecodeItem(, but did not use it to reference 
SECOID_AlgorithmIDTemplate in the second call.

3. nit: Please change all the expressions of the form
 &(*param.hashAlg).something 
to
 &param.hashAlg->something 
and similarly change expressions for 

 &(*param.maskAlg).something

4. The correct check for the absence of optional primitive members of a 
SEQUENCE or SET is to check the .data pointer for NULL, rather than to 
check the .len member for zero.  The reason for this is that in some 
cases, it is allowed for members to be present but have zero length,
and that is distinct from being absent.  Only a NULL pointer truly means
"absent".

>+	if (!param.saltLength.len) {
>+	    SECU_Indent(out,level+1);
>+	    fprintf(out, "Salt Length: default, %i (0x%2X)\n", 20, 20);
>+	} else
>+	    SECU_PrintInteger(out, &param.saltLength, "Salt Length", level+1);

5. I believe these new PSS templates and structs are not so much properties 
of certs as they are properties of keys.  As such, I believe that lib/certdb
is the wrong place for them.  However, I'm not exactly sure of the right 
place for them.  I think the safest bet is to put them in nss/lib/util,
which is actually a different shared library.  This choice also affects choice 
of header file for the associated symbol declarations, and the .def file.  
Let's ask Bob Relyea and/or Wan-Teh to speak to this point.

6. Finally, regarding your patch to nss.def.  I think it's too late for 
this change to make it into 3.12.7.  That means either 3.12.8 or 3.13.
The NSS team will need to decide that this coming Thursday.

> ;+NSS_3.12.7 { 	# NSS 3.12.7 release
> ;+    global:
>+;;SEC_RSAPSSParamsTemplate DATA ;
>+NSS_Get_SEC_RSAPSSParamsTemplate;
> CERT_GetConstrainedCertificateNames;
> ;+    local:
> ;+       *;
> ;+};

Comment 27

[Hanno Boeck]

Attachment: 0005-prettyprint-pss-params-v5.diff

Fixed according to Nelson's notes. I also thought about putting it into util, but there lived no similar structs. It is both key and cert-related, as it's used for both. I'll leave that up to your discussion to decide.
Also for the api, I don't mind much, it's of minor use anyway within 3.12.7 without the verification-changes that are not ready anyway, so it can probably wait until the next version.

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Bob, Wan-Teh, 
Is softoken going to want access to these new templates and structs?
If so, then they belong in lib/util.  Otherwise ...

Comment 29

[Konstantin Andreev]

(In reply to comment #25)
> (In reply to comment #23)
> > Sorry for stupid question, but whether creating two new hi-level KeyType's (rsaPssKey, rsaOaepKey) is unavoidable ? They are both RSA keys, aren't they ?
> > 
> > I dislike that new KeyType's are created, which differ from existing one only by their usages.
> 
> Konstantin: No, in fact there are new keytypes which are only for PSS and OAEP.
> They differ that they can have specified parameters (like hash function) bound
> to them. Though it's also possible to use "normal" RSA keys to create PSS
> signatures and do OAEP encryption, so the issue is a bit more complex. I'm not
> sure if my approach is the best, but imho it fit's best in the current
> structure of the code in secvfy.c. But I'm open for arguments how to solve it
> better (maybe the mailing list would be a better place to discuss this).

Hello, Hanno.

I believe your approach works, but it is kinda hackerish. Hacks typically works, but hamper further development and dramatically increase maintenance costs.

Please, justify the requirement for the new KeyType's. I am not closely familiar with PSS and OAEP, thereby some explanation where these algorithms depend on the "cryptohi/KeyType"'s would be reasonable.

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 456885 [details] [diff] [review]
0005-prettyprint-pss-params-v5.diff

The NSS team decided yesterday that we DEFINITELY do want the PSS template
array and chooser function to be in nss/lib/util, so that softoken will 
have access to it.

Comment 31

[Wan-Teh Chang]

Nelson, what we decided is the opposite, because a PKCS #11
module doesn't not need to do any DER-decoding of RSA-PSS
parameters.  Here is how CK_RSA_PKCS_PSS_PARAMS is defined
in pkcs11t.h:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/util/pkcs11t.h&rev=1.20&mark=1292-1294#1288

CK_RSA_PKCS_PSS_PARAMS is already in a decoded form (just a
structure of three CK_ULONGs, after resolving the typedefs).

So the softoken should not need SEC_RSAPSSParamsTemplate.

Comment 32

[Hanno Boeck]

Attachment: 0003-add-mgf-v5.diff

Fixes a memory leak

Comment 33

[Wan-Teh Chang]

Comment on attachment 456782 [details] [diff] [review]
0006-add-mgf-and-sha224-constants-to-hasht.diff

We cannot add HASH_AlgSHA224 before NSS supports SHA-224,
because there is NSS code that assumes every hash algorithm
type < HASH_AlgTOTAL is implemented.  Here is an example:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/freebl/rawhash.c&rev=1.5&mark=163,167#160

So HASH_AlgSHA224 should be added as part of the fix for
bug 356713.  Although it's fine to add SHA224_LENGTH earlier,
I see no need to do that.

As for MASK_MGFType, since there are no MASK_xxx functions,
we don't need to use the MASK_ prefix for this type's name.
I think it can be named simply MGFType or SECMGFType.
Should we define an enum constant for MGF1 or mgf1SHA1?

Comment 34

[Wan-Teh Chang]

Attachment: 0003-add-mgf-v6.diff by Hanno Boeck

I reviewed Hanno's 0003-add-mgf-v5.diff and cleaned it up a bit.

I added the missing text to the license header in the new file
mgf1.c.  I reordered the parameters of MGF1() so that input
parameters are listed before output parameters.

I also fixed the incorrect "End of Version 3.0xx" comments
in ldvector.c.  This required doing a lot of cvs diff between
NSS 3.11.x and 3.12.x releases.

I will check in this patch no later than tomorrow morning.
There are two possible performance improvements to MGF1.
1. We can call hash->create and hash->destroy outside the
for loop.  The hash context can be reused after a hash->end
call.
2. If maskLen is a multiple of hash-length, the last
iteration of the for loop does not need to allocate the
'temp' buffer.  If this is not the common case, it's not
worth optimizing.

Nelson, if you has any comments, Hanno and I will make the
suggested changes.

Comment 35

[Wan-Teh Chang]

Attachment: Fix incorrect "End of Version 3.0xx" comments in ldvector.c (checked in)

Found by comparint with the output of
    cvs diff -u -rNSS_3_12_2_RTM -rNSS_3_12_3_RTM loader.h

I checked this in on the NSS trunk (NSS 3.13).

Checking in ldvector.c;
/cvsroot/mozilla/security/nss/lib/freebl/ldvector.c,v  <--  ldvector.c
new revision: 1.22; previous revision: 1.21
done

We should consider this patch (note: a softoken change) for
the NSS_3_12_BRANCH.

Comment 36

[Wan-Teh Chang]

Attachment: 0003-add-mgf-v7.diff by Hanno Boeck (checked in)

I removed the unrelated comment changes from the patch,
and checked it in on the NSS trunk (NSS 3.13).

Checking in blapi.h;
/cvsroot/mozilla/security/nss/lib/freebl/blapi.h,v  <--  blapi.h
new revision: 1.34; previous revision: 1.33
done
Checking in ldvector.c;
/cvsroot/mozilla/security/nss/lib/freebl/ldvector.c,v  <--  ldvector.c
new revision: 1.23; previous revision: 1.22
done
Checking in loader.c;
/cvsroot/mozilla/security/nss/lib/freebl/loader.c,v  <--  loader.c
new revision: 1.46; previous revision: 1.45
done
Checking in loader.h;
/cvsroot/mozilla/security/nss/lib/freebl/loader.h,v  <--  loader.h
new revision: 1.27; previous revision: 1.26
done
Checking in manifest.mn;
/cvsroot/mozilla/security/nss/lib/freebl/manifest.mn,v  <--  manifest.mn
new revision: 1.59; previous revision: 1.58
done
RCS file: /cvsroot/mozilla/security/nss/lib/freebl/mgf1.c,v
done
Checking in mgf1.c;
/cvsroot/mozilla/security/nss/lib/freebl/mgf1.c,v  <--  mgf1.c
initial revision: 1.1
done

Hanno: another suggested change is to handle the failures of
hash->create and PORT_Alloc.

Nelson: I think "MGF1" seems too short for a public function
name (public for freebl).  Do you have any suggestion?
Alternatively, we can move it into the .c file for RSA-PSS
as a static function in the future.

Comment 38

[Konstantin Andreev]

Wan-Teh, Nelson,

please, comment how do you consider adding in cryptohi/keythi.h`KeyType:

    rsaPssKey = 7,
    rsaOaepKey = 8

Key type is key type, it is not the same as "signature algorithm". They shouldn't mix. Key type is RSA, but signature algorithms may vary.

If GS0C has too limited time frame for implementing PSS right, it's better to delay implementation. The need in PSS is not urgent enough to break NSS design.

Comment 39

[Wan-Teh Chang]

Konstantin: thanks for your comment.  I didn't know that neither I nor Hanno
answered your question in comment 29.  Sorry about that.

I had the same objection when I first saw Hanno's patch to add rsaPssKey and
rsaOaepKey.  Hanno convinced me that it's necessary, so I asked him to add
a block comment, with a reference to RFC 4055:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/cryptohi/keythi.h&rev=1.13&mark=46-56#45

I guess the block comment can further point out it's Section 1.2 of
RFC 4055.  Please read that section carefully.  Then, here is the
NSS function that converts the SubjectPublicKeyInfo ASN.1 type
to the SECKEYPublicKey NSS type.  Note that the 'algorithm' field
of SubjectPublicKeyInfo gets mapped to the keyType field of
SECKEYPublicKey:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/cryptohi/seckey.c&rev=1.54&mark=1017,1023,1044,1050-1053#1016

Without changing the SECKEYPublicKey type, we must encode
the new id-RSASSA-PSS and id-RSAES-OAEP algorithm OIDs
from RFC 4055 entirely in the keyType field of SECKEYPublicKey,
otherwise we lose information in the conversion.

So this explains why Hanno and I think we need new rsaRssKey
and rsaOaepKey types.  If this explanation is still not satisfactory.
please suggest an alternative solution.

I came up with these alternatives:

1. Consider the new id-RSASSA-PSS and id-RSAES-OAEP OIDs
for SubjectPublicKeyInfo's algorithm field a bad idea, and refuse
to support them in NSS.

2. Add a new enum type for key usage restrictions, and
modify the NSS structures and functions that currently use
just KeyType to add a new field/argument for key usage
restriction.  I'm afraid that this would be a disruptive change
to the NSS API.

Any other ideas?

Comment 40

[Hanno Boeck]

Attachment: 0007-optimize-parameter-ordering-mgf.diff (checked in)

Change the parameter ordering to comply with the NSS API Guidelines (dest, src instead of src, dest), also optimize to only create/destroy hash context once.

Comment 41

[Wan-Teh Chang]

Comment on attachment 459580 [details] [diff] [review]
0007-optimize-parameter-ordering-mgf.diff (checked in)

r=wtc.  I removed the white spaces that Hanno intentionally
added at the end of some lines because he thought that's the
NSS convention.  In NSS, we don't add white space at the end
of line except for careless mistakes.

I checked this patch in on the NSS trunk (NSS 3.13).

Checking in blapi.h;
/cvsroot/mozilla/security/nss/lib/freebl/blapi.h,v  <--  blapi.h
new revision: 1.35; previous revision: 1.34
done
Checking in loader.c;
/cvsroot/mozilla/security/nss/lib/freebl/loader.c,v  <--  loader.c
new revision: 1.47; previous revision: 1.46
done
Checking in loader.h;
/cvsroot/mozilla/security/nss/lib/freebl/loader.h,v  <--  loader.h
new revision: 1.28; previous revision: 1.27
done
Checking in mgf1.c;
/cvsroot/mozilla/security/nss/lib/freebl/mgf1.c,v  <--  mgf1.c
new revision: 1.2; previous revision: 1.1
done

Comment 42

[Wan-Teh Chang]

Attachment: 0005-prettyprint-pss-params-v6.diff (checked in)

Hanno, I edited your 0005-prettyprint-pss-params-v5.diff patch
and checked it in on the NSS trunk (NSS 3.13).

Checking in cmd/lib/secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.100; previous revision: 1.99
done
Checking in lib/cryptohi/keythi.h;
/cvsroot/mozilla/security/nss/lib/cryptohi/keythi.h,v  <--  keythi.h
new revision: 1.14; previous revision: 1.13
done
Checking in lib/cryptohi/seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.55; previous revision: 1.54
done
Checking in lib/nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.207; previous revision: 1.206
done

I believe all of my edits were coding style changes except one
change: I added a return statement after the secu_PrintRSAPSSParams
call in SECU_PrintAlgorithmID, because we should not fall through
to call SECU_PrintAsHex.  Let me know if this fix is wrong.

Please study the coding style fixes I made.  Also, I moved
the RSAPSSParams type from lib/certdb to lib/cryptohi because
I think lib/cryptohi (the SECKEY stuff) is the right home for
RSAPSSParams.

Comment 43

[Konstantin Andreev]

(In reply to comment #39)
> (In reply to comment #38)
> 
> I had the same objection when I first saw Hanno's patch to add rsaPssKey and rsaOaepKey. Hanno convinced me that it's necessary, so ...
>
> [ ... SKIP ... ]
> 
> Any other ideas?

Hello, Wan-Teh. Thank you for detailed explanation. I will investigate it closely a bit later.

My interest is inspired by the fact I had been adding gostKey into cryptohi/keythi.h`KeyType for bug 518787. Since then the underlying PKCS#11 spec was reengineered, and bug 518787 should also be reengineered.

I had planned to get rid of gostKey, and use ecKey instead. But I will face the same problem - the one key type (ecKey), multiple signature algorithms. If I will find the solution, I will inform you.

Comment 44

[Konstantin Andreev]

Technical question. The patches of this bug target branch 3.13. Patches affect both softoken and non-softoken parts.

I thought only softoken is developed in branch 3.13, everything else should be done in 3.12. Am I wrong ?

Comment 45

[Wan-Teh Chang]

Konstantin: I hope you can come up with a better solution for
the key types that are restricted to certain signature or
encryption algorithms.  I just realized that we face the same
issue in PKCS #11 because PKCS #11 only has CKK_RSA.  So we'd
need to add CKK_RSA_PSS and CKK_RSA_OAEP for full RFC 4055
support.

This Monday we created NSS_3_12_BRANCH for future 3.12.x
releases, and started to use the trunk for 3.13 developement,
including the softoken.  As a result, SOFTOKEN_3_13_BRANCH is
dead now.

Comment 46

[Robert Relyea]

as a general rule any code you add to NSS 3.12 should fail gracefully if the corresponding softoken features aren't there.

bob

Comment 47

[Hanno Boeck]

Attachment: 000a-pss-verification-v9.diff

I created now a patch for pss verification that I think is ready for a bit more review. I sorted out most of the hackish stuff and hopefully all things Nelson mentioned in comment #16.

There are imho two important design decisions where I'd like to get feedback if people think I did this in a sane way:
- the already mentioned and discussed keytype issue. PSS is special here compared to earlier algorithms that it can be done both with special pss keys and normal rsa keys.
- I needed a way to pass down some meta algorithm information (the mask algorithm, mask hash algoritm and salt length), I did this by changing a couple of (non-public) functions in secvfy.c to contain an additional void* parameter, that is of type CK_RSA_PKCS_PSS_PARAMS in the pss case, but could be used for other meta information by future algorithms that need to pass down that info.
In pk11obj.c, I created a new function PK11_VerifyWithMech that has (compared to PK11_Verify) two additional parameters to specify the mechanism type and the mechanism parameters (in pkcs #11 structs).
Then, in pkcs11c.c the parameter information will be passed into an info struct together with the key that is saved in context->cipherInfo.

Comment 48

[Wan-Teh Chang]

Attachment: Review comments on 000a-pss-verification-v9.diff

Hanno,

Thanks for the patch.  Given that you have a deadline,
I suggest that you try to get a working patch first,
and not let design decisions block your progress.

Please add test programs so that we can polish your
patch and check it in for you after GSoC ends.

Since I wrote a lot of comments, I decided to use
an attachment for my review comments.

You didn't use "cvs diff" to generate the patch.
Is it because you added a new file?  If so, I can
add that file (with empty contents) to enable you
to use "cvs diff" in the future.

Comment 49

[Wan-Teh Chang]

Attachment: Additional review comments on 000a-pss-verification-v9.diff

I reviewed the emsa_pss_verify function.  This completes my
review of 000a-pss-verification-v9.diff.

Comment 50

[Hanno Boeck]

WTC, thanks for your comments, I'll soon attach a patch that adresses most of your remarks.

I don't like the name rsautil.c for the new file as long as it only contains pss specific tasks. Maybe rsapssutil.c? (breaks 8.3 compatibility, don't know if that matters) Also, should we move the asn1 template to that file, too?

"After reading the entire patch, I concluded that this function doesn't need the hashAlg output parameter, because its caller never uses the hashAlg output."
That's wrong, it's used in VFY_Begin.

hasht.h: I also don't like mgftype to be here, but do you have a better suggestion? blapi.h?
Also I dislike the idea of having a merged mgf-and-hashtype param for emsa-pss-verify, as this would introduce one more "convert algorithm representation around" step.

rsawrapr.c: "Please use "sig" instead of "sign" in "sign" and "sign_len"." Really? All other functions in this file use sign/sign_len.

"Rename: H_ to H_prime (H' is read "H prime")." I don't understand that - what has this value to do with primes? The name H_ was used because it seemed to me likely someone reading code+spec will associate that with H' (and independently openssl authors had the same idea).

Comment 51

[Hanno Boeck]

Attachment: 000a-pss-verification-v10.diff

Comment 52

[Wan-Teh Chang]

In English (at least in the US), the math symbol H' is pronounced
"H prime".  This is why I suggest the C variable name H_prime for
H'.

Comment 53

[Robert Relyea]

Comment on attachment 459046 [details] [diff] [review]
Fix incorrect "End of Version 3.0xx" comments in ldvector.c (checked in)

yes,  thank you.

bob

Comment 54

[Wan-Teh Chang]

Comment on attachment 459046 [details] [diff] [review]
Fix incorrect "End of Version 3.0xx" comments in ldvector.c (checked in)

I checked in this patch on the NSS_3_12_BRANCH (NSS 3.12.8).

Checking in ldvector.c;
/cvsroot/mozilla/security/nss/lib/freebl/ldvector.c,v  <--  ldvector.c
new revision: 1.21.22.1; previous revision: 1.21
done

Comment 55

[Hanno Boeck]

Attachment: 000a-pss-verification-v11.diff

Comment 56

[Hanno Boeck]

Attachment: 000b-pss-signatures-v1.diff (not for review)

This is still a lot of work in progress, but to make my progress public here is a first version of a working signature generation patch. It currently hard codes parameter values in certutil.c, you can adapt them there if you wanna play with it.

Comment 57

[Konstantin Andreev]

Folks, the continuous check-in of work-in-progress makes trunk very unstable. I could even say, monopolize trunk by particular bug-id.

Typically, trunk gets updated when the whole story is competed and approved. Could you, please, delay updates until this bug will complete and stabilize ?

I am asking for this, because I am preparing a set of patches, based on sources from 04 Jun 2010, and need port them to current head. But head is unstable...

Comment 58

[Hanno Boeck]

Attachment: 000b-pss-sign-v10.diff

Comment 59

[Wan-Teh Chang]

Attachment: pss-verification-softoken.diff by Hanno Boeck (checked in)

This patch is the subset of Hanno's 000a-pss-verification-v11.diff
(attachment 461831 [details] [diff] [review]) for mozilla/security/nss/lib/softoken.

I eliminated the MGFType typedef, based on chat conversation with
Hanno.

In lib/softoken/pkcs11.c, I changed CKF_DUZ_IT_ALL to
CKF_SN_VR for CKM_RSA_PKCS_PSS because this mechanism
can only be used for signature generation and verification.

I also renamed some functions and reformatted some code,
trying to make the new code "fit in".

Hanno, please update your source tree.  When you resolve
the merging conflicts, please study my changes and let
me know if you have questions or disagree with any of
my changes.  Thanks!

Comment 60

[Wan-Teh Chang]

Comment on attachment 462560 [details] [diff] [review]
pss-verification-softoken.diff by Hanno Boeck (checked in)

I checked in this patch on the NSS trunk (NSS 3.13).

Checking in pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.170; previous revision: 1.169
done
Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.113; previous revision: 1.112
done
Checking in pkcs11i.h;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11i.h,v  <--  pkcs11i.h
new revision: 1.53; previous revision: 1.52
done
Checking in rsawrapr.c;
/cvsroot/mozilla/security/nss/lib/softoken/rsawrapr.c,v  <--  rsawrapr.c
new revision: 1.13; previous revision: 1.12
done
Checking in softoken.h;
/cvsroot/mozilla/security/nss/lib/softoken/softoken.h,v  <--  softoken.h
new revision: 1.25; previous revision: 1.24
done

Comment 61

[Hanno Boeck]

Attachment: 000c-tests.diff

Comment 62

[Wan-Teh Chang]

Attachment: pss-verification-softoken2.diff (checked in)

Remove two duplicate lines that I added by mistake
when I checked in pss-verification-softoken.diff
(attachment 462560 [details] [diff] [review]).

Checking in rsawrapr.c;
/cvsroot/mozilla/security/nss/lib/softoken/rsawrapr.c,v  <--  rsawrapr.c
new revision: 1.14; previous revision: 1.13
done

Comment 63

[Wan-Teh Chang]

Attachment: pss-sign-softoken.diff by Hanno Boeck (checked in)

This patch is the subset of Hanno's 000b-pss-sign-v10.diff
(attachment 462399 [details] [diff] [review]) for mozilla/security/nss/lib/softoken.

I removed the one-line change to sftk_CryptInit in
lib/softoken/pkcs11c.c.  I verified that sftk_CryptInit
is not used for RSA signature generation or verification.

Comment 64

[Hanno Boeck]

Attachment: 0008-psstemplate-pointerto.diff

This is an addition to the already applied patch 0005 and changes the pss template to use a "pointer to"-template. Else it won't work for encoding (see dev list and #583308 for details).

Comment 65

[Hanno Boeck]

Attachment: 0009-add-pk11-mgfmap-v1.diff

This adds a function that maps OIDs to a PKCS #11 mask generation function mechanism.

Comment 66

[Hanno Boeck]

Attachment: 000a-pss-verification-v13.diff

Comment 67

[Hanno Boeck]

Attachment: 000b-pss-sign-v12.diff

Comment 68

[Wan-Teh Chang]

Attachment: Fix nits in the RSA-PSS code in lib/softoken (checked in)

This patch fixes miscellaneous nits in the RSA-PSS code
that I checked in to lib/softoken yesterday.

I checked in the patch on the NSS trunk (NSS 3.13).

Checking in rsawrapr.c;
/cvsroot/mozilla/security/nss/lib/softoken/rsawrapr.c,v  <--  rsawrapr.c
new revision: 1.16; previous revision: 1.15
done
Checking in softoken.h;
/cvsroot/mozilla/security/nss/lib/softoken/softoken.h,v  <--  softoken.h
new revision: 1.27; previous revision: 1.26
done

Comment 69

[Wan-Teh Chang]

Hanno: I have some suggested changes to your lib/softoken code.

1. Please add a comment to explain why you don't pass emBits
to emsa_pss_encode and emsa_pss_verify.

2. Step 11 in emsa_pss_encode implies that 8*emLen - emBits
is 1, correct?  Please add a comment to explain this.  (This
can be combined with the comment I asked for above.)

3. In emsa_pss_verify, why is the check in Step 6 missing?

4. Optional: The existing RSA code in lib/softoken uses
inconsistent variable naming conventions (e.g., foo_bar
vs. fooBar).  The new RSA-PSS code should pick one
convention and follow it.  Also, variables within a single
function should be named consistently (i.e., do not
mix foo_bar and fooBar within the same function).

5. I highly recommend that we reverse the input/output
parameter ordering of MGF1 so that it matches the
order used in PKCS #1.  I know this violates the NSS
API Guidelines, but I had a hard time with our MGF1
function's parameter ordering when I compared our
code with PKCS #1.

Comment 70

[Wan-Teh Chang]

Attachment: Fix memory leaks of 'info' on error paths in NSC_VerifyInit (checked in)

To review this patch, please compare it with the equivalent
code in NSC_SignInit:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/pkcs11c.c&rev=1.114&mark=2009,2015,2049#2005

Comment 71

[Wan-Teh Chang]

Attachment: 0008-psstemplate-pointerto-v2.diff by Hanno Boeck and Nelson Bolyard (checked in)

Hanno, I fixed the compilation errors on Windows and
merged with Nelson's code in attachment 461834 [details] (the
third set of templates) from bug 583308 comment 9.

Please test this patch.

I checked in this patch on the NSS trunk (NSS 3.13).

Checking in seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.57; previous revision: 1.56
done

Comment 72

[Hanno Boeck]

Attachment: 0010-rsawrapr-additional-bits.diff

wtc, patch for your remarks in comment 69.

For emBits, there was already a comment above verify, I've now also added it to the encode-function and referenced the section of the rfc where this can be seen.

Also, you're right that the check from step 6 was missing. I've now added it.

Comment 73

[Wan-Teh Chang]

Attachment: 0010-rsawrapr-additional-bits-v2.diff by Hanno Boeck (checked in)

I checked in the patch on the NSS trunk (NSS 3.13).

Checking in rsawrapr.c;
/cvsroot/mozilla/security/nss/lib/softoken/rsawrapr.c,v  <--  rsawrapr.c
new revision: 1.17; previous revision: 1.16
done

I fixed the section number for RSASSA-PSS-SIGN (which
passes modBits -1 to EMSA-PSS-ENCODE), and realized
that your code works only if modBits is a multiple of
8.  Your code assumes 8*emLen - emBits is 1, which is
true only if modBits is a multiple of 8.

In practice modBits is often a multiple of 8, but
it would be good to eliminate this assumption, and
pass emBits to emsa_pss_encode and emsa_pss_verify.

Note that nsslowkey_PublicModulusLen returns the
modulus length in bytes (called 'k' in the RFC).
There may not be an existing function that returns
modulus bits, so you need to add one.  You can
model it after nsslowkey_PublicModulusLen.  Note
that sometimes there is an initial zero byte,
whose purpose is to make the modulus an unsigned
integer.  So you need to handle the initial zero
byte properly.

Once we have the new nsslowkey_PublicModulusBits
function, we can pass modBits - 1 as the emBits
argument to emsa_pss_verify, which can compute
emLen as ceil((emBits - 1)/8).

Similarly for nsslowkey_PrivateModulusBits and
emsa_pss_encode.

If you don't have time to do this, please at
least add the nsslowkey_*ModulusBits functions
add a test to RSA_CheckSignPSS and RSA_SignPSS
to verify that modBits is a multiple of 8.

Comment 74

[Hanno Boeck]

Attachment: 0011-add-modulusbits-functions-to-lowkey.diff

wtc, thanks for bringing that up, see attached patch for modulusbits functions. Will try to make the code work for uncommon keysizes now.

Comment 75

[Hanno Boeck]

Attachment: 0012-fix-pss-verification-for-uncommon-keysizes.diff

This changes the RSA_CheckSignPSS and emsa_pss_verify functions to use emBits and also work with key sizes not dividable by 8.
(For encoding, this is slightly more complicated, as certutil is not able to create such keys or reuse the openssl ones)

Comment 76

[Hanno Boeck]

Attachment: 0011-add-modulusbits-functions-to-lowkey-v2.diff

Comment 77

[Hanno Boeck]

Attachment: 0012-fix-pss-verification-for-uncommon-keysizes-v2.diff

I found another problem with the corner-case when keysize mod 8 = 1. This causes the modulus length to differ from the encoded message length, we have to check that case and adjust accordingly.

Comment 78

[Wan-Teh Chang]

Comment on attachment 463007 [details] [diff] [review]
Fix memory leaks of 'info' on error paths in NSC_VerifyInit (checked in)

I checked in the patch on the NSS trunk (NSS 3.13).

Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.115; previous revision: 1.114
done

Comment 79

[Wan-Teh Chang]

Comment on attachment 464404 [details] [diff] [review]
0011-add-modulusbits-functions-to-lowkey-v2.diff

Hanno, this patch is correct, but I'd like to suggest
some changes.

In mozilla/security/nss/lib/softoken/lowkey.c:

Please create a new function that takes a
"SECItem *modulus" input parameter for the common
code between nsslowkey_PublicModulusBits and
nsslowkey_PrivateModulusBits.  Then you can
pass &pubk->u.rsa.modulus or &privk->u.rsa.modulus
to this new function.

>+	while(b0 != 0) {
>+	    bits++;
>+	    b0 >>= 1;
>+	}

Add a space after 'while'.

In security/nss/lib/softoken/lowkeyi.h:

> /*
> ** Return the modulus length of "pubKey".
> */
> extern unsigned int nsslowkey_PublicModulusLen(NSSLOWKEYPublicKey *pubKey);

Please change the comment to
    Return the modulus length of "pubKey" in bytes.

> /*
> ** Return the modulus length of "privKey".
> */
> extern unsigned int nsslowkey_PrivateModulusLen(NSSLOWKEYPrivateKey *privKey);

Please change the comment to
    Return the modulus length of "privKey" in bytes.

Comment 80

[Wan-Teh Chang]

Comment on attachment 464405 [details] [diff] [review]
0012-fix-pss-verification-for-uncommon-keysizes-v2.diff

Same as above -- although the bug is correct, I'd
like suggest some changes.  Please combine the two
patches into one, and also fix emsa_pss_encode.

In security/nss/lib/softoken/rsawrapr.c:

> static SECStatus
> emsa_pss_verify(const unsigned char *mHash,
>                 const unsigned char *em, unsigned int emLen,
>                 HASH_HashType hashAlg, HASH_HashType maskHashAlg,
>-                unsigned int sLen)
>+                unsigned int sLen, unsigned int emBits)

Please list the 'emBits' parameter after 'emLen'.
This matches the ordering in the spec:
    EMSA-PSS-VERIFY(M, EM, emBits)

>+    if (modulus_bits % 8 == 1) {
>+	rv = emsa_pss_verify(hash, &buffer[1], modulus_len - 1, hashAlg,
>+			     maskHashAlg, pss_params->sLen, modulus_bits - 1);
>+    } else {
>+	rv = emsa_pss_verify(hash, buffer, modulus_len, hashAlg,
>+			     maskHashAlg, pss_params->sLen, modulus_bits - 1);
>+    }

I suggest rewriting this as follows to avoid code duplication:

    const unsigned char *em = buffer;
    unsigned int emLen = modulus_len;
    if (modulus_bits % 8 == 1) {
        // You can use ++ and -- instead, if you prefer.
        em += 1;
        emLen -= 1;
    }
    rv = emsa_pss_verify(hash, em, emLen, hashAlg,
                         maskHashAlg, pss_params->sLen, modulus_bits - 1);

Comment 81

[Hanno Boeck]

Attachment: 0012-fix-pss-verification-for-uncommon-keysizes-v3.diff

Combined modulus bits and keysize fixes. I applied all your changes, but I have not yet done any fixes to the encode function. The problem is that I have no way to test that. nss' own RSA_NewKey function is not able to generate keys not divdable by 16 and certutil has no option to import keys. Neither does any of the other tools shipped with nss.

We could either implement a key import in certutil or change the key generation to allow any keysize, though neither is trivial.

Still, I think you already can apply the verify-bits, as they don't interfer with the encode stuff.

Comment 82

[Wan-Teh Chang]

Hanno: thanks for the new patch.  I'll review it
tomorrow.

You should be able to fix emsa_pss_encode without
a test case of uncommon key size.  You just need
to test your fix with key sizes that are a multiple
of 8.

Comment 83

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 459054 [details] [diff] [review]
0003-add-mgf-v7.diff by Hanno Boeck (checked in)

r=nelson

Comment 84

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 459703 [details] [diff] [review]
0005-prettyprint-pss-params-v6.diff (checked in)

This patch adds new templates that contain both the POINTER and EXPLICIT 
flags, which we now know is a no-no. :) But you've already fixed that 
in a subsequent patch, so no worries here.

Comment 85

[Hanno Boeck]

Attachment: 000a-pss-verification-v14.diff

Change: allocs the space for the certificate making the union-hack/solution not neccessary any more and avoiding problems with larger key sizes in the future.

Comment 86

[Hanno Boeck]

Attachment: 000b-pss-sign-v13.diff

Only change is fixing a reject against current cvs head.

Comment 87

[Hanno Boeck]

Attachment: 0012-fix-pss-verification-for-uncommon-keysizes-v5.diff

Comment 88

[Markus Kilås]

Will this issue also add RSASSA-PSS signature verification support to the p7verify tool or is it only about verifying certificates?

Comment 89

[Konstantin Andreev]

Hanno, could you, please, provide examples of objects that use these algorithms (e.g. certificates, S/MIME messages) ?

Otherwise, it's impossible to test your work.

Comment 90

[Hanno Boeck]

@Konstantin: I've setup various https tests on
http://ssl.hboeck.de/

Also, here are some samples by someone else:
http://www.kaiser.cx/x509Pss.html

Beside, look at the 000c-patch, it ships a couple of test certs and also creates some with certutil. You can also generate them with a cvs snapshot of openssl.

@Markus: This is currently only about X.509, but I'll may do support for other stuff (mainly S/MIME) in the future, it is not that much work based on the existing patches.

Comment 91

[Markus Kilås]

You can create certificates with that signature algorithm using EJBCA (www.ejbca.org) as well.

@Hanno: Ok, then I know thanks.

Comment 92

[Hanno Boeck]

Attachment: 0009-add-pk11-mgfmap-v2.diff

Minor adjustment to apply on current CVS head.

Comment 93

[Hanno Boeck]

Attachment: 0009-add-pk11-mgfmap-v3.diff

Comment 94

[Hanno Boeck]

Attachment: 000b-pss-sign-v15.diff

Previous sign patch had a bug in certutil making the parameters not work.

Comment 95

[Hanno Boeck]

Attachment: 000a-pss-verification-v15.diff

Re-diffed against CVS head, minor fixes, also support SHA224 (this depends on the patches attached to #647706 ).

Comment 96

[Hanno Boeck]

Attachment: 000c-tests-v2.diff

Just change paths so that all patches can be applied with -p0.

Comment 97

[Hanno Boeck]

As all of this patching maybe very confusing, I'm now providing all patches neccessary to get pss working for download:
http://rsapss.hboeck.de/download/current/patches/
just running "patch -p0 < [path_to]/patches/*" inside the mozilla dir should do it.

Also, you can find a current cvs snap, a cvs snap with all pss patches applied and the patches tar-ed here:
http://rsapss.hboeck.de/download/current/

Hope this makes it easier for testers. I'm trying to keep this updated with changes in nss cvs head.

Comment 98

[Wan-Teh Chang]

Attachment: Move MGF1 from lib/freebl to lib/softoken

It is unlikely that MGF1 will be used by a crypto algorithm
other than RSA-PSS, so I move it from lib/freebl to lib/softoken.

Comment 99

[Wan-Teh Chang]

Comment on attachment 564380 [details] [diff] [review]
Move MGF1 from lib/freebl to lib/softoken

Elio: I checked in this patch first to get more time
on the NSS tinderbox.  I will address any issues
when you review this.  Thanks.

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

Checking in lib/freebl/blapi.h;
/cvsroot/mozilla/security/nss/lib/freebl/blapi.h,v  <--  blapi.h
new revision: 1.42; previous revision: 1.41
done
Checking in lib/freebl/ldvector.c;
/cvsroot/mozilla/security/nss/lib/freebl/ldvector.c,v  <--  ldvector.c
new revision: 1.29; previous revision: 1.28
done
Checking in lib/freebl/loader.c;
/cvsroot/mozilla/security/nss/lib/freebl/loader.c,v  <--  loader.c
new revision: 1.54; previous revision: 1.53
done
Checking in lib/freebl/loader.h;
/cvsroot/mozilla/security/nss/lib/freebl/loader.h,v  <--  loader.h
new revision: 1.35; previous revision: 1.34
done
Checking in lib/freebl/manifest.mn;
/cvsroot/mozilla/security/nss/lib/freebl/manifest.mn,v  <--  manifest.mn
new revision: 1.61; previous revision: 1.60
done
Removing lib/freebl/mgf1.c;
/cvsroot/mozilla/security/nss/lib/freebl/mgf1.c,v  <--  mgf1.c
new revision: delete; previous revision: 1.2
done
Checking in lib/softoken/rsawrapr.c;
/cvsroot/mozilla/security/nss/lib/softoken/rsawrapr.c,v  <--  rsawrapr.c
new revision: 1.18; previous revision: 1.17
done

Comment 100

[Elio Maldonado]

Comment on attachment 564380 [details] [diff] [review]
Move MGF1 from lib/freebl to lib/softoken

The move from freebl to softoken has already been applied.

Comment 101

[Alicja Kario]

Attachment: certificates with rsa-pss signatures

Verifying certificates with RSA-PSS signatures doesn't work in TLS connections when using tstclnt, but it does with certutil -V.

Reproducer:
tar xJf certs.tar.xz

mkdir srv-db
certutil -N -d sql:./srv-db --empty-password
certutil -A -d sql:./srv-db -a -i ca/cert.pem -t 'cC,,' -n ca
pk12util -i server-pss/bundle.p12 -d sql:./srv-db/ -W ''
certutil -V -d sql:./srv-db -n server-pss -u V

selfserv -d sql:./srv-db -p 4433 -n server-pss

mkdir clnt-db
certutil -N -d sql:./clnt-db --empty-password
certutil -A -d sql:./clnt-db -a -i ca/cert.pem -t 'cC,,' -n ca

tstclnt -d sql:./clnt-pss-db -h localhost -p 4433

The connection is aborted with 

tstclnt: authentication of server cert failed: SEC_ERROR_BAD_SIGNATURE: Peer's certificate has an invalid signature.
selfserv: HDX PR_Read returned error -12271:
SSL peer cannot verify your certificate.


but certutil -V accepts it:
certutil: certificate is valid

Comment 102

[Adriano Santoni]

All, any chance to see Thunderbird support RSAPSS-signed S/MIME certificates in the near future?

Comment 103

[Kai Engert [:KaiE:]]

(In reply to Adriano Santoni from comment #102)

All, any chance to see Thunderbird support RSAPSS-signed S/MIME certificates in the near future?

Let's track that in bug 1597201 and bug 1597202. It's unclear what priority it should have.