Fix string downcast to `PrintableString` as issued in #1033 #1641

thomas-dee · 2018-05-18T10:03:38Z

Description

Fix string downcast to PrintableString as issued in #1033.

Implemented mbedtls_asn1_write_any_string allowing to write a string with any given tag
Extended x509_attrs in x509_create.c to handle tag types correctly
Fixed x509_write_name to use mbedtls_asn1_write_any_string using the tag types specified in x509_attrs

Status

READY

Requires Backporting

T.b.d.

…nto development

simonbutcher · 2018-05-21T18:18:06Z

Thanks for the contribution and welcome to the project!

This looks like it's your first contribution and I can't find you in our list of contributors. Unfortunately we have a policy of only accepting contributions with a Contributor’s Licence Agreement (CLA), signed or authorised by yourself, so to be able to accept your PR, we'll need your, or your company's approval of a CLA.

The easiest way to do this is if you create an mbed account and accept this click through agreement which is applicable if this is a personal contribution. Alternatively, you can find a slightly different agreement to sign here, which can be signed and returned to us, and is applicable if you don't want to create an mbed account or alternatively if this is a corporate contribution.

Thanks for your understanding!

thomas-dee · 2018-05-22T05:10:23Z

Ok, I just agreed to the CLA with my new mbed account.

simonbutcher · 2018-05-22T08:36:23Z

Thanks @thomas-dee! Could you please confirm that this is your profile? Thanks.

thomas-dee · 2018-05-22T09:07:26Z

Yes it is. I just linked my GitHub account in the Mbed profile settings.

hanno-becker · 2018-09-18T16:27:58Z

library/x509_create.c

+    { ADD_STRLEN( "serialNumber" ),             MBEDTLS_OID_AT_SERIAL_NUMBER,   MBEDTLS_ASN1_PRINTABLE_STRING },
+    { ADD_STRLEN( "postalAddress" ),            MBEDTLS_OID_AT_POSTAL_ADDRESS,  MBEDTLS_ASN1_PRINTABLE_STRING },
+    { ADD_STRLEN( "postalCode" ),               MBEDTLS_OID_AT_POSTAL_CODE,     MBEDTLS_ASN1_PRINTABLE_STRING },
+    { ADD_STRLEN( "dnQualifier" ),              MBEDTLS_OID_AT_DN_QUALIFIER,    MBEDTLS_ASN1_PRINTABLE_STRING },


@thomas-dee Could you explain the varying choices of encodings here?

As mentioned in line 45 I tried to default to the types given in RFC3280. For the given types it was not clear for me, so I defaulted to PrintableString.

@thomas-dee I overlooked the reference to RFC 3280, thanks for pointing that out.

We should reference the newer RFC 5280 here.

hanno-becker · 2018-09-18T16:38:42Z

library/x509_create.c

 };

-static const char *x509_at_oid_from_name( const char *name, size_t name_len )
+static const x509_attr_descriptor_t *x509_at_oid_from_name( const char *name, size_t name_len )


The name of this function is no longer accurate when it returns the entire descriptor, and not only the OID.

What about x509_at_descr_from_name?

hanno-becker · 2018-09-18T16:59:59Z

library/asn1write.c

@@ -253,34 +253,37 @@ int mbedtls_asn1_write_int( unsigned char **p, unsigned char *start, int val )
    return( (int) len );
 }

