Fixed potential null dereference when record splitting is enabled. #185
Conversation
|
Any comments on this one? |
|
Hi, I think this is (I was mistaken about the problem in the application. Please disregard the next paragraph, which I leave just to void breaking the replies.) The bug in our code, obviously, is that we should handle that more gracefully than by segfaulting. While your patch indeed avoids the segfault, I'll probably re-arrange things a bit to solve the problem in another way. Thanks for your report and the proposed patch (even if I'm not using it this time)! |
|
I've tried the patch and there were indeed no more segfaults, but OpenVPN still couldn't finish the initialization sequence https://community.openvpn.net/openvpn/ticket/524#comment:3 |
|
Please try the following: a2fce21 |
|
The outcome was the same as with xdarklight's patch: TLS handshake failed. I've used https://tls.mbed.org/download/mbedtls-1.3.10-gpl.tgz + a2fce21 |
|
There are 2 patches being applied when building OpenWRT with libpolarssl: One of them disables POLARSSL_SSL_PROTO_SSL3, for example. Could this cause the issue? |
|
I can confirm @amq's findings (and sorry for the late reply). With "record splitting" enabled all I get is TLS errors. Please let us know if you need more information. |
|
The patches applied do not seem to be related to this issue at all. What I would need to get a better grasp of the issue is:
Anyway, I'm not sure what your use case is, but disabling record splitting is not an issue if the BEAST attack is not a threat to you. By the way, which version of TLS are you using? Normally record splitting should only happen when TLS 1.0 is used... Also, this is most probably completely unrelated but I'm just curious: you seem to be doing TLS over UDP? |
|
@mpg: OpenVPN does not explicitly perform the handshake. It never has since PolarSSL 0.14, and has always worked fine. Your documentation seems to imply it should not be needed. See https://tls.mbed.org/module-level-design-ssl-tls, which says: Currently, this only fails in OpenVPN when record splitting is being used. I.e. when a TLSv1.0 connection is set up. (And wrt UDP: OpenVPN has its own reliability layer on top of UDP for TLS, which predates DTLS. It uses this to multiplex a TLS connection and its own datagram VPN connection over a single UDP 'connection'.) Edit: The SSL context init is done in key_state_ssl_init(): Call to ssl_write() are done from key_state_ssl_write_plaintext_const(): (Edit: I changed openvpn to call ssl_write() from one function only, and updated the links to that commit.) |
|
Oh, and for the record: commit a2fce21 indeed fixes the polarssl part of this issue for me. The other part is that openvpn assumes that its TLS payload is sent (and then received) all at once, so enabling record splitting will indeed break openvpn. Disabling record splitting is the quick fix (and should not be a problem for openvpn), but being able to deal with record splitting feels like the better solution to me. But I guess that's openvpn's problem. |
|
@syzzer: So should we simply add this to key_state_ssl_init() in OpenVPN? That disables record splitting for OpenVPN even though it's enabled in the config. |
|
@xdarklight: sort of, but '0' actually enables record splitting. Also see my comments in the ticket in the openvpn bugtracker. I posted the code that works for me: |
OpenVPN assumes that its control channel messages are sent and received unfragmented, this assumption is broken when CBC record splitting is enabled in mbedTLS. The record splitting is intended as countermeasure against BEAST attacks which do not apply to OpenVPN, therefore we simply disable it until upstream OpenVPN gains the ability to process fragmented control messages. Disabling the splitting also works around a (not remotely triggerable) segmentation fault in mbedTLS. References: * https://dev.openwrt.org/ticket/19101 * https://community.openvpn.net/openvpn/ticket/524 * Mbed-TLS/mbedtls#185 Signed-off-by: Jo-Philipp Wich <jow@openwrt.org> git-svn-id: svn://svn.openwrt.org/openwrt/trunk@45602 3c298f89-4303-0410-b956-a3cf2f4a3e73
|
@syzzer: regarding the need to call Regarding the remaining issue, thanks for clarifying that the cause is OpenVPN assumes unfragmented records. I agree with you that disabling record splitting is the way to go here. (Thanks for the explanation regarding TLS/UDP.) @xdarklight: please always use the defined constants with By the way, since you're already patching Also, I'd like to mention, just in case, that we also provide a script |
OpenVPN assumes that its control channel messages are sent and received unfragmented, this assumption is broken when CBC record splitting is enabled in mbedTLS. The record splitting is intended as countermeasure against BEAST attacks which do not apply to OpenVPN, therefore we simply disable it until upstream OpenVPN gains the ability to process fragmented control messages. Disabling the splitting also works around a (not remotely triggerable) segmentation fault in mbedTLS. References: * https://dev.openwrt.org/ticket/19101 * https://community.openvpn.net/openvpn/ticket/524 * Mbed-TLS/mbedtls#185 Signed-off-by: Jo-Philipp Wich <jow@openwrt.org> git-svn-id: svn://svn.openwrt.org/openwrt/trunk@45602 3c298f89-4303-0410-b956-a3cf2f4a3e73
Remove method dispatch from md
Found by some OpenWrt users that are reporting problems with OpenVPN (in client mode):
https://dev.openwrt.org/ticket/19101