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.

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

PEM parsing functions expect the last byte of the buffer to be 0 #3896

Open
gilles-peskine-arm opened this issue Nov 18, 2020 · 1 comment
Open
Labels
component-crypto Crypto primitives and low-level interfaces component-x509

Comments

@gilles-peskine-arm
Copy link
Contributor

Several key and X.509 parsing functions expect either DER or PEM input. When the input is PEM, these functions check that the last byte of the buffer is a null byte. The intent is to ensure that the buffer is null-terminated, but the check is too tight: arguably, PEM data followed by a null byte followed by trailing garbage is a valid input.

The functions with this idiom are: mbedtls_dhm_parse_dhm, mbedtls_pk_parse_key, mbedtls_pk_parse_public_key, mbedtls_x509_crl_parse, mbedtls_x509_crt_parse, mbedtls_x509_csr_parse.

The behavior dates back to fb07757 which was first released in Mbed TLS 2.0. (Before, PEM data did not need to be null-terminated. #222 complained about the behavior change.) It is not documented: the documentation only states that the PEM data must be null-terminated. There is one exception: for the pk functions, the documentation was changed in 159171b (not present in any LTS branch) to state

The buffer must contain the input exactly, with no extra trailing material. For PEM, the buffer must contain a null-terminated string.

The first sentence actually only applies to DER. PEM is allowed to have trailing material, but this trailing material must be null-terminated.

Goal of this issue:

  • Decide exactly what behavior we want.
  • Implement it.
  • Document it.
  • Test it.

We may choose different behavior in LTS branches to maximize backward compatibility.

Note that the parsing behavior is not symmetrical to the writing behavior, and this has caused a break of backward compatibility in LTS branches: #3682. Writing PEM data leaves non-null-terminated trailing garbage, as an unintended consequence of #3488.

@gilles-peskine-arm gilles-peskine-arm added component-x509 component-crypto Crypto primitives and low-level interfaces Product Backlog labels Nov 18, 2020
@laumor01 laumor01 moved this from Incoming Items to Prioritised Backlog in OBSOLETE - SEE https://github.com/orgs/Mbed-TLS/projects/3 Jan 6, 2021
jerome-pouiller added a commit to SiliconLabs/wisun-br-linux that referenced this issue Sep 2, 2021
Because of Mbed-TLS/mbedtls#3896, mbedtls
expects PEM certificates to be strings. Therefore, they needs to be
terminated by \0.

Note that we take to not break support for DER format.

Also check mbedtls_x509_crt_parse() to see the real algorithm used by
mbedtls.
@DarkDust
Copy link

This bug forces me to manually wrap a DER key in a PEM armor. If I wrap the key in a buffer with additional \0, the gets past the PEM null-termination checks but then pk_get_rsapubkey exits because *p + len != end (off-by-one…).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-crypto Crypto primitives and low-level interfaces component-x509
Projects
None yet
Development

No branches or pull requests

4 participants