-int mbedtls_asn1_write_printable_string( unsigned char **p, unsigned char *start,
-                                 const char *text, size_t text_len )
+int mbedtls_asn1_write_any_string( unsigned char **p, unsigned char *start, int tag,


I don't like the name mbedtls_asn1_write_any_string - what about mbedtls_asn1_write_tagged_string, reflecting the flexibility in the tag?

mbedtls_asn1_write_tagged_string sounds nice.

hanno-becker

Overall the change looks good to me, thanks @thomas-dee for your work!

There are some issues that need to be addressed, though:

The name of the new API function mbedtls_asn1_write_any_string should be changed.
The default choice of string format for RDN's should be explained.
Multiple test suites fail because they compare the output of X.509 writing functions to hardcoded files. Since many RDNs now use the UTF8String format, as
opposed to PrintableString previously, the resulting ASN.1 data is different, and the files containing the expected output need to be updated.

thomas-dee · 2018-09-18T17:50:52Z

@hanno-arm Thanks for your review.

Do you have any ideas how to elaborate the choices of string formats besides referencing RFC3280?

- renamed `mbedtls_asn1_write_any_string` to `mbedtls_asn1_write_tagged_string` - renamed `x509_at_oid_from_name` to `x509_attr_descr_from_name`

hanno-becker · 2018-09-19T10:37:10Z

include/mbedtls/asn1write.h

+ *
+ * \param p         reference to current position pointer
+ * \param start     start of the buffer (for bounds-checking)
+ * \param tag       the tag to write


Suggestion: The tag defining the character encoding used in \p text, e.g. #MBEDTLS_ASN1_UTF8_STRING or #MBEDTLS_ASN1_PRINTABLE_STRING.

hanno-becker · 2018-09-19T10:37:57Z

include/mbedtls/asn1write.h

+ * \param p         reference to current position pointer
+ * \param start     start of the buffer (for bounds-checking)
+ * \param tag       the tag to write
+ * \param text      the text to write


Suggestion: The string to write, encoded according to \p tag.

hanno-becker · 2018-09-19T10:38:24Z

include/mbedtls/asn1write.h

+ * \param start     start of the buffer (for bounds-checking)
+ * \param tag       the tag to write
+ * \param text      the text to write
+ * \param text_len  length of the text


Suggestion: The length of \p text in bytes (not necessarily characters).

hanno-becker · 2018-09-19T10:38:55Z

include/mbedtls/asn1write.h

+ *
+ * \param p         reference to current position pointer
+ * \param start     start of the buffer (for bounds-checking)
+ * \param text      the text to write


Pre-existing, but in line with the above suggestion I'd find it clearer to write The string to write, in UTF-8 encoding. here.

hanno-becker · 2018-09-19T10:39:13Z

include/mbedtls/asn1write.h

+ * \param p         reference to current position pointer
+ * \param start     start of the buffer (for bounds-checking)
+ * \param text      the text to write
+ * \param text_len  length of the text


Pre-existing, but in line with the above suggestion I'd find it clearer to write The length of \p text in bytes. here.

hanno-becker · 2018-09-19T10:41:38Z

include/mbedtls/asn1write.h

+ * \param text      the text to write
+ * \param text_len  length of the text
+ *
+ * \return          the length written or a negative error code


Pre-existing, but it should be clarified that "length" refers to the entire ASN.1 data, so The number of bytes written to the target buffer \c *p on success. Also, a negative error code should be in a separate line, i.e. \return A negative error code on failure.

@thomas-dee Do you mind making these changes while at it?

hanno-becker · 2018-09-19T10:41:54Z

include/mbedtls/asn1write.h

+ * \return          the length written or a negative error code
+ */
+int mbedtls_asn1_write_utf8_string( unsigned char **p, unsigned char *start,
+                                 const char *text, size_t text_len );


The indentation is wrong here.

hanno-becker · 2018-11-02T10:54:33Z

This PR includes #2167 which should be merged first.

hanno-becker · 2018-11-02T12:47:06Z

Travis CI failed because of an unlucky seed leading to the UDP proxy dropping too much - such failures should be rare but in principle expected as long as we don't set a limit on how much the UDP proxy may drop.

hanno-becker · 2018-11-02T12:47:11Z

retest

This commit introduces variants test-ca_utf8.crt, test-ca_printablestring.crt and test-ca_uppercase.crt of tests/data_files/test-ca.crt which differ from test-ca.crt in their choice of string encoding and upper and lower case letters in the DN field. These changes should be immaterial to the recovation check, and three tests are added that crl.pem, which applies to test-ca.crt, is also considered as applying to test-ca_*.crt. The test files were generated using PR Mbed-TLS#1641 which - adds a build instruction for test-ca.crt to tests/data_files/Makefile which allows easy change of the subject DN. - changes the default string format from `PrintableString` to `UTF8String`. Specifically: - `test-ca_utf8.crt` was generated by running `rm test-ca.crt && make test-ca.crt` on PR Mbed-TLS#1641. - `test-ca_uppercase.crt`, too, was generated by running `rm test-ca.crt && make test-ca.crt` on PR Mbed-TLS#1641, after modifying the subject DN line in the build instruction for `test-ca.crt` in `tests/data_files/Makefile`. - `test-ca_printable.crt` is a copy of `test-ca.crt` because at the time of this commit, `PrintableString` is still the default string format.

k-stachowiak

Looks good. I only have one minor remark about preexisting code, also I noticed one commit message claiming to add something, whereas it was actually added in another commit, but in my opinion that is irrelevant.

k-stachowiak · 2018-11-05T15:36:58Z

library/x509_crt.c

+/*
+ * Like memcmp, but case-insensitive and always returns -1 if different
+ */
+static int x509_memcasecmp( const void *s1, const void *s2, size_t len )


This doesn't maintain the law of trichotomy like memcmp does. Therefore, I'd change the documenting comment, or reimplement the function to return adequate values.

@k-stachowiak Thanks for remarking that - however, while I agree that the documentation could be more verbose, it does already remark that the function 'always returns -1 if different`.

This commit introduces variants test-ca_utf8.crt, test-ca_printablestring.crt and test-ca_uppercase.crt of tests/data_files/test-ca.crt which differ from test-ca.crt in their choice of string encoding and upper and lower case letters in the DN field. These changes should be immaterial to the recovation check, and three tests are added that crl.pem, which applies to test-ca.crt, is also considered as applying to test-ca_*.crt. The test files were generated using PR Mbed-TLS#1641 which - adds a build instruction for test-ca.crt to tests/data_files/Makefile which allows easy change of the subject DN. - changes the default string format from `PrintableString` to `UTF8String`. Specifically: - `test-ca_utf8.crt` was generated by running `rm test-ca.crt && make test-ca.crt` on PR Mbed-TLS#1641. - `test-ca_uppercase.crt`, too, was generated by running `rm test-ca.crt && make test-ca.crt` on PR Mbed-TLS#1641, after modifying the subject DN line in the build instruction for `test-ca.crt` in `tests/data_files/Makefile`. - `test-ca_printable.crt` is a copy of `test-ca.crt` because at the time of this commit, `PrintableString` is still the default string format.

k-stachowiak

The new commit looks good. As a side note, I can see, that the commit messages document the data files generation method; I wonder if these are documented equally well elsewhere. If not, then the commit messages could be published somewhere.

simonbutcher · 2018-11-06T15:43:34Z

ChangeLog

+     one using PrintableString and the other UTF8String) or
+     in the choice of upper and lower case. Reported by
+     HenrikRosenquistAndersson in #1784.
+


