Modbus/TCP Security

[mkferst]

This is a first approach to adding TLS support to libmodbus, by creating a new modbus_backend_t. Although functional, the Modbus/TCP Security is not fully implemented, notably missing Role-Based Client Authorization.

OpenSSL is used as the TLS implementation, currently tested with version 1.1.1. The new "--enable-tls" option, witch defaults to "no", is added to control the inclusion of this new dependency. I also developed this backend based on mbedTLS, but omited it here since OpenSSL seems to be a more sensible option for general use cases. If desired, I can send the patches to make "--enable-tls" select which lib to use.

In the second patch, in modbus-tcp.c, when send-ing (line 182 to 223) or recv-ing (line 235 to 276), it may be necessary to select for read or write. I'm not sure whitch timeout to use for these calls, so I left it with NULL and marked the code with "TODOs". The code works this way, but its not a desirable solution.

This work was funded by the Research and Development project PD 2866-0468/2017, granted by the Brazilian Electricity Regulatory Agency (ANEEL) and Companhia Paranaense de Energia (COPEL). Some results using the mbedTLS backend where presented at INDUSCON 2018 and COBEP 2019.

[calligp]

Hello Matheus,

First of all, many thanks for you good work on implementing TLS. It has helped me a lot to integrate this security layer in my project.

My first remark is about the compatibility with OpenSSL version. Indeed, your code only compiles with a recent release (>= 1.1.1). In my case I am using a version 1.0.2 (soon to be migrated to 1.1.1 but not yet, unfortunately). That's why I have added a preprocessor check around the creation of the SSL context. These macros (TLS_method & TLS1_2_VERSION) are not yet available in 1.0.2. Based on the preprocessor check, we replace the missing macros by the equivalent ones in previous OpenSSL version. This ensures the retro-compatibility with OpenSSL 1.0.2 and 1.1.1. I have not tested with any other versions.
See the attached patch 0001-Try-to-fix-compatibility-with-openssl-1.0.2.txt

The second is about the unit tests. When I configure the library to build without TLS, the compilation fails. I think the macro USE_TLS is defined even though it should not be.
Maybe I am missing something?

$ ./autogen.sh
$ ./configure
...

        libmodbus 3.1.6
        ===============

        prefix:                 /usr/local
        sysconfdir:             ${prefix}/etc
        libdir:                 ${exec_prefix}/lib
        includedir:             ${prefix}/include

        compiler:               gcc
        cflags:                 -g -O2
        ldflags:                

        tls:                    no
        documentation:          no
        tests:                  yes
...
$ make
...
CCLD     unit-test-server
/usr/bin/ld: unit-test-server.o: in function `main':
/tmp/libmodbus/tests/unit-test-server.c:77: undefined reference to `modbus_new_tls'
/usr/bin/ld: /tmp/libmodbus/tests/unit-test-server.c:121: undefined reference to `modbus_tls_listen'
/usr/bin/ld: /tmp/libmodbus/tests/unit-test-server.c:122: undefined reference to `modbus_tls_accept'
collect2: error: ld returned 1 exit status
make[3]: *** [Makefile:669: unit-test-server] Error 1
make[2]: *** [Makefile:589: all] Error 2
make[1]: *** [Makefile:502: all-recursive] Error 1
make: *** [Makefile:388: all] Error 2

My first adaptation of this pull request has been to add a new errno value to notify about certificate validation issue.
Indeed my backend application, integrating the modbus library, requires to be notified about this reason of connection failure. To this end, I have added a new modbus error code EMBCERT which is triggered when the SSL prerequisites are not satisfied.
See the attached patch 0002-Add-special-modbus-error-about-certificate-validatio.txt

Finally, I encountered some issues when I tried to conciliate my project requirements and your TLS integration. Mainly I need to be able to accept a self-signed certificate and the SSL library default certificate validation (which you are using in your implementation) does not allow it. Without any changes, when my client receives a self-signed certificate, the method SSL_get_verify_result returns an error X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT.
To have the capability to customize SSL behaviour regarding certificate validation, I have changed the prototype of the method modbus_new_tls to replace the PEM files by a pointer on a SSL_CTX instance.
This way, the criteria to establish communication with a server (certificate, encryption, ...) are delegated outside of the library. This should allow more flexibility in configuring security.
See the attached patch 0003-Review-interface-for-tls-initialization.txt

Below an example of use of this modified interface.

    typedef struct sTcpSecurityType
    {
        int mSecurityMode;
        const char* mClientCrt;
        const char* mClientKey;
        const char* mServerCA;
        bool mAcceptSelfSigned;
    } sTcpSecurityType;

    static int verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
    {
        int err;
        sTcpSecurityType *wSecurity;
        SSL *ssl;
        SSL_CTX *sslCtx;

        /*
         * Retrieve the pointer to the SSL of the connection currently treated
         * and the application specific data stored into the SSL object.
         */
        ssl = static_cast<SSL*>(X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx()));
        sslCtx = SSL_get_SSL_CTX(ssl);
        wSecurity = static_cast<sTcpSecurityType*>(SSL_CTX_get_ex_data(sslCtx, 0));
        if (wSecurity)
        {
            err = X509_STORE_CTX_get_error(ctx);

            // Accept self signed cert is not standard in OpenSSL
            if (wSecurity->mAcceptSelfSigned)
            {
                if (
                    err == X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT ||
                    err == X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN
                )
                {
                    // reset error
                    X509_STORE_CTX_set_error(ctx, 0);
                }
            }
        }
        else
        {
            // security is null then let validation continue
        }

        return 1;
    };

