Constant pointers and asn1 parsing

[ov7a]

Should asn1 parse functions like mbedtls_asn1_get_len receive const unsigned char** p instead of unsigned char** p?

Consider mbedtls_pk_parse_public_key in pkparse.c

int mbedtls_pk_parse_public_key( mbedtls_pk_context *ctx,
                         const unsigned char *key, size_t keylen )
{
    int ret;
    unsigned char *p;
    ...
    p = (unsigned char *) key;

This is a quite bad cast. Some compilers may complain. E.g. clang with -Wcast-qual:

error: cast from 'const unsigned char *' to 'unsigned char *' drops const qualifier [-Werror,-Wcast-qual]
       p = (unsigned char *) key;

[RonEld]

Thank you for reporting. We will have a look regarding what is the best approach to overcome this

[ciarmcom]

ARM Internal Ref: IOTSSL-1222

[mpg]

Thanks for the suggestion, but I'm not sure this is a good idea, unfortunately the C type system is not really our friend here: char ** and const char ** are not compatible pointers types, so you can't pass one when the other is expected (contrary to char * which can be passed when a const char * is expected).

void foo_const( const char **a );
void foo_nocst( char **a );

int main( void )
{
    const char *c = "Abc";
    char *n = "Abc";

    /* works */
    foo_const( &c );
    foo_nocst( &n );

    /* gives warning */
    foo_const( &n );
    foo_nocst( &c );
}

GCC and Clang, even without -Wall both give a warnings in both cases:

passing argument 1 of ‘foo_const’ from incompatible pointer type [-Wincompatible-pointer-types]
passing argument 1 of ‘foo_nocst’ from incompatible pointer type [-Wincompatible-pointer-types]
passing 'char **' to parameter of type 'const char **' discards qualifiers in nested pointer types [-Wincompatible-pointer-types-discards-qualifiers]
passing 'const char **' to parameter of type 'char **' discards qualifiers in nested pointer types [-Wincompatible-pointer-types-discards-qualifiers]

So I don't think adding a const here (with pointers to pointers) would make the situation better contrary to the situation with simple pointers, where adding the const gives more freedom to users.

I also think adding const here would have a pretty high cost as it would force all users to change their code when it was working.

Therefore, I'm inclined to close as "won't fix". (I'm not doing it right now because I think a second opinion would be useful first.)

[ov7a]

@mpg thank you for your reply. Unfortunately, I don't think I can add something valuable to your suggestion. This issue is no longer relevant for me.

[mpg]

Ok, thanks for your reply. I'm closing this, then.

[gilles-peskine-arm]

@mpg I'm revisiting this because I just got bitten by it again. On second reading, I find I have to take back my prior backing of your message. This is about const foo_t**, not about foo_t *const *.

If you have memory that you aren't allowed to modify, a cursor into that memory is a const uint8_t*. If you want to pass it to a parsing function without degrading constness, that parsing function must take a const uint8_t**. Otherwise you need to lie to the compiler to allow it to modify the memory even though you don't actually hold a mutable reference to it. With the parsing functions taking uint8_t**, you need to cast away the constness:

my_parse(const uint8_t *buf, size_t size) {
    const uint8_t *end = buf + size;
    uint8_t *p = (uint8_t*)buf; // cast away
    mbedtls_parse_xxx(&p, end);
}

On the other hand, if the parsing function takes a const uint8_t**, all you have to do is declare a const pointer and use that as the cursor, which is perfectly safe: the const pointer is a separate reference, and you can always make read-only references to memory that you're allowed to write.

load_and_parse() {
    size_t size = get_file_size();
    uint8_t *buf = malloc(size);
    load_file(buf);
    const uint8_t *end = buf + size;
    const uint8_t *p = buf; // cast away
    good_parse_xxx(&p, end);
}

No cast, no lie.

Fixing this involves a fairly large amount of code which needs to be done all at once, or at least in major increments, so I don't know if we have time to do it with our short time frame for Mbed TLS 3.0. But it would be a good thing.

[mpg]

@gilles-peskine-arm That's an excellent point. My point was that either way, there are situations where you'll need a cast (or a second reference with a new type); the essential difference I missed though is that in one case, this cast would be a lie, while in the other case it would be legal.

So, I now agree that this is something we should fix at some point and I'm re-opening this issue.

Unfortunately, I also don't see a way of doing this in small increments. OTOH, depending in how much of the asn1parse API we can make private in 3.0, this might be something we could do in 3.x. Wdyt?

[gilles-peskine-arm]

The lack of constness trickles all the way up to other modules, in particular X.509. But at least mbedtls_x509_crt_parse_der and friends have const in the expected place. We need to do a more systematic check, or a time-bounded attempt to see how many places need to be modified.

[tom-cosgrove-arm]

Closing this again. While there are merits, in practice it would impact a lot of functions, and it doesn't seem likely that this will happen except as part of a rewrite of the impacted parts of the code (which would have its own, larger issue to cover it).

[inorick]

I too was bitten by this just now. Do you have an idea how much effort fixing this would be? If it is manageable, I would attempt to do it.

[tom-cosgrove-arm]

@ronald-cron-arm @gilles-peskine-arm @mpg If we were ever to do this, it would have to be as part of 4.0. I don't think our team will have bandwidth to do this (there will always be a higher priority) - however I think it would be a very large contribution (lots of small changes) and so reviewing it would also be something we might not have capacity to do.

However, if it were possible to script a fix to this, then that would be a much easier review (review the script and verify the output) and something we should be able to fit in.

The scale of this is going to be "in a lot of places"

[gilles-peskine-arm]

I'm reopening this issue because this is a legitimate request. This doesn't mean that we will do it.

This will be backward-incompatible, so it pretty much has to be done in a major release: if we need to keep both the current API and a new one, we'd need to essentially duplicate at least ASN.1 parsing functions and possibly some X.509 code as well. We're currently working on the next major release of the library (Mbed TLS 4.0), to be released in the second quarter of 2025. So now is a good time to work on this.

Among the large amount of work we have to prepare 4.0, this is relatively low priority. So I'm afraid I can't promise that we'll spend much effort.

@inorick If you're willing to help, thank you very much! I'm afraid I don't know how much effort it is. In fact, at the moment, figuring out the design is something we could use help with. At the moment, the main part of my job is designing the parts of 4.0 that we absolutely must do, and I don't have spare bandwidth for this. Where you could help is to do a proof of concept.

What I mean by a proof of concept is:

• It doesn't need to be complete. But it needs to do enough so that we can estimate how hard it is to do the rest.
• The changes need to be presented in a way that's clear enough so that we can understand the implications. In particular, will this break existing working code? How can such code be repaired? Are there use cases that are no longer possible? In terms of presentation, if it's just a matter of adding const in places, that's fine, but if there are other changes, we need to understand what those are.
• We need to know that the code works, so at least make all && make test has to pass in the default configuration.
• Don't worry about all the other changes that are going into 4.0. In fact, I would suggest working on the mbedtls-3.6 branch.

We would need this to be ready in November 2024, so that we can schedule work in Q1 2025 if we go ahead.

[inorick]

Thank you for picking this up again. I fear I will not make it until November 2024, but let's see.