Restrict API access to LAN peers and origins - #1847
Conversation
johnny9
left a comment
There was a problem hiding this comment.
Reviewed exact head 18bd5de. I found one hostname-validation defect and one boundary-coverage gap. The malformed-label regression fails on the PR head (79 tests, 1 failure); the two sequential proposal patches pass 82/82 ESP32-S3 QEMU tests.
| return false; | ||
| } | ||
|
|
||
| for (const unsigned char *p = (const unsigned char *)host; *p != '\0'; p++) { |
There was a problem hiding this comment.
Medium: this validates only the character set and the first/last character of the whole hostname, not DNS label structure. As a result, foo..local, foo-.local, and a 64-byte label are all classified as valid local hostnames. The proposal reply enforces the DNS limits per label (1..63 bytes, no leading/trailing hyphen, total <=253) and includes regressions for the confirmed cases.
There was a problem hiding this comment.
Proposed fix. The regression fails on the reviewed head and passes with this patch; the combined result is 82/82 in ESP32-S3 QEMU:
diff --git a/components/api_rx/api_rx.c b/components/api_rx/api_rx.c
index db259a7..106b88c 100644
--- a/components/api_rx/api_rx.c
+++ b/components/api_rx/api_rx.c
@@ -99,16 +99,30 @@ static bool api_rx_local_hostname_is_valid(const char *host)
return false;
}
- for (const unsigned char *p = (const unsigned char *)host; *p != '\0'; p++) {
- if (!isalnum(*p) && *p != '-' && *p != '.') {
+ size_t host_len = strlen(host);
+ if (host_len > 253) {
+ return false;
+ }
+
+ const unsigned char *label = (const unsigned char *)host;
+ for (const unsigned char *p = label;; p++) {
+ if (*p != '\0' && *p != '.') {
+ if (!isalnum(*p) && *p != '-') {
+ return false;
+ }
+ continue;
+ }
+
+ size_t label_len = (size_t)(p - label);
+ if (label_len == 0 || label_len > 63 ||
+ label[0] == '-' || label[label_len - 1] == '-') {
return false;
}
- }
- size_t host_len = strlen(host);
- if (host[0] == '.' || host[0] == '-' ||
- host[host_len - 1] == '.' || host[host_len - 1] == '-') {
- return false;
+ if (*p == '\0') {
+ break;
+ }
+ label = p + 1;
}
if (strchr(host, '.') == NULL) {
diff --git a/components/api_rx/test/test_api_rx.c b/components/api_rx/test/test_api_rx.c
index be8f72a..951758e 100644
--- a/components/api_rx/test/test_api_rx.c
+++ b/components/api_rx/test/test_api_rx.c
@@ -79,6 +79,14 @@ TEST_CASE("LAN address validation handles IPv4 and IPv6", "[api_rx]")
TEST_ASSERT_FALSE(api_rx_ipv6_address_is_lan(deprecated_site_local_ipv6));
}
+TEST_CASE("HTTP origins reject malformed DNS labels", "[api_rx]")
+{
+ TEST_ASSERT_FALSE(api_rx_origin_is_lan("http://foo..local"));
+ TEST_ASSERT_FALSE(api_rx_origin_is_lan("http://foo-.local"));
+ TEST_ASSERT_FALSE(api_rx_origin_is_lan(
+ "http://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.local"));
+}
+
TEST_CASE("HTTP origins must identify a LAN host", "[api_rx]")
{
TEST_ASSERT_TRUE(api_rx_origin_is_lan("http://192.168.1.42"));
18bd5de to
00a129d
Compare
|
Physical Bitaxe 602 smoke test passed on head The application-only OTA reported the expected firmware version, the device returned to healthy mining with three stable API samples, zero fault indication, and the configured pool intact. An independent authorized Stratum V1 probe received a fresh Full test result and artifacts: https://mining-qa-status.vercel.app/results/9591e91e-ff7d-487d-9b15-b991f4ec0871 Test harness: |
b4b747f to
0db611e
Compare
|
I'm getting confused by #1846 and this one. Is it possible to decouple them, or make these PRs a GitHub stack or something like that? |
Summary
Make the HTTP server's existing trusted-LAN boundary explicit and fail closed for malformed or non-LAN clients.
.localnames.Review follow-up
Incorporates @johnny9's finding that local-looking names could contain empty or overlong labels or leading/trailing hyphens. Names are now at most 253 bytes and each label must be 1-63 bytes with valid hyphen placement.
This preserves LAN operation and adds no authentication.
Stack and related work
Builds on #1846; #1848 builds on this branch. Related to #1845. Because fork branches cannot target each other upstream, the displayed diff is cumulative and the LAN-origin changes are at the tip.
Validation
git diff --check