Simplify checks in ssl_write_certificate_request

[irwir]

Description

Resolve #2916 - remove condition in the middle, because it is enough to do the strongest check only.

Status

READY

Requires Backporting

NO

Which branch?
development

Migrations

NO

[mpg]

Thanks for your contribution!

However I'm a bit conflicted about that change. Strictly speaking and using only information available in this function, the middle check is not redundant, due to the risk of overflow: if dn_size is SIZE_MAX or SIZE_MAX - 1, then 2 + dn_size is 1 or 0 and the final check becomes weaker than the middle one.

Now using extra information, we can see that dn_size is crt->subject_raw.len which clearly can't be that close to SIZE_MAX because otherwise the size of the overall certificate could not be represented with a size_t.

So I think removing the middle check is OK, but makes the correctness of the code a bit less obvious. So perhaps we should add a comment?

[irwir]

Strictly speaking and using only information available in this function, the middle check is not redundant, due to the risk of overflow

Good point.
However, based on this funcion's code, dn_size should be defined as uint16_t.
If size_t is 16 bits wide, it could be verified that the dn_size value is below _UI16_MAX - 2.

[mpg]

However, based on this funcion's code, dn_size should be defined as uint16_t.
So far, we tend to use size_t for every sizes, and I'd rather stick to that convention unless we have a good reason not to.

[mpg]

I think your code change is correct, but could you add a comment explaining why? For example "2 + dn_size can't overflow because the total length of the certificate is greater than that and fits in a size_t".

[irwir]

@mpg, thanks for reviewing.

So far, we tend to use size_t for every sizes, and I'd rather stick to that convention unless we have a good reason not to.

Please compare to the PR #2856.
Only two low bytes of dn_size and total_dn_size are usable, therefore switch to uint16_t in this function would be appropriate. Even comment could be omitted because the library supports only 32 and 64 bits architectures.
if( end < p || (size_t)( end - p ) < 2 + (size_t)dn_size )

[mpg]

