Add support for certificate name checking

Add support for certificate name checking
There's nothing OpenSSL specific about this implementation, other than this is the only platform that does not already supply an implementation of this method.

BUG=60719
TEST=net_unittests --gtest_filter=X509CertificateCertificateNameVerifyTest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66263

[joth, 2010-10-28 12:17:47]

Wasn't so bad in the end. Please double check my test expectations though (and
suggest any others you may have).

http://codereview.chromium.org/4184004/diff/7001/8003
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/4184004/diff/7001/8003#newcode758
net/base/x509_certificate_unittest.cc:758: { true, "f", "f." },
should these two match? If not, what is the rule which we exclude them under?

[Ryan Sleevi, 2010-10-28 13:16:11]

I haven't had a chance to look in depth yet, but a few comments.

The wildcard issue was discussed in depth on the certid mailing list at
http://www.ietf.org/mail-archive/web/certid/current/msg00452.html if you want
more context.

http://codereview.chromium.org/4184004/diff/7001/8002
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/4184004/diff/7001/8002#newcode441
net/base/x509_certificate_openssl.cc:441: if (host_domain.empty() ||
!cert_match.starts_with("*"))
The saintandre draft permits wildcards in arbitrary positions within labels in
subsection 3 as a MAY.

RFC 2818, HTTP over TLS, describes them as supported in 3.1. with the following
example:

"Names may contain the wildcard character * which is considered to match any
single domain name component or component fragment. E.g., *.a.com matches
foo.a.com but not bar.foo.a.com. f*.com matches foo.com but not bar.com."

I know in general, the sentiment in the TLS/PKIX WG seems to be disfavoring
wildcards, and that the CABForum prohibits them, I just felt it was worth
mentioning here.

