Skip to content

Introduce ASN.1 SEQUENCE traversal API #263

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

Merged
merged 7 commits into from
Feb 3, 2020
Merged

Introduce ASN.1 SEQUENCE traversal API #263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cba7122
ASN.1: Add helper macro to detect string types
Sep 11, 2019
63e38fe
ASN.1: Add helper macro to compare ASN.1 buffer to OID string
Sep 11, 2019
12ae27d
ASN.1: Introduce helper function to free ASN.1 sequence
Sep 11, 2019
b5c74a5
Document one more error code for mbedtls_asn1_get_sequence_of
gilles-peskine-arm Oct 30, 2019
199b709
ASN.1: Add ASN.1 SEQUENCE traversal API
Sep 11, 2019
1505f63
ASN.1: Reimplement mbedtls_asn1_get_sequence_of() via traversal API
Sep 11, 2019
34aada2
Replace use of uint8_t by unsigned char in ASN.1 seq traversal API
Feb 3, 2020
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
140 changes: 139 additions & 1 deletion include/mbedtls/asn1.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,18 @@
#define MBEDTLS_ASN1_CONSTRUCTED 0x20
#define MBEDTLS_ASN1_CONTEXT_SPECIFIC 0x80

/* Slightly smaller way to check if tag is a string tag
* compared to canonical implementation. */
#define MBEDTLS_ASN1_IS_STRING_TAG( tag ) \
( ( tag ) < 32u && ( \
( ( 1u << ( tag ) ) & ( ( 1u << MBEDTLS_ASN1_BMP_STRING ) | \
( 1u << MBEDTLS_ASN1_UTF8_STRING ) | \
( 1u << MBEDTLS_ASN1_T61_STRING ) | \
( 1u << MBEDTLS_ASN1_IA5_STRING ) | \
( 1u << MBEDTLS_ASN1_UNIVERSAL_STRING ) | \
( 1u << MBEDTLS_ASN1_PRINTABLE_STRING ) | \
( 1u << MBEDTLS_ASN1_BIT_STRING ) ) ) != 0 ) )

/*
* Bit masks for each of the components of an ASN.1 tag as specified in
* ITU X.690 (08/2015), section 8.1 "General rules for encoding",
Expand Down Expand Up @@ -120,6 +132,10 @@
( ( MBEDTLS_OID_SIZE(oid_str) != (oid_buf)->len ) || \
memcmp( (oid_str), (oid_buf)->p, (oid_buf)->len) != 0 )

#define MBEDTLS_OID_CMP_RAW(oid_str, oid_buf, oid_buf_len) \
( ( MBEDTLS_OID_SIZE(oid_str) != (oid_buf_len) ) || \
memcmp( (oid_str), (oid_buf), (oid_buf_len) ) != 0 )

#ifdef __cplusplus
extern "C" {
#endif
Expand Down Expand Up @@ -327,6 +343,9 @@ int mbedtls_asn1_get_bitstring_null( unsigned char **p,
* \brief Parses and splits an ASN.1 "SEQUENCE OF <tag>".
* Updates the pointer to immediately behind the full sequence tag.
*
* This function allocates memory for the sequence elements. You can free
* the allocated memory with mbedtls_asn1_sequence_free().
*
* \note On error, this function may return a partial list in \p cur.
* You must set `cur->next = NULL` before calling this function!
* Otherwise it is impossible to distinguish a previously non-null
Expand Down Expand Up @@ -360,14 +379,133 @@ int mbedtls_asn1_get_bitstring_null( unsigned char **p,
* \return 0 if successful.
* \return #MBEDTLS_ERR_ASN1_LENGTH_MISMATCH if the input contains
* extra data after a valid SEQUENCE OF \p tag.
* \return #MBEDTLS_ERR_ASN1_UNEXPECTED_TAG if the input starts with
* an ASN.1 SEQUENCE in which an element has a tag that
* is different from \p tag.
* \return #MBEDTLS_ERR_ASN1_ALLOC_FAILED if a memory allocation failed.
* \return An ASN.1 error code if the input does not start with
* a valid ASN.1 BIT STRING.
* a valid ASN.1 SEQUENCE.
*/
int mbedtls_asn1_get_sequence_of( unsigned char **p,
const unsigned char *end,
mbedtls_asn1_sequence *cur,
int tag );
/**
* \brief Free a heap-allocated linked list presentation of
* an ASN.1 sequence, including the first element.
*
* There are two common ways to manage the memory used for the representation
* of a parsed ASN.1 sequence:
* - Allocate a head node `mbedtls_asn1_sequence *head` with mbedtls_calloc().
* Pass this node as the `cur` argument to mbedtls_asn1_get_sequence_of().
* When you have finished processing the sequence,
* call mbedtls_asn1_sequence_free() on `head`.
* - Allocate a head node `mbedtls_asn1_sequence *head` in any manner,
* for example on the stack. Make sure that `head->next == NULL`.
* Pass `head` as the `cur` argument to mbedtls_asn1_get_sequence_of().
* When you have finished processing the sequence,
* call mbedtls_asn1_sequence_free() on `head->cur`,
* then free `head` itself in the appropriate manner.
*
* \param seq The address of the first sequence component. This may
* be \c NULL, in which case this functions returns
* immediately.
*/
void mbedtls_asn1_sequence_free( mbedtls_asn1_sequence *seq );

