Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 31 commits into from
Nov 8, 2018
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
23f954d
Fix string downcast to `PrintableString` as issued in #1033
Patater May 17, 2018
c5c9aaf
Merge branch 'development' of https://github.com/thomas-dee/mbedtls i…
May 18, 2018
c150f0d
fixed missing initializer
May 18, 2018
020c823
fixed segmentation fault
May 18, 2018
eba6c9b
changes requested by @hanno-arm
thomas-dee Sep 19, 2018
d2c9009
Improve documentation of x509_attr_descriptor_t
Oct 8, 2018
d0e21fb
Improve documentation of ASN.1 string-writing functions
Oct 8, 2018
d355e69
Rename `tag` to `default_tag` in x509_attr_descriptor_t
Oct 8, 2018
cfc47ba
Correct some indentation and line lengths in x509_create.c
Oct 8, 2018
35b6854
Replace reference to RFC 3280 by reference to newer RFC 5280
Oct 8, 2018
1624e2e
Avoid overly long lines X.509 DN attr array def in x509_create.c
Oct 8, 2018
f745733
Add 'md' cmd line parameter to cert_req example program
Oct 8, 2018
56e8463
Add 'password' cmd line parameter to cert_req example program
Nov 1, 2018
50cb93a
Generate server1* CRTs and CSRs through Mbed TLS applications
Oct 8, 2018
b837775
Generate test-ca* CSRs and CRTs through Mbed TLS applications
Oct 31, 2018
ebc1f40
Generate server2* CSRs and CRTs through Mbed TLS applications
Oct 31, 2018
0dd1139
Generate server5.req.ku.sha1 through Mbed TLS application
Nov 2, 2018
386f99c
Generate cli-rsa* CSRs and CRTs through Mbed TLS applications
Nov 1, 2018
b963081
Generate tests/data_files/test-ca_cat[12|21].crt from Makefile
Oct 31, 2018
52acdb5
Add tests for relaxed CRL-CA name comparison
Nov 2, 2018
0f6903d
Move static x509_name_cmp() in library/x509_crt.c
Nov 2, 2018
cb93813
Don't perform binary comparison of CRL issuer and CA subject
Nov 2, 2018
b12fd31
Adapt ChangeLog
Nov 2, 2018
381c77c
Change serial in test-ca.crt from 0 to 3 to circumvent ASN.1 bug
Nov 1, 2018
6e1adee
Regenerate test files
Nov 1, 2018
beffcd8
Update hardcoded certificates in library/certs.c
Nov 1, 2018
ee334a3
Remove Doxygen tags from documentation of private structure
Oct 24, 2018
cec1c26
Break overly long line in library/x509_create.c
Oct 24, 2018
5517755
Improve wording and formatting of ASN.1 write module documentation
Oct 24, 2018
710f203
Merge branch 'iotssl-1770' into development_thomas_dee
Nov 2, 2018
f143a78
Adapt ChangeLog
Nov 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions include/mbedtls/asn1write.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,21 @@ int mbedtls_asn1_write_bool( unsigned char **p, unsigned char *start, int boolea
*/
int mbedtls_asn1_write_int( unsigned char **p, unsigned char *start, int val );

/**
* \brief Write a given string tag and
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation should be expanded here. Suggestion: Write a string in ASN.1 format using a specific encoding tag.

* value in ASN.1 format
* Note: function works backwards in data buffer
*
* \param p reference to current position pointer
* \param start start of the buffer (for bounds-checking)
* \param tag the tag to write
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* \param text the text to write
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* \param text_len length of the text
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*
* \return the length written or a negative error code
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As below, it should be clarified that "length" refers to the total length in bytes of the ASN.1 data that has been written, and the failing case should be mentioned in a separate \return statement.

*/
int mbedtls_asn1_write_tagged_string( unsigned char **p, unsigned char *start,
int tag, const char *text, size_t text_len );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation is wrong here.

/**
* \brief Write a printable string tag (MBEDTLS_ASN1_PRINTABLE_STRING) and
* value in ASN.1 format
Expand All @@ -167,6 +182,21 @@ int mbedtls_asn1_write_int( unsigned char **p, unsigned char *start, int val );
int mbedtls_asn1_write_printable_string( unsigned char **p, unsigned char *start,
const char *text, size_t text_len );

/**
* \brief Write a UTF8 string tag (MBEDTLS_ASN1_UTF8_STRING) and
* value in ASN.1 format
* Note: function works backwards in data buffer
*
* \param p reference to current position pointer
* \param start start of the buffer (for bounds-checking)
* \param text the text to write
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* \param text_len length of the text
Copy link

@hanno-becker hanno-becker Sep 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*
* \return the length written or a negative error code
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

*/
int mbedtls_asn1_write_utf8_string( unsigned char **p, unsigned char *start,
const char *text, size_t text_len );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation is wrong here.


/**
* \brief Write an IA5 string tag (MBEDTLS_ASN1_IA5_STRING) and
* value in ASN.1 format
Expand Down
31 changes: 17 additions & 14 deletions library/asn1write.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_tagged_string( unsigned char **p, unsigned char *start, int tag,
const char *text, size_t text_len )
{
int ret;
size_t len = 0;

MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_raw_buffer( p, start,
(const unsigned char *) text, text_len ) );
(const unsigned char *) text, text_len ) );

MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_len( p, start, len ) );
MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( p, start, MBEDTLS_ASN1_PRINTABLE_STRING ) );
MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( p, start, tag ) );

return( (int) len );
}

int mbedtls_asn1_write_ia5_string( unsigned char **p, unsigned char *start,
const char *text, size_t text_len )
int mbedtls_asn1_write_utf8_string( unsigned char **p, unsigned char *start,
const char *text, size_t text_len )
{
int ret;
size_t len = 0;

MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_raw_buffer( p, start,
(const unsigned char *) text, text_len ) );
return( mbedtls_asn1_write_tagged_string(p, start, MBEDTLS_ASN1_UTF8_STRING, text, text_len) );
}

MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_len( p, start, len ) );
MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( p, start, MBEDTLS_ASN1_IA5_STRING ) );
int mbedtls_asn1_write_printable_string( unsigned char **p, unsigned char *start,
const char *text, size_t text_len )
{
return( mbedtls_asn1_write_tagged_string(p, start, MBEDTLS_ASN1_PRINTABLE_STRING, text, text_len) );
}

return( (int) len );
int mbedtls_asn1_write_ia5_string( unsigned char **p, unsigned char *start,
const char *text, size_t text_len )
{
return( mbedtls_asn1_write_tagged_string(p, start, MBEDTLS_ASN1_IA5_STRING, text, text_len) );
}

int mbedtls_asn1_write_bitstring( unsigned char **p, unsigned char *start,
Expand Down
116 changes: 58 additions & 58 deletions library/x509_create.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,44 +37,46 @@ typedef struct {
const char *name;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-existing: Even though it's internal, this structure should be properly documented. E.g.:

/* Structure linking OIDs for X.509 DN AttributeTypes to their
 * string representations and default string encodings used by Mbed TLS. */
typedef struct {
   const char *name; /* String representation of AttributeType, e.g.
                      * "CN" or "emailAddress". */
   size_t name_len;  /* Length of \c name, without trailing \c 0 byte. */
   const char *oid;  /* String representation of OID of AttributeType,
                      * as per RFC 5280, Appendix A.1. */
   int default_tag;  /* The default character encoding used for the
                      * given attribute type, e.g. 
                      * #MBEDTLS_ASN1_UTF8_STRING for UTF-8. */
};

size_t name_len;
const char*oid;
int tag;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to call this default_tag instead of tag, as in all cases where we use MBEDTLS_ASN1_UTF8_STRING, the other string tags would actually be allowed, too.

} x509_attr_descriptor_t;

