From 2e90b8fd067b3f493d1193d517f48d64c0ee9296 Mon Sep 17 00:00:00 2001 From: Michael Sauer Date: Fri, 11 Sep 2026 23:52:03 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20kleinere=20Netzwerk-H=C3=A4rtungen?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit M2: WLAN-Passwort muss leer (offenes Netz) oder 8..64 Zeichen lang sein (400 "invalid password" bei 1..7 Zeichen). CMake prüft NINLIGHT_WIFI_PASSWORD ebenso (0 oder 8..63 Zeichen) und der Zeichen-Blacklist-Check bekommt `;` dazu. M4: Ein Authorization-Wert, der länger als NL_TOKEN_LEN-1 (64) ist, wird von nl_http_parse() nicht mehr stillschweigend abgeschnitten, sondern setzt `auth_too_long` in nl_http_request_t; nl_api_handle() behandelt eine solche Anfrage als unauthentifiziert (außer bei offenem Zugriff), damit ein abgeschnittener Wert nie zufällig auf ein kürzeres Token passt. M5: get_int() prüfte die (long)-Cast-Gleichheit vor dem Bereichscheck - bei sehr großen oder NaN-Werten ist der Cast von double nach long undefiniertes Verhalten. Bereichs- und NaN-Check laufen jetzt vor dem Cast. T1: sdk/Platform/Drivers/internalflash/wm_internal_fls.c: tls_fls_init() ignorierte den Rückgabewert von tls_os_sem_create(&s_fls2_lock, 1) - jetzt eine TLS_DBGPRT_ERR-Meldung, falls das fehlschlägt (dokumentiert in sdk/PATCHES.md). Tests: test_api.c (Passwortlängen, auth_too_long, 1e300-Brightness), test_http.c (auth_too_long wird gesetzt). Für M5 zeigt der Host-Test kein sichtbares RED, weil der (double)->(long)-Cast auf dieser Plattform/diesem Compiler keinen Crash auslöst (implementation-defined statt UB-Crash) - der Fix wird trotzdem angewendet, da er auf dem Zielcompiler (arm-none-eabi 4.9) nicht garantiert sicher ist. --- CMakeLists.txt | 9 +++++-- sdk/PATCHES.md | 4 ++++ .../Drivers/internalflash/wm_internal_fls.c | 3 ++- src/net/nl_api.c | 19 +++++++++++---- src/net/nl_http.c | 4 +++- src/net/nl_http.h | 2 ++ test/test_api.c | 24 +++++++++++++++++++ test/test_http.c | 17 +++++++++++++ 8 files changed, 74 insertions(+), 8 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index d777537..ea31c4e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -33,9 +33,14 @@ string(LENGTH "${NINLIGHT_AP_PASSWORD}" _nl_ap_len) if(_nl_ap_len LESS 8) message(FATAL_ERROR "NINLIGHT_AP_PASSWORD needs at least 8 characters (WPA2)") endif() +# M2: WPA2 station passwords must be empty (open network) or 8..63 chars. +string(LENGTH "${NINLIGHT_WIFI_PASSWORD}" _nl_wifi_pass_len) +if(NOT _nl_wifi_pass_len EQUAL 0 AND (_nl_wifi_pass_len LESS 8 OR _nl_wifi_pass_len GREATER 63)) + message(FATAL_ERROR "NINLIGHT_WIFI_PASSWORD must be empty or 8..63 characters (WPA2)") +endif() foreach(_v NINLIGHT_WIFI_SSID NINLIGHT_WIFI_PASSWORD NINLIGHT_AP_PASSWORD NINLIGHT_TOKEN) - if("${${_v}}" MATCHES "[\"\\\\]") - message(FATAL_ERROR "${_v} must not contain \" or \\") + if("${${_v}}" MATCHES "[\"\\\\;]") + message(FATAL_ERROR "${_v} must not contain \", \\ or ;") endif() endforeach() diff --git a/sdk/PATCHES.md b/sdk/PATCHES.md index a8a54e1..14027b5 100644 --- a/sdk/PATCHES.md +++ b/sdk/PATCHES.md @@ -27,3 +27,7 @@ Lib/GNU, Bin/secboot.img, Tools/GNU/link_w600_2m.ld (→ ld/) und wm_tool.c (→ `light_task` und `app_task` blockieren und der Watchdog den Chip zurücksetzen muss. 5. `Platform/Drivers/internalflash/wm_internal_fls.c`: Semaphor `s_fls2_lock` serialisiert den Flash-Pfad oberhalb von 1 MB (Pin-Umschaltung via `tls_spifls_cs/ck/di/do_switch` war ungeschützt). + T1 (Plan 2 final review): `tls_fls_init()` ignorierte den Rückgabewert von + `tls_os_sem_create(&s_fls2_lock, 1)` stillschweigend - jetzt eine `TLS_DBGPRT_ERR`-Meldung, + falls das Anlegen fehlschlägt (dann bliebe der Bereich oberhalb von 1 MB unprotokolliert + ungeschützt). diff --git a/sdk/Platform/Drivers/internalflash/wm_internal_fls.c b/sdk/Platform/Drivers/internalflash/wm_internal_fls.c index 0a1e512..d3d8e9d 100644 --- a/sdk/Platform/Drivers/internalflash/wm_internal_fls.c +++ b/sdk/Platform/Drivers/internalflash/wm_internal_fls.c @@ -1173,7 +1173,8 @@ int tls_fls_init(void) TLS_DBGPRT_ERR("create semaphore @fls_lock fail!\n"); return TLS_FLS_STATUS_ENOMEM; } - tls_os_sem_create(&s_fls2_lock, 1); + if (tls_os_sem_create(&s_fls2_lock, 1) != TLS_OS_SUCCESS) + TLS_DBGPRT_ERR("create semaphore @s_fls2_lock fail! flash access above 1 MB is unprotected!\n"); inside_fls = fls; diff --git a/src/net/nl_api.c b/src/net/nl_api.c index 0577003..9e78be2 100644 --- a/src/net/nl_api.c +++ b/src/net/nl_api.c @@ -51,12 +51,19 @@ static bool token_ok(const char *given, const char *expected) static int get_int(const cJSON *obj, const char *name, double lo, double hi, long *out) { const cJSON *v = cJSON_GetObjectItemCaseSensitive(obj, name); + double d; if (!v) return 0; - if (!cJSON_IsNumber(v) || v->valuedouble != (double)(long)v->valuedouble || v->valuedouble < lo || - v->valuedouble > hi) + if (!cJSON_IsNumber(v)) return -1; - *out = (long)v->valuedouble; + d = v->valuedouble; + /* M5: reject NaN and out-of-range values before casting to (long) - a huge or NaN + * double is undefined behaviour to cast, so lo/hi must be checked first. */ + if (d != d || d < lo || d > hi) + return -1; + if (d != (double)(long)d) + return -1; + *out = (long)d; return 1; } @@ -277,6 +284,9 @@ static void handle_put_settings(const nl_api_ctx_t *ctx, const nl_http_request_t err = "invalid ssid"; } else if ((pr = get_string(j, "password", 0, NL_PASS_LEN - 1, &b)) < 0) { err = "invalid password"; + } else if (pr == 1 && strlen(b) != 0 && strlen(b) < 8) { + /* M2: WPA2 passwords must be empty (open network) or 8..64 chars */ + err = "invalid password"; } else { if (ctx->wifi_configure(a, pr == 1 ? b : "") != 0) reply_error(resp, 503, "wifi busy"); @@ -353,7 +363,8 @@ void nl_api_handle(const nl_api_ctx_t *ctx, const nl_http_request_t *req, nl_api memset(resp, 0, sizeof *resp); ctx->get_settings(&s); - authed = ctx->open_access() || token_ok(req->auth, s.token); + /* M4: a truncated Authorization value must never accidentally match a shorter token. */ + authed = ctx->open_access() || (!req->auth_too_long && token_ok(req->auth, s.token)); if (strcmp(req->path, P_STATE) == 0) { if (s.auth_all && !authed) diff --git a/src/net/nl_http.c b/src/net/nl_http.c index ae44b42..af92669 100644 --- a/src/net/nl_http.c +++ b/src/net/nl_http.c @@ -85,8 +85,10 @@ nl_http_parse_t nl_http_parse(const char *buf, size_t len, nl_http_request_t *re if (line_end - v > 7 && strncasecmp(v, "Bearer ", 7) == 0) v += 7; n = (size_t)(line_end - v); - if (n >= sizeof req->auth) + if (n >= sizeof req->auth) { + req->auth_too_long = true; /* M4: never silently truncate a bearer token */ n = sizeof req->auth - 1; + } memcpy(req->auth, v, n); req->auth[n] = '\0'; } diff --git a/src/net/nl_http.h b/src/net/nl_http.h index afb4174..a58d523 100644 --- a/src/net/nl_http.h +++ b/src/net/nl_http.h @@ -1,6 +1,7 @@ #ifndef NL_HTTP_H #define NL_HTTP_H +#include #include #include "config/nl_settings.h" @@ -15,6 +16,7 @@ typedef struct { nl_http_method_t method; char path[64]; /* without query string */ char auth[NL_TOKEN_LEN]; /* Authorization value without "Bearer ", "" if absent */ + bool auth_too_long; /* Authorization value did not fit into auth[] (truncated) */ const char *body; /* points into the caller's buffer */ size_t body_len; } nl_http_request_t; diff --git a/test/test_api.c b/test/test_api.c index 2e6fd05..aedac44 100644 --- a/test/test_api.c +++ b/test/test_api.c @@ -106,6 +106,7 @@ static void test_post_validation(void) {"{\"brightness\":101}", "invalid brightness"}, {"{\"brightness\":1.5}", "invalid brightness"}, {"{\"brightness\":\"50\"}", "invalid brightness"}, + {"{\"brightness\":1e300}", "invalid brightness"}, /* M5: range check before the (long) cast */ {"{\"color\":{\"r\":1,\"g\":2}}", "invalid color"}, {"{\"color\":{\"r\":1,\"g\":2,\"b\":256}}", "invalid color"}, {"{\"color_temp\":999}", "invalid color_temp"}, @@ -171,6 +172,20 @@ static void test_auth_all_protects_device_state(void) TEST_ASSERT_EQUAL_INT(200, resp.status); } +static void test_auth_too_long_is_unauthenticated(void) +{ + /* M4: even a token that starts with the real one but was truncated at NL_TOKEN_LEN - 1 + * (i.e. the header carried more than that) must not authenticate. */ + nl_http_request_t r; + memset(&r, 0, sizeof r); + r.method = NL_HTTP_GET; + strcpy(r.path, "/api/v1/system/settings"); + strcpy(r.auth, TOKEN); /* would match on its own */ + r.auth_too_long = true; + nl_api_handle(&fake_ctx, &r, &resp); + TEST_ASSERT_EQUAL_INT(401, resp.status); +} + /* --- system/info --- */ static void test_info_is_open(void) { @@ -240,6 +255,14 @@ static void test_settings_wifi(void) TEST_ASSERT_EQUAL_STRING("invalid ssid", error_text()); call(NL_HTTP_PUT, "/api/v1/system/settings", TOKEN, "{\"type\":\"wifi\",\"ssid\":\"x\",\"password\":5}"); TEST_ASSERT_EQUAL_STRING("invalid password", error_text()); + /* M2: password must be empty (open network) or 8..64 chars, never 1..7 */ + call(NL_HTTP_PUT, "/api/v1/system/settings", TOKEN, "{\"type\":\"wifi\",\"ssid\":\"x\",\"password\":\"short\"}"); + TEST_ASSERT_EQUAL_INT(400, resp.status); + TEST_ASSERT_EQUAL_STRING("invalid password", error_text()); + call(NL_HTTP_PUT, "/api/v1/system/settings", TOKEN, "{\"type\":\"wifi\",\"ssid\":\"x\",\"password\":\"\"}"); + TEST_ASSERT_EQUAL_INT(202, resp.status); + call(NL_HTTP_PUT, "/api/v1/system/settings", TOKEN, "{\"type\":\"wifi\",\"ssid\":\"x\",\"password\":\"12345678\"}"); + TEST_ASSERT_EQUAL_INT(202, resp.status); fake_wifi_result = -1; call(NL_HTTP_PUT, "/api/v1/system/settings", TOKEN, "{\"type\":\"wifi\",\"ssid\":\"x\"}"); TEST_ASSERT_EQUAL_INT(503, resp.status); @@ -329,6 +352,7 @@ int main(void) RUN_TEST(test_post_settings_fields); RUN_TEST(test_post_save_failure); RUN_TEST(test_auth_all_protects_device_state); + RUN_TEST(test_auth_too_long_is_unauthenticated); RUN_TEST(test_info_is_open); RUN_TEST(test_info_with_sta_ip_and_error); RUN_TEST(test_settings_auth); diff --git a/test/test_http.c b/test/test_http.c index 572bbaf..05f1863 100644 --- a/test/test_http.c +++ b/test/test_http.c @@ -1,3 +1,4 @@ +#include #include #include "unity.h" #include "net/nl_http.h" @@ -41,6 +42,21 @@ static void test_headers_case_insensitive_and_bearer(void) TEST_ASSERT_EQUAL(NL_HTTP_PUT, r.method); TEST_ASSERT_EQUAL_STRING("abc123", r.auth); TEST_ASSERT_EQUAL_UINT(2, r.body_len); + TEST_ASSERT_FALSE(r.auth_too_long); +} + +static void test_auth_too_long_sets_flag(void) +{ + /* M4: token max is 64 chars (NL_TOKEN_LEN - 1); one char beyond that must be flagged + * rather than silently truncated and possibly matching by accident. */ + char hdr[128]; + nl_http_request_t r; + char token[66]; + memset(token, 'a', sizeof token - 1); + token[sizeof token - 1] = '\0'; /* 65 chars: one more than NL_TOKEN_LEN - 1 */ + snprintf(hdr, sizeof hdr, "GET / HTTP/1.1\r\nAuthorization: %s\r\n\r\n", token); + TEST_ASSERT_EQUAL(NL_HTTP_COMPLETE, parse(hdr, &r)); + TEST_ASSERT_TRUE(r.auth_too_long); } static void test_plain_token_and_query_string(void) @@ -108,6 +124,7 @@ int main(void) RUN_TEST(test_simple_get); RUN_TEST(test_post_body_arrives_in_parts); RUN_TEST(test_headers_case_insensitive_and_bearer); + RUN_TEST(test_auth_too_long_sets_flag); RUN_TEST(test_plain_token_and_query_string); RUN_TEST(test_other_method); RUN_TEST(test_body_too_large);