Skip to content

skip cert when local #23

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.

Sign up for GitHub

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

Closed
tavdog wants to merge 3 commits into from
Closed

skip cert when local #23

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ddea011
skip cert when local
tavdog Apr 8, 2025
68364ea
just use strstr to detect .local and //192, etc.
tavdog Apr 8, 2025
0cd709b
use strstr correctly
tavdog Apr 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion sdkconfig.defaults
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,9 @@ CONFIG_ESP_TLS_USING_MBEDTLS=y
# CONFIG_ESP_TLS_SERVER_CERT_SELECT_HOOK is not set
# CONFIG_ESP_TLS_SERVER_MIN_AUTH_MODE_OPTIONAL is not set
# CONFIG_ESP_TLS_PSK_VERIFICATION is not set
# CONFIG_ESP_TLS_INSECURE is not set
CONFIG_ESP_TLS_INSECURE=y
CONFIG_ESP_TLS_SKIP_SERVER_CERT_VERIFY=y
Comment on lines +565 to +566
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

Setting CONFIG_ESP_TLS_INSECURE=y and CONFIG_ESP_TLS_SKIP_SERVER_CERT_VERIFY=y globally is a major security risk. This should not be done globally. Consider using environment variables or build flags to control this setting for specific builds or configurations. How will this be controlled in production?

Copy link
Copy Markdown
Member

@IngmarStein IngmarStein Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 :)

Copy link
Copy Markdown
Member Author

@tavdog tavdog Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of course it's gonna say that.

Copy link
Copy Markdown
Member Author

@tavdog tavdog Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it with just CONFIG_ESP_TLS_SKIP_SERVER_CERT_VERIFY=y and it still fails to fetch https over LAN.


# end of ESP-TLS

#
Expand Down
63 changes: 62 additions & 1 deletion src/remote.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <esp_tls.h>

static const char* TAG = "remote";
bool is_local_address(const char* url);