#define ADD_STRLEN( s ) s, sizeof( s ) - 1

// note: preset tag types as proposed in RFC3280 and widely used
static const x509_attr_descriptor_t x509_attrs[] =
{
{ ADD_STRLEN( "CN" ), MBEDTLS_OID_AT_CN },
{ ADD_STRLEN( "commonName" ), MBEDTLS_OID_AT_CN },
{ ADD_STRLEN( "C" ), MBEDTLS_OID_AT_COUNTRY },
{ ADD_STRLEN( "countryName" ), MBEDTLS_OID_AT_COUNTRY },
{ ADD_STRLEN( "O" ), MBEDTLS_OID_AT_ORGANIZATION },
{ ADD_STRLEN( "organizationName" ), MBEDTLS_OID_AT_ORGANIZATION },
{ ADD_STRLEN( "L" ), MBEDTLS_OID_AT_LOCALITY },
{ ADD_STRLEN( "locality" ), MBEDTLS_OID_AT_LOCALITY },
{ ADD_STRLEN( "R" ), MBEDTLS_OID_PKCS9_EMAIL },
{ ADD_STRLEN( "OU" ), MBEDTLS_OID_AT_ORG_UNIT },
{ ADD_STRLEN( "organizationalUnitName" ), MBEDTLS_OID_AT_ORG_UNIT },
{ ADD_STRLEN( "ST" ), MBEDTLS_OID_AT_STATE },
{ ADD_STRLEN( "stateOrProvinceName" ), MBEDTLS_OID_AT_STATE },
{ ADD_STRLEN( "emailAddress" ), MBEDTLS_OID_PKCS9_EMAIL },
{ ADD_STRLEN( "serialNumber" ), MBEDTLS_OID_AT_SERIAL_NUMBER },
{ ADD_STRLEN( "postalAddress" ), MBEDTLS_OID_AT_POSTAL_ADDRESS },
{ ADD_STRLEN( "postalCode" ), MBEDTLS_OID_AT_POSTAL_CODE },
{ ADD_STRLEN( "dnQualifier" ), MBEDTLS_OID_AT_DN_QUALIFIER },
{ ADD_STRLEN( "title" ), MBEDTLS_OID_AT_TITLE },
{ ADD_STRLEN( "surName" ), MBEDTLS_OID_AT_SUR_NAME },
{ ADD_STRLEN( "SN" ), MBEDTLS_OID_AT_SUR_NAME },
{ ADD_STRLEN( "givenName" ), MBEDTLS_OID_AT_GIVEN_NAME },
{ ADD_STRLEN( "GN" ), MBEDTLS_OID_AT_GIVEN_NAME },
{ ADD_STRLEN( "initials" ), MBEDTLS_OID_AT_INITIALS },
{ ADD_STRLEN( "pseudonym" ), MBEDTLS_OID_AT_PSEUDONYM },
{ ADD_STRLEN( "generationQualifier" ), MBEDTLS_OID_AT_GENERATION_QUALIFIER },
{ ADD_STRLEN( "domainComponent" ), MBEDTLS_OID_DOMAIN_COMPONENT },
{ ADD_STRLEN( "DC" ), MBEDTLS_OID_DOMAIN_COMPONENT },
{ NULL, 0, NULL }
{ ADD_STRLEN( "CN" ), MBEDTLS_OID_AT_CN, MBEDTLS_ASN1_UTF8_STRING },
{ ADD_STRLEN( "commonName" ), MBEDTLS_OID_AT_CN, MBEDTLS_ASN1_UTF8_STRING },
{ ADD_STRLEN( "C" ), MBEDTLS_OID_AT_COUNTRY, MBEDTLS_ASN1_PRINTABLE_STRING },
{ ADD_STRLEN( "countryName" ), MBEDTLS_OID_AT_COUNTRY, MBEDTLS_ASN1_PRINTABLE_STRING },
{ ADD_STRLEN( "O" ), MBEDTLS_OID_AT_ORGANIZATION, MBEDTLS_ASN1_UTF8_STRING },
{ ADD_STRLEN( "organizationName" ), MBEDTLS_OID_AT_ORGANIZATION, MBEDTLS_ASN1_UTF8_STRING },
{ ADD_STRLEN( "L" ), MBEDTLS_OID_AT_LOCALITY, MBEDTLS_ASN1_UTF8_STRING },
{ ADD_STRLEN( "locality" ), MBEDTLS_OID_AT_LOCALITY, MBEDTLS_ASN1_UTF8_STRING },
{ ADD_STRLEN( "R" ), MBEDTLS_OID_PKCS9_EMAIL, MBEDTLS_ASN1_IA5_STRING},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A whitespace is missing near the end of this line

{ ADD_STRLEN( "OU" ), MBEDTLS_OID_AT_ORG_UNIT, MBEDTLS_ASN1_UTF8_STRING },
{ ADD_STRLEN( "organizationalUnitName" ), MBEDTLS_OID_AT_ORG_UNIT, MBEDTLS_ASN1_UTF8_STRING },
{ ADD_STRLEN( "ST" ), MBEDTLS_OID_AT_STATE, MBEDTLS_ASN1_UTF8_STRING },
{ ADD_STRLEN( "stateOrProvinceName" ), MBEDTLS_OID_AT_STATE, MBEDTLS_ASN1_UTF8_STRING },
{ ADD_STRLEN( "emailAddress" ), MBEDTLS_OID_PKCS9_EMAIL, MBEDTLS_ASN1_IA5_STRING },
{ 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 },
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should reference the newer RFC 5280 here.

{ ADD_STRLEN( "title" ), MBEDTLS_OID_AT_TITLE, MBEDTLS_ASN1_UTF8_STRING },
{ ADD_STRLEN( "surName" ), MBEDTLS_OID_AT_SUR_NAME, MBEDTLS_ASN1_UTF8_STRING },
{ ADD_STRLEN( "SN" ), MBEDTLS_OID_AT_SUR_NAME, MBEDTLS_ASN1_UTF8_STRING },
{ ADD_STRLEN( "givenName" ), MBEDTLS_OID_AT_GIVEN_NAME, MBEDTLS_ASN1_UTF8_STRING },
{ ADD_STRLEN( "GN" ), MBEDTLS_OID_AT_GIVEN_NAME, MBEDTLS_ASN1_UTF8_STRING },
{ ADD_STRLEN( "initials" ), MBEDTLS_OID_AT_INITIALS, MBEDTLS_ASN1_UTF8_STRING },
{ ADD_STRLEN( "pseudonym" ), MBEDTLS_OID_AT_PSEUDONYM, MBEDTLS_ASN1_UTF8_STRING },
{ ADD_STRLEN( "generationQualifier" ), MBEDTLS_OID_AT_GENERATION_QUALIFIER, MBEDTLS_ASN1_UTF8_STRING },
{ ADD_STRLEN( "domainComponent" ), MBEDTLS_OID_DOMAIN_COMPONENT, MBEDTLS_ASN1_IA5_STRING },
{ ADD_STRLEN( "DC" ), MBEDTLS_OID_DOMAIN_COMPONENT, MBEDTLS_ASN1_IA5_STRING },
{ NULL, 0, NULL, MBEDTLS_ASN1_NULL }
};

