Allow CertificateRequest to have an empty certificate_authorities list

RESOLVED FIXED in 3.14

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Wan-Teh Chang, Assigned: ekr)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 612087 [details] [diff] [review]
Patch by Eric Rescorla

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)
(Reporter)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
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.
(Reporter)

Comment 4

6 years ago
Created attachment 612762 [details] [diff] [review]
Patch by Eric Rescorla, v2

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 5

6 years ago
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).

bob
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+
(Reporter)

Comment 7

6 years ago
Created attachment 621796 [details] [diff] [review]
Patch by Eric Rescorla, v3

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
done
Checking in sslerr.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslerr.h,v  <--  sslerr.h
new revision: 1.24; previous revision: 1.23
done
Attachment #612762 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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.