espressif / arduino-esp32

Arduino core for the ESP32
GNU Lesser General Public License v2.1
13.42k stars 7.37k forks source link

Incorrect validation when calling HTTPClient::begin(....) methods to use ssl for self-certificate #9131

Open davsuapas opened 8 months ago

davsuapas commented 8 months ago

Board

ESP32 Dev Module

Device Description

Not relevant

Hardware Configuration

Not relevant

Version

latest master (checkout manually)

IDE Name

VSCode

Operating System

Linux

Flash frequency

40 Mhz

PSRAM enabled

no

Upload speed

115200

Description

I am using HttpClient to connect to a server with a self-certificate in development mode and I use the method:

bool HTTPClient::begin(String host, uint16_t port, String uri, const char* CAcert)

As I don't want it to validate the certificate in the CAcert parameter I use nullptr. As I have seen the TLSTraits class, if the _cacert == nullptr property configures the client as insecure using setInsecure(). Therefore, the validation done in the "begin" method does not make sense:

if (strlen(CAcert) == 0) {
        return false;
}

If CAcert is set to nullptr it causes a panic. I think this validation should not exist.

Sketch

http->begin(host, port, uri, nullptr)

Debug Message

Not relevant

Other Steps to Reproduce

No response

I have checked existing issues, online documentation and the Troubleshooting Guide

me-no-dev commented 8 months ago

What if you just call bool HTTPClient::begin(String host, uint16_t port, String uri) instead?

davsuapas commented 8 months ago

If you use bool HTTPClient::begin(String host, uint16_t port, String uri) it causes a http 400 errors. When you use this begin(...) it does not work with WIFICiientSecure. You could use bool HTTPClient::begin(WiFiClient &client, String host, uint16_t port, String uri, bool https) and send as parameter a WIFICiientSecure object by setting setInsecure(), but this complicates it. It would be as easy asbool HTTPClient::begin(String host, uint16_t port, String uri, const char* CAcert)could receive in CAcert the value nullptr. The nullptr is intended for self-certificate use, as it configures setInsecure(). The validation inside this begin(....) would look like:

if (CAcert != nullptr) {
   if (strlen(CAcert) == 0) {
      return false;
   }
}

This way, it would support this begin(...) self-certificate

Another fact is that in this HTTPClient::begin(String url, const char* CAcert) the above check is not done.