static const char *x509_at_oid_from_name( const char *name, size_t name_len )
static const x509_attr_descriptor_t *x509_attr_descr_from_name( const char *name, size_t name_len )
{
const x509_attr_descriptor_t *cur;

Expand All @@ -83,7 +85,10 @@ static const char *x509_at_oid_from_name( const char *name, size_t name_len )
strncmp( cur->name, name, name_len ) == 0 )
break;

return( cur->oid );
if ( cur->name == NULL )
return( NULL );

return( cur );
}

int mbedtls_x509_string_to_names( mbedtls_asn1_named_data **head, const char *name )
Expand All @@ -92,6 +97,7 @@ int mbedtls_x509_string_to_names( mbedtls_asn1_named_data **head, const char *na
const char *s = name, *c = s;
const char *end = s + strlen( s );
const char *oid = NULL;
const x509_attr_descriptor_t* attr_descr = NULL;
int in_tag = 1;
char data[MBEDTLS_X509_MAX_DN_NAME_SIZE];
char *d = data;
Expand All @@ -103,12 +109,13 @@ int mbedtls_x509_string_to_names( mbedtls_asn1_named_data **head, const char *na
{
if( in_tag && *c == '=' )
{
if( ( oid = x509_at_oid_from_name( s, c - s ) ) == NULL )
if( ( attr_descr = x509_attr_descr_from_name( s, c - s ) ) == NULL )
{
ret = MBEDTLS_ERR_X509_UNKNOWN_OID;
goto exit;
}

oid = attr_descr->oid;
s = c + 1;
in_tag = 0;
d = data;
Expand All @@ -127,13 +134,18 @@ int mbedtls_x509_string_to_names( mbedtls_asn1_named_data **head, const char *na
}
else if( !in_tag && ( *c == ',' || c == end ) )
{
if( mbedtls_asn1_store_named_data( head, oid, strlen( oid ),
(unsigned char *) data,
d - data ) == NULL )
mbedtls_asn1_named_data* cur = mbedtls_asn1_store_named_data( head, oid, strlen( oid ),
(unsigned char *) data,
d - data );

if(cur == NULL )
{
return( MBEDTLS_ERR_X509_ALLOC_FAILED );
}

// set tagType
cur->val.tag = attr_descr->tag;

while( c < end && *(c + 1) == ' ' )
c++;

Expand Down Expand Up @@ -192,29 +204,19 @@ int mbedtls_x509_set_extension( mbedtls_asn1_named_data **head, const char *oid,
*
* AttributeValue ::= ANY DEFINED BY AttributeType
*/
static int x509_write_name( unsigned char **p, unsigned char *start,
const char *oid, size_t oid_len,
const unsigned char *name, size_t name_len )
static int x509_write_name( unsigned char **p, unsigned char *start, mbedtls_asn1_named_data* cur_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line has got too long. Please break it.

{
int ret;
size_t len = 0;

// Write PrintableString for all except MBEDTLS_OID_PKCS9_EMAIL
//
if( MBEDTLS_OID_SIZE( MBEDTLS_OID_PKCS9_EMAIL ) == oid_len &&
memcmp( oid, MBEDTLS_OID_PKCS9_EMAIL, oid_len ) == 0 )
{
MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_ia5_string( p, start,
(const char *) name,
name_len ) );
}
else
{
MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_printable_string( p, start,
(const char *) name,
name_len ) );
}

const char *oid = (const char*)cur_name->oid.p;
size_t oid_len = cur_name->oid.len;
const unsigned char *name = cur_name->val.p;
size_t name_len = cur_name->val.len;

// Write correct string tag and value
MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tagged_string( p, start, cur_name->val.tag,
(const char *) name,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation looks off by one character here.

name_len ) );
// Write OID
//
MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_oid( p, start, oid, oid_len ) );
Expand All @@ -239,9 +241,7 @@ int mbedtls_x509_write_names( unsigned char **p, unsigned char *start,

while( cur != NULL )
{
MBEDTLS_ASN1_CHK_ADD( len, x509_write_name( p, start, (char *) cur->oid.p,
cur->oid.len,
cur->val.p, cur->val.len ) );
MBEDTLS_ASN1_CHK_ADD( len, x509_write_name( p, start, cur ) );
cur = cur->next;
}

Expand Down