Skip to content

set _use_insecure back to false in setCACert method on WiFiClientSecure.cpp #8386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

tuan-karma
Copy link
Contributor

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

Description of Change

  • A small change at the WiFiClientSecure::setCACert that will help improve security when using WiFiClientSecure library.
  • This change does not break any current code interface.
  • I've just added _use_insecure = false at the end of the above method. It is neccessary to prevent accidentlly insecure connection of the end user in case they use wifiClientSecure.setInsecure() then in other endpoint they need to use setCACert.

Tests scenarios

The following test code (on ESP32 dev kit) will show potential security problem of using the library:

#include <Arduino.h>
#include "WiFiClientSecure.h"
#include "utils/WiFiFSM.h"
#include "utils/LED.h"
#include "root_ca.h"
#include "invalid_cert.h"

LED led(LED_PIN, LED_ACT);
WiFiFSM wifi(WF_SSID, WF_PASS, led);

WiFiClientSecure tlsClient;

void connect_443_insecure();
void connect_443_root_ca();
void connect_443_invalid_cert();

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

bool tested = false;

void loop()
{
    wifi.loop();

    if (!tested)
    {
        Serial.println("\nConnecting https with setInsecure():");
        connect_443_insecure();
        Serial.println("\nConnecting https with Root CA:");
        connect_443_root_ca();
        Serial.println("\nConnecting https with Invalid Cert:");
        connect_443_invalid_cert();
        tested = true;
    }
}

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

    tlsClient.setTimeout(10000); // default is 1000 ms
    tlsClient.setInsecure();

    bool connectRes = tlsClient.connect(host, port);
    Serial.printf("Connect Res: %s\n", connectRes ? "Success" : "Fail");

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

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

    tlsClient.setTimeout(10000); // default is 1000 ms
    tlsClient.setCACert(root_ca);

    bool connectRes = tlsClient.connect(host, port);
    Serial.printf("Connect Res: %s\n", connectRes ? "Success" : "Fail");

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

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

    tlsClient.setTimeout(3000); // default is 1000 ms
    tlsClient.setCACert(invalid_cert);

    bool connectRes = tlsClient.connect(host, port);
    Serial.printf("Connect Res: %s\n", connectRes ? "Success" : "Fail");

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

Following is the debuging message output from the above code:

Connecting https with setInsecure():
[  3244][D][ssl_client.cpp:176] start_ssl_client(): WARNING: Skipping SSL Verification. INSECURE!
Connect Res: Success

Connecting https with Root CA:
[  4440][E][WiFiClient.cpp:313] setSocketOption(): fail on 0, errno: 9, "Bad file number"
[  4490][D][ssl_client.cpp:176] start_ssl_client(): WARNING: Skipping SSL Verification. INSECURE!
Connect Res: Success

Connecting https with Invalid Cert:
[  5615][E][WiFiClient.cpp:313] setSocketOption(): fail on 0, errno: 9, "Bad file number"
[  5666][D][ssl_client.cpp:176] start_ssl_client(): WARNING: Skipping SSL Verification. INSECURE!

In this usage, we see that:

  • The setCACert will take no effect in the code. If the user hasn't turn on the debug flag, they will not notice this.

After modification the code as mentioned in the description section, the output debug message of the test code is:

Connecting https with setInsecure():
[  3230][D][ssl_client.cpp:176] start_ssl_client(): WARNING: Skipping SSL Verification. INSECURE!
Connect Res: Success

Connecting https with Root CA:
[  4348][E][WiFiClient.cpp:313] setSocketOption(): fail on 0, errno: 9, "Bad file number"
Connect Res: Success

Connecting https with Invalid Cert:
[  7176][E][WiFiClient.cpp:313] setSocketOption(): fail on 0, errno: 9, "Bad file number"
[  9027][E][ssl_client.cpp:37] _handle_error(): [start_ssl_client():273]: (-9984) X509 - Certificate verification failed, e.g. CRL, CA or signature check failed
[  9030][E][WiFiClientSecure.cpp:144] connect(): start_ssl_client: -9984
Connect Res: Fail

Now the library works as expected.

Related links

There is another problem with the library as you can see in the debug message:
[E][WiFiClient.cpp:313] setSocketOption(): fail on 0, errno: 9, "Bad file number" . I see it was reported at various places. I've try to solve this but it seem to be too complicated for me. I suggest to add a new void WiFiClientSecure::begin() method to address this problem, but it will break the existing code API. So I leave it as discussion (I am sorry).

Thank you!

@CLAassistant
Copy link

CLAassistant commented Jul 6, 2023

CLA assistant check
All committers have signed the CLA.

@me-no-dev me-no-dev merged commit 0950089 into espressif:master Jul 17, 2023
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.

3 participants