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

Padding-only Application Data frames seem to be decoded incorrectly in CBC ciphers #1632

Closed
kFYatek opened this Issue May 16, 2018 · 5 comments

Comments

Projects
None yet
7 participants
@kFYatek

kFYatek commented May 16, 2018

Description

  • Type: Bug
  • Priority: Major

Bug

OS
linux

mbed TLS build:
Version: cdd97fd63282779907a37b6a785bfbcf1b8fa964 (current development), also reproduced at least on 2.8.0
OS version: Ubuntu 18.04
Configuration: default (cmake .)
Compiler and options: gcc 7.3.0

Peer device TLS stack and version
OpenSSL 1.1.0g (from Ubuntu 18.04 default repo); also reproduced on OpenSSL 1.1.0h from Arch Linux' default repo

Steps to reproduce
Easiest way to reproduce consistently is to use a Docker VM:

$ docker run -ti ubuntu:18.04
docker# apt update
docker# apt install openssl build-essential cmake git python3
docker# git clone https://github.com/ARMmbed/mbedtls.git
docker# cd mbedtls
docker# cmake .
docker# make
docker# yes '' | openssl req -x509 -newkey rsa:2048 -keyout key.pem -out cert.pem -days 365 -nodes
docker# openssl s_server -tls1 -cert cert.pem -key key.pem -www &
docker# programs/ssl/ssl_client1 | python3 -c "import sys; list(print('%r' % line) for line in sys.stdin.readlines())"

Note that:

  • Python is used here only to filter ssl_client1's output so that non-printable characters are immediately visible; they otherwise are usually invisible in common terminal emulators
  • the problem happens when TLS version is forced to TLS 1.0 (here I'm doing it with the -tls1 option to OpenSSL's server) - it causes the ECDHE-RSA-AES256-SHA ciphersuite to be used in CBC mode; with TLS 1.2 the ciphersuite is negotiated to ECDHE-RSA-AES256-GCM-SHA384, which does not cause this problem
  • I know that using TLS 1.0 is insecure; with this bug report I'm not concerned with security, rather than with interoperability; I haven't checked whether the same behaviour is reproducible on TLS 1.2 or not

After running the above commands, you can see something like this:

'  > Write to server: 18 bytes written\n'
'\n'
'GET / HTTP/1.0\r\n'
'\r\n'
'  < Read from server: 16 bytes read\n'
'\n'
'\x0f\x0f\x0f\x0f\x0f\x0f\x0f\x0f\x0f\x0f\x0f\x0f\x0f\x0f\x0f\x0f 1023 bytes read\n'
'\n'
'HTTP/1.0 200 ok\r\n'
'Content-type: text/html\r\n'
[...]

The client receives 16 \x0f bytes before the actual content. Examining the packets in e.g. Wireshark shows that the OpenSSL-based server in this configuration sends an additional Application Data frame which is 36 bytes long, that, after decryption, contains just these 16 bytes of padding. However, mbed TLS does not treat it as padding-only (i.e. effectively 0-byte long) data frame, but passes this padding to the application.

The problem seems to be caused by the way padding is handled in ssl_decrypt_buf(); ssl_tls.c:1915 is:

            correct &= ( ssl->in_msglen >= padlen + 1 );

And in this case both in_msglen and padlen are 16, so padding is treated as incorrect by this code. I couldn't find any information in the TLS 1.0 (or TLS 1.2 for that matter) RFC that would prohibit such padding-only frame, so I think that it is a bug on mbed TLS side rather than in OpenSSL.

Another issue is: why padding that is considered incorrect is passed to the application layer instead of e.g. causing an error code to be returned from mbedtls_ssl_read()?

Disclaimer: I don't consider myself an expert in TLS or cryptography in general by any means; my analysis is based on a humble programmer's common sense, and I do realize that it does not always work in the field of security. It might be the case that it is OpenSSL where the actual bug is.

@hanno-arm

This comment has been minimized.

Contributor

hanno-arm commented May 16, 2018

Hi @kFYatek,

thank you very much for your report!

You are entirely right, this is a known bug in Mbed TLS that we are aware of for a while and are tracking it internally; another customer also reported it recently in the Mbed TLS forum.

Also, your analysis of the source of the problem matches ours.

A fix for this bug will hopefully be available soon, although I cannot make any promises on when exactly we will be able to schedule time for this.

Thank you very much for your interest and help - good catch! 👍
Hanno

@hanno-arm

This comment has been minimized.

Contributor

hanno-arm commented May 16, 2018

Internal Reference: IOTSSL-1920.

@tzxespressio

This comment has been minimized.

tzxespressio commented Jun 19, 2018

Hi @hanno-arm , what's the status of the bug?Is it solved?

projectgus added a commit to espressif/mbedtls that referenced this issue Jun 20, 2018

@projectgus

This comment has been minimized.

Contributor

projectgus commented Jun 20, 2018

There's a possible fix for this in the commit linked above. Currently waiting for signed corporate CLA (for Espressif Systems) to come through, then I'll make a dedicated bugfix branch and send this in a PR for consideration.

projectgus added a commit to espressif/mbedtls that referenced this issue Jul 6, 2018

@RonEld RonEld added the fix available label Jul 8, 2018

projectgus added a commit to espressif/mbedtls that referenced this issue Jul 12, 2018

projectgus added a commit to espressif/mbedtls that referenced this issue Jul 12, 2018

projectgus added a commit to espressif/mbedtls that referenced this issue Jul 13, 2018

andresag01 added a commit to andresag01/mbedtls that referenced this issue Jul 16, 2018

andresag01 added a commit to andresag01/mbedtls that referenced this issue Jul 16, 2018

@sbutcher-arm sbutcher-arm added tracking and removed mirrored labels Jul 16, 2018

@ciarmcom

This comment has been minimized.

Member

ciarmcom commented Jul 16, 2018

ARM Internal Ref: IOTSSL-2396

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment