mbedtls_ssl_get_alert(): getter for fatal alerts

[ng-gsmk]

Description

Even though the TLS RFCs do not mandate libraries to expose Error Alerts (as defined in RFC8446 6.2 for TLS 1.3 and in RFC5246 7.2.2 for TLS 1.2) to the user, there are use cases when it is handy to get the actual last received fatal error instead of a generic one. For instance this enables the user to differ between received fatal errors in case mbedtls_ssl_handshake(), mbedtls_ssl_handshake_step() or mbedtls_ssl_read() returned MBEDTLS_ERR_SSL_FATAL_ALERT_MESSAGE.

This changesets stores the last incoming fatal alert in mbedtls_ssl_context and provides mbedtls_ssl_get_alert() as a getter for retrieving it. Another option would be to provide a callback mechanisms for all kinds of alerts (not only fatals) but for simplicity I discarded this option.

PR checklist

• changelog provided
• development PR provided
• TF-PSA-Crypto PR not required
• framework PR not required
• 3.6 PR not required because of new feature?
• tests provided

[bjwtaylor]

Hi @ng-gsmk, thanks for uploading this. There are currently CI failures, are you able to look into them?

[ng-gsmk]

I removed mbedtls_ssl_conf_rng() from the test case as it seems not needed and not available on all target platforms but I was wondering about the following report:

<problems_with_types severity="Medium">
  <header name="ssl.h">
    <type name="struct mbedtls_ssl_context">
      <problem id="Added_Field_And_Layout">
        <change target="in_alert_recv">Field @target has been added at the middle position of this structural type.</change>
        <effect>Layout of structure fields has been changed and therefore fields at higher positions of the structure definition may be incorrectly accessed by applications.</effect>
      </problem>
      <problem id="Added_Field_And_Layout">
        <change target="in_alert_type">Field @target has been added at the middle position of this structural type.</change>
        <effect>Layout of structure fields has been changed and therefore fields at higher positions of the structure definition may be incorrectly accessed by applications.</effect>
      </problem>
      </type>
  </header>
</problems_with_types>

<problems_with_types severity="Low">
  <header name="ssl.h">
    <type name="struct mbedtls_ssl_context">
      <problem id="Added_Field">
        <change target="in_alert_recv">Field @target has been added to this type.</change>
        <effect>This field will not be initialized or used by old client applications.</effect>
      </problem>
      <problem id="Added_Field">
        <change target="in_alert_type">Field @target has been added to this type.</change>
        <effect>This field will not be initialized or used by old client applications.</effect>
      </problem>
      </type>
  </header>
</problems_with_types>

What are the best practices in mbedtls when adding a new fields to mbedtls_ssl_context. I grouped it with the other alert stuff but this report indicates I should add it to the end?

[gilles-peskine-arm]

It's ok to change structures in the development branch and there's no particular reason to add new fields at the end. In Mbed TLS, struct types that are public are usually caller-allocated, so adding a field is an ABI change, but fields are usually private, and moving a private field isn't an ABI change. The tool we use for ABI comparison is not aware of how we use structures and which fields we consider to be private.

We tend to use the following considerations to order fields:

• Fields that are accessed often go earlier. If there's a field that's accessed much more often than others, it goes first. That tends to result in smaller code.
• Try to avoid gaps due to alignment requirements. An easy way is to put fields in decreasing order of alignment.
• But don't follow the guidelines above blindly. Try to group related fields, to make things easier on maintainers.

[ng-gsmk]

@bjwtaylor: PR tests seem to pass. The interface stability tests fail but this is expected I would say.

[minosgalanakis]

Looks good overall

This change is non intrusive and is simply adding a flag that is set on mbedtls_ssl_handle_message_type and reset at mbedtls_ssl_session_reset_msg_layer used for both the TLS1.2 and 1.3 stack.

A rebase on top of latest development and fixing the minor issues is needed but looks good

[minosgalanakis]

Extra whitespace here and we are missing clearing the ssl->in_alert_type

[ng-gsmk]

I'm sorry but I can't follow what you mean with the extra whitespace? Is there an extra whitespace in the changeset missing or one that should be removed?

Regarding the extra clearing: I followed the way how send alert was implemented (there is no clearing for the type for a reset neither) but added it now with 33bd8f8.

[minosgalanakis]

With regards to the whitespace I meant that instead of doing

ssl->in_alert_recv = 0;
ssl->in_alert_type = 0;

ssl->send_alert = 0;

We could have

ssl->in_alert_recv = 0;
ssl->in_alert_type = 0;
ssl->send_alert = 0;

But do not push a change just for that, wait for the second reviewer's comments.

Also same applies with rebase, I would recommend to do it after both reviewers have approved a pr, so you do not have to redo it every time the base branch has created a conflict ;)

Thanks for getting on it so quickly.

[minosgalanakis]

Not sure how I feel about it. The change is small to warrant a new discrete allert type for when there is no allert, but on the other side, a valid ssl context containing no allert is technically not a bad input, whereas it is a bad input for mbedtls_ssl_get_alert.

An option would be to return MBEDTLS_SSL_ALERT_MSG_CLOSE_NOTIFY when ssl->in_alert_recv != 1 but I also does not feel right error as well.

@ronald-cron-arm wdyt?

[ng-gsmk]

@minosgalanakis thanks for the review. As requested I rebased the PR on top of development and fixed some remarks (see in-place comments).