struct remote_state {
void* buf;
Expand Down Expand Up @@ -155,9 +156,15 @@ int remote_get(const char* url, uint8_t** buf, size_t* len, int* b_int, int32_t*
.event_handler = _httpCallback,
.user_data = &state,
.timeout_ms = 10e3,
.crt_bundle_attach = esp_crt_bundle_attach,

};

if (is_local_address(url)) {
ESP_LOGI(TAG, "local address, skipping cert validation");
} else {
config.crt_bundle_attach = esp_crt_bundle_attach;
}
Comment on lines +162 to +166
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

While the conditional check for local addresses is a step in the right direction, simply skipping certificate verification is still insecure. Explore alternative, secure methods for identifying local connections. For example, you could check for a specific local IP address range or use a more robust method to verify the identity of the server. What are the implications of this approach in a production environment?

Suggested change
if (is_local_address(url)) {
ESP_LOGI(TAG, "local address, skipping cert validation");
} else {
config.crt_bundle_attach = esp_crt_bundle_attach;
}
if (is_local_address(url)) {
ESP_LOGI(TAG, "local address, using secure local connection method");
// Implement secure local connection method here
} else {
config.crt_bundle_attach = esp_crt_bundle_attach;
}


esp_http_client_handle_t http = esp_http_client_init(&config);

// Do the request
Expand Down Expand Up @@ -191,3 +198,57 @@ int remote_get(const char* url, uint8_t** buf, size_t* len, int* b_int, int32_t*
ESP_LOGI(TAG,"fetched new webp");
return 0;
}


bool is_local_address(const char* url) {
// Skip protocol prefix if present
const char* http_prefix = "http://";
const char* https_prefix = "https://";

if (strncmp(url, http_prefix, strlen(http_prefix)) == 0) {
url += strlen(http_prefix);
} else if (strncmp(url, https_prefix, strlen(https_prefix)) == 0) {
url += strlen(https_prefix);
}

// Make a copy of the host part (without the port)
char host[256];
strncpy(host, url, sizeof(host) - 1);
host[sizeof(host) - 1] = '\0';

// Remove port number if present
char* port = strchr(host, ':');
if (port != NULL) {
*port = '\0'; // Terminate string at the colon
}

// Check for .local TLD
size_t len = strlen(host);
if (len >= 6 && strcmp(host + len - 6, ".local") == 0) {
return true;
}

// Check if starts with common local IP prefixes
if (strncmp(host, "10.", 3) == 0) {
Copy link
Copy Markdown
Member

@IngmarStein IngmarStein Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TLS certificates for local IP addresses? This doesn't make a lot of sense.
Certificates technically can be issued for IP addresses instead of FQDNs, but this is rare and it's only for public addresses.

Copy link
Copy Markdown
Member Author

@tavdog tavdog Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, any ip should just return true i guess.

Copy link
Copy Markdown
Member

@IngmarStein IngmarStein Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh? You want is_local_address to return true for publicly routable IP addresses? I really don't think this is worth spending time on. Disabling TLS verification is worse than just not using it because it gives some false sense of security.

Copy link
Copy Markdown
Member Author

@tavdog tavdog Apr 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not disabled though. It only skips if you don't set the cert_bundle.

Screenshot 2025-04-08 at 12 05 11 PM

Copy link
Copy Markdown
Member

@IngmarStein IngmarStein Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that, but that flag is still only meant for testing and should not be set by default.
There's no good way to determine whether a given hostname is "local" or not. .local is not a TLD (https://en.wikipedia.org/wiki/.local) and only used for mDNS in a local network, but there can be infinitely other local domains (.lan is common, mine is .localdomain) or just plain hostnames resolved against a search domain delivered via DHCP.
Even if you resolve the hostname via DNS and look at whether the resulting IP (v4 and/or v6) are local, it's vulnerable to DNS spoofing.
Because there's no good way to do that, you can't decider when to supply the default CAs and when to skip verification.

Copy link
Copy Markdown
Member Author

@tavdog tavdog Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so what's the solution for someone who wants to use https on a local address ?

Copy link
Copy Markdown
Member

@IngmarStein IngmarStein Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either use a valid certificate from a default CA (e.g. using the DNS-01 challenge), provide a custom CA certificate to the firmware, or use HTTP (without the S).

I personally use the first option, for example.

return true;
}

if (strncmp(host, "192.168.", 8) == 0) {
return true;
}

if (strncmp(host, "172.", 4) == 0) {
// Check for 172.16.0.0 to 172.31.255.255 range
char* end;
long second_octet = strtol(host + 4, &end, 10);
if (*end == '.' && second_octet >= 16 && second_octet <= 31) {
return true;
}
}

if (strncmp(host, "127.", 4) == 0) {
return true;
}

return false;
}
Comment on lines +203 to +261
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The is_local_address function needs more robust error handling and clearer documentation. The strncpy function should check for potential buffer overflows. The strtol function should also be checked for errors. Add comments to explain the logic and the rationale behind the IP address ranges used. Consider using a more efficient approach to avoid redundant string operations.

bool is_local_address(const char* url) {
  // Skip protocol prefix if present
  const char* http_prefix = "http://";
  const char* https_prefix = "https://";

  if (strncmp(url, http_prefix, strlen(http_prefix)) == 0) {
    url += strlen(http_prefix);
  } else if (strncmp(url, https_prefix, strlen(https_prefix)) == 0) {
    url += strlen(https_prefix);
  }

  // Make a copy of the host part (without the port)
  char host[256];
  if (strncpy(host, url, sizeof(host) - 1) == NULL) { //Error check
    ESP_LOGE(TAG, "strncpy failed");
    return false;
  }
  host[sizeof(host) - 1] = '\0';

  // Remove port number if present
  char* port = strchr(host, ':');
  if (port != NULL) {
    *port = '\0';  // Terminate string at the colon
  }

  // Check for .local TLD
  size_t len = strlen(host);
  if (len >= 6 && strcmp(host + len - 6, ".local") == 0) {
    return true;
  }

  // Check if starts with common local IP prefixes
  char *end;
  long ip_num = strtol(host, &end, 10);
  if (*end != '.' || errno == ERANGE) { //Error check
    ESP_LOGE(TAG, "strtol failed");
    return false;
  }
  // ... (rest of the IP address checks with error handling)
  return false;
}