Skip to content

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

Merged
alkonosst merged 2 commits intoalkonosst:masterfrom
tuan-karma:fix-global-linkage-problem-at-compiletime
Oct 6, 2023
Merged

add code to initialize default values for internal/protected variable…#8
alkonosst merged 2 commits intoalkonosst:masterfrom
tuan-karma:fix-global-linkage-problem-at-compiletime

Conversation

@tuan-karma
Copy link
Copy Markdown

@tuan-karma tuan-karma commented Jul 5, 2023

…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:

  • Hardware: ESP32 Dev Module
  • SSLClient version: v2.0.0

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

@alkonosst alkonosst merged commit 07e01ef into alkonosst:master Oct 6, 2023
@alkonosst
Copy link
Copy Markdown
Owner

Thank you :)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants