1215295 - Support RSA-PSS for digital signing

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

In TLS 1.3, struct DigitallySigned switches from RSASSA-PKCS1-v1_5 to RSASSA-PSS signatures.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-PK11_-Sign-Verify-WithMechanism.patch

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Add-isTLS13-parameter-to-ssl3_SignHashes-and-ssl3_Ve.patch

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Use-PK11_SignWithMechanism-and-PK11_VerifyWithMechan.patch

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Patch at: https://codereview.appspot.com/265700044

Eric, Martin, can you please take a look and give some feedback? I split it into three patches but didn't know how to do that with rietveld, so there it's just one big patch.

This needs a few new tests of course but I first wanted to see whether that's the right direction. All existing tests pass.

Thanks!

Comment 5

[Martin Thomson [:mt:]]

I shall await the tests.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Martin Thomson [:mt:] from comment #5)
> I shall await the tests.

Working on it, thanks!

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Updated the patch at https://codereview.appspot.com/265700044/

I fixed a few mistakes and ensured that we not only support PSS signatures but also use ssl_sign_rsapss for TLS 1.3. I added quite a few tests that ensure we properly enforce PSS signatures in TLS 1.3.

The tests run only with NSS_ENABLE_TLS_1_3=1.

Comment 8

[Eric Rescorla (:ekr)]

Sorry to be a downer, but it would really be better if you restricted this patch to PKCS#11.

We have quite a large TLS 1.3 patch already in flight and messing with 1.3 here is more likey
to get in the way.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Eric Rescorla (:ekr) from comment #8)
> Sorry to be a downer, but it would really be better if you restricted this
> patch to PKCS#11.

Ok, I can do only the PKCS#11 stuff now and keep the rest for after your patch landed.

> We have quite a large TLS 1.3 patch already in flight and messing with 1.3
> here is more likey
> to get in the way.

Is there a bug for that?

Comment 10

[Eric Rescorla (:ekr)]

(In reply to Tim Taubert [:ttaubert] from comment #9)
> (In reply to Eric Rescorla (:ekr) from comment #8)
> > Sorry to be a downer, but it would really be better if you restricted this
> > patch to PKCS#11.
> 
> Ok, I can do only the PKCS#11 stuff now and keep the rest for after your
> patch landed.

Well, I'm going to wire up PSS when I do it. A lot of the mechanisms
have already change here.


> > We have quite a large TLS 1.3 patch already in flight and messing with 1.3
> > here is more likey
> > to get in the way.
> 
> Is there a bug for that?

There's a meta-bug somewhere.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Eric Rescorla (:ekr) from comment #10)
> (In reply to Tim Taubert [:ttaubert] from comment #9)
> > (In reply to Eric Rescorla (:ekr) from comment #8)
> > > Sorry to be a downer, but it would really be better if you restricted this
> > > patch to PKCS#11.
> > 
> > Ok, I can do only the PKCS#11 stuff now and keep the rest for after your
> > patch landed.
> 
> Well, I'm going to wire up PSS when I do it. A lot of the mechanisms
> have already change here.

Hmm, ok. Too bad. I'll write a few lower-level tests then.

Comment 12

[Eric Rescorla (:ekr)]

Those lower-level tests are needed in any case.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Eric Rescorla (:ekr) from comment #12)
> Those lower-level tests are needed in any case.

Yeah, agreed. Forgot about those.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

New patch at: https://codereview.appspot.com/271240043

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Patch updated and review feedback addressed: https://codereview.appspot.com/271240043

Rob, can you please take a look at the nss.def changes? Thanks!

Comment 16

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/372db375adaa

Comment 17

[Martin Thomson [:mt:]]

Attachment: bug1215295-scoped.patch

Looks like some of our builders aren't happy about template<class T> using x<T> = y<t>;

Macros to the rescue.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8687464 [details] [diff] [review]
bug1215295-scoped.patch

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

LGTM, thanks for fixing this so fast!

Comment 19

[Martin Thomson [:mt:]]

Comment on attachment 8687464 [details] [diff] [review]
bug1215295-scoped.patch

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

https://hg.mozilla.org/projects/nss/rev/f741565a4e34