Closed Bug 1215295 Opened 4 years ago Closed 4 years ago

Support RSA-PSS for digital signing

Categories

(NSS :: Libraries, defect)

defect
Not set

Tracking

(firefox44 affected)

RESOLVED FIXED
Tracking Status
firefox44 --- affected

People

(Reporter: ttaubert, Assigned: ttaubert)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

In TLS 1.3, struct DigitallySigned switches from RSASSA-PKCS1-v1_5 to RSASSA-PSS signatures.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
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!
Flags: needinfo?(martin.thomson)
Flags: needinfo?(ekr)
Blocks: 1191936
I shall await the tests.
Flags: needinfo?(martin.thomson)
Attachment #8674498 - Attachment is obsolete: true
Attachment #8674499 - Attachment is obsolete: true
Attachment #8674500 - Attachment is obsolete: true
(In reply to Martin Thomson [:mt:] from comment #5)
> I shall await the tests.

Working on it, thanks!
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.
Flags: needinfo?(martin.thomson)
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.
Flags: needinfo?(ekr)
(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?
(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.
(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.
Flags: needinfo?(martin.thomson)
Those lower-level tests are needed in any case.
(In reply to Eric Rescorla (:ekr) from comment #12)
> Those lower-level tests are needed in any case.

Yeah, agreed. Forgot about those.
New patch at: https://codereview.appspot.com/271240043
Flags: needinfo?(martin.thomson)
Flags: needinfo?(ekr)
Flags: needinfo?(ekr)
Patch updated and review feedback addressed: https://codereview.appspot.com/271240043

Rob, can you please take a look at the nss.def changes? Thanks!
Flags: needinfo?(rrelyea)
Flags: needinfo?(ekr)
Flags: needinfo?(ekr)
Flags: needinfo?(martin.thomson)
Blocks: 554827
https://hg.mozilla.org/projects/nss/rev/372db375adaa
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.22
Blocks: 1224737
Looks like some of our builders aren't happy about template<class T> using x<T> = y<t>;

Macros to the rescue.
Attachment #8687464 - Flags: review?(ttaubert)
Comment on attachment 8687464 [details] [diff] [review]
bug1215295-scoped.patch

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

LGTM, thanks for fixing this so fast!
Attachment #8687464 - Flags: review?(ttaubert) → review+
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
Attachment #8687464 - Flags: checked-in+
Flags: needinfo?(rrelyea)
Depends on: 1227665
Blocks: 1280439
You need to log in before you can comment on or make changes to this bug.