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

Simplify checks in ssl_write_certificate_request #3150

Merged
merged 1 commit into from
Apr 22, 2020
Merged

Simplify checks in ssl_write_certificate_request #3150

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 5 additions & 5 deletions library/ssl_srv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2841,7 +2841,7 @@ static int ssl_write_certificate_request( mbedtls_ssl_context *ssl )
int ret = MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE;
const mbedtls_ssl_ciphersuite_t *ciphersuite_info =
ssl->handshake->ciphersuite_info;
size_t dn_size, total_dn_size; /* excluding length bytes */
uint16_t dn_size, total_dn_size; /* excluding length bytes */
size_t ct_len, sa_len; /* including length bytes */
unsigned char *buf, *p;
const unsigned char * const end = ssl->out_msg + MBEDTLS_SSL_OUT_CONTENT_LEN;
Expand Down Expand Up @@ -2969,11 +2969,11 @@ static int ssl_write_certificate_request( mbedtls_ssl_context *ssl )

while( crt != NULL && crt->version != 0 )
{
dn_size = crt->subject_raw.len;
/* It follows from RFC 5280 A.1 that this length
* can be represented in at most 11 bits. */
dn_size = (uint16_t) crt->subject_raw.len;
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm Apr 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ronald-cron-arm ronald-cron-arm Apr 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@irwir irwir Apr 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@irwir irwir Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.


if( end < p ||
(size_t)( end - p ) < dn_size ||
(size_t)( end - p ) < 2 + dn_size )
if( end < p || (size_t)( end - p ) < 2 + (size_t) dn_size )
{
MBEDTLS_SSL_DEBUG_MSG( 1, ( "skipping CAs: buffer too short" ) );
break;
Expand Down