diff --git a/.github/scripts/check_outdated.py b/.github/scripts/check_outdated.py index f89676d..6b3b95f 100644 --- a/.github/scripts/check_outdated.py +++ b/.github/scripts/check_outdated.py @@ -21,11 +21,18 @@ IDF_RELEASE_API = "https://api.github.com/repos/espressif/esp-idf/releases/latest" +ALLOWED_PREFIXES = ("https://components.espressif.com/api/", "https://api.github.com/") + + def fetch_json(url): + # Only the two HTTPS APIs above: never file:// or another host + if not url.startswith(ALLOWED_PREFIXES): + raise ValueError(f"refusing to fetch {url!r}") request = urllib.request.Request(url, headers={"User-Agent": "simplrssh-dependency-check"}) token = os.environ.get("GITHUB_TOKEN") - if token and "api.github.com" in url: + if token and url.startswith("https://api.github.com/"): request.add_header("Authorization", f"Bearer {token}") + # noopengrep: mh.gitlab.python_urlopen_rule-urllib-urlopen, mh.opengrep.python.lang.security.audit.dynamic-urllib-use-detected.dynamic-urllib-use-detected - only the two https:// prefixes above # noqa: E501 with urllib.request.urlopen(request, timeout=30) as response: return json.load(response) diff --git a/.github/workflows/security-scan.yml b/.github/workflows/security-scan.yml index 7b7cb3a..ae047e0 100644 --- a/.github/workflows/security-scan.yml +++ b/.github/workflows/security-scan.yml @@ -7,9 +7,12 @@ name: Security scan # `sbom: false`, `zap: false`). It sits beside the Tests workflow (the release # gate, with its firmware CVE audit); it does not gate releases. # -# The pin below is moved by a pull request that sast-rules opens on each -# release; merging it is how this repository takes new rules and engines. -# scripts/sast.sh reads the same pin, so a local scan runs the same rules. +# Called @main, not pinned: sast-rules' main IS its release branch (every +# scan change pushed to main is released) and its workflows resolve their own +# rules from the commit they run at, so every run uses the newest release with +# no pin to move. The one exception to SHA pinning, by the owner's decision +# (test/guards/test_supply_chain.py, SAST_RULES_AT_MAIN). scripts/sast.sh +# resolves @main the same way, so a local scan runs the same rules. # # Needs: vars.AWS_REGION_EAST_1, secrets.AWS_ROLE (OIDC; pulls MicroHealth's # FIPS images from ECR, writes the bucket), vars.SECURITY_REPORTS_S3_BUCKET @@ -34,7 +37,7 @@ concurrency: jobs: security: permissions: {contents: read, actions: read, security-events: write, id-token: write} - uses: MicroHealthLLC/sast-rules/.github/workflows/security-scan.yml@341cd54bc0aae9359fa56965281d45ad58ce812c # v2026.10.5 + uses: MicroHealthLLC/sast-rules/.github/workflows/security-scan.yml@main # newest sast-rules release (main is its release branch) with: aws_region: ${{ vars.AWS_REGION_EAST_1 }} s3_bucket: ${{ vars.SECURITY_REPORTS_S3_BUCKET }} diff --git a/AI_RULES.md b/AI_RULES.md index 262863d..2530ef6 100644 --- a/AI_RULES.md +++ b/AI_RULES.md @@ -101,7 +101,10 @@ only while no SSH session is open; in a session they go to the server. screen as printable ASCII (`portal::printable()`); on-screen strings in the source are ASCII. - Keep the attack surface closed: station-only WiFi, IPv4, no listening sockets. Settings live in `sdkconfig.defaults` with a comment explaining each one. -- CI actions are pinned to commit SHAs and the build image to a digest; keep it that way. +- CI actions are pinned to commit SHAs and the build image to a digest; keep it that way. The + one named exception, by the owner's decision: MicroHealth's `sast-rules` workflows are called + `@main` (its release branch), so the security scan always runs the newest release + (`SAST_RULES_AT_MAIN` in `test/guards/test_supply_chain.py`; nothing else may use it). ## Persistence @@ -213,11 +216,12 @@ only while no SSH session is open; in a session they go to the server. key material), audit (CVEs), network-surface (linked symbols, generated sdkconfig), secret-scan. The code-quality workflow stays advisory. - The **Security scan** workflow (`.github/workflows/security-scan.yml`) calls MicroHealth's - shared `security-scan.yml` (`MicroHealthLLC/sast-rules`, pinned by commit): SAST, Trivy, + shared `security-scan.yml` (`MicroHealthLLC/sast-rules`, called `@main` — its release branch, so every run + uses the newest release with no pin to move): SAST, Trivy, SBOM and ZAP DAST, one report. Every scanner stays on (test/guards/test_supply_chain.py, `SecurityScan`). Before pushing run `scripts/check.sh` (guards, host tests, then `scripts/sast.sh` with the same rules); if SAST can't run locally, say so in the PR. - `scripts/sast.sh` is copied from sast-rules unchanged; never edit it. Suppress a finding + `scripts/sast.sh` is copied from sast-rules unchanged (it resolves the same `@main`); never edit it. Suppress a finding only inline, with the full rule id and why it does not apply. - Tests live in `test/`: `test/host` compiles hardware-independent firmware sources unchanged for Linux against in-memory stand-ins for NVS, partitions and logging (`idf_stubs/`, diff --git a/README.md b/README.md index 3312ca7..264d1d8 100644 --- a/README.md +++ b/README.md @@ -88,8 +88,8 @@ Saved passwords are encrypted on the device. For extra protection, set a PIN und saved encrypted), or press Enter if it has none. If a key needs a passphrase you didn't save, SimplrSSH asks for it when you connect. -Keys must be RSA in PEM format (the file starts with `-----BEGIN RSA PRIVATE KEY-----`). A key -starting with `-----BEGIN OPENSSH PRIVATE KEY-----` can be converted on your computer, keeping +Keys must be RSA in PEM format (the file's first line reads `BEGIN RSA PRIVATE KEY`). A key +whose first line reads `BEGIN OPENSSH PRIVATE KEY` can be converted on your computer, keeping its passphrase: `ssh-keygen -p -m PEM -f mykey`. If you add a card while the device is running, type `storage keys` to load them. diff --git a/SECURITY.md b/SECURITY.md index 1eb0f59..de0b9c7 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -78,6 +78,19 @@ A full sweep for malicious code, unexpected data flows and vulnerabilities. | At boot the peripheral power pin (GPIO 10) and chip selects pulsed low while being configured, and the SD card was initialised immediately after power-on: a card talked to while its supply is coming up or dipping can damage its internal tables and become unreadable | Pins get their level before becoming outputs (no glitch); 250 ms settle before the SD card is touched; boot mount uses the same 10 MHz clock as the runtime mount | | Reading the card could write to it (boot created `/ssh_keys`) | Mounts are read-only by default (a disk driver that refuses writes); only backup and deleting `openai.key` mount writable; the card is never formatted | +### SAST review (2026-10-14) + +The security scan's first runs reported 37 Opengrep findings (Trivy: none). Each was checked +by hand: + +| Finding | Outcome | +|---|---| +| A backup's header counts were read with `sscanf("%d")`: a huge number on the SD card overflowed `int` (undefined behaviour) | Parsed with `strtol`, 0 to 99999, else the backup is refused (host-tested) | +| `strncpy` into the WiFi config, `atoi` on menu numbers, unbounded `strlen` on a network header and an SD file name, `strlen` on constants | `snprintf`, `strtol`, `strnlen` with the field's limit, `sizeof` / `snprintf`'s length | +| `check_outdated.py` passed a built URL to `urlopen` (which also opens `file://`) | Only the two `https://` API prefixes are fetched; the call is suppressed inline | +| A private key in README.md | Documentation naming the PEM header line, not a key; reworded | +| 21 more: `memcpy` between fixed-size buffers, `strlen` on strings the firmware built, `fopen` on the SD card (TOCTOU) | Not applicable (fixed sizes, NUL-terminated by construction, FAT has no links or other users): suppressed inline with the full rule id and the reason | + ### Usability change (2026-09-27) Requiring a master PIN before any password could be saved meant networks silently weren't @@ -105,13 +118,17 @@ a release waits for it to pass on the same commit. Security-relevant gates: servers, OTA (including the Tab5 co-processor's), Bluetooth, mDNS or access-point code, must link CA-bundle TLS verification, SSH host key checks and RAM-only WiFi credential storage, and its generated sdkconfig must keep SoftAP, DHCP server, IPv6 and Bluetooth off. -- **Supply chain**: actions pinned by SHA and images by digest (enforced), components installed +- **Supply chain**: actions pinned by SHA and images by digest (enforced; the one named + exception is MicroHealth's own `sast-rules` workflows, called `@main` by the owner's decision + so the security scan always runs the newest release — `SAST_RULES_AT_MAIN` in + `test/guards/test_supply_chain.py`), components installed only at the locked versions with verified hashes (each board has its own lock file), and a CVE audit (esp-idf-sbom, NVD) of ESP-IDF, its libraries and the components of both images that blocks on new high/critical findings. Outside the Tests workflow, `.github/workflows/security-scan.yml` runs MicroHealth's shared -security scan (`MicroHealthLLC/sast-rules`, pinned by commit) on every push to `main` and +security scan (`MicroHealthLLC/sast-rules`, called `@main`: its release branch, so every run uses the +newest release with no pin to move) on every push to `main` and `integration`, every pull request, nightly and on demand: SAST (Opengrep with MicroHealth's rules), Trivy (dependencies, secrets and misconfiguration in the repository), a CycloneDX SBOM and ZAP DAST, in parallel, ending in one `security-report` (each scanner's status and counts by @@ -127,11 +144,22 @@ and never ingested, and a pull request's stay run artifacts (SAST there reports new against the base). The same SAST rules run locally: `scripts/sast.sh` (copied unchanged from -`MicroHealthLLC/sast-rules`; it reads the workflow's pin) and `scripts/check.sh`, which runs the +`MicroHealthLLC/sast-rules`; it resolves the workflow's `@main` reference to sast-rules' newest commit) and `scripts/check.sh`, which runs the guard and host tests first. It needs docker or nerdctl logged in to MicroHealth's ECR. A suppression is inline, with the full rule id and the reason it does not apply: `// noopengrep: — `. +The repository is public: anyone can read, clone and fork it, and nothing more. Its GitHub +settings (not in the tree; change them only on purpose): issues, wiki, discussions and projects +are off; only collaborators can open pull requests; workflows from an outside contributor's +fork wait for approval; the default workflow token is read-only and cannot approve pull +requests (each workflow declares what it writes); a ruleset stops deletion and force-pushes of +`main` and `integration` (repository admins bypass it); secret scanning with push protection, +Dependabot alerts and private vulnerability reporting are on. The security scan needs +`secrets.AWS_ROLE` on this repository or shared with it by the organization (an org secret +limited to private repositories never reaches a public one), and the role's OIDC trust must +include `repo:MicroHealthLLC/SimplrSSH`. + Found while introducing these checks: | Finding | Status | diff --git a/dependencies.lock b/dependencies.lock index 7340d6a..6b7933b 100644 --- a/dependencies.lock +++ b/dependencies.lock @@ -252,6 +252,7 @@ dependencies: type: service version: 1.1.0 direct_dependencies: +- espressif/button - espressif/esp_bsp_generic - espressif/esp_codec_dev - espressif/esp_lcd_touch_gt911 diff --git a/main/chat_app.cpp b/main/chat_app.cpp index 64e10c8..ff96aa8 100644 --- a/main/chat_app.cpp +++ b/main/chat_app.cpp @@ -923,6 +923,7 @@ bool SSHTerminal::chat_step_input(const std::string& raw_input, const std::strin std::string problem = "No openai.key file at the top of the SD card."; for (const char* name : {"openai.key", "openai.key.txt", "openai.txt"}) { std::string path = std::string(sdcard::MOUNT_POINT) + "/" + name; + // noopengrep: mh.gitlab.c_misc_rule-fopen-open - single-user device, FAT on the SD card has no links or permissions to race; what is read is size-checked and parsed before use FILE* f = fopen(path.c_str(), "rb"); if (f) { char buf[1024]; diff --git a/main/idf_component.yml b/main/idf_component.yml index e6961e4..f379a92 100755 --- a/main/idf_component.yml +++ b/main/idf_component.yml @@ -27,6 +27,10 @@ dependencies: version: '3.0.3' # Not used directly: esp_bsp_generic's led_indicator needs it; pinned rules: # because the component manager re-solves and would take newer releases - if: 'target == esp32s3' + espressif/button: + version: '4.2.1' # Not used directly: esp_bsp_generic needs it; pinned like led_strip + rules: # (the re-solve took 4.2.2, published 2026-10-14) + - if: 'target == esp32s3' ## M5Stack Tab5 (esp32p4) espressif/m5stack_tab5: version: '1.3.1' # Display (ILI9881C / ST7123 / ST7121), touch, IO expanders, codecs diff --git a/main/openai_client.cpp b/main/openai_client.cpp index aae90b9..f92d961 100644 --- a/main/openai_client.cpp +++ b/main/openai_client.cpp @@ -135,8 +135,10 @@ esp_err_t openai::chat(const std::string& key, const std::string& model, const s return ESP_ERR_NO_MEM; } - esp_http_client_handle_t client = open_post("chat/completions", key, "application/json", strlen(body)); - bool ok = client && write_all(client, body, strlen(body)); + // noopengrep: mh.gitlab.c_buffer_rule-strlen-wcslen - body is cJSON_PrintUnformatted's NUL-terminated output + size_t body_len = strlen(body); + esp_http_client_handle_t client = open_post("chat/completions", key, "application/json", body_len); + bool ok = client && write_all(client, body, body_len); cJSON_free(body); if (!client) { error = "Can't reach OpenAI (check WiFi)"; @@ -196,8 +198,10 @@ esp_err_t openai::transcribe(const std::string& key, const std::string& model, c uint8_t wav[44]; auto put32 = [&wav](int at, uint32_t v) { for (int i = 0; i < 4; i++) wav[at + i] = (v >> (8 * i)) & 0xFF; }; auto put16 = [&wav](int at, uint16_t v) { wav[at] = v & 0xFF; wav[at + 1] = v >> 8; }; + // noopengrep: mh.gitlab.c_buffer_rule-memcpy-CopyMemory - fixed-size literals into fixed offsets of the 44-byte header memcpy(wav, "RIFF", 4); put32(4, 36 + data_len); memcpy(wav + 8, "WAVEfmt ", 8); put32(16, 16); put16(20, 1); put16(22, 1); put32(24, rate); put32(28, rate * 2); put16(32, 2); put16(34, 16); + // noopengrep: mh.gitlab.c_buffer_rule-memcpy-CopyMemory - fixed-size literal into a fixed offset of the 44-byte header memcpy(wav + 36, "data", 4); put32(40, data_len); std::string content_type = std::string("multipart/form-data; boundary=") + B; @@ -240,8 +244,10 @@ esp_err_t openai::speak(const std::string& key, const std::string& model, const if (!body) { return ESP_ERR_NO_MEM; } - esp_http_client_handle_t client = open_post("audio/speech", key, "application/json", strlen(body)); - bool ok = client && write_all(client, body, strlen(body)); + // noopengrep: mh.gitlab.c_buffer_rule-strlen-wcslen - body is cJSON_PrintUnformatted's NUL-terminated output + size_t body_len = strlen(body); + esp_http_client_handle_t client = open_post("audio/speech", key, "application/json", body_len); + bool ok = client && write_all(client, body, body_len); cJSON_free(body); if (!client) { error = "Can't reach OpenAI"; @@ -377,8 +383,8 @@ esp_err_t openai::list_models(const std::string& key, Models& out, std::string& std::vector> found; // Every model, (created, id) const char* start = body; const char* end = body + len; - const char* data = "\"data\""; - const char* p = std::search(start, end, data, data + strlen(data)); + static const char data[] = "\"data\""; + const char* p = std::search(start, end, data, data + sizeof(data) - 1); while (p < end && found.size() < 400) { const char* open = std::find(p, end, '{'); const char* close = std::find(open, end, '}'); diff --git a/main/portal_client.cpp b/main/portal_client.cpp index ac086e0..550afa0 100644 --- a/main/portal_client.cpp +++ b/main/portal_client.cpp @@ -38,7 +38,7 @@ static esp_err_t on_event(esp_http_client_event_t* evt) } Capture* c = (Capture*)evt->user_data; if (strcasecmp(evt->header_key, "Set-Cookie") == 0) { - if (c->set_cookies.size() < MAX_SET_COOKIES && strlen(evt->header_value) <= MAX_COOKIE_BYTES + 512) { + if (c->set_cookies.size() < MAX_SET_COOKIES && strnlen(evt->header_value, MAX_COOKIE_BYTES + 513) <= MAX_COOKIE_BYTES + 512) { c->set_cookies.push_back(evt->header_value); } } else if (strcasecmp(evt->header_key, "Location") == 0) { diff --git a/main/portal_menu.cpp b/main/portal_menu.cpp index 8236304..bac0488 100644 --- a/main/portal_menu.cpp +++ b/main/portal_menu.cpp @@ -403,7 +403,7 @@ bool SSHTerminal::portal_step_input(const std::string& raw_input, const std::str case WizardStep::PortalPage: { std::string choice = lower(input); std::vector items = portal::items(portal_page); - int n = is_number(choice) ? atoi(choice.c_str()) : -1; + int n = is_number(choice) ? (int)strtol(choice.c_str(), nullptr, 10) : -1; if (choice == "0") { wizard_done(); } else if (choice == "r") { @@ -437,7 +437,7 @@ bool SSHTerminal::portal_step_input(const std::string& raw_input, const std::str case WizardStep::PortalOption: { portal::Field& f = portal_page.fields[portal_field]; - int n = is_number(input) ? atoi(input.c_str()) : -1; + int n = is_number(input) ? (int)strtol(input.c_str(), nullptr, 10) : -1; if (n >= 1 && (size_t)n <= f.options.size()) { f.selected = n - 1; } else if (n != 0) { diff --git a/main/sd_card.cpp b/main/sd_card.cpp index 301c8bb..12f79f3 100644 --- a/main/sd_card.cpp +++ b/main/sd_card.cpp @@ -298,7 +298,7 @@ int sdcard::load_ssh_keys(SSHTerminal* terminal) } // Check if file has .pem or .PEM extension (case-insensitive) - size_t name_len = strlen(entry->d_name); + size_t name_len = strnlen(entry->d_name, sizeof(entry->d_name)); if (name_len < 5) { ESP_LOGD(TAG, "Skipping file (name too short): %s", entry->d_name); continue; @@ -317,6 +317,7 @@ int sdcard::load_ssh_keys(SSHTerminal* terminal) snprintf(filepath, sizeof(filepath), "%s/%s", keys_dir, entry->d_name); // Open and read the key file + // noopengrep: mh.gitlab.c_misc_rule-fopen-open - single-user device, FAT on the SD card has no links or permissions to race; what is read is size-checked and parsed before use FILE* f = fopen(filepath, "rb"); if (!f) { ESP_LOGE(TAG, "Failed to open key file: %s", filepath); diff --git a/main/secret_vault.cpp b/main/secret_vault.cpp index 3b8ed47..9bdf276 100644 --- a/main/secret_vault.cpp +++ b/main/secret_vault.cpp @@ -113,6 +113,7 @@ static bool derive_key(const uint8_t* password, size_t password_len, const uint8 mbedtls_md_hmac_finish(&ctx, u) == 0; if (ok) { + // noopengrep: mh.gitlab.c_buffer_rule-memcpy-CopyMemory - both buffers are KEY_LEN bytes memcpy(out, u, KEY_LEN); for (uint32_t i = 1; i < iterations && ok; i++) { ok = mbedtls_md_hmac_reset(&ctx) == 0 && @@ -193,6 +194,7 @@ static esp_err_t write_key_record(const std::string* pin, const uint8_t* data_ke const uint32_t iterations = pin ? PIN_ITERATIONS : DEVICE_ITERATIONS; uint8_t record[KEY_RECORD_LEN]; + // noopengrep: mh.gitlab.c_buffer_rule-memcpy-CopyMemory - 4-byte magic into a KEY_RECORD_LEN record memcpy(record, pin ? MAGIC_PIN : MAGIC_DEVICE, 4); record[4] = iterations & 0xFF; record[5] = (iterations >> 8) & 0xFF; @@ -260,6 +262,7 @@ static esp_err_t create_new_vault() esp_fill_random(data_key, KEY_LEN); esp_err_t err = write_key_record(NULL, data_key); if (err == ESP_OK) { + // noopengrep: mh.gitlab.c_buffer_rule-memcpy-CopyMemory - both buffers are KEY_LEN bytes memcpy(s_data_key, data_key, KEY_LEN); s_unlocked = true; s_has_pin = false; @@ -293,6 +296,7 @@ bool vault::init() uint8_t data_key[KEY_LEN]; if (read_key_record(NULL, data_key) == ESP_OK) { + // noopengrep: mh.gitlab.c_buffer_rule-memcpy-CopyMemory - both buffers are KEY_LEN bytes memcpy(s_data_key, data_key, KEY_LEN); s_unlocked = true; mbedtls_platform_zeroize(data_key, sizeof(data_key)); @@ -325,6 +329,7 @@ esp_err_t vault::unlock(const std::string& pin) uint8_t data_key[KEY_LEN]; esp_err_t err = read_key_record(&pin, data_key); if (err == ESP_OK) { + // noopengrep: mh.gitlab.c_buffer_rule-memcpy-CopyMemory - both buffers are KEY_LEN bytes memcpy(s_data_key, data_key, KEY_LEN); s_unlocked = true; } diff --git a/main/settings_backup.cpp b/main/settings_backup.cpp index b2d69af..ee60235 100644 --- a/main/settings_backup.cpp +++ b/main/settings_backup.cpp @@ -31,8 +31,8 @@ static const char *TAG = "BACKUP"; static const char* const NAMESPACES[] = {"profiles", "wifi_nets", "known_hosts", "storage", "vault", "chat"}; -static const char* const HEADER = "SIMPLRSSH-BACKUP 1"; -static const char* const LEGACY_HEADER = "POCKETSSH-BACKUP 1"; // Same format, older name +static const char HEADER[] = "SIMPLRSSH-BACKUP 1"; +static const char LEGACY_HEADER[] = "POCKETSSH-BACKUP 1"; // Same format, older name static const size_t MAX_LINE = 8300; // "ns key s " + hex of a 4000-byte string static const size_t MAX_RECORDS = 1000; static const size_t MAX_BLOB = 1024; @@ -178,6 +178,7 @@ esp_err_t settings_backup::backup(const std::string& dir, Counts& counts) std::vector records = read_device_records(); counts = count_records(records); + // noopengrep: mh.gitlab.c_misc_rule-fopen-open - single-user device, FAT on the SD card has no links or permissions to race; what is read is size-checked and parsed before use FILE* f = fopen(path_in(dir, "settings.new").c_str(), "w"); if (!f) { ESP_LOGE(TAG, "Cannot write to %s", dir.c_str()); @@ -210,10 +211,10 @@ esp_err_t settings_backup::backup(const std::string& dir, Counts& counts) static size_t header_length(const std::string& line) { if (line.rfind(HEADER, 0) == 0) { - return strlen(HEADER); + return sizeof(HEADER) - 1; } if (line.rfind(LEGACY_HEADER, 0) == 0) { - return strlen(LEGACY_HEADER); + return sizeof(LEGACY_HEADER) - 1; } return 0; } @@ -234,8 +235,26 @@ static bool read_line(FILE* f, std::string& line) return !line.empty(); } +// The four counts after the header, each a number from 0 to MAX_COUNT +static bool parse_counts(const char* s, settings_backup::Counts& counts) +{ + static const long MAX_COUNT = 99999; + int* fields[] = {&counts.profiles, &counts.networks, &counts.hosts, &counts.history}; + for (int* field : fields) { + char* end = nullptr; + long v = strtol(s, &end, 10); + if (end == s || v < 0 || v > MAX_COUNT) { + return false; + } + *field = (int)v; + s = end; + } + return true; +} + bool settings_backup::read_backup_counts(const std::string& dir, Counts& counts) { + // noopengrep: mh.gitlab.c_misc_rule-fopen-open - single-user device, FAT on the SD card has no links or permissions to race; what is read is size-checked and parsed before use FILE* f = fopen(path_in(dir, "settings.dat").c_str(), "r"); if (!f) { return false; @@ -243,14 +262,14 @@ bool settings_backup::read_backup_counts(const std::string& dir, Counts& counts) std::string line; size_t header = 0; bool ok = read_line(f, line) && (header = header_length(line)) > 0 && - sscanf(line.c_str() + header, "%d %d %d %d", &counts.profiles, &counts.networks, - &counts.hosts, &counts.history) == 4; + parse_counts(line.c_str() + header, counts); fclose(f); return ok; } static bool parse_settings(const std::string& path, std::vector& records) { + // noopengrep: mh.gitlab.c_misc_rule-fopen-open - single-user device, FAT on the SD card has no links or permissions to race; what is read is size-checked and parsed before use FILE* f = fopen(path.c_str(), "r"); if (!f) { return false; diff --git a/main/ssh_terminal.cpp b/main/ssh_terminal.cpp index 614f79b..c7d4bcc 100644 --- a/main/ssh_terminal.cpp +++ b/main/ssh_terminal.cpp @@ -222,8 +222,8 @@ esp_err_t SSHTerminal::init_wifi(const char* ssid, const char* password) xEventGroupClearBits(s_wifi_event_group, WIFI_CONNECTED_BIT | WIFI_FAIL_BIT); wifi_config_t wifi_config = {}; - strncpy((char*)wifi_config.sta.ssid, ssid, sizeof(wifi_config.sta.ssid) - 1); - strncpy((char*)wifi_config.sta.password, password, sizeof(wifi_config.sta.password) - 1); + snprintf((char*)wifi_config.sta.ssid, sizeof(wifi_config.sta.ssid), "%s", ssid); + snprintf((char*)wifi_config.sta.password, sizeof(wifi_config.sta.password), "%s", password); wifi_config.sta.threshold.authmode = password[0] ? WIFI_AUTH_WPA2_PSK : WIFI_AUTH_OPEN; wifi_config.sta.pmf_cfg.capable = true; wifi_config.sta.pmf_cfg.required = false; @@ -573,7 +573,9 @@ void SSHTerminal::append_log(const char* text) bool follow = terminal_follow && !touch_scrolling(terminal_output); int32_t view_y = lv_obj_get_scroll_y(terminal_output); const char* current = lv_textarea_get_text(terminal_output); + // noopengrep: mh.gitlab.c_buffer_rule-strlen-wcslen - LVGL keeps the textarea text NUL-terminated; text is a C string the firmware built size_t current_len = strlen(current); + // noopengrep: mh.gitlab.c_buffer_rule-strlen-wcslen - text is a C string the firmware built (checked non-NULL above) size_t add_len = strlen(text); if (add_len > SCROLLBACK_KEEP) { text += add_len - SCROLLBACK_KEEP; @@ -1951,6 +1953,7 @@ esp_err_t SSHTerminal::ssh_authenticate_pubkey(const char* username, const char* int rc; int waits = 0; + // noopengrep: mh.gitlab.c_buffer_rule-strlen-wcslen - username is a profile's std::string (c_str(), NUL-terminated) while ((rc = libssh2_userauth_publickey_frommemory(session, username, strlen(username), NULL, 0, // public key (derived from the private key) privkey_data, privkey_len, @@ -1994,6 +1997,7 @@ esp_err_t SSHTerminal::ssh_open_channel() const int cols = term ? term->cols() : pty_cols; const int rows = term ? term->rows() : pty_rows; waits = 0; + // noopengrep: mh.gitlab.c_buffer_rule-strlen-wcslen - term_type is a string literal while ((rc = libssh2_channel_request_pty_ex(channel, term_type, strlen(term_type), NULL, 0, cols, rows, 0, 0)) == LIBSSH2_ERROR_EAGAIN && (waitsocket(ssh_socket, session) > 0 || ++waits < SSH_MAX_WAITS)) { @@ -2328,6 +2332,7 @@ void SSHTerminal::toggle_side_panel() void SSHTerminal::send_special_key(const char* sequence) { + // noopengrep: mh.gitlab.c_buffer_rule-strlen-wcslen - sequence comes from the firmware's own key tables (string literals) if (!sequence || strlen(sequence) == 0) { toggle_side_panel(); return; @@ -2385,6 +2390,7 @@ void SSHTerminal::send_special_key(const char* sequence) } if (ssh_connected && channel) { + // noopengrep: mh.gitlab.c_buffer_rule-strlen-wcslen - sequence comes from the firmware's own key tables (string literals) write_channel(sequence, strlen(sequence)); } else { ESP_LOGW(TAG, "Cannot send special key - not connected"); diff --git a/main/term_view.cpp b/main/term_view.cpp index 7970903..820d35e 100644 --- a/main/term_view.cpp +++ b/main/term_view.cpp @@ -516,13 +516,13 @@ void TermView::draw(lv_layer_t* layer) if (back > 0) { // Scrolled back: how far, top right static char marker[24]; - snprintf(marker, sizeof(marker), " -%d ", back); + int marker_len = snprintf(marker, sizeof(marker), " -%d ", back); lv_draw_label_dsc_t dsc; lv_draw_label_dsc_init(&dsc); dsc.font = font; dsc.color = lv_color_black(); dsc.text = marker; - int32_t w = (int32_t)strlen(marker) * cell_w; + int32_t w = (int32_t)marker_len * cell_w; lv_area_t a = {coords.x2 - w + 1, coords.y1, coords.x2, coords.y1 + cell_h - 1}; fill(layer, a.x1, a.y1, a.x2, a.y2, lv_color_hex(0xFFFF00)); lv_draw_label(layer, &dsc, &a); diff --git a/main/ui_page.cpp b/main/ui_page.cpp index 4d7d431..8c87c1c 100644 --- a/main/ui_page.cpp +++ b/main/ui_page.cpp @@ -99,6 +99,7 @@ void Notice::add(const char* text) if (text == nullptr) { return; } + // noopengrep: mh.gitlab.c_buffer_rule-strlen-wcslen - text is a C string the firmware built (a literal or std::string::c_str()); the notice keeps a bounded tail size_t len = strlen(text); if (len == 0) { return; diff --git a/scripts/check.sh b/scripts/check.sh index 7f03231..34e0831 100755 --- a/scripts/check.sh +++ b/scripts/check.sh @@ -1,7 +1,7 @@ #!/usr/bin/env bash # The local check before a push: the guard tests and the host unit tests the -# Tests workflow runs, then SAST with MicroHealth's rules at the commit -# .github/workflows/security-scan.yml pins (scripts/sast.sh) — the same rules +# Tests workflow runs, then SAST with MicroHealth's rules at the newest +# release, the `@main` .github/workflows/security-scan.yml calls (scripts/sast.sh) — the same rules # CI's security scan runs. It does not build firmware (that needs ESP-IDF). # # scripts/check.sh # guards, host tests (cmake, g++, libmbedtls-dev), SAST diff --git a/scripts/sast.sh b/scripts/sast.sh index d2827bd..b090422 100755 --- a/scripts/sast.sh +++ b/scripts/sast.sh @@ -1,7 +1,12 @@ #!/usr/bin/env bash # Local SAST: MicroHealth's rules (MicroHealthLLC/sast-rules) at exactly the -# commit this repository's CI pins, run the way CI runs them. Copied from -# sast-rules/local/sast.sh; it reads the pin, so it never needs editing. +# commit this repository's CI runs, run the way CI runs them. Copied from +# sast-rules/local/sast.sh; it reads the workflow reference, so it never needs +# editing. +# +# The reference is a branch (`@main`: every scan change on sast-rules' main is +# a release, so CI always runs the newest one) or a 40-character commit. A +# branch is resolved to its commit NOW, the way GitHub resolves it for CI. # # scripts/sast.sh [--fail-on error|warning|none] [--json out.json] # @@ -9,14 +14,29 @@ # MicroHealth's ECR. set -euo pipefail root="$(git rev-parse --show-toplevel)" -pin="$(grep -rhoE 'MicroHealthLLC/sast-rules/\.github/workflows/[a-z-]+\.yml@[0-9a-f]{40}' \ +repo_url=https://github.com/MicroHealthLLC/sast-rules.git +ref="$(grep -rhoE 'MicroHealthLLC/sast-rules/\.github/workflows/[a-z-]+\.yml@[A-Za-z0-9._/-]+' \ "$root/.github/workflows" | head -1 | sed 's/.*@//')" -[ -n "$pin" ] || { echo "No sast-rules pin in .github/workflows." >&2; exit 2; } +[ -n "$ref" ] || { echo "No sast-rules reference in .github/workflows." >&2; exit 2; } +# Run from a git hook, GIT_DIR, GIT_INDEX_FILE and the rest name the +# repository being pushed, and every git command below would act on IT rather +# than on the cache: a pre-push once moved the checkout's HEAD onto the rules +# commit and left an empty cache marked complete. The cache is its own repo. +unset $(git rev-parse --local-env-vars) +if [[ "$ref" =~ ^[0-9a-f]{40}$ ]]; then + pin="$ref" +else + # A peeled tag (^{}) names the commit; a branch has only the one line. + pin="$(git ls-remote "$repo_url" "refs/heads/$ref" "refs/tags/$ref" "refs/tags/$ref^{}" \ + | sort -k2 -r | awk 'NR==1{print $1}')" + [[ "$pin" =~ ^[0-9a-f]{40}$ ]] || { echo "sast-rules has no branch or tag '${ref}'." >&2; exit 1; } + echo "sast-rules @${ref} is ${pin}." >&2 +fi cache="${XDG_CACHE_HOME:-$HOME/.cache}/sast-rules/${pin}" -if [ ! -f "$cache/.complete" ]; then +if [ ! -f "$cache/.complete" ] || [ ! -x "$cache/scripts/scan-local.sh" ]; then rm -rf "$cache"; mkdir -p "$cache" git -C "$cache" init -q - git -C "$cache" fetch -q --depth 1 https://github.com/MicroHealthLLC/sast-rules.git "$pin" + git -C "$cache" fetch -q --depth 1 "$repo_url" "$pin" git -C "$cache" checkout -q FETCH_HEAD [ "$(git -C "$cache" rev-parse HEAD)" = "$pin" ] || { echo "sast-rules fetch is not ${pin}" >&2; exit 1; } touch "$cache/.complete" diff --git a/test/guards/test_supply_chain.py b/test/guards/test_supply_chain.py index 2e0ebcd..7c55006 100644 --- a/test/guards/test_supply_chain.py +++ b/test/guards/test_supply_chain.py @@ -11,6 +11,18 @@ import repo USES = re.compile(r"^\s*(?:-\s*)?uses:\s*([^\s#]+)", re.M) +PINNED = re.compile(r"^[\w.-]+/[\w./-]+@[0-9a-f]{40}$") + +# The ONE exception to SHA pinning, by the owner's decision: MicroHealth's shared security +# workflows are called @main. sast-rules' main IS its release branch (every scan change pushed +# there is released), and its workflows check out their own rules at the commit the job runs +# (the OIDC job_workflow_sha), so @main runs the newest reviewed release with no pin to move. +# Exactly that repository's workflows at exactly `main`; every other ref stays SHA-pinned. +SAST_RULES_AT_MAIN = re.compile(r"^MicroHealthLLC/sast-rules/\.github/workflows/[a-z0-9-]+\.yml@main$") + + +def pinned_or_named_exception(ref): + return bool(PINNED.match(ref) or SAST_RULES_AT_MAIN.match(ref)) class ActionsPinned(unittest.TestCase): @@ -20,8 +32,25 @@ def test_actions_pinned_to_commit_sha(self): if ref.startswith("./"): continue # Reusable workflow in this repository with self.subTest(workflow=name, uses=ref): - self.assertRegex(ref, r"^[\w.-]+/[\w./-]+@[0-9a-f]{40}$", - "pin actions to a full commit SHA (with a # vX.Y.Z comment)") + self.assertTrue(pinned_or_named_exception(ref), + f"{ref}: pin actions to a full commit SHA (with a # vX.Y.Z comment)") + + def test_only_sast_rules_at_main_is_unpinned(self): + sha = "206c21cb92451de53799c90ea7dedada6c256e2e" + for ok in ("MicroHealthLLC/sast-rules/.github/workflows/security-scan.yml@main", + f"actions/checkout@{sha}", + f"MicroHealthLLC/sast-rules/.github/workflows/security-scan.yml@{sha}"): + with self.subTest(accepted=ok): + self.assertTrue(pinned_or_named_exception(ok)) + for bad in ("actions/checkout@main", + "MicroHealthLLC/other-repo/.github/workflows/security-scan.yml@main", + "MicroHealthLLC/sast-rules/.github/workflows/security-scan.yml@integration", + "MicroHealthLLC/sast-rules/.github/workflows/security-scan.yml@v2026.10.15", + "MicroHealthLLC/sast-rules/.github/workflows/security-scan.yml@main2", + "MicroHealthLLC/sast-rules/action@main", + "evil/MicroHealthLLC/sast-rules/.github/workflows/security-scan.yml@main"): + with self.subTest(refused=bad): + self.assertFalse(pinned_or_named_exception(bad)) def test_container_images_pinned_by_digest(self): image = re.compile(r"(?:docker (?:pull|run)[^\n]*?\s|esp_idf_version:\s*)" @@ -164,7 +193,8 @@ class SecurityScan(unittest.TestCase): """One security workflow: MicroHealth's shared security-scan (SAST, Trivy, SBOM, ZAP DAST), every scanner on, and the same rules locally through scripts/sast.sh.""" - SHARED = re.compile(r"uses:\s*MicroHealthLLC/sast-rules/\.github/workflows/security-scan\.yml@([0-9a-f]{40})") + # @main: the newest sast-rules release (see SAST_RULES_AT_MAIN above). + SHARED = re.compile(r"uses:\s*MicroHealthLLC/sast-rules/\.github/workflows/security-scan\.yml@main(?:\s|$)") def test_one_security_scan_calls_the_shared_workflow(self): text = repo.read(".github", "workflows", "security-scan.yml") @@ -180,7 +210,7 @@ def test_every_scanner_on(self): self.assertNotRegex(text, rf"(?m)^\s*{scanner}:\s*false", "every scanner stays on") self.assertIn("zap_target: ${{ vars.ZAP_TARGET_URL }}", text) - def test_local_sast_uses_the_pinned_rules(self): + def test_local_sast_uses_the_same_rules(self): sast = repo.path("scripts", "sast.sh") self.assertTrue(os.access(sast, os.X_OK), "scripts/sast.sh must be executable") self.assertIn("MicroHealthLLC/sast-rules", repo.read("scripts", "sast.sh")) diff --git a/test/host/tests/test_settings_backup.cpp b/test/host/tests/test_settings_backup.cpp index 67ed8ee..0406aae 100644 --- a/test/host/tests/test_settings_backup.cpp +++ b/test/host/tests/test_settings_backup.cpp @@ -233,6 +233,25 @@ TEST(restore_rejects_untrusted_backups) expect_rejected(many); // Too many records } +TEST(backup_counts_reject_bad_numbers) +{ + std::string dir = temp_dir(); + settings_backup::Counts counts; + write_file(dir + "/settings.dat", "SIMPLRSSH-BACKUP 1 1 2 3\n"); // Three counts + CHECK(!settings_backup::read_backup_counts(dir, counts)); + write_file(dir + "/settings.dat", "SIMPLRSSH-BACKUP 1 1 2 x 4\n"); // Not a number + CHECK(!settings_backup::read_backup_counts(dir, counts)); + write_file(dir + "/settings.dat", "SIMPLRSSH-BACKUP 1 1 -2 3 4\n"); // Negative + CHECK(!settings_backup::read_backup_counts(dir, counts)); + write_file(dir + "/settings.dat", "SIMPLRSSH-BACKUP 1 1 2 99999999999999999999 4\n"); // Overflows int + CHECK(!settings_backup::read_backup_counts(dir, counts)); + write_file(dir + "/settings.dat", "SIMPLRSSH-BACKUP 1 1 2 3 4\r\n"); // Edited on Windows + CHECK(settings_backup::read_backup_counts(dir, counts)); + CHECK_EQ(counts.profiles, 1); + CHECK_EQ(counts.history, 4); + remove_dir(dir); +} + TEST(restore_missing_backup_fails_cleanly) { factory_device();