Mbed TLS is now part of TrustedFirmware.org.

Question about CVE-2015-5291


Oct 19, 2015 17:57
James Cowgill

Hi,

The Debian bug about this CVE has been sitting for a week now with no apparent action so I thought I'd look at it. https://bugs.debian.org/801413

The versions in Debian are a bit old and since there has been an ABI break since then, actually updating to the new versions is very difficult.

I had a look at which exact commits fixed this. Would applying these commits be enough to fix the CVE?

To 1.2:

13ca8951

Author: Simon Butcher <simon.butcher@arm.com>
Date:   Wed Sep 30 00:45:21 2015 +0100
    Added max length checking of hostname

To 1.3:

c988f32a

Author: Simon Butcher <simon.butcher@arm.com>
Date:   Tue Sep 29 23:27:20 2015 +0100
    Added max length checking of hostname

b1e325d6

Author: Simon Butcher <simon.butcher@arm.com>
Date:   Thu Oct 1 00:24:36 2015 +0100
    Added bounds checking for TLS extensions
    IOTSSL-478 - Added checks to prevent buffer overflows.

643a922c

Author: Simon Butcher <simon.butcher@arm.com>
Date:   Thu Oct 1 01:17:10 2015 +0100
    Reordered extension fields and added to ChangeLog
    Reordered the transmission sequence of TLS extension fields in client hello
    and added to ChangeLog.

f3e6e4ba

Author: Manuel Pégourié-Gonnard <mpg2@elzevir.fr>
Date:   Fri Oct 2 09:53:52 2015 +0200
    Add extra check before integer conversion
    end < p should never happen, but just be extra sure

James

 
Oct 20, 2015 14:30
Simon Butcher

The maintenance release that was made to address CVE-2015-5291 contains additional fixes for other weaknesses unrelated to the CVE, as can be seen from the ChangeLog or release notes.

We recommend you use the last release for the branch most suited for your needs, and do not recommend you only apply the commits you list as otherwise you'll miss other security related improvements and fixes.

The Debian PolarSSL/mbed TLS package appears to be using version 1.3.9 at the moment (see here), so we would suggest you move to 1.3.14 to avoid missing other fixes.

 
Oct 20, 2015 15:29
James Cowgill

Obviously that would be the best solution and that's ok for unstable/testing, but that can't be done for any of the stable releases since the SOVERSION of libpolarssl was changed. Uploading it now would break all the reverse dependencies which are using a different ABI.

I suppose one alternative is to take 1.3.14 and then revert the changes which caused the break in ABI, instead of trying to fix this bug on top of 1.3.9 currently in Debian jessie.

 
Oct 20, 2015 15:54
Manuel Pégourié-Gonnard

Sounds like a viable option. Actually since ABI changes were only introduced when adding new features, I don't think you'd need to revert the changes themselves, just disable the corresponding features in config.h.

 
Oct 20, 2015 16:16
Manuel Pégourié-Gonnard

% git diff polarssl-1.3.9..mbedtls-1.3.14 include/polarssl/config.h | grep '^+#'
+#define POLARSSL_SSL_ENCRYPT_THEN_MAC
+#define POLARSSL_SSL_EXTENDED_MASTER_SECRET
+#define POLARSSL_SSL_FALLBACK_SCSV
+#define POLARSSL_SSL_CBC_RECORD_SPLITTING

So, just disabling those four things in config.h (either by applying a patch to it or by invoking scripts/config.pl in a pre-build script) should be enough to restore ABI compatibility.

If you want to check that disabling those 4 options is indeed enough to preserve ABI, you can use a small program that includes mbedtls/ssl.h and then prints the size of the various structures defined in that header: they should be identical between 1.3.9 and 1.3.14+patch.

Of course you'll also need to patch library/Makefile to restore the old soversion if you go that way.

It should be noted that since the 1.3 branch is now in maintenance mode, we will never change the ABI in that branch again. So, patching config.h now might be worth the effort since once it's done you can then apply all future upgrades to the 1.3 branch without any more trouble.

That might the best way to guarantee you're able to release security fixes in a timely fashion in the future.

 
Oct 20, 2015 16:29
James Cowgill

Thanks!

That sounds like a good idea. I hadn't actually checked what parts of the ABI were broken, but if it's just new functions then what you suggest should be ok.

 
Oct 20, 2015 16:54
Manuel Pégourié-Gonnard

Actually the issue is not new functions, if new members in structures (mostly ssl_context, ssl_handshake, ssl_session, etc) that causes ABI incompatibilities. Since we take care to skip those members when the corresponding feature is not enabled, disabling the flags in config.h should result in structure definitions that are the same as before the feature is introduced.

 
Oct 20, 2015 17:46
Manuel Pégourié-Gonnard

Ok, I ran some tests and I was wrong, disabling those four flags does not seem to be enough to restore ABI compatibility, due to other changes that are not controlled by config.h flags, as well as a prototype change in an internal but technically public function.

It might still be possible to isolate the changes that caused ABI changes and exclude them somehow, but it's not as easy as I initially thought.

This would need more investigation, but unfortunately I'm unlikely to have the time for that in the next few days. So maybe hand-picking the changes that fix the recent severe issue is the best short-term solution, though it would be more satisfying long-term to be able to incorporate all fixes, including for the less severe issues.

 
Oct 21, 2015 12:52
James Cowgill

I did some investigation and came up with this here: https://github.com/jcowgill/mbedtls/commits/debian-jessie-compatibility

It should preserve the ABI, although there are a lot of changes. I am a little uneasy about applying it to something like Debian which will then have to maintain it (and be responsible for any bugs).

I posted the libabigail diffs here: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=801413#25

James

 
Oct 21, 2015 13:17
Manuel Pégourié-Gonnard

Nice job! I guess now the decision belongs to the Debian developers who will have to take core of future updates in Debian stable. On one hand, your ABI-restoring patches are fairly big, but on the other hand, handpicking all security fixes (not just the most severe) would likely result in an even bigger set of patches, that is unfortunately likely to keep growing in the future, while the ABI-restoring patches should remain more stable now that the 1.3 branch is in maintenance mode.