http://codereview.chromium.org/4184004/diff/7001/8003
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/4184004/diff/7001/8003#newcode754
net/base/x509_certificate_unittest.cc:754: CertificateNameVerifyTestData
kNameVerifyTestData[] = {
I would suggest adding some CaSe-InSenSiTIVE matches, as well as possibly some
punycode-encoded IDNs.

http://codereview.chromium.org/4184004/diff/7001/8003#newcode758
net/base/x509_certificate_unittest.cc:758: { true, "f", "f." },
On 2010/10/28 12:17:48, joth wrote:
> should these two match? If not, what is the rule which we exclude them under?

Good question. Apple's TP accepts them, it looks like NSS rejects. I didn't
check what CryptoAPI/Windows does.

For Apple, see libsecurity_apple_x509_tp-37601 - lib\certGroupUtils.cpp -
tpCompareHostNames -
http://www.opensource.apple.com/source/libsecurity_apple_x509_tp/libsecurity_...

For NSS, see cert_TestHostName -
http://mxr.mozilla.org/mozilla/source/security/nss/lib/certdb/certdb.c#1455

[joth, 2010-10-28 13:33:08]

Many thanks for the pointers to background discussion -- at this point this
is just as valuable to me as detailed code comments.


On 28 October 2010 14:16, <rsleevi@chromium.org> wrote:

> I haven't had a chance to look in depth yet, but a few comments.
>
> The wildcard issue was discussed in depth on the certid mailing list at
> http://www.ietf.org/mail-archive/web/certid/current/msg00452.html if you
> want
> more context.
>
>
> http://codereview.chromium.org/4184004/diff/7001/8002
> File net/base/x509_certificate_openssl.cc (right):
>
> http://codereview.chromium.org/4184004/diff/7001/8002#newcode441
> net/base/x509_certificate_openssl.cc:441: if (host_domain.empty() ||
> !cert_match.starts_with("*"))
> The saintandre draft permits wildcards in arbitrary positions within
> labels in subsection 3 as a MAY.
>

AFAICT the -09 revision only allows a leading wildcard (see below).

http://tools.ietf.org/html/draft-saintandre-tls-server-id-check-09#ref-IDNA-DEFS

4.4.3.  Checking of Wildcard Labels

   A client employing this specification's rules MAY match the reference
   identifier against a presented identifier containing one instance of
   the wildcard character '*', but only as the left-most label of the
   domain name, e.g. *.example.com (following the definition of "label"
   from [DNS
<http://tools.ietf.org/html/draft-saintandre-tls-server-id-check-09#ref-DNS>]).

   If such a wildcard identifier is presented, the wildcard MUST be used
   to match only the one position-wise corresponding label (thus
   *.example.com matches foo.example.com but not bar.foo.example.com or
   example.com).  The client MUST fail to match a presented identifier
   in which the wildcard character is contained within a label fragment
   (e.g., baz*.example.net is not allowed and MUST NOT be taken to match
   baz1.example.net and baz2.example.net), or in which the wildcard
   character does not comprise the left-most label in the presented
   identifier (e.g., neither bar.*.example.net nor bar.f*o.example.net
   are allowed).




> RFC 2818, HTTP over TLS, describes them as supported in 3.1. with the
> following example:
>
> "Names may contain the wildcard character * which is considered to match
> any single domain name component or component fragment. E.g., *.a.com
> matches foo.a.com but not bar.foo.a.com. f*.com matches foo.com but not
> bar.com."
>
> I know in general, the sentiment in the TLS/PKIX WG seems to be
> disfavoring wildcards, and that the CABForum prohibits them, I just felt
> it was worth mentioning here.
>
>


>
> http://codereview.chromium.org/4184004/diff/7001/8003
> File net/base/x509_certificate_unittest.cc (right):
>
> http://codereview.chromium.org/4184004/diff/7001/8003#newcode754
> net/base/x509_certificate_unittest.cc:754: CertificateNameVerifyTestData
> kNameVerifyTestData[] = {
> I would suggest adding some CaSe-InSenSiTIVE matches, as well as
> possibly some punycode-encoded IDNs.
>
> Good suggestion. I meant to TODO the IDN ones anyway. I'll look into that
now.


> http://codereview.chromium.org/4184004/diff/7001/8003#newcode758
> net/base/x509_certificate_unittest.cc:758: { true, "f", "f." },
> On 2010/10/28 12:17:48, joth wrote:
>
>> should these two match? If not, what is the rule which we exclude them
>>
> under?
>
> Good question. Apple's TP accepts them, it looks like NSS rejects. I
> didn't check what CryptoAPI/Windows does.
>
> For Apple, see libsecurity_apple_x509_tp-37601 - lib\certGroupUtils.cpp
> - tpCompareHostNames -
>
>
http://www.opensource.apple.com/source/libsecurity_apple_x509_tp/libsecurity_...
>
> For NSS, see cert_TestHostName -
> http://mxr.mozilla.org/mozilla/source/security/nss/lib/certdb/certdb.c#1455
>
>
> http://codereview.chromium.org/4184004/show
>

[agl, 2010-10-28 14:30:13]

NACK because of the use of googleurl.

http://codereview.chromium.org/4184004/diff/7001/8001
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/4184004/diff/7001/8001#newcode235
net/base/x509_certificate.h:235: // Verifies that |hostname| matches one of the
certificate names in
nit: What's a 'certificate name'?

// Verifies that |hostname| matches one of the names in |cert_names|, based on
TLS name matching rules. The members of |cert_names| must have been extracted
from the Subject CN or SAN fields of a certificate. See also GetDNSNames.

http://codereview.chromium.org/4184004/diff/7001/8002
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/4184004/diff/7001/8002#newcode388
net/base/x509_certificate_openssl.cc:388: const std::string
host(net::CanonicalizeHost(hostname.as_string(),
I don't think we should use googleurl in this code path. It's lighting up too
much complex code and any changes in that code suddenly become security
critical.

Even after chasing down a few calls, I still have no idea what this function
actually does!

http://codereview.chromium.org/4184004/diff/7001/8002#newcode412
net/base/x509_certificate_openssl.cc:412: DVLOG(3) << "Not allowing wildcard for
registry-level host " << hostname;
I think this should be removed. It'll fire so much that it'll be useless even at
vlog 3.

http://codereview.chromium.org/4184004/diff/7001/8002#newcode439
net/base/x509_certificate_openssl.cc:439: // Next see this cert name is a
wildcard, so long as the hostname we're
typo: missing 'if'

http://codereview.chromium.org/4184004/diff/7001/8002#newcode465
net/base/x509_certificate_openssl.cc:465: std::vector<std::string> cert_names;
You're changing the behaviour of all platforms here, no? I think this should be
Android specific.

[Ryan Sleevi, 2010-10-28 15:36:46]

On 2010/10/28 13:33:08, joth wrote:
> Many thanks for the pointers to background discussion -- at this point this
> is just as valuable to me as detailed code comments.
<SNIP>
> AFAICT the -09 revision only allows a leading wildcard (see below).

The -09 revision is what was discussed on certid at the aforementioned link.
That language was removed from -10 specifically because it contravened the
language of RFC 2818. The saintandre draft is a merely a BCP, and was not meant
to obsolete or replace any of the relevant RFCs.

The conclusion of the discussion, and the cause of the wording change in -10,
was specifically because of the language in RFC 2818 needed to be permitted. As
a BCP for future protocols, wildcard matching was prohibited (EV / CAB Forum,
SIP) or restricted to left-most labels (LDAP, XMPP, NNTP, SMTP, POP3, ACAP).
However, existing protocols specify wildcard matching on partial left-most
labels (HTTP) and the draft doesn't change that.

I defer to agl and wtc as to whether or not it deserves to be implemented as
specified. As discussed in the above thread, not everyone agreed with the RFC
2818 language, and some TLS stack implementors decided specifically to ignore it
(adopting language similar to the -09 draft in their implementations).

However, with respect to both NSS and the Apple implementation, they respect
wildcards as specified in 2818, and I believe CryptoAPI does as well (though I
haven't tested)

[joth, 2010-10-28 15:50:54]

On 28 October 2010 16:36, <rsleevi@chromium.org> wrote:

> On 2010/10/28 13:33:08, joth wrote:
>
>> Many thanks for the pointers to background discussion -- at this point
>> this
>> is just as valuable to me as detailed code comments.
>>
> <SNIP>
>
>  AFAICT the -09 revision only allows a leading wildcard (see below).
>>
>
> The -09 revision is what was discussed on certid at the aforementioned
> link.
> That language was removed from -10 specifically because it contravened the
> language of RFC 2818. The saintandre draft is a merely a BCP, and was not
> meant
> to obsolete or replace any of the relevant RFCs.
>
>
Ah, gothca. I completely hadn't realized there was a -10 version. (I didn't
spot it in the email thread and the -09 doesn't seem to list it in its
version table at the top either).

The perils of following a draft spec!



> The conclusion of the discussion, and the cause of the wording change in
> -10,
> was specifically because of the language in RFC 2818 needed to be
> permitted. As
> a BCP for future protocols, wildcard matching was prohibited (EV / CAB
> Forum,
> SIP) or restricted to left-most labels (LDAP, XMPP, NNTP, SMTP, POP3,
> ACAP).
> However, existing protocols specify wildcard matching on partial left-most
> labels (HTTP) and the draft doesn't change that.
>
> OK, I see. So my current implementation can be extended to cope with this
fairly easily, by replacing the straight forward test for "*" with a call to
MatchPattern on the reference and candidate left-most labels (having
pre-validated the candidate only contains exactly one * and no \ or ? chars)

Will await agl & wtc's recommendation on whether to go that way (now, or in
a followup patch)


> I defer to agl and wtc as to whether or not it deserves to be implemented
> as
> specified. As discussed in the above thread, not everyone agreed with the
> RFC
> 2818 language, and some TLS stack implementors decided specifically to
> ignore it
> (adopting language similar to the -09 draft in their implementations).
>
> However, with respect to both NSS and the Apple implementation, they
> respect
> wildcards as specified in 2818, and I believe CryptoAPI does as well
> (though I
> haven't tested)
>
>
>
Thanks!

[joth, 2010-10-28 15:57:24]

Think I've addressed everything. Does the use of
RegistryControlledDomainService::GetRegistryLength() seem OK? Again, this is
only used on the browser-supplied hostname, not the name from the certificate.

http://codereview.chromium.org/4184004/diff/7001/8001
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/4184004/diff/7001/8001#newcode235
net/base/x509_certificate.h:235: // Verifies that |hostname| matches one of the
certificate names in
On 2010/10/28 14:30:13, agl wrote:
> nit: What's a 'certificate name'?
> 
> // Verifies that |hostname| matches one of the names in |cert_names|, based on
> TLS name matching rules. The members of |cert_names| must have been extracted
> from the Subject CN or SAN fields of a certificate. See also GetDNSNames.

Done.

http://codereview.chromium.org/4184004/diff/7001/8002
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/4184004/diff/7001/8002#newcode388
net/base/x509_certificate_openssl.cc:388: const std::string
host(net::CanonicalizeHost(hostname.as_string(),
On 2010/10/28 14:30:13, agl wrote:
> I don't think we should use googleurl in this code path. It's lighting up too
> much complex code and any changes in that code suddenly become security
> critical.
> 
> Even after chasing down a few calls, I still have no idea what this function
> actually does!

Done.
IKWYM, although I had believed gurl was blessed for security sensitive stuff,
and |hostname| will probably already been through GURL by the time it gets here

http://codereview.chromium.org/4184004/diff/7001/8002#newcode412
net/base/x509_certificate_openssl.cc:412: DVLOG(3) << "Not allowing wildcard for
registry-level host " << hostname;
On 2010/10/28 14:30:13, agl wrote:
> I think this should be removed. It'll fire so much that it'll be useless even
at
> vlog 3.

Done.

http://codereview.chromium.org/4184004/diff/7001/8002#newcode439
net/base/x509_certificate_openssl.cc:439: // Next see this cert name is a
wildcard, so long as the hostname we're
On 2010/10/28 14:30:13, agl wrote:
> typo: missing 'if'

Done.

http://codereview.chromium.org/4184004/diff/7001/8002#newcode441
net/base/x509_certificate_openssl.cc:441: if (host_domain.empty() ||
!cert_match.starts_with("*"))
On 2010/10/28 13:16:11, rsleevi wrote:
> The saintandre draft permits wildcards in arbitrary positions within labels in
> subsection 3 as a MAY.
> 
> RFC 2818, HTTP over TLS, describes them as supported in 3.1. with the
following
> example:
> 
> "Names may contain the wildcard character * which is considered to match any
> single domain name component or component fragment. E.g., *.a.com matches
> http://foo.a.com but not http://bar.foo.a.com. http://f*.com matches
http://foo.com but not bar.com."
> 
> I know in general, the sentiment in the TLS/PKIX WG seems to be disfavoring
> wildcards, and that the CABForum prohibits them, I just felt it was worth
> mentioning here.
> 

Added a TODO

http://codereview.chromium.org/4184004/diff/7001/8002#newcode465
net/base/x509_certificate_openssl.cc:465: std::vector<std::string> cert_names;
On 2010/10/28 14:30:13, agl wrote:
> You're changing the behaviour of all platforms here, no?

No. This is all inside x509_certificate_openssl.cc and it would be needed by all
openssl-using platforms.

http://codereview.chromium.org/4184004/diff/7001/8003
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/4184004/diff/7001/8003#newcode754
net/base/x509_certificate_unittest.cc:754: CertificateNameVerifyTestData
kNameVerifyTestData[] = {
On 2010/10/28 13:16:11, rsleevi wrote:
> I would suggest adding some CaSe-InSenSiTIVE matches, as well as possibly some
> punycode-encoded IDNs.

Done.

[wtc, 2010-10-28 19:08:59]

I didn't review the code.  Just two high-level comments.

http://codereview.chromium.org/4184004/diff/28001/7002
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/4184004/diff/28001/7002#newcode239
net/base/x509_certificate.h:239: static bool VerifyHostname(const
base::StringPiece& hostname,
It looks strange to have one input parameter in StringPiece
and the other in std::string.

If you made this method public for the unit tests, you can
also solve the problem using the FRIEND_TEST_ALL_PREFIXES
macro.  See an example at line 283 below.

http://codereview.chromium.org/4184004/diff/28001/7003
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/4184004/diff/28001/7003#newcode383
net/base/x509_certificate_openssl.cc:383: bool X509Certificate::VerifyHostname(
The lack of such a function is a deficiency of a certificate
library.  Could you please check with the OpenSSL team?  If
they agree, it would be better to write this function in a way
that allows it to be incorporated into a future version of
OpenSSL.

[wtc, 2010-10-28 19:20:21]

One more high-level comment:

http://codereview.chromium.org/4184004/diff/28001/7002
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/4184004/diff/28001/7002#newcode179
net/base/x509_certificate.h:179: void GetDNSNames(std::vector<std::string>*
dns_names) const;
The GetDNSNames function is intended for UI.  It is not
suitable for serious certificate processing, because it
drops a lot of info (such as whether the string is a common
name in the subject field, or a DNS name or IP address in
the subjectAltName extension).

VerifyHostname should go to the Subject field and
subjectAltName extension directly.

It is fine to use GetDNSNames in the initial implementation
of VerifyHostname under time pressure, but a proper
implementation of VerifyHostname cannot be based on GetDNSNames.

[joth, 2010-10-29 10:56:37]

Thinking about this, it seems far cleaner to put this in the openssl_util file.
I'll do that now and re-upload

Cheers!

http://codereview.chromium.org/4184004/diff/28001/7002
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/4184004/diff/28001/7002#newcode179
net/base/x509_certificate.h:179: void GetDNSNames(std::vector<std::string>*
dns_names) const;
On 2010/10/28 19:20:21, wtc wrote:
> The GetDNSNames function is intended for UI.  It is not
> suitable for serious certificate processing, because it
> drops a lot of info (such as whether the string is a common
> name in the subject field, or a DNS name or IP address in
> the subjectAltName extension).
> 
> VerifyHostname should go to the Subject field and
> subjectAltName extension directly.
> 
> It is fine to use GetDNSNames in the initial implementation
> of VerifyHostname under time pressure, but a proper
> implementation of VerifyHostname cannot be based on GetDNSNames.

Thanks for the warning. Added a TODO

http://codereview.chromium.org/4184004/diff/28001/7002#newcode239
net/base/x509_certificate.h:239: static bool VerifyHostname(const
base::StringPiece& hostname,
On 2010/10/28 19:08:59, wtc wrote:
> It looks strange to have one input parameter in StringPiece
> and the other in std::string.
> 
> If you made this method public for the unit tests, you can
> also solve the problem using the FRIEND_TEST_ALL_PREFIXES
> macro.  See an example at line 283 below.

Made both use string (as converting a vector<string> to vector<StringPiece> is
too much of a bind)
Moved to private and added FRIEND_TEST_ALL_PREFIXES

(FWIW my own preference is to avoid making test fixtures a friend when the
function is 'safe' to just be public, as with class-level friendship very easy
for the test fixture to then start accessing all private members and so become
very brittle. Not a big risk in this case as it's a tiny test fixture)

http://codereview.chromium.org/4184004/diff/28001/7003
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/4184004/diff/28001/7003#newcode383
net/base/x509_certificate_openssl.cc:383: bool X509Certificate::VerifyHostname(
On 2010/10/28 19:08:59, wtc wrote:
> The lack of such a function is a deficiency of a certificate
> library.  Could you please check with the OpenSSL team?  If
> they agree, it would be better to write this function in a way
> that allows it to be incorporated into a future version of
> OpenSSL.

Added a TODO. Assuming they are keen, it'll take a while to get it in and rolled
out to target platforms, so I'll need this one in the interim.

Hmm. Maybe better if I put it in x509_openssl_util to avoid churn in these
files?

[joth, 2010-10-29 13:20:17]

Updated with the verify certname method in x509_openssl_util, which means no
changes needed in any cross platform x509 files.
Functionally identical to previous version.

[wtc, 2010-11-03 00:29:49]

joth: I'm not sure if you're waiting for my review.

I reviewed Patch Set 8, but I didn't review x509_openssl_util.cc, which is the
meat of this CL.  Has anyone else already reviewed x509_openssl_util.cc?

I have some comments below.

http://codereview.chromium.org/4184004/diff/33001/22004
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/4184004/diff/33001/22004#newcode382
net/base/x509_certificate_openssl.cc:382: // TODO(joth): We should fetch the
subjectAltNames directly rather than via
Please file a bug report for this work.

http://codereview.chromium.org/4184004/diff/33001/22005
File net/base/x509_openssl_util.cc (right):

http://codereview.chromium.org/4184004/diff/33001/22005#newcode120
net/base/x509_openssl_util.cc:120: // and also square braces and colons for IPv6
addresses.
IPv6 address literals passed to this function should not contain square braces
[].

http://codereview.chromium.org/4184004/diff/33001/22005#newcode168
net/base/x509_openssl_util.cc:168: if (host_domain.length() == registry_length +
1) {
Nit: omit curly braces {} for one-liner "if" bodies.

http://codereview.chromium.org/4184004/diff/33001/22006
File net/base/x509_openssl_util.h (right):

http://codereview.chromium.org/4184004/diff/33001/22006#newcode37
net/base/x509_openssl_util.h:37: // TLS name matching rules. The members of
|cert_names| must have been
Nit: cite the RFC or Internet-Draft that specifies the TLS name matching rules.

http://codereview.chromium.org/4184004/diff/33001/22007
File net/base/x509_openssl_util_unittest.cc (right):

http://codereview.chromium.org/4184004/diff/33001/22007#newcode41
net/base/x509_openssl_util_unittest.cc:41: { false, "192.168.1.11", "*.168.1.11"
},
It's important that a wildcard '*' not match IP addresses.
'*' is only allowed in a pattern for DNS names.

http://codereview.chromium.org/4184004/diff/33001/22007#newcode65
net/base/x509_openssl_util_unittest.cc:65: { true,
"[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]",
Standalone IPv6 address literals should not include the square brackets [].

Square brackets are only needed when an IPv6 address literal is part of a URL.

http://codereview.chromium.org/4184004/diff/33001/22007#newcode79
net/base/x509_openssl_util_unittest.cc:79: protected:
Nit: delete 'protected'.

http://codereview.chromium.org/4184004/diff/33001/22007#newcode89
net/base/x509_openssl_util_unittest.cc:89: const bool result =
VerifyHostname(test_data.hostname, cert_names);
Nit: I think it is excessive to use a 'const' local variable (and a 'const'
function
parameter that's passed by value) like this.

I know there are proponents of this style, so I'll just express my opinion once.

[Ryan Sleevi, 2010-11-03 03:42:21]

@wtc: I believe your/agl's feedback is desired on at least part of this. There
was an early discussion between joth and I on the handling of wildcard
certificates, which was a rather complicated issue on IETF certid mailing list,
where the rules being used here originated.

What is currently implemented matches an earlier draft of the
draft-saintandre-tls-server-id-check and does NOT perform RFC 2818 (HTTP over
TLS) wildcard matching.

The question posed here echoed the question posed on the certid list, where
there are two options for matching wildcard certificates:
  Option 1) Strict RFC 2818 compliance, which allows for situations such as f*
matching "foo" or fo*ar matching "foobar"
  Option 2) Non-RFC 2818 compliance, which uses the matching rules of nearly
every subsequent RFC that uses TLS and permits wildcards: Only allowing them to
appear as the left-most label, as the whole of the label.

Currently implemented is option 2 (which conforms to the -09 version of the
draft), whereas the draft was updated in -10 specifically to allow RFC 2818
rules/Option 1, after discussion on certid. I've annotated in the code where the
matching happens.

@joth: I believe there to be a possible security issue, described below.

http://codereview.chromium.org/4184004/diff/33001/22005
File net/base/x509_openssl_util.cc (right):

http://codereview.chromium.org/4184004/diff/33001/22005#newcode120
net/base/x509_openssl_util.cc:120: // and also square braces and colons for IPv6
addresses.
BUG: I'm not sure that this function should consider IP address matches as
valid. RFC 2818, and by transitive properties,
draft-saintandre-tls-server-id-check, stated

"   In some cases, the URI is specified as an IP address rather than a
   hostname. In this case, the iPAddress subjectAltName must be present
   in the certificate and must exactly match the IP in the URI."

So if |hostname| is supplied as an IP address, then it should only match if a
subjectAltName is present. Since you've TODO'd the SAN support, I think it'd be
appropriate here to return FALSE if |hostname| is an IP.

I'm wondering if there is a "blessed" IP canonicalization, since net_util's
ParseIPLiteralToNumber depends on googleurl (which agl didn't want in the
security hot path).

For NSS, it also compares if the IPv4-as-IPv6 (::ffff:192.168.0.1) matches: if
|hostname| is an IPv4 address and the subjectAltName is an IPv6 address, or the
inverse. 

For Apple, it lacks any support for IPv6 matching (I'd mention the Radar # but
Apple's bugreporter hasn't been working for me tonight)

http://codereview.chromium.org/4184004/diff/33001/22005#newcode199
net/base/x509_openssl_util.cc:199: // is finalized, we may need to update this
check to use MatchPattern.
@wtc: here

[wtc, 2010-11-03 18:46:08]

On 2010/11/03 03:42:21, rsleevi wrote:
>
> The question posed here echoed the question posed on the certid list, where
> there are two options for matching wildcard certificates:
>   Option 1) Strict RFC 2818 compliance, which allows for situations such as f*
> matching "foo" or fo*ar matching "foobar"

RFC 2818 does not give an example of fo*ar matching "foobar".
Could you clarify this point?

>   Option 2) Non-RFC 2818 compliance, which uses the matching rules of nearly
> every subsequent RFC that uses TLS and permits wildcards: Only allowing them
to
> appear as the left-most label, as the whole of the label.
> 
> Currently implemented is option 2 (which conforms to the -09 version of the
> draft), whereas the draft was updated in -10 specifically to allow RFC 2818
> rules/Option 1, after discussion on certid.

Implementing Option 2 is fine by me.  rsleevi, do you know why
draft-saintandre-tls-server-id-check changed to specify Option 1?
To reflect what current browsers actually support?  This is what NSS
does:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/c...

Re: IP address: this is caused by the use of GetDNSNames.  Our
test server certificate has the string "127.0.0.1" in the Common
Name of the Subject field.  We'll need to issue a new test server
certificate to properly use the iPAddress subjectAltName at some
point.

[joth, 2010-11-03 18:57:17]

Thanks for the comments wtc, and that nice summary of the status rsleevi.

I'm tending toward going with the -10 draft spec, I started work on this
(4368002) but would be good to get agreement this is the sane way to go
before I merge these patches.

Thanks also for pointing out this IP address issue. I totally agree that
disabling verification for IP addresses is going to be the best idea with
respect to the work in hand (which ever way I implement it)

Cheers!



On 3 November 2010 03:42, <rsleevi@chromium.org> wrote:

> @wtc: I believe your/agl's feedback is desired on at least part of this.
> There
> was an early discussion between joth and I on the handling of wildcard
> certificates, which was a rather complicated issue on IETF certid mailing
> list,
> where the rules being used here originated.
>
> What is currently implemented matches an earlier draft of the
> draft-saintandre-tls-server-id-check and does NOT perform RFC 2818 (HTTP
> over
> TLS) wildcard matching.
>
> The question posed here echoed the question posed on the certid list, where
> there are two options for matching wildcard certificates:
>  Option 1) Strict RFC 2818 compliance, which allows for situations such as
> f*
> matching "foo" or fo*ar matching "foobar"
>  Option 2) Non-RFC 2818 compliance, which uses the matching rules of nearly
> every subsequent RFC that uses TLS and permits wildcards: Only allowing
> them to
> appear as the left-most label, as the whole of the label.
>
> Currently implemented is option 2 (which conforms to the -09 version of the
> draft), whereas the draft was updated in -10 specifically to allow RFC 2818
> rules/Option 1, after discussion on certid. I've annotated in the code
> where the
> matching happens.
>
> @joth: I believe there to be a possible security issue, described below.
>
>
>
> http://codereview.chromium.org/4184004/diff/33001/22005
> File net/base/x509_openssl_util.cc (right):
>
> http://codereview.chromium.org/4184004/diff/33001/22005#newcode120
> net/base/x509_openssl_util.cc:120: // and also square braces and colons
> for IPv6 addresses.
> BUG: I'm not sure that this function should consider IP address matches
> as valid. RFC 2818, and by transitive properties,
> draft-saintandre-tls-server-id-check, stated
>
> "   In some cases, the URI is specified as an IP address rather than a
>   hostname. In this case, the iPAddress subjectAltName must be present
>   in the certificate and must exactly match the IP in the URI."
>
> So if |hostname| is supplied as an IP address, then it should only match
> if a subjectAltName is present. Since you've TODO'd the SAN support, I
> think it'd be appropriate here to return FALSE if |hostname| is an IP.
>
> I'm wondering if there is a "blessed" IP canonicalization, since
> net_util's ParseIPLiteralToNumber depends on googleurl (which agl didn't
> want in the security hot path).
>
> For NSS, it also compares if the IPv4-as-IPv6 (::ffff:192.168.0.1)
> matches: if |hostname| is an IPv4 address and the subjectAltName is an
> IPv6 address, or the inverse.
>
> For Apple, it lacks any support for IPv6 matching (I'd mention the Radar
> # but Apple's bugreporter hasn't been working for me tonight)
>
> http://codereview.chromium.org/4184004/diff/33001/22005#newcode199
> net/base/x509_openssl_util.cc:199: // is finalized, we may need to
> update this check to use MatchPattern.
> @wtc: here
>
>
> http://codereview.chromium.org/4184004/show
>

[Ryan Sleevi, 2010-11-03 22:35:04]

On 2010/11/03 18:46:08, wtc wrote:
> On 2010/11/03 03:42:21, rsleevi wrote:
> >
> > The question posed here echoed the question posed on the certid list, where
> > there are two options for matching wildcard certificates:
> >   Option 1) Strict RFC 2818 compliance, which allows for situations such as
f*
> > matching "foo" or fo*ar matching "foobar"
> 
> RFC 2818 does not give an example of fo*ar matching "foobar".
> Could you clarify this point?

That interpretation of RFC 2818 came from Peter Saint-Andre on certid when
discussing the talking points of -09, an interpretation of RFC 2818 which was
shared by others on the list. While not listed in the examples, RFC 2818 simply
stated that the wildcard could match label fragments.

Two choice messages where that interpretation was supported:
http://www.ietf.org/mail-archive/web/certid/current/msg00454.html
http://www.ietf.org/mail-archive/web/certid/current/msg00476.html

> 
> Implementing Option 2 is fine by me.  rsleevi, do you know why
> draft-saintandre-tls-server-id-check changed to specify Option 1?
> To reflect what current browsers actually support?  This is what NSS
> does:
>
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/c...

There were some browser tests done as to what is actually implemented -
http://www.ietf.org/mail-archive/web/certid/current/msg00472.html

The language -10 was changed from the -09 language specifically as the result of
this discussion thread:
http://www.ietf.org/mail-archive/web/certid/current/threads.html#00452 . The
conclusion reached was that, as a BCP for protocols using TLS, it's NOT
RECOMMENDED, but as it does not replace/update any existing deployed protocols,
it remains permissible.

Further, the argument was advanced that, for TLS stacks, "full" matching should
be implemented (or controllable), since until things like RFC 2818 are updated,
the language does support the more complex matching.

http://www.ietf.org/mail-archive/web/certid/current/msg00483.html

That's why I suggested you or agl should make the call - it's mostly a question
about whether it's better to strictly interpret RFC 2818 (as it appears Opera
has), or to go for a more minimalistic approach. The argument that it favors
"security" to only allow terminal wildcards was generally disagreed with after
discussion on certid - f*.bar.com was not seen as any more secure than
fo*ar.bar.com

[joth, 2010-11-12 18:55:22]

Just got catch to follow up.
As discussed with wtc & agl, I have trimmed this back to be a conservative -09
draft name verifier w.r.t. wildcards, removed any attempt at IP address support
(deferred to http://crbug.com/62973) as this is sufficient for what I need right
now.

@rsleevi - quick heads up, when you land the root-cert loading change we'll
probably need to add a special case to allow hostname = 127.0.0.1, so other
tests can connect to the test server. (depending who get there first :)

Cheers!
Joth

http://codereview.chromium.org/4184004/diff/33001/net/base/x509_certificate_o...
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/4184004/diff/33001/net/base/x509_certificate_o...
net/base/x509_certificate_openssl.cc:382: // TODO(joth): We should fetch the
subjectAltNames directly rather than via
On 2010/11/03 00:29:49, wtc wrote:
> Please file a bug report for this work.

Done.

http://codereview.chromium.org/4184004/diff/33001/net/base/x509_openssl_util.cc
File net/base/x509_openssl_util.cc (right):

http://codereview.chromium.org/4184004/diff/33001/net/base/x509_openssl_util....
net/base/x509_openssl_util.cc:120: // and also square braces and colons for IPv6
addresses.
On 2010/11/03 03:42:21, rsleevi wrote:
> BUG: I'm not sure that this function should consider IP address matches as
> valid. RFC 2818, and by transitive properties,
> draft-saintandre-tls-server-id-check, stated
> 
> "   In some cases, the URI is specified as an IP address rather than a
>    hostname. In this case, the iPAddress subjectAltName must be present
>    in the certificate and must exactly match the IP in the URI."
> 
> So if |hostname| is supplied as an IP address, then it should only match if a
> subjectAltName is present. Since you've TODO'd the SAN support, I think it'd
be
> appropriate here to return FALSE if |hostname| is an IP.
> 
> I'm wondering if there is a "blessed" IP canonicalization, since net_util's
> ParseIPLiteralToNumber depends on googleurl (which agl didn't want in the
> security hot path).
> 
> For NSS, it also compares if the IPv4-as-IPv6 (::ffff:192.168.0.1) matches: if
> |hostname| is an IPv4 address and the subjectAltName is an IPv6 address, or
the
> inverse. 
> 
> For Apple, it lacks any support for IPv6 matching (I'd mention the Radar # but
> Apple's bugreporter hasn't been working for me tonight)

Done.

http://codereview.chromium.org/4184004/diff/33001/net/base/x509_openssl_util....
net/base/x509_openssl_util.cc:120: // and also square braces and colons for IPv6
addresses.
On 2010/11/03 00:29:49, wtc wrote:
> IPv6 address literals passed to this function should not contain square braces
> [].

Done.
Removed all attempt at IP support, for now.

http://codereview.chromium.org/4184004/diff/33001/net/base/x509_openssl_util....
net/base/x509_openssl_util.cc:168: if (host_domain.length() == registry_length +
1) {
On 2010/11/03 00:29:49, wtc wrote:
> Nit: omit curly braces {} for one-liner "if" bodies.

Done.

http://codereview.chromium.org/4184004/diff/33001/net/base/x509_openssl_util....
net/base/x509_openssl_util.cc:199: // is finalized, we may need to update this
check to use MatchPattern.
On 2010/11/03 03:42:21, rsleevi wrote:
> @wtc: here

Done.

http://codereview.chromium.org/4184004/diff/33001/net/base/x509_openssl_util.h
File net/base/x509_openssl_util.h (right):

http://codereview.chromium.org/4184004/diff/33001/net/base/x509_openssl_util....
net/base/x509_openssl_util.h:37: // TLS name matching rules. The members of
|cert_names| must have been
On 2010/11/03 00:29:49, wtc wrote:
> Nit: cite the RFC or Internet-Draft that specifies the TLS name matching
rules.

Done.

http://codereview.chromium.org/4184004/diff/33001/net/base/x509_openssl_util_...
File net/base/x509_openssl_util_unittest.cc (right):

http://codereview.chromium.org/4184004/diff/33001/net/base/x509_openssl_util_...
net/base/x509_openssl_util_unittest.cc:41: { false, "192.168.1.11", "*.168.1.11"
},
On 2010/11/03 00:29:49, wtc wrote:
> It's important that a wildcard '*' not match IP addresses.
> '*' is only allowed in a pattern for DNS names.

The 'false' indicates these shouldn't match.

http://codereview.chromium.org/4184004/diff/33001/net/base/x509_openssl_util_...
net/base/x509_openssl_util_unittest.cc:65: { true,
"[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]",
On 2010/11/03 00:29:49, wtc wrote:
> Standalone IPv6 address literals should not include the square brackets [].
> 
> Square brackets are only needed when an IPv6 address literal is part of a URL.

Removed IPv6 non-support for now (crbug.com/62973)

http://codereview.chromium.org/4184004/diff/33001/net/base/x509_openssl_util_...
net/base/x509_openssl_util_unittest.cc:79: protected:
On 2010/11/03 00:29:49, wtc wrote:
> Nit: delete 'protected'.

Done.

http://codereview.chromium.org/4184004/diff/33001/net/base/x509_openssl_util_...
net/base/x509_openssl_util_unittest.cc:89: const bool result =
VerifyHostname(test_data.hostname, cert_names);
On 2010/11/03 00:29:49, wtc wrote:
> Nit: I think it is excessive to use a 'const' local variable (and a 'const'
> function
> parameter that's passed by value) like this.
> 
> I know there are proponents of this style, so I'll just express my opinion
once.

Done.

[agl, 2010-11-15 17:55:04]

LGTM

http://codereview.chromium.org/4184004/diff/44001/net/base/x509_openssl_util.cc
File net/base/x509_openssl_util.cc (right):

http://codereview.chromium.org/4184004/diff/44001/net/base/x509_openssl_util....
net/base/x509_openssl_util.cc:131: it != hostname.end(); ++it) {
indentation looks to be off by a space here.

http://codereview.chromium.org/4184004/diff/44001/net/base/x509_openssl_util....
net/base/x509_openssl_util.cc:198: // the wildcard to appear anywhere in the
leftmost  label, rather than
double space here.

http://codereview.chromium.org/4184004/diff/44001/net/base/x509_openssl_util....
net/base/x509_openssl_util.cc:199: // requiring it to be the only character. See
'See' what?

[wtc, 2010-11-16 00:15:21]

joth: I merely checked the API and the issue that
GetDNSNames performs a lossy conversion.  I have one
comment related to that issue below.

Since agl has reviewed the CL, you can check it in.

http://codereview.chromium.org/4184004/diff/44001/net/base/x509_certificate_o...
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/4184004/diff/44001/net/base/x509_certificate_o...
net/base/x509_certificate_openssl.cc:108: if (name->type == GEN_DNS) {
The ParseSubjectAltNames function only includes DNS names,
so it will also need to be enhanced to include IP addresses.

[joth, 2010-11-16 14:01:10]

Thanks both, I've landed this. I'll look into what's needed to get it working
with the test server.

http://codereview.chromium.org/4184004/diff/44001/net/base/x509_certificate_o...
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/4184004/diff/44001/net/base/x509_certificate_o...
net/base/x509_certificate_openssl.cc:108: if (name->type == GEN_DNS) {
On 2010/11/16 00:15:21, wtc wrote:
> The ParseSubjectAltNames function only includes DNS names,
> so it will also need to be enhanced to include IP addresses.

Yes. Hopefully my TODO on line 382, plus details in the linked bug, should cover
this.

http://codereview.chromium.org/4184004/diff/44001/net/base/x509_openssl_util.cc
File net/base/x509_openssl_util.cc (right):

http://codereview.chromium.org/4184004/diff/44001/net/base/x509_openssl_util....
net/base/x509_openssl_util.cc:131: it != hostname.end(); ++it) {
On 2010/11/15 17:55:04, agl wrote:
> indentation looks to be off by a space here.

Done.

http://codereview.chromium.org/4184004/diff/44001/net/base/x509_openssl_util....
net/base/x509_openssl_util.cc:198: // the wildcard to appear anywhere in the
leftmost  label, rather than
On 2010/11/15 17:55:04, agl wrote:
> double space here.

Done.

http://codereview.chromium.org/4184004/diff/44001/net/base/x509_openssl_util....
net/base/x509_openssl_util.cc:199: // requiring it to be the only character. See
On 2010/11/15 17:55:04, agl wrote:
> 'See' what?

Done.
See http://crbug.com/60719