...

    SSL_CTX* sslCtx = SSL_CTX_new(TLSv1_2_method());

    // Declare callback for certificate validation and associated context
    // This part is optional
    SSL_CTX_set_verify(aSslCtx, SSL_VERIFY_PEER, verify_callback);
    SSL_CTX_set_ex_data(aSslCtx, 0, (void*)&mSecurityContext);

    // Load the client/server certificates
    SSL_CTX_use_certificate_file(aSslCtx, mSecurityContext.mClientCrt, SSL_FILETYPE_PEM);
    SSL_CTX_use_PrivateKey_file(aSslCtx, mSecurityContext.mClientKey, SSL_FILETYPE_PEM);
    SSL_CTX_check_private_key(aSslCtx);
    SSL_CTX_load_verify_locations(aSslCtx, mSecurityContext.mServerCA, NULL);

    SSL_CTX_set_verify_depth(aSslCtx, 1);

    mModbusContext = modbus_new_tls(aAddress.data(), aPort, sslCtx);

This last modification is very specific and is a big rework of your implementation. Nevertheless in my point of view, the dependency injection can be a good practice to allow users to define and customize the SSL library behaviour without patch.
I let you decide if you want to integrate my patches.

Best regards.

[mkferst]

Hello Matheus,

First of all, many thanks for you good work on implementing TLS. It has helped me a lot to integrate this security layer in my project.

Hi @calligp. I'm glad it was useful! Do keep in mind the TODOs though, it's something that needs to be fixed before it gets to production. Otherwise, your application may be stuck forever in one of the calls to select.

My first remark is about the compatibility with OpenSSL version. Indeed, your code only compiles with a recent release (>= 1.1.1). In my case I am using a version 1.0.2 (soon to be migrated to 1.1.1 but not yet, unfortunately). That's why I have added a preprocessor check around the creation of the SSL context. These macros (TLS_method & TLS1_2_VERSION) are not yet available in 1.0.2. Based on the preprocessor check, we replace the missing macros by the equivalent ones in previous OpenSSL version. This ensures the retro-compatibility with OpenSSL 1.0.2 and 1.1.1. I have not tested with any other versions. See the attached patch 0001-Try-to-fix-compatibility-with-openssl-1.0.2.txt

The project that I was working on when this patch was developed started with 1.0.x, and we did the upgrade. I preferred to submit this newer version as major distributions with libmodbus packages were already using 1.1.x. Supporting both OpenSSL versions can be tricky because some methods may have functional differences that wouldn't break the compilation but have some problem at runtime.

The second is about the unit tests. When I configure the library to build without TLS, the compilation fails. I think the macro USE_TLS is defined even though it should not be. Maybe I am missing something?

Yeah, there is something wrong. It's not defined in the generated config.h, but in tests/unit-test.h:

#define HAVE_INTTYPES_H 1
#define HAVE_STDINT_H 1
#define USE_TLS @USE_TLS@

I don't know much about autotools, but I guess we'll need USE_TLS always defined with some value:

diff --git a/configure.ac b/configure.ac
index 363b35f..a7ff189 100644
--- a/configure.ac
+++ b/configure.ac
@@ -160,7 +160,8 @@ AM_CONDITIONAL(BUILD_TLS, [test $enable_tls != no])
 AS_IF([test $enable_tls != no],
        [AC_DEFINE([USE_TLS], [1], [Define if SSL/TLS feature is enabled])]
        [AC_CHECK_LIB([ssl], [SSL_new])]
-       [AC_CHECK_LIB([crypto], [CRYPTO_new_ex_data])],)
+       [AC_CHECK_LIB([crypto], [CRYPTO_new_ex_data])],
+       [AC_DEFINE([USE_TLS], [0],)])
 
 AC_CONFIG_HEADERS([config.h tests/unit-test.h])
 AC_CONFIG_FILES([

And then change all #if defined(USE_TLS) to #if USE_TLS. There may be better solutions, but then again, I don't know much about autotools.

My first adaptation of this pull request has been to add a new errno value to notify about certificate validation issue. Indeed my backend application, integrating the modbus library, requires to be notified about this reason of connection failure. To this end, I have added a new modbus error code EMBCERT which is triggered when the SSL prerequisites are not satisfied. See the attached patch 0002-Add-special-modbus-error-about-certificate-validatio.txt

At the time, we didn't have to handle connection failure reasons, but it sure is a necessary error code.

Finally, I encountered some issues when I tried to conciliate my project requirements and your TLS integration. Mainly I need to be able to accept a self-signed certificate and the SSL library default certificate validation (which you are using in your implementation) does not allow it. Without any changes, when my client receives a self-signed certificate, the method SSL_get_verify_result returns an error X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT. To have the capability to customize SSL behaviour regarding certificate validation, I have changed the prototype of the method modbus_new_tls to replace the PEM files by a pointer on a SSL_CTX instance. This way, the criteria to establish communication with a server (certificate, encryption, ...) are delegated outside of the library. This should allow more flexibility in configuring security. See the attached patch 0003-Review-interface-for-tls-initialization.txt

As I mentioned in the PR, the code originally supported OpenSSL and mbedTLS, so it couldn't have any parameter specific to one or the other. The application code itself wouldn't even include any TLS related header. However, if it's decided to support a single TLS implementation, modbus_new_tls receiving the context struct is probably the way to go.

This last modification is very specific and is a big rework of your implementation. Nevertheless in my point of view, the dependency injection can be a good practice to allow users to define and customize the SSL library behaviour without patch. I let you decide if you want to integrate my patches.

Thanks for your feedback and for taking the time to post the patches here. I'm not planning to maintain this code as a fork, so I wouldn't change it without a clear path to upstream it. However, I encourage you to keep this work and have the patchs in a public branch, so people with these use cases can more easily find them.