Closed Bug 742162 Opened 8 years ago Closed 8 years ago

Allow CertificateRequest to have an empty certificate_authorities list


(NSS :: Libraries, defect, P1)



(Not tracked)



(Reporter: wtc, Assigned: ekr)



(1 file, 2 obsolete files)

Attached patch Patch by Eric Rescorla (obsolete) — Splinter Review
The attached patch makes ssl3_SendCertificateRequest allow nnames to
be 0.  This results in a CertificateRequest message with an empty
certificate_authorities list.  This is explicitly permitted by RFC 5246.

In DTLS-SRTP, the certificates are self-signed, so there is no sensible
value to put in the certificate_authorities list.

EKR said he would be more than happy to have an extra option required to
disable this check if it makes people nervous.  Bob, Nelson, do you
think this check is valuable for a regular SSL server?

Note to myself: if we remove this check, we should annotate sslerr.h
to note that SSL_ERROR_NO_TRUSTED_SSL_CLIENT_CA is not reported in
NSS 3.13.5 or later.
Attachment #612087 - Flags: superreview?(nelson)
Attachment #612087 - Flags: review?(rrelyea)
I forgot to express my own opinion.  I think it is fine to simply
remove this check rather than adding an option to disable this check.
But I'd appreciate Bob and Nelson's opinions.

I found that RFC 4346 (TLS 1.1) also explicitly allows the
certificate_authorities list to be empty.
To follow-up: it's important to me to be able to do this. I don't care what the mechanism is or how many options the user has to set.
FWIW, I agree with Wan-Teh that an option is not needed.
Attached patch Patch by Eric Rescorla, v2 (obsolete) — Splinter Review
Brian: please review and check this in.  Feel free to edit
the comments.  Thanks.
Attachment #612087 - Attachment is obsolete: true
Attachment #612087 - Flags: superreview?(nelson)
Attachment #612087 - Flags: review?(rrelyea)
Attachment #612762 - Flags: superreview?(rrelyea)
Attachment #612762 - Flags: review?(bsmith)
Comment on attachment 612762 [details] [diff] [review]
Patch by Eric Rescorla, v2

r+ rrelyea

With some tentative thoughts.

My main worry is SSL servers. I believe the check was added because users would enable Client Auth on their SSL servers without adding any trusted client auth CAs. Then clients would fail to connect to the servers.

Most NSS servers are pretty aggressive on failing on startup if there is a problem.

So that being said, why did I r+ this? Because I don't think this failure would trigger the startup fail. At this point we are already too late.

What this means is that we won't get the message in the server log the the connection failed because there are no trusted client auth certs (the failure would simply be that the client was not trusted). I think this is probably OK, certainly simpler than trying to detect if we are running dtls and that it's a case where an empty calist is allowed (or reasonable).

Attachment #612762 - Flags: superreview?(rrelyea) → superreview+
Comment on attachment 612762 [details] [diff] [review]
Patch by Eric Rescorla, v2

Review of attachment 612762 [details] [diff] [review]:

r+. I suggest removing the comment in ssl3con.c, so that the change is purely a removal of code in ssl3con.c + the comment in sslerr.h.
Attachment #612762 - Flags: review?(bsmith) → review+
Patch checked in on the NSS trunk (NSS 3.14) with
the change suggested by bsmith.

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.180; previous revision: 1.179
Checking in sslerr.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslerr.h,v  <--  sslerr.h
new revision: 1.24; previous revision: 1.23
Attachment #612762 - Attachment is obsolete: true
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: 3.13.5 → 3.14
You need to log in before you can comment on or make changes to this bug.