/**
* \brief Traverse an ASN.1 SEQUENCE container and
* call a callback for each entry.
*
* This function checks that the input is a SEQUENCE of elements that
* each have a "must" tag, and calls a callback function on the elements
* that have a "may" tag.
*
* For example, to validate that the input is a SEQUENCE of `tag1` and call
* `cb` on each element, use
* ```
* mbedtls_asn1_traverse_sequence_of(&p, end, 0xff, tag1, 0, 0, cb, ctx);
* ```
*
* To validate that the input is a SEQUENCE of ANY and call `cb` on
* each element, use
* ```
* mbedtls_asn1_traverse_sequence_of(&p, end, 0, 0, 0, 0, cb, ctx);
* ```
*
* To validate that the input is a SEQUENCE of CHOICE {NULL, OCTET STRING}
* and call `cb` on each element that is an OCTET STRING, use
* ```
* mbedtls_asn1_traverse_sequence_of(&p, end, 0xfe, 0x04, 0xff, 0x04, cb, ctx);
* ```
*
* The callback is called on the elements with a "may" tag from left to
* right. If the input is not a valid SEQUENCE of elements with a "must" tag,
* the callback is called on the elements up to the leftmost point where
* the input is invalid.
*
* \warning This function is still experimental and may change
* at any time.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why should this be experimental? When can it be non-experimental? Could we make asn1_internal.h and place it there if it's only for internal use for now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It isn't internal since it will be used by Mbed TLS.

Copy link
Author

Choose a reason for hiding this comment

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

@gilles-peskine-arm @Patater What's your view on this? This function must be public, as @gilles-peskine-arm said, so what remains is to decide whether we want to declare it as experimental or not. I'm open either way. If you think the API is good as-is, I'm happy to remove the qualification.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My preference is what you wrote: declare this function as experimental. We don't care about this function except as an internal detail of the implementation an X509 feature that we do care about. It has a complex API that I would prefer not to make official, but that I'm ok to include with the understanding that we're likely to modify it in the future.

Copy link
Contributor

@mpg mpg Feb 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The situation should be temporary, since I think we're aiming at making the whole asn1.h private in Mbed TLS 3.0 (or some point in the future at least) anyway.

*
* \param p The address of the pointer to the beginning of
* the ASN.1 SEQUENCE header. This is updated to
* point to the end of the ASN.1 SEQUENCE container
* on a successful invocation.
* \param end The end of the ASN.1 SEQUENCE container.
* \param tag_must_mask A mask to be applied to the ASN.1 tags found within
* the SEQUENCE before comparing to \p tag_must_value.
* \param tag_must_val The required value of each ASN.1 tag found in the
* SEQUENCE, after masking with \p tag_must_mask.
* Mismatching tags lead to an error.
* For example, a value of \c 0 for both \p tag_must_mask
* and \p tag_must_val means that every tag is allowed,
* while a value of \c 0xFF for \p tag_must_mask means
* that \p tag_must_val is the only allowed tag.
* \param tag_may_mask A mask to be applied to the ASN.1 tags found within
* the SEQUENCE before comparing to \p tag_may_value.
* \param tag_may_val The desired value of each ASN.1 tag found in the
* SEQUENCE, after masking with \p tag_may_mask.
* Mismatching tags will be silently ignored.
* For example, a value of \c 0 for \p tag_may_mask and
* \p tag_may_val means that any tag will be considered,
* while a value of \c 0xFF for \p tag_may_mask means
* that all tags with value different from \p tag_may_val
* will be ignored.
* \param cb The callback to trigger for each component
* in the ASN.1 SEQUENCE that matches \p tag_may_val.
* The callback function is called with the following
* parameters:
* - \p ctx.
* - The tag of the current element.
* - A pointer to the start of the current element's
* content inside the input.
* - The length of the content of the current element.
* If the callback returns a non-zero value,
* the function stops immediately,
* forwarding the callback's return value.
* \param ctx The context to be passed to the callback \p cb.
*
* \return \c 0 if successful the entire ASN.1 SEQUENCE
* was traversed without parsing or callback errors.
* \return #MBEDTLS_ERR_ASN1_LENGTH_MISMATCH if the input
* contains extra data after a valid SEQUENCE
* of elements with an accepted tag.
* \return #MBEDTLS_ERR_ASN1_UNEXPECTED_TAG if the input starts
* with an ASN.1 SEQUENCE in which an element has a tag
* that is not accepted.
* \return An ASN.1 error code if the input does not start with
* a valid ASN.1 SEQUENCE.
* \return A non-zero error code forwarded from the callback
* \p cb in case the latter returns a non-zero value.
*/
int mbedtls_asn1_traverse_sequence_of(
unsigned char **p,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the unsigned char be const unsigned char? I don't think we'd be modifying **p, only p or *p.

Copy link
Collaborator

Choose a reason for hiding this comment

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

const unsigned char ** and unsigned char ** do not mix. If this function used a different type, it couldn't be used with the other asn1 parsing functions.

Copy link
Author

Choose a reason for hiding this comment

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

I agree with @Patater in that in principle the type of p should be const unsigned char ** here. However, the rest of the ASN.1 parsing library unfortunately also uses unsigned char **, which we can't change and which forces the use of unsigned char ** here, too (not just for consistency, but simply because this function calls e.g. mbedtls_asn1_get_tag() which has a unsigned char ** argument). An alternative would be to extend the documentation of all ASN.1 parsing functions to explicitly mention that, despite the missing const qualification, the **p is never written to, and that it is hence safe to use the functions in a const unsigned char ** context. If we did that, we could start improving the API by using const unsigned char ** at least for this new API function, and doing a const-cast when calling the other parsing functions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can tweak the API in Mbed TLS 3. Until we get there, let's keep the API consistent.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, no change here, then.

const unsigned char *end,
unsigned char tag_must_mask, unsigned char tag_must_val,
unsigned char tag_may_mask, unsigned char tag_may_val,
int (*cb)( void *ctx, int tag,
unsigned char* start, size_t len ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can start be made const?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering about that. In 99% of the cases it would be const: parsing a read-only string. But it may be useful occasionally to modify an input in place.

Copy link
Author

Choose a reason for hiding this comment

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

I agree that it should be const. However, if we want to call other ASN.1 parsing functions from within the callback, then we can't have const here because those parsing functions don't have const qualification, as mentioned above.

void *ctx );

#if defined(MBEDTLS_BIGNUM_C)
/**
Expand Down
144 changes: 101 additions & 43 deletions library/asn1parse.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,58 @@ int mbedtls_asn1_get_bitstring( unsigned char **p, const unsigned char *end,
return( 0 );
}

/*
* Traverse an ASN.1 "SEQUENCE OF <tag>"
* and call a callback for each entry found.
*/
int mbedtls_asn1_traverse_sequence_of(
unsigned char **p,
const unsigned char *end,
unsigned char tag_must_mask, unsigned char tag_must_val,
unsigned char tag_may_mask, unsigned char tag_may_val,
int (*cb)( void *ctx, int tag,
unsigned char *start, size_t len ),
void *ctx )
{
int ret;
size_t len;

/* Get main sequence tag */
if( ( ret = mbedtls_asn1_get_tag( p, end, &len,
MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE ) ) != 0 )
{
return( ret );
}

if( *p + len != end )
return( MBEDTLS_ERR_ASN1_LENGTH_MISMATCH );

while( *p < end )
{
unsigned char const tag = *(*p)++;

if( ( tag & tag_must_mask ) != tag_must_val )
return( MBEDTLS_ERR_ASN1_UNEXPECTED_TAG );

if( ( ret = mbedtls_asn1_get_len( p, end, &len ) ) != 0 )
return( ret );

if( ( tag & tag_may_mask ) == tag_may_val )
{
if( cb != NULL )
{
ret = cb( ctx, tag, *p, len );
if( ret != 0 )
return( ret );
}
}

*p += len;
}

return( 0 );
}

/*
* Get a bit string without unused bits
*/
Expand All @@ -269,61 +321,67 @@ int mbedtls_asn1_get_bitstring_null( unsigned char **p, const unsigned char *end
return( 0 );
}



/*
* Parses and splits an ASN.1 "SEQUENCE OF <tag>"
*/
int mbedtls_asn1_get_sequence_of( unsigned char **p,
const unsigned char *end,
mbedtls_asn1_sequence *cur,
int tag)
void mbedtls_asn1_sequence_free( mbedtls_asn1_sequence *seq )
{
int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
size_t len;
mbedtls_asn1_buf *buf;

/* Get main sequence tag */
if( ( ret = mbedtls_asn1_get_tag( p, end, &len,
MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE ) ) != 0 )
return( ret );

if( *p + len != end )
return( MBEDTLS_ERR_ASN1_LENGTH_MISMATCH );

while( *p < end )
while( seq != NULL )
{
buf = &(cur->buf);
buf->tag = **p;

if( ( ret = mbedtls_asn1_get_tag( p, end, &buf->len, tag ) ) != 0 )
return( ret );
mbedtls_asn1_sequence *next = seq->next;
mbedtls_platform_zeroize( seq, sizeof( *seq ) );
mbedtls_free( seq );
seq = next;
}
}

buf->p = *p;
*p += buf->len;
typedef struct
{
int tag;
mbedtls_asn1_sequence *cur;
} asn1_get_sequence_of_cb_ctx_t;

static int asn1_get_sequence_of_cb( void *ctx,
int tag,
unsigned char *start,
size_t len )
{
asn1_get_sequence_of_cb_ctx_t *cb_ctx =
(asn1_get_sequence_of_cb_ctx_t *) ctx;
mbedtls_asn1_sequence *cur =
cb_ctx->cur;

/* Allocate and assign next pointer */
if( *p < end )
{
cur->next = (mbedtls_asn1_sequence*)mbedtls_calloc( 1,
sizeof( mbedtls_asn1_sequence ) );
if( cur->buf.p != NULL )
{
cur->next =
mbedtls_calloc( 1, sizeof( mbedtls_asn1_sequence ) );

if( cur->next == NULL )
return( MBEDTLS_ERR_ASN1_ALLOC_FAILED );
if( cur->next == NULL )
return( MBEDTLS_ERR_ASN1_ALLOC_FAILED );

cur = cur->next;
}
cur = cur->next;
}

/* Set final sequence entry's next pointer to NULL */
cur->next = NULL;

if( *p != end )
return( MBEDTLS_ERR_ASN1_LENGTH_MISMATCH );
cur->buf.p = start;
cur->buf.len = len;
cur->buf.tag = tag;

cb_ctx->cur = cur;
return( 0 );
}

/*
* Parses and splits an ASN.1 "SEQUENCE OF <tag>"
*/
int mbedtls_asn1_get_sequence_of( unsigned char **p,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, too bad this is not const. I guess we can't have our callback with p const because of this.

Copy link
Author

Choose a reason for hiding this comment

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

This way it would work, but what doesn't work is calling parsing functions from within mbedtls_asn1_traverse_sequence_of().

const unsigned char *end,
mbedtls_asn1_sequence *cur,
int tag)
{
asn1_get_sequence_of_cb_ctx_t cb_ctx = { tag, cur };
memset( cur, 0, sizeof( mbedtls_asn1_sequence ) );
return( mbedtls_asn1_traverse_sequence_of(
p, end, 0xFF, tag, 0, 0,
asn1_get_sequence_of_cb, &cb_ctx ) );
}

int mbedtls_asn1_get_alg( unsigned char **p,
const unsigned char *end,
mbedtls_asn1_buf *alg, mbedtls_asn1_buf *params )
Expand Down
Loading