alkonosst / SSLClientESP32

SSLClient - generic secure client Arduino library using mbedtls
GNU General Public License v3.0
7 stars 5 forks source link

add code to initialize default values for internal/protected variable… #8

Closed tuan-karma closed 1 year ago

tuan-karma commented 1 year ago

…s in the SSLClientESP32 class, improve the security of the code

Description

All the private/protected variables need to be initialized to default values as following: (in the SSLClientESP32.h file)

class SSLClientESP32 : public Client
{
protected:
    SSLClientLib::sslclient_context *sslclient;

    int _lastError = 0;
    int _peek = -1;
    int _timeout = 0;
    bool _use_insecure = false;
    const char *_CA_cert = nullptr;
    const char *_cert = nullptr;
    const char *_private_key = nullptr;
    const char *_pskIdent = nullptr; // identity for PSK cipher suites
    const char *_psKey = nullptr; // key in hex for PSK cipher suites
    const char **_alpn_protos = nullptr;
    bool _use_ca_bundle = false;

Also, insert _use_insecure = false; to the SSLClientESP32::SSLClientESP32(Client* client) constructor in the SSLClientESP32.cpp in order to prevent an accidently insecure behavior (skip certificate validation without users notice) when initialize the object at run time (for example, the users declare and initialize an SSLClientESP32 object inside a local function).

Motivation and context

The following usage will lead to unexpected insecure behavior:

#include <Arduino.h>
#include <WiFi.h>
#include "SSLClientESP32.h"

#include "utils/WiFiFSM.h"
#include "utils/LED.h"
#include "root_ca.h"

void get_request_root_ca();

void setup()
{
    Serial.begin(115200);
    wifi.on();
    delay(3000);
    wifi.loop();

    get_request_root_ca();
}

void loop()
{
    wifi.loop();
}

void get_request_root_ca()
{
    const char *host = "jsonplaceholder.typicode.com";
    const uint16_t port = 443U;
    const char *resource = "/posts/2";

    WiFiClient baseClient;
    SSLClientESP32 sslClient(&baseClient);

    sslClient.setCACert(root_ca);
    bool connected = sslClient.connect(host, port);

    sslClient.stop(); // disconnect and clear the buffer
}

The above code will successfully connect and perform a GET request even with the invalid/wrong root_ca, without the users' notice.

How this has been tested?

This library will do correctly as expected if users declare and initiallize a SSLClientESP32 sslClient(&baseClient); at the global scope. But it fail to verify the root_ca if you initiallize the object in a local scope (so it will initiallize at runtime).

After do some small modifications as decribed in the first section, I've tested the above code with a valid root_ca --> it will perform verification successfully. If the root_ca is invalid it will reject the connection.

I've also tested other usual use-cases as indicated in the documentation, it still works. That means my modification does not break any previous code.

Formally, the library will wrongly fallback to insecure mode, and the setCACert(root_ca) never takes effect when initiallizing the object locally.

Tested with:

I would like to thank the author of this library for his time and contribution!

alkonosst commented 1 year ago

Thank you :)