There needs to be credit to @thomas-dee here also as well as mention of his work in #1033!

simonbutcher · 2018-11-06T15:45:35Z

include/mbedtls/asn1write.h

- * \param p         reference to current position pointer
- * \param start     start of the buffer (for bounds-checking)
- * \param len       the length to write
+ * \note            This function works backwards in data buffer.


Not a blocker, but it should be 'This function works backwards in the data buffer'.

hanno-becker · 2018-11-06T17:13:13Z

@sbutcher-arm Thanks for noticing, indeed this PR never got a ChangeLog entry. Are you adding it during merging?

simonbutcher

LGTM

simonbutcher · 2018-11-06T22:00:23Z

Jenkins CI failed (again) on the BSD timing test, which is a known issue and good enough to be a pass.

In the 2.7 branch, test-ca.crt has all the components of its Subject name encoded as PrintableString, because it's generated with our cert_write program, and our code writes all components that way until Mbed TLS 2.14. But the default RSA SHA-256 certificate, server2-sha256.crt, has the O and CN components of its Issuer name encoded as UTF8String, because it was generated with OpenSSL and that's what OpenSSL does, regardless of how those components were encoded in the CA's Subject name. This triggers some overly strict behaviour in some libraries, most notably NSS and GnuTLS (of interest to us in ssl-opt.sh) which won't recognize the trusted root as a possible parent for the presented certificate, see for example: Mbed-TLS#1033 Fortunately, we have at our disposal a version of test-ca.crt with encodings matching the ones in server2-sha256.crt, in the file test-ca_utf8.crt. So let's append that to gnutls-cli's list of trusted roots, so that it recognizes certs signed by this CA but with the O and CN components as UTF8String. Note: Since Mbed-TLS#1641 was merged (in Mbed TLS 2.14), we changed how we encode those components, so in the 2.16 branch, cert_write generates test-ca.crt with encodings that matches the ones used by openssl when generating server2-sha256.crt, so the issue of gnutls-cli rejecting server2-sha256.crt is specific to the 2.7 branch.

