On 1/28/15 6:48 PM, Evgeny Kotkov wrote:
> I grepped everything between BEGIN/END certificate markers and (with a bit
> of post-processing) plugged the results into the 'x509_test cert_tests[]'
> data set. As a consequence, the PEM → DER conversion happened within the
> test_x509_parse_cert() test.
I've attached a patch that adds an x509-parser command line tool that lets you
use our X.509 parser easily against arbitrary files (or stdin if you don't pass
a file on the command line). Which makes it quick and easy to test stuff like
this.
> As it turns out, almost every failing certificate has X.509 Version: 1 (0x0),
> but also happens to have V3 extensions. These are ill-formed per RFC 5280
> s. 4.1 [1], but we could encounter them in the real world. For instance,
> OpenSSL 1.0.1f does produce such certificates when signing end-entities
> with the ee.cnf config from [2] (might be a bug). Googling shows at least
> one thread with a person having an X.509 V1 certificate with V3 extensions.
>
> I tend to think that being too strict in this aspect might be harmful. We
> only want to display data in a certificate, and I think we should be able
> to parse these certificates.
Yup I'd come to the same conclusion. I was writing up an email but you beat me
to it. Being liberal in what we accept here seems to be fine.
> I came up with the attached patch for /branches/svn-auth-x509.
Patch is fine with me. Although I'd prefer that the subject on the test
includes something that describes what it's for. In this case I'd put
www.example.com in the SAN and put v1withv3ext.example.com in the CN.
> Now, the only remaining fail is the 'fail-2.cer.txt' from my previous e-mail,
> which has multiple AttributeTypeAndValue entries in RelativeDistinguishedName
> (RDN). I am not yet sure why do we fail to parse the certificate, but the
> certificate itself is structurally valid per RFC 5280 s. 4.1.2.4 [3].
We don't parse it because the parser is just wrong in this case. Something we
inherited from TropicSSL and I didn't realize was wrong. That said the
specific certificate we're talking about is of a very questionable formatting.
However, I agree with Google's view [1] that it's probably not something worth
worrying about because the subject/issuer is really only used for display or
equality testing in our context that this shouldn't be treated as a serious
error.
I've attached a patch that fixes this. Which would have this commit message:
[[[
Fix the X.509 parser to support multiple Relative Distinguished Names (RDN).
Certificates of this nature can be somewhat questionable as to their validity,
but for our purposes it's irrelevant and sometimes people generate certificates
this way. So simply accept them and ignore the minor semantic difference.
* subversion/libsvn_subr/x509parse.c
(x509_get_attribute): New function.
(x509_get_name): Remove the code that went into x509_get_attribute() and
iterate over the members of the RDN set. Adjust some variables and
comments variables to be clearer in the process.
* subversion/tests/libsvn_subr/x509-test.c
(cert_tests): Add the cert from Chromium's test suite for this.
]]]
I'd like to generate our own certificate for this test but I haven't tried to
figure out how to generate such a certificate.
[1] https://code.google.com/p/chromium/issues/detail?id=101009
Index: build.conf
===================================================================
--- build.conf (revision 1654359)
+++ build.conf (working copy)
@@ -1619,3 +1619,10 @@
path = tools/dev/svnraisetreeconflict
libs = libsvn_wc libsvn_subr apriconv apr
install = tools
+
+[x509-parser]
+type = exe
+path = tools/dev
+sources = x509-parser.c
+install = tools
+libs = libsvn_subr apr
Index: tools/dev/x509-parser.c
===================================================================
--- tools/dev/x509-parser.c (nonexistent)
+++ tools/dev/x509-parser.c (working copy)
@@ -0,0 +1,178 @@
+/* x509-parser.c -- print human readable info from an X.509 certificate
+ *
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ */
+
+#include "svn_pools.h"
+#include "svn_cmdline.h"
+#include "svn_string.h"
+#include "svn_dirent_uri.h"
+#include "svn_io.h"
+#include "svn_base64.h"
+#include "svn_x509.h"
+#include "svn_time.h"
+
+#include "svn_private_config.h"
+
+#define PEM_BEGIN_CERT "-----BEGIN CERTIFICATE-----"
+#define PEM_END_CERT "-----END CERTIFICATE-----"
+
+static svn_error_t *
+show_cert(const svn_string_t *der_cert, apr_pool_t *scratch_pool)
+{
+ svn_x509_certinfo_t *certinfo;
+ const apr_array_header_t *hostnames;
+
+ SVN_ERR(svn_x509_parse_cert(&certinfo, der_cert->data, der_cert->len,
+ scratch_pool, scratch_pool));
+
+ SVN_ERR(svn_cmdline_printf(scratch_pool, _("Subject: %s\n"),
+ svn_x509_certinfo_get_subject(certinfo,
scratch_pool)));
+ SVN_ERR(svn_cmdline_printf(scratch_pool, _("Valid from: %s\n"),
+ svn_time_to_human_cstring(
+ svn_x509_certinfo_get_valid_from(certinfo),
+ scratch_pool)));
+ SVN_ERR(svn_cmdline_printf(scratch_pool, _("Valid until: %s\n"),
+ svn_time_to_human_cstring(
+ svn_x509_certinfo_get_valid_to(certinfo),
+ scratch_pool)));
+ SVN_ERR(svn_cmdline_printf(scratch_pool, _("Issuer: %s\n"),
+ svn_x509_certinfo_get_issuer(certinfo,
scratch_pool)));
+ SVN_ERR(svn_cmdline_printf(scratch_pool, _("Fingerprint: %s\n"),
+ svn_checksum_to_cstring_display(
+ svn_x509_certinfo_get_digest(certinfo),
+ scratch_pool)));
+
+ hostnames = svn_x509_certinfo_get_hostnames(certinfo);
+ if (hostnames && !apr_is_empty_array(hostnames))
+ {
+ int i;
+ svn_stringbuf_t *buf = svn_stringbuf_create_empty(scratch_pool);
+ for (i = 0; i < hostnames->nelts; ++i)
+ {
+ const char *hostname = APR_ARRAY_IDX(hostnames, i, const char*);
+ if (i > 0)
+ svn_stringbuf_appendbytes(buf, ", ", 2);
+ svn_stringbuf_appendbytes(buf, hostname, strlen(hostname));
+ }
+ SVN_ERR(svn_cmdline_printf(scratch_pool, _("Hostnames: %s\n"),
+ buf->data));
+ }
+
+ return SVN_NO_ERROR;
+}
+
+static svn_boolean_t
+is_der_cert(const svn_string_t *raw)
+{
+ /* really simplistic fingerprinting of a DER. By definition it must
+ * start with an ASN.1 tag of a constructed (0x20) sequence (0x10).
+ * It's somewhat unfortunate that 0x30 happens to also come out to the
+ * ASCII for '0' which may mean this will create false positives. */
+ return raw->data[0] == 0x30 ? TRUE : FALSE;
+}
+
+static svn_error_t *
+get_der_cert_from_stream(const svn_string_t **der_cert, svn_stream_t *in,
+ apr_pool_t *pool)
+{
+ svn_string_t *raw;
+ SVN_ERR(svn_string_from_stream(&raw, in, pool, pool));
+
+ *der_cert = NULL;
+
+ /* look for a DER cert */
+ if (is_der_cert(raw))
+ {
+ *der_cert = raw;
+ return SVN_NO_ERROR;
+ }
+ else
+ {
+ const svn_string_t *base64_decoded;
+ const char *start, *end;
+
+ /* Try decoding as base64 without headers */
+ base64_decoded = svn_base64_decode_string(raw, pool);
+ if (base64_decoded && is_der_cert(base64_decoded))
+ {
+ *der_cert = base64_decoded;
+ return SVN_NO_ERROR;
+ }
+
+ /* Try decoding as a PEM with begining and ending headers. */
+ start = strstr(raw->data, PEM_BEGIN_CERT);
+ end = strstr(raw->data, PEM_END_CERT);
+ if (start && end && end > start)
+ {
+ svn_string_t *encoded;
+
+ start += sizeof(PEM_BEGIN_CERT) - 1;
+ end -= 1;
+ encoded = svn_string_ncreate(start, end - start, pool);
+ base64_decoded = svn_base64_decode_string(encoded, pool);
+ if (is_der_cert(base64_decoded))
+ {
+ *der_cert = base64_decoded;
+ return SVN_NO_ERROR;
+ }
+ }
+ }
+
+ return svn_error_create(SVN_ERR_X509_CERT_INVALID_PEM, NULL,
+ _("Couldn't find certificate in input data"));
+}
+
+int main (int argc, const char *argv[])
+{
+ apr_pool_t *pool = NULL;
+ svn_error_t *err;
+ svn_stream_t *in;
+
+ apr_initialize();
+ atexit(apr_terminate);
+
+ pool = svn_pool_create(NULL);
+
+ if (argc == 2)
+ {
+ const char *target = svn_dirent_canonicalize(argv[1], pool);
+ err = svn_stream_open_readonly(&in, target, pool, pool);
+ }
+ else if (argc == 1)
+ {
+ err = svn_stream_for_stdin(&in, pool);
+ }
+ else
+ err = svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL, _("Too many
arguments"));
+
+ if (!err)
+ {
+ const svn_string_t *der_cert;
+ err = get_der_cert_from_stream(&der_cert, in, pool);
+ if (!err)
+ err = show_cert(der_cert, pool);
+ }
+
+ if (err)
+ return svn_cmdline_handle_exit_error(err, pool, "x509-parser: ");
+
+ return 0;
+}
Property changes on: tools/dev/x509-parser.c
___________________________________________________________________
Added: svn:eol-style
## -0,0 +1 ##
+native
\ No newline at end of property
Index: subversion/libsvn_subr/x509parse.c
===================================================================
--- subversion/libsvn_subr/x509parse.c (revision 1654359)
+++ subversion/libsvn_subr/x509parse.c (working copy)
@@ -280,9 +280,6 @@ x509_get_alg(const unsigned char **p, const unsign
}
/*
- * RelativeDistinguishedName ::=
- * SET OF AttributeTypeAndValue
- *
* AttributeTypeAndValue ::= SEQUENCE {
* type AttributeType,
* value AttributeValue }
@@ -292,32 +289,20 @@ x509_get_alg(const unsigned char **p, const unsign
* AttributeValue ::= ANY DEFINED BY AttributeType
*/
static svn_error_t *
-x509_get_name(const unsigned char **p, const unsigned char *end,
- x509_name * cur, apr_pool_t *result_pool)
+x509_get_attribute(const unsigned char **p, const unsigned char *end,
+ x509_name *cur, apr_pool_t *result_pool)
{
svn_error_t *err;
ptrdiff_t len;
- const unsigned char *end2;
x509_buf *oid;
x509_buf *val;
- err = asn1_get_tag(p, end, &len, ASN1_CONSTRUCTED | ASN1_SET);
+ err = asn1_get_tag(p, end, &len, ASN1_CONSTRUCTED | ASN1_SEQUENCE);
if (err)
return svn_error_create(SVN_ERR_X509_CERT_INVALID_NAME, err, NULL);
- end2 = end;
end = *p + len;
- err = asn1_get_tag(p, end, &len, ASN1_CONSTRUCTED | ASN1_SEQUENCE);
- if (err)
- return svn_error_create(SVN_ERR_X509_CERT_INVALID_NAME, err, NULL);
-
- if (*p + len != end)
- {
- err = svn_error_create(SVN_ERR_ASN1_LENGTH_MISMATCH, NULL, NULL);
- return svn_error_create(SVN_ERR_X509_CERT_INVALID_NAME, err, NULL);
- }
-
oid = &cur->oid;
err = asn1_get_tag(p, end, &oid->len, ASN1_OID);
@@ -360,15 +345,54 @@ static svn_error_t *
return svn_error_create(SVN_ERR_X509_CERT_INVALID_NAME, err, NULL);
}
+ return SVN_NO_ERROR;
+}
+
+/*
+ * RelativeDistinguishedName ::=
+ * SET SIZE (1..MAX) OF AttributeTypeAndValue
+ */
+static svn_error_t *
+x509_get_name(const unsigned char **p, const unsigned char *name_end,
+ x509_name *name, apr_pool_t *result_pool)
+{
+ svn_error_t *err;
+ ptrdiff_t len;
+ const unsigned char *set_end;
+ x509_name *cur = NULL;
+
+ err = asn1_get_tag(p, name_end, &len, ASN1_CONSTRUCTED | ASN1_SET);
+ if (err)
+ return svn_error_create(SVN_ERR_X509_CERT_INVALID_NAME, err, NULL);
+
+ set_end = *p + len;
+
/*
- * recurse until end of SEQUENCE is reached
+ * iterate until the end of the SET is reached
*/
- if (*p == end2)
+ while (*p < set_end)
+ {
+ if (!cur)
+ {
+ cur = name;
+ }
+ else
+ {
+ cur->next = apr_palloc(result_pool, sizeof(x509_name));
+ cur = cur->next;
+ }
+ SVN_ERR(x509_get_attribute(p, set_end, cur, result_pool));
+ }
+
+ /*
+ * recurse until end of SEQUENCE (name) is reached
+ */
+ if (*p == name_end)
return SVN_NO_ERROR;
cur->next = apr_palloc(result_pool, sizeof(x509_name));
- return svn_error_trace(x509_get_name(p, end2, cur->next, result_pool));
+ return svn_error_trace(x509_get_name(p, name_end, cur->next, result_pool));
}
/* Retrieve the date from the X.509 cert data between *P and END in either
Index: subversion/tests/libsvn_subr/x509-test.c
===================================================================
--- subversion/tests/libsvn_subr/x509-test.c (revision 1654989)
+++ subversion/tests/libsvn_subr/x509-test.c (working copy)
@@ -437,6 +437,36 @@ static struct x509_test cert_tests[] = {
"x509v1.example.com",
"5730dd65a7f77fdf0dfd90e5a53119f38854af29"
},
+ /* X.509 certificate with multiple Relative Distinguished Names
+ * Borrowed form the Chromium test suite see thier bug here
+ * https://code.google.com/p/chromium/issues/detail?id=101009
+ */
+ { "MIICsDCCAhmgAwIBAgIJAO9sL1fZ/VoPMA0GCSqGSIb3DQEBBQUAMHExbzAJBgNV"
+ "BAYTAlVTMA8GA1UECgwIQ2hyb21pdW0wFgYKCZImiZPyLGQBGRYIQ2hyb21pdW0w"
+ "GgYDVQQDDBNNdWx0aXZhbHVlIFJETiBUZXN0MB0GA1UECwwWQ2hyb21pdW0gbmV0"
+ "X3VuaXR0ZXN0czAeFw0xMTEyMDIwMzQ3MzlaFw0xMjAxMDEwMzQ3MzlaMHExbzAJ"
+ "BgNVBAYTAlVTMA8GA1UECgwIQ2hyb21pdW0wFgYKCZImiZPyLGQBGRYIQ2hyb21p"
+ "dW0wGgYDVQQDDBNNdWx0aXZhbHVlIFJETiBUZXN0MB0GA1UECwwWQ2hyb21pdW0g"
+ "bmV0X3VuaXR0ZXN0czCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEAnSMQ7YeC"
+ "sOuk+0n128F7TfDtG/X48sG10oTe65SC8N6LBLfo7YYiQZlWVHEzjsFpaiv0dx4k"
+ "cIFbVghXAky/r5qgM1XiAGuzzFw7R27cBTC9DPlRwHArP3CiEKO3iz8i+qu9x0il"
+ "/9N70LcSSAu/kGLxikDbHRoM9d2SKhy2LGsCAwEAAaNQME4wHQYDVR0OBBYEFI1e"
+ "cfoqc7qfjmMyHF2rh9CrR6u3MB8GA1UdIwQYMBaAFI1ecfoqc7qfjmMyHF2rh9Cr"
+ "R6u3MAwGA1UdEwQFMAMBAf8wDQYJKoZIhvcNAQEFBQADgYEAGKwN01A47nxVHOkw"
+ "wFdbT8t9FFkY3pIg5meoqO3aATNaSEzkZoUljWtWgWfzr+n4ElwZBxeYv9cPurVk"
+ "a+wXygzWzsOzCUMKBI/aS8ijRervyvh6LpGojPGn1HttnXNLmhy+BLECs7cq6f0Z"
+ "hvImrEWhD5uZGlOxaZk+bFEjQHA=",
+ "C=US, O=Chromium, 0.9.2342.19200300.100.1.25=Chromium, "
+ "CN=Multivalue RDN Test, OU=Chromium net_unittests",
+ "2.5.4.6 2.5.4.10 0.9.2342.19200300.100.1.25 2.5.4.3 2.5.4.11",
+ "C=US, O=Chromium, 0.9.2342.19200300.100.1.25=Chromium, "
+ "CN=Multivalue RDN Test, OU=Chromium net_unittests",
+ "2.5.4.6 2.5.4.10 0.9.2342.19200300.100.1.25 2.5.4.3 2.5.4.11",
+ "2011-12-02T03:47:39.000000Z",
+ "2012-01-01T03:47:39.000000Z",
+ NULL,
+ "99302ca2824f585a117bb41302a388daa0519765"
+ },
{ NULL }
};