OK, good point. Feel free to update this PR to use uint16_t, then. (We'll probably need to add an explicit cast in dn_size = crt->subject_raw.len; though, otherwise some compilers are likely to warn about implicit conversion losing precision.)

[mpg]

Thanks for making the change! I'm happy with the result except for two style issues.

Also, could you update the commit message to make the first line more informative? When using git log --pretty=one-line or similar, we should be able to get an idea what the commit is about without needed to check external references or the full commit message.

Here's an example:

Simplify bounds check in ssl_write_certificate_request

It is sufficient to check for the strongest limit only. Using a smaller type ensures there is no overflow (assuming size_t is at least 32 bits).

Fixes #2916 

[mpg]

Style: we usually leave a space in (size_t) dn_size (we only omit it before a parenthesis).

[mpg]

Style: same as below.

[irwir]

The commit was updated as discussed.
However, on closer inspection of code, a few similar cases were found by searching for << 8 in ssl_srv.c module.
Similar case here means using of size_t instead of more natural type uint16_t that stores a combination of two bytes. It would be better to treat these cases the same way everywhere.
What would you say, should the scope of this PR to be expanded?

[mpg]

What would you say, should the scope of this PR to be expanded?

I think it's up to you. You can either expand the scope of this PR, or address the other similar issues in a subsequent PR - IMO both ways are fine, so pick the one your prefer :)

[ronald-cron-arm]

This commit removes one check "(size_t)( end - p) < dn_size" and adds two casts (casts of dn_size line 2972 and 2974. Do we gain something eventually in terms of memory space and performance?

[mpg]

Regarding performance, I think it really doesn't matter here. The handshake includes heavy crypto computations, so writing and parsing messages is completely negligible in comparison. Correctness and safety (including readability and maintainability) are much more important than performance here.

Regarding code size, it's probably better to experiment than to try guessing, so I tried with arm-none-eabi-gcc -mthumb -march=armv6-m -Os -Wall -Wextra -Iinclude library/ssl_srv.c -c && arm-none-eabi-size ssl_srv.o and the code is 4 bytes smaller after the change.

Again, I think such a small difference is less important than correctness and readability here.

[irwir]

The header sais "ronald-cron-arm requested changes", but there is only a question.
Do you suggest any specific changes?

Do we gain something eventually in terms of memory space and performance?

In the very least, one check is removed, and it makes source code shorter.
Type casts were necessary to avoid compiler warnings, also smaller integer type makes overflow impossible.
The savings strongly depend on processor architecture and compiler, but memory access time with 16-bit values should not be greater compared to 32- or 64-bit values.

[ronald-cron-arm]

@mpg Thanks for clarifying the context.
"The header sais "ronald-cron-arm requested changes", but there is only a question."
Sorry I just didn't check the right choice.

Looking at this a bit more, there is a change in behavior that we may want to avoid. The typical (if not only) value for the size of the output buffer (defined by MBEDTLS_SSL_OUT_CONTENT_LEN) seems to be 16384 bytes. With this value and the current code, we detect with the checks if a "crt->subject_raw.len" length is greater that UINT16_MAX and return in error in that case. With the code change in this commit we could miss it because of the truncation when casting. This could be fixed here by checking the value of "crt->subject_raw.len" before the cast but then we are back to square one in terms of number of checks.

The current code seems quite optimal as long as MBEDTLS_SSL_OUT_CONTENT_LEN is lower or equal to UINT16_MAX. For larger sizes, things can go wrong it seems. Adding a static assert asserting that MBEDTLS_SSL_OUT_CONTENT_LEN is lower than UINT16_MAX and explaining that the code depends on that could be a possibility.

[irwir]

@ronald-cron-arm

With this value and the current code, we detect with the checks if a "crt->subject_raw.len" length is greater that UINT16_MAX and return in error in that case.

Both the original and this PR's code make no checks for 16-bit overflow.
Moreover, Visual Studio finds no UINT16_MAX string in the whole library code.

This could be fixed here by checking the value of "crt->subject_raw.len" before the cast but then we are back to square one in terms of number of checks.

That would be a new additional check, hence we are not back to square one.
The length always had to fit into 16 bits, but this PR makes the fact more evident.

Validation of MBEDTLS_SSL_OUT_CONTENT_LEN and MBEDTLS_SSL_IN_CONTENT_LEN probably could be done in check_config.h, but that deemed to be beyond the scope of this PR.

[mpg]

I think this is a very interesting conversation and unless I missed something it all boils down to the following two questions:

1. Is it safe to assume that the value stored in crt->subject_raw.len always fits in a uint16_t?
2. Is it safe to assume that the value of MBEDTLS_SSL_OUT_CONTENT_LEN will always fit in a uint16_t?

I think the second question is easier to answer: each version of the (D)TLS protocol has always fixed the default, and maximum value of MBEDTLS_SSL_OUT_CONTENT_LEN to 2^14 (see for example RFC 8446 Sec. 5.1). Furthermore, there are numerous places in the TLS protocol where length of records are encoded using two bytes. This includes protected records, including the header, padding and authenticated-encryption overhead, so even if some extension to the standard tried to replace the current 2^14 maximum with something higher, clearly the max length of an unprotected record's content will always be less than with some margin for the overhead unless the protocol changes deeply, which seems very unlikely at this point (we just went through the biggest change in the protocol since the transition from SSL 2.0 to SSL 3.0, and AFAIK allowing for larger records wasn't even on the radar).

So I think it's really safe to assume MBEDTLS_SSL_OUT_CONTENT_LEN is always strictly less than UINT16_MAX, with some margin actually.

The first point is a bit more complex: while the spec might limit the length so that it always fit in 11 bits (btw could you provide a more specific reference for that? RFC 5280 is pretty big), we can't just expect adversaries to respect the spec. (It just so happens that in the past we had a DoS where a crafted certificate with an overlong name could cause a stack overflow. We fixed that by parsing the name in an iterative rather than recursive way, but apparently missed that we could also enforce limits on the length of names.)

However in this case the name comes from from a list of locally-configured trusted roots. Clearly we have to assume that this list is not adversarial, or we're in much deeper trouble already.

So I think it's also safe to assume that crt->subject_raw.len fits, but since the reasoning was non-trivial, I think this cast would deserve a comment in the code explaining why it's safe.

[irwir]

could you provide a more specific reference for that?

The easiest key was already given, just search for upper bounds in the rfc.
It should be found near the end of rfc5280.appendix-A.1
Bounds values were given as character counts; that should be multiplied by character size. DIstinguished names should fit into 64 characters, but I voluntarily took the maximum of 256 for email string, and 4 bytes for UTF-8 encoding. It gives 1024, thus 11 bits for counter, which should be an overkill.

we can't just expect adversaries to respect the spec.

Of course; but in this respect truncation to 16 bits is safer than memcpy with 32 or 64 bits count.

Clearly we have to assume that this list is not adversarial, or we're in much deeper trouble already.

I could be wrong, but probably rfcs do not impose limits on certificate chain depth and total memory size.
These values might be limited in SSL libraries. For example, google found that OpenSSL sets limits as 100 KB for size (30 KB for 16-bit version) and 10 for depth.

[mpg]

As a matter of fact, we also impose limits on chain depth for verification, see MBEDTLS_X509_MAX_INTERMEDIATE_CA. That doesn't apply here, as we're not dealing with a chain to be verified but a list of trusted certs, but that's quite orthogonal to the changes your PR is doing anyway, so I'm just mentioning it FYI since we're touching on the subject.

So, as I said in my previous message, I think the code in the current version of this PR is correct (and slightly better than the code before the PR), but I'd like a comment above the cast dn_size = (uint16_t) crt->subject_raw.len; explaining briefly why it's correct, for example /* It follows from RFC 5280 A.1 that this length can be represented in at most 11 bits. */

@ronald-cron-arm Have your questions been answered to your satisfaction? Would you approve the PR with the current code plus a comment as I just suggested?

[ronald-cron-arm]

Yes I would approve the PR with the current code plus the suggested comment.

[irwir]

Thanks, the comment added.

That doesn't apply here, as we're not dealing with a chain to be verified but a list of trusted certs, but that's quite orthogonal to the changes your PR is doing anyway, so I'm just mentioning it FYI since we're touching on the subject.

I was not clear enough on this point.
It refers to total_dn_length that is also limited to 16 bits, and this could be enforced with restrictions similar to the one for the chain depth.

[irwir]

I think it's up to you. You can either expand the scope of this PR, or address the other similar issues in a subsequent PR - IMO both ways are fine, so pick the one your prefer :)

Thanks.
As I thied to implement the mentioned ideas, there were over 20 changes, and some byproducts too. It would be safer to make a separate PR, and hope that the current small PR could be merged soon.

[ronald-cron-arm]

Thanks for answering my various questions and for adding the comment. LGTM.

[mpg]

Thanks for adding the comment. All clear now.