The test certificate used for clients in compat.sh, cert_sha256.crt, had the issuer ON and CN encoded as UTF8String, but the corresponding CA certificate test-ca_cat12.crt had them encoded as PrintableString. The strings matched, which is sufficient according to RFC 5280 §7.1 and RFC 4518 §2.1. However, GnuTLS 3.4.10 requires the strings to have the same encoding, so it did not accept that the certificate issued by UTF8String "PolarSSL Test CA" was validly issued by the PrintableString "PolarSSL Test CA" CA. ebc1f40, merged via Mbed-TLS#1641 and released in Mbed TLS 2.14, updated these certificates. 4f928c0 merged, via Mbed-TLS#2418 fixed this in the 2.7 LTS branch for the SHA-1 certificate which was used at the time. The present commit applies the same fix for the SHA-256 certificate that is now in use. For uniformity, this commit regenerates all the cert_*.crt. Signed-off-by: Gilles Peskine <[email protected]>

server2-sha256.crt had the issuer ON and CN encoded as UTF8String, but the corresponding CA certificate test-ca_cat12.crt had them encoded as PrintableString. The strings matched, which is sufficient according to RFC 5280 §7.1 and RFC 4518 §2.1. However, GnuTLS 3.4.10 requires the strings to have the same encoding, so it did not accept that the UTF8String "PolarSSL Test CA" certificate was signed by the PrintableString "PolarSSL Test CA" CA. Since Mbed TLS 2.14 (specifically ebc1f40 merged via Mbed-TLS#1641), server2-sha256.crt is generated by Mbed TLS's own cert_write program, which emits a PrintableString. In older versions, this file was generated by OpenSSL, which started emitting UTF8String at some point. 4f928c0 merged via Mbed-TLS#2418 fixed this for the SHA-1 certificate which was used at the time. The present commit applies the same fix for the SHA-256 certificate that is now in use. Signed-off-by: Gilles Peskine <[email protected]>

Patater and others added 4 commits May 18, 2018 11:49

Fix string downcast to PrintableString as issued in Mbed-TLS#1033

23f954d

Merge branch 'development' of https://github.com/thomas-dee/mbedtls i…

c5c9aaf

…nto development

fixed missing initializer

c150f0d

fixed segmentation fault

020c823

simonbutcher added enhancement CLA requested component-x509 labels May 21, 2018

simonbutcher added CLA valid and removed CLA requested labels May 22, 2018

simonbutcher added the needs-review Every commit must be reviewed by at least two team members, label Jun 14, 2018

simonbutcher self-assigned this Aug 1, 2018

hanno-becker reviewed Sep 18, 2018

View reviewed changes

hanno-becker suggested changes Sep 18, 2018

View reviewed changes

hanno-becker added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Sep 18, 2018

changes requested by @hanno-arm

eba6c9b

- renamed `mbedtls_asn1_write_any_string` to `mbedtls_asn1_write_tagged_string` - renamed `x509_at_oid_from_name` to `x509_attr_descr_from_name`

hanno-becker reviewed Sep 19, 2018

View reviewed changes

hanno-becker force-pushed the development branch from bf0c60d to 710f203 Compare November 2, 2018 10:53

hanno-becker mentioned this pull request Nov 2, 2018

Fix overly strict DN name comparison when traversing CRLs #2167

Merged

k-stachowiak approved these changes Nov 5, 2018

View reviewed changes

k-stachowiak previously approved these changes Nov 6, 2018

View reviewed changes

simonbutcher previously approved these changes Nov 6, 2018

View reviewed changes

hanno-becker dismissed stale reviews from simonbutcher and k-stachowiak via f42683f November 6, 2018 17:43

Adapt ChangeLog

f143a78

hanno-becker force-pushed the development branch from f42683f to f143a78 Compare November 6, 2018 17:59

k-stachowiak approved these changes Nov 6, 2018

View reviewed changes

simonbutcher approved these changes Nov 6, 2018

View reviewed changes

simonbutcher added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Nov 6, 2018

simonbutcher merged commit f143a78 into Mbed-TLS:development Nov 8, 2018

This was referenced Nov 8, 2018

An encoding tag problem of mbedtls_x509write_crt_der #1860

Closed

PRINTABLE_STRING in signing request prevents wildcard domains #468

Closed

Certificates created are not accepted by curl/firefox #1033

Closed

RonEld mentioned this pull request Jan 7, 2019

fix handling of commas in X509 Strings #784

Closed

gilles-peskine-arm mentioned this pull request Aug 21, 2020

Backport 2.7: Fix compat.sh with ubuntu 16.04 gnutls 2.7 #3594

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix string downcast to `PrintableString` as issued in #1033 #1641

Fix string downcast to `PrintableString` as issued in #1033 #1641

thomas-dee commented May 18, 2018

simonbutcher commented May 21, 2018

thomas-dee commented May 22, 2018

simonbutcher commented May 22, 2018

thomas-dee commented May 22, 2018

hanno-becker Sep 18, 2018

thomas-dee Sep 18, 2018

hanno-becker Sep 18, 2018

hanno-becker Sep 19, 2018

hanno-becker Sep 18, 2018

thomas-dee Sep 18, 2018

hanno-becker Sep 18, 2018

thomas-dee Sep 18, 2018

hanno-becker left a comment

thomas-dee commented Sep 18, 2018

hanno-becker Sep 19, 2018

hanno-becker Sep 19, 2018

hanno-becker Sep 19, 2018

hanno-becker Sep 19, 2018

hanno-becker Sep 19, 2018 •

edited

Loading

hanno-becker Sep 19, 2018

hanno-becker Sep 19, 2018

hanno-becker commented Nov 2, 2018

hanno-becker commented Nov 2, 2018

hanno-becker commented Nov 2, 2018

k-stachowiak left a comment

k-stachowiak Nov 5, 2018 •

edited by hanno-becker

Loading

hanno-becker Nov 6, 2018

k-stachowiak left a comment

simonbutcher Nov 6, 2018

simonbutcher Nov 6, 2018

hanno-becker commented Nov 6, 2018

simonbutcher left a comment

simonbutcher commented Nov 6, 2018

Fix string downcast to PrintableString as issued in #1033 #1641

Fix string downcast to PrintableString as issued in #1033 #1641

Conversation

thomas-dee commented May 18, 2018

Description

Status

Requires Backporting

simonbutcher commented May 21, 2018

thomas-dee commented May 22, 2018

simonbutcher commented May 22, 2018

thomas-dee commented May 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanno-becker left a comment

Choose a reason for hiding this comment

thomas-dee commented Sep 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanno-becker Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanno-becker commented Nov 2, 2018

hanno-becker commented Nov 2, 2018

hanno-becker commented Nov 2, 2018

k-stachowiak left a comment

Choose a reason for hiding this comment

k-stachowiak Nov 5, 2018 • edited by hanno-becker Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-stachowiak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanno-becker commented Nov 6, 2018

simonbutcher left a comment

Choose a reason for hiding this comment

simonbutcher commented Nov 6, 2018

Fix string downcast to `PrintableString` as issued in #1033 #1641

Fix string downcast to `PrintableString` as issued in #1033 #1641

hanno-becker Sep 19, 2018 •

edited

Loading

k-stachowiak Nov 5, 2018 •

edited by hanno-becker

Loading