You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by br...@apache.org on 2014/02/07 05:32:51 UTC
svn commit: r1565531 - /subversion/trunk/subversion/libsvn_ra_serf/util.c
Author: breser
Date: Fri Feb 7 04:32:50 2014
New Revision: 1565531
URL: http://svn.apache.org/r1565531
Log:
ra_serf: Do not compare the CommonName of a certificate if there are
subjectAltName extensions of the DNS type when validating the certificate
as per RFC 2818.
* subversion/libsvn_ra_serf/util.c
(ssl_server_certificate): add a boolean to know if there were san
entries, skip the CN match if there were and move the failure
flag outside of the CN match code.
Modified:
subversion/trunk/subversion/libsvn_ra_serf/util.c
Modified: subversion/trunk/subversion/libsvn_ra_serf/util.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/util.c?rev=1565531&r1=1565530&r2=1565531&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/util.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/util.c Fri Feb 7 04:32:50 2014
@@ -225,6 +225,7 @@ ssl_server_cert(void *baton, int failure
### This should really be handled by serf, which should pass an error
for this case, but that has backwards compatibility issues. */
apr_array_header_t *san;
+ svn_boolean_t found_san_entry;
serf_cert = serf_ssl_cert_certificate(cert, scratch_pool);
@@ -232,6 +233,7 @@ ssl_server_cert(void *baton, int failure
/* Try to find matching server name via subjectAltName first... */
if (san) {
int i;
+ found_san_entry = san->nelts > 0;
for (i = 0; i < san->nelts; i++) {
const char *s = APR_ARRAY_IDX(san, i, const char*);
if (apr_fnmatch(s, conn->session->session_url.hostname,
@@ -243,8 +245,11 @@ ssl_server_cert(void *baton, int failure
}
}
- /* Match server certificate CN with the hostname of the server */
- if (!found_matching_hostname)
+ /* Match server certificate CN with the hostname of the server iff
+ * we didn't find any subjectAltName fields and try to match them.
+ * Per RFC 2818 they are authoritative if present and CommonName
+ * should be ignored. */
+ if (!found_matching_hostname && !found_san_entry)
{
const char *hostname = NULL;
@@ -253,13 +258,16 @@ ssl_server_cert(void *baton, int failure
if (subject)
hostname = svn_hash_gets(subject, "CN");
- if (!hostname
- || apr_fnmatch(hostname, conn->session->session_url.hostname,
- APR_FNM_PERIOD | APR_FNM_CASE_BLIND) != APR_SUCCESS)
+ if (hostname
+ && apr_fnmatch(hostname, conn->session->session_url.hostname,
+ APR_FNM_PERIOD | APR_FNM_CASE_BLIND) == APR_SUCCESS)
{
- svn_failures |= SVN_AUTH_SSL_CNMISMATCH;
+ found_matching_hostname = 1;
}
}
+
+ if (!found_matching_hostname)
+ svn_failures |= SVN_AUTH_SSL_CNMISMATCH;
}
if (!svn_failures)
Re: svn commit: r1565531 - /subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Julian Foad <ju...@btopenworld.com>.
Ben fixed this in r1568349. Thanks, Ben.
- Julian
Julian Foad wrote:
> This commit is causing a change of behaviour for me against the Apache svn repo.
[...]
>> + svn_boolean_t found_san_entry;
>>
>> @@ -232,6 +233,7 @@ ssl_server_cert(void *baton, int failure
>> /* Try to find matching server name via subjectAltName first... */
>> if (san) {
>
> Here, "san" is false (no SubjectAltName found), so found_san_entry
> remains uninitialized...
>
>> int i;
>> + found_san_entry = san->nelts > 0;
>> for (i = 0; i < san->nelts; i++) {
>> @@ -243,8 +245,11 @@ ssl_server_cert(void *baton, int failure
>> }
>> }
>>
>> - /* Match server certificate CN with the hostname of the server */
>> - if (!found_matching_hostname)
>> + /* Match server certificate CN with the hostname of the server iff
>> + * we didn't find any subjectAltName fields and try to match them.
>> + * Per RFC 2818 they are authoritative if present and CommonName
>> + * should be ignored. */
>> + if (!found_matching_hostname && !found_san_entry)
>> {
>
> ... and here we skip this block because found_san_entry is -134885336 i.e.
> "true".
>
> This results in the certificate being considered invalid.
>
> - Julian
Re: svn commit: r1565531 - /subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Julian Foad <ju...@btopenworld.com>.
This commit is causing a change of behaviour for me against the Apache svn repo. I think there is an error in this code.
The error I get when trying to run a merge into an svn trunk WC from an svn branch is:
$ svn merge --non-interactive ^/subversion/branches/fsfs-ucsnorm/
/home/julianfoad/src/subversion-c/subversion/svn/util.c:548,
/home/julianfoad/src/subversion-c/subversion/libsvn_client/merge.c:11811,
/home/julianfoad/src/subversion-c/subversion/libsvn_client/merge.c:12465,
/home/julianfoad/src/subversion-c/subversion/libsvn_client/ra.c:474,
/home/julianfoad/src/subversion-c/subversion/libsvn_client/ra.c:453,
/home/julianfoad/src/subversion-c/subversion/libsvn_ra/ra_loader.c:485: (apr_err=SVN_ERR_RA_CANNOT_CREATE_SESSION)
svn: E170013: Unable to connect to a repository at URL 'https://svn.apache.org/repos/asf/subversion/trunk'
/home/julianfoad/src/subversion-c/subversion/libsvn_ra_serf/serf.c:593,
/home/julianfoad/src/subversion-c/subversion/libsvn_ra_serf/options.c:491,
/home/julianfoad/src/subversion-c/subversion/libsvn_ra_serf/util.c:941,
/home/julianfoad/src/subversion-c/subversion/libsvn_ra_serf/util.c:915,
/home/julianfoad/src/subversion-c/subversion/libsvn_ra_serf/util.c:880,
/home/julianfoad/src/subversion-c/subversion/libsvn_ra_serf/util.c:431,
/home/julianfoad/src/subversion-c/subversion/libsvn_ra_serf/util.c:413: (apr_err=SVN_ERR_RA_SERF_SSL_CERT_UNTRUSTED)
svn: E230001: Server SSL certificate verification failed: certificate issued for a different hostname
> Author: breser
> Date: Fri Feb 7 04:32:50 2014
> New Revision: 1565531
>
> URL: http://svn.apache.org/r1565531
> Log:
> ra_serf: Do not compare the CommonName of a certificate if there are
> subjectAltName extensions of the DNS type when validating the certificate
> as per RFC 2818.
>
> * subversion/libsvn_ra_serf/util.c
> (ssl_server_certificate): add a boolean to know if there were san
> entries, skip the CN match if there were and move the failure
> flag outside of the CN match code.
>
> Modified:
> subversion/trunk/subversion/libsvn_ra_serf/util.c
>
> Modified: subversion/trunk/subversion/libsvn_ra_serf/util.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/util.c?rev=1565531&r1=1565530&r2=1565531&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_ra_serf/util.c (original)
> +++ subversion/trunk/subversion/libsvn_ra_serf/util.c Fri Feb 7 04:32:50 2014
> @@ -225,6 +225,7 @@ ssl_server_cert(void *baton, int failure
> ### This should really be handled by serf, which should pass an error
> for this case, but that has backwards compatibility issues. */
> apr_array_header_t *san;
> + svn_boolean_t found_san_entry;
>
> serf_cert = serf_ssl_cert_certificate(cert, scratch_pool);
>
> @@ -232,6 +233,7 @@ ssl_server_cert(void *baton, int failure
> /* Try to find matching server name via subjectAltName first... */
> if (san) {
Here, "san" is false (no SubjectAltName found), so found_san_entry remains uninitialized...
> int i;
> + found_san_entry = san->nelts > 0;
> for (i = 0; i < san->nelts; i++) {
> const char *s = APR_ARRAY_IDX(san, i, const char*);
> if (apr_fnmatch(s, conn->session->session_url.hostname,
> @@ -243,8 +245,11 @@ ssl_server_cert(void *baton, int failure
> }
> }
>
> - /* Match server certificate CN with the hostname of the server */
> - if (!found_matching_hostname)
> + /* Match server certificate CN with the hostname of the server iff
> + * we didn't find any subjectAltName fields and try to match them.
> + * Per RFC 2818 they are authoritative if present and CommonName
> + * should be ignored. */
> + if (!found_matching_hostname && !found_san_entry)
> {
... and here we skip this block because found_san_entry is -134885336 i.e. "true".
This results in the certificate being considered invalid.
- Julian
> const char *hostname = NULL;
>
> @@ -253,13 +258,16 @@ ssl_server_cert(void *baton, int failure
> if (subject)
> hostname = svn_hash_gets(subject, "CN");
>
> - if (!hostname
> - || apr_fnmatch(hostname,
> conn->session->session_url.hostname,
> - APR_FNM_PERIOD | APR_FNM_CASE_BLIND) !=
> APR_SUCCESS)
> + if (hostname
> + && apr_fnmatch(hostname,
> conn->session->session_url.hostname,
> + APR_FNM_PERIOD | APR_FNM_CASE_BLIND) ==
> APR_SUCCESS)
> {
> - svn_failures |= SVN_AUTH_SSL_CNMISMATCH;
> + found_matching_hostname = 1;
> }
> }
> +
> + if (!found_matching_hostname)
> + svn_failures |= SVN_AUTH_SSL_CNMISMATCH;
> }
>
> if (!svn_failures)
>
Re: svn commit: r1565531 - /subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Ben Reser <br...@apache.org>.
On 8/1/14 8:23 AM, Ivan Zhakov wrote:
> I think it will be more clear to write code in the following way:
> [[
> san = svn_hash_gets(serf_cert, "subjectAltName");
> /* Match server certificate CN with the hostname of the server iff
> * we didn't find any subjectAltName fields and try to match them.
> * Per RFC 2818 they are authoritative if present and CommonName
> * should be ignored. */
> if (san && san->nelts > 0) {
> int i;
> found_san_entry = ;
> for (i = 0; i < san->nelts; i++) {
> const char *s = APR_ARRAY_IDX(san, i, const char*);
> if (apr_fnmatch(s, conn->session->session_url.hostname,
> APR_FNM_PERIOD | APR_FNM_CASE_BLIND) == APR_SUCCESS)
> {
> found_matching_hostname = 1;
> break;
> }
> }
> }
> else
> {
> const char *hostname = NULL;
>
> subject = serf_ssl_cert_subject(cert, scratch_pool);
>
> if (subject)
> hostname = svn_hash_gets(subject, "CN");
>
> if (hostname
> && apr_fnmatch(hostname, conn->session->session_url.hostname,
> APR_FNM_PERIOD | APR_FNM_CASE_BLIND) ==
> APR_SUCCESS)
> {
> found_matching_hostname = 1;
> }
> }
> ]]
>
> Did I miss something important?
Agreed, committed in r1615272.
Re: svn commit: r1565531 - /subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 7 February 2014 08:32, <br...@apache.org> wrote:
> Author: breser
> Date: Fri Feb 7 04:32:50 2014
> New Revision: 1565531
>
> URL: http://svn.apache.org/r1565531
> Log:
> ra_serf: Do not compare the CommonName of a certificate if there are
> subjectAltName extensions of the DNS type when validating the certificate
> as per RFC 2818.
>
> * subversion/libsvn_ra_serf/util.c
> (ssl_server_certificate): add a boolean to know if there were san
> entries, skip the CN match if there were and move the failure
> flag outside of the CN match code.
>
> Modified:
> subversion/trunk/subversion/libsvn_ra_serf/util.c
>
> Modified: subversion/trunk/subversion/libsvn_ra_serf/util.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/util.c?rev=1565531&r1=1565530&r2=1565531&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_ra_serf/util.c (original)
> +++ subversion/trunk/subversion/libsvn_ra_serf/util.c Fri Feb 7 04:32:50 2014
> @@ -225,6 +225,7 @@ ssl_server_cert(void *baton, int failure
> ### This should really be handled by serf, which should pass an error
> for this case, but that has backwards compatibility issues. */
> apr_array_header_t *san;
> + svn_boolean_t found_san_entry;
>
> serf_cert = serf_ssl_cert_certificate(cert, scratch_pool);
>
> @@ -232,6 +233,7 @@ ssl_server_cert(void *baton, int failure
> /* Try to find matching server name via subjectAltName first... */
> if (san) {
> int i;
> + found_san_entry = san->nelts > 0;
> for (i = 0; i < san->nelts; i++) {
> const char *s = APR_ARRAY_IDX(san, i, const char*);
> if (apr_fnmatch(s, conn->session->session_url.hostname,
> @@ -243,8 +245,11 @@ ssl_server_cert(void *baton, int failure
> }
> }
>
> - /* Match server certificate CN with the hostname of the server */
> - if (!found_matching_hostname)
> + /* Match server certificate CN with the hostname of the server iff
> + * we didn't find any subjectAltName fields and try to match them.
> + * Per RFC 2818 they are authoritative if present and CommonName
> + * should be ignored. */
> + if (!found_matching_hostname && !found_san_entry)
> {
> const char *hostname = NULL;
>
> @@ -253,13 +258,16 @@ ssl_server_cert(void *baton, int failure
> if (subject)
> hostname = svn_hash_gets(subject, "CN");
>
> - if (!hostname
> - || apr_fnmatch(hostname, conn->session->session_url.hostname,
> - APR_FNM_PERIOD | APR_FNM_CASE_BLIND) != APR_SUCCESS)
> + if (hostname
> + && apr_fnmatch(hostname, conn->session->session_url.hostname,
> + APR_FNM_PERIOD | APR_FNM_CASE_BLIND) == APR_SUCCESS)
> {
> - svn_failures |= SVN_AUTH_SSL_CNMISMATCH;
> + found_matching_hostname = 1;
> }
> }
> +
> + if (!found_matching_hostname)
> + svn_failures |= SVN_AUTH_SSL_CNMISMATCH;
> }
>
Hi Ben,
I think it will be more clear to write code in the following way:
[[
san = svn_hash_gets(serf_cert, "subjectAltName");
/* Match server certificate CN with the hostname of the server iff
* we didn't find any subjectAltName fields and try to match them.
* Per RFC 2818 they are authoritative if present and CommonName
* should be ignored. */
if (san && san->nelts > 0) {
int i;
found_san_entry = ;
for (i = 0; i < san->nelts; i++) {
const char *s = APR_ARRAY_IDX(san, i, const char*);
if (apr_fnmatch(s, conn->session->session_url.hostname,
APR_FNM_PERIOD | APR_FNM_CASE_BLIND) == APR_SUCCESS)
{
found_matching_hostname = 1;
break;
}
}
}
else
{
const char *hostname = NULL;
subject = serf_ssl_cert_subject(cert, scratch_pool);
if (subject)
hostname = svn_hash_gets(subject, "CN");
if (hostname
&& apr_fnmatch(hostname, conn->session->session_url.hostname,
APR_FNM_PERIOD | APR_FNM_CASE_BLIND) ==
APR_SUCCESS)
{
found_matching_hostname = 1;
}
}
]]
Did I miss something important?
--
Ivan Zhakov