From c1750407de0a530d87165da1d80d21b2b4a1f958 Mon Sep 17 00:00:00 2001 From: aviralgarg05 Date: Wed, 12 Aug 2026 00:36:07 +0530 Subject: [PATCH] system/nxpkg: Validate catalog and database contents before use. A manifest and an installed-database entry were trusted as read. A name or a version carrying a path separator therefore reached the filesystem, and a database could name a current version it did not list as installed. Check that those fields are usable as single path components, that the current and previous versions appear among the installed ones, and apply a size limit to each field. Compare versions by their numeric prefix without overflowing, falling back to the remainder so that 1a and 1b are not read as the same version. Carry an optional icon and record the manifest of the version actually installed, which a rollback can leave behind the catalog. Assisted-by: OpenAI Codex:gpt-5.6-sol Signed-off-by: aviralgarg05 --- system/nxpkg/pkg_manifest.c | 71 +++++++ system/nxpkg/pkg_metadata.c | 365 ++++++++++++++++++++++++++++++------ 2 files changed, 380 insertions(+), 56 deletions(-) diff --git a/system/nxpkg/pkg_manifest.c b/system/nxpkg/pkg_manifest.c index 3e7b56a2637..dca17546c21 100644 --- a/system/nxpkg/pkg_manifest.c +++ b/system/nxpkg/pkg_manifest.c @@ -78,6 +78,49 @@ static bool pkg_validate_hex(FAR const char *value) * Public Functions ****************************************************************************/ +/**************************************************************************** + * Name: pkg_validate_path_component + * + * Description: + * Reject any value that could escape the intended directory when spliced + * into a filesystem path (pkg_store.c's PKG_STORE_DIR "/%s/%s/..." + * formatters). This is required for "name" and "version" specifically, + * since both come straight from an untrusted, network-fetched + * index.json and are used unsanitized to build install paths - a + * version of "../../evil" would otherwise let a malicious index write + * or delete files outside the package store entirely. + * + ****************************************************************************/ + +bool pkg_validate_path_component(FAR const char *value) +{ + FAR const char *p; + + if (pkg_validate_required(value) < 0) + { + return false; + } + + /* Reject a leading '.' outright: blocks ".", "..", and any + * "../"-prefixed traversal in one check. + */ + + if (value[0] == '.') + { + return false; + } + + for (p = value; *p != '\0'; p++) + { + if (*p == '/' || *p == '\\') + { + return false; + } + } + + return true; +} + const char *pkg_manifest_type_str(enum pkg_payload_type_e type) { switch (type) @@ -95,6 +138,8 @@ const char *pkg_manifest_type_str(enum pkg_payload_type_e type) int pkg_manifest_validate(FAR const struct pkg_manifest_s *manifest) { + size_t i; + if (manifest == NULL) { return -EINVAL; @@ -110,6 +155,19 @@ int pkg_manifest_validate(FAR const struct pkg_manifest_s *manifest) return -EINVAL; } + /* "name" and "version" get spliced unsanitized into on-disk paths + * (pkg_store.c) - they must not contain path separators or traversal + * sequences. "artifact" is validated separately in pkg_repo.c, where + * it's legitimately allowed to be a relative repo path (just not an + * absolute one or one that escapes the repo root). + */ + + if (!pkg_validate_path_component(manifest->name) || + !pkg_validate_path_component(manifest->version)) + { + return -EINVAL; + } + if (strlen(manifest->sha256) != PKG_HASH_HEX_LEN) { return -EINVAL; @@ -126,6 +184,19 @@ int pkg_manifest_validate(FAR const struct pkg_manifest_s *manifest) return -EINVAL; } + if (manifest->launch_argc > PKG_LAUNCH_ARGS_MAX) + { + return -EINVAL; + } + + for (i = 0; i < manifest->launch_argc; i++) + { + if (pkg_validate_required(manifest->launch_args[i]) < 0) + { + return -EINVAL; + } + } + return 0; } diff --git a/system/nxpkg/pkg_metadata.c b/system/nxpkg/pkg_metadata.c index aadb4692e58..951b4772bf8 100644 --- a/system/nxpkg/pkg_metadata.c +++ b/system/nxpkg/pkg_metadata.c @@ -24,6 +24,7 @@ * Included Files ****************************************************************************/ +#include #include #include #include @@ -100,6 +101,54 @@ static FAR cJSON *pkg_metadata_packages_array(FAR cJSON *root) return cJSON_GetObjectItemCaseSensitive(root, "packages"); } +static int pkg_metadata_parse_launch_args( + FAR cJSON *item, FAR struct pkg_manifest_s *manifest) +{ + FAR cJSON *field; + FAR cJSON *arg; + size_t argc = 0; + FAR const char *value; + int ret; + + field = cJSON_GetObjectItemCaseSensitive(item, "launch_args"); + if (field == NULL) + { + manifest->launch_argc = 0; + return 0; + } + + if (!cJSON_IsArray(field)) + { + return -EINVAL; + } + + cJSON_ArrayForEach(arg, field) + { + if (argc >= PKG_LAUNCH_ARGS_MAX) + { + return -E2BIG; + } + + value = cJSON_GetStringValue(arg); + if (value == NULL) + { + return -EINVAL; + } + + ret = pkg_copy_string(manifest->launch_args[argc], + sizeof(manifest->launch_args[argc]), value); + if (ret < 0) + { + return ret; + } + + argc++; + } + + manifest->launch_argc = argc; + return 0; +} + static int pkg_metadata_parse_manifest(FAR cJSON *item, FAR struct pkg_manifest_s *manifest) { @@ -170,6 +219,39 @@ static int pkg_metadata_parse_manifest(FAR cJSON *item, return -EINVAL; } + /* description/category/icon are optional and purely for UI display; + * missing fields just leave the manifest's copy empty. + */ + + field = cJSON_GetObjectItemCaseSensitive(item, "description"); + value = cJSON_GetStringValue(field); + if (value != NULL) + { + pkg_copy_string(manifest->description, sizeof(manifest->description), + value); + } + + field = cJSON_GetObjectItemCaseSensitive(item, "category"); + value = cJSON_GetStringValue(field); + if (value != NULL) + { + pkg_copy_string(manifest->category, sizeof(manifest->category), + value); + } + + field = cJSON_GetObjectItemCaseSensitive(item, "icon"); + value = cJSON_GetStringValue(field); + if (value != NULL) + { + pkg_copy_string(manifest->icon, sizeof(manifest->icon), value); + } + + ret = pkg_metadata_parse_launch_args(item, manifest); + if (ret < 0) + { + return ret; + } + return pkg_manifest_validate(manifest); } @@ -221,6 +303,9 @@ static int pkg_metadata_parse_installed_entry( { FAR cJSON *field; FAR const char *value; + bool current_found = false; + bool previous_found = false; + size_t i; int ret; memset(entry, 0, sizeof(*entry)); @@ -285,38 +370,82 @@ static int pkg_metadata_parse_installed_entry( return ret; } + if (!pkg_validate_path_component(entry->name) || + !pkg_validate_path_component(entry->current) || + (entry->previous[0] != '\0' && + !pkg_validate_path_component(entry->previous))) + { + return -EINVAL; + } + + for (i = 0; i < entry->version_count; i++) + { + if (!pkg_validate_path_component(entry->versions[i])) + { + return -EINVAL; + } + + current_found |= strcmp(entry->versions[i], entry->current) == 0; + previous_found |= strcmp(entry->versions[i], entry->previous) == 0; + } + + if (!current_found || + (entry->previous[0] != '\0' && !previous_found)) + { + return -EINVAL; + } + return 0; } static int pkg_metadata_version_token_cmp(FAR const char *lhs, FAR const char *rhs) { - long leftnum; - long rightnum; - FAR char *leftend; - FAR char *rightend; + FAR const char *cmpleft; + FAR const char *cmpright; + FAR const char *leftdigits; + FAR const char *rightdigits; + size_t leftlen; + size_t rightlen; + int ret; - leftnum = strtol(lhs, &leftend, 10); - rightnum = strtol(rhs, &rightend, 10); + leftdigits = lhs; + rightdigits = rhs; + while (isdigit((unsigned char)*leftdigits)) + { + leftdigits++; + } + + while (isdigit((unsigned char)*rightdigits)) + { + rightdigits++; + } - if (leftend != lhs && rightend != rhs) + if (leftdigits != lhs && rightdigits != rhs) { - if (leftnum < rightnum) + while (*lhs == '0' && lhs + 1 < leftdigits) + { + lhs++; + } + + while (*rhs == '0' && rhs + 1 < rightdigits) + { + rhs++; + } + + leftlen = (size_t)(leftdigits - lhs); + rightlen = (size_t)(rightdigits - rhs); + if (leftlen < rightlen) { return -1; } - if (leftnum > rightnum) + if (leftlen > rightlen) { return 1; } - } - else - { - int ret; - ret = pkg_string_cmp(lhs, PKG_VERSION_MAX + 1, - rhs, PKG_VERSION_MAX + 1); + ret = memcmp(lhs, rhs, leftlen); if (ret < 0) { return -1; @@ -326,6 +455,26 @@ static int pkg_metadata_version_token_cmp(FAR const char *lhs, { return 1; } + + cmpleft = leftdigits; + cmpright = rightdigits; + } + else + { + cmpleft = lhs; + cmpright = rhs; + } + + ret = pkg_string_cmp(cmpleft, PKG_VERSION_MAX + 1, + cmpright, PKG_VERSION_MAX + 1); + if (ret < 0) + { + return -1; + } + + if (ret > 0) + { + return 1; } return 0; @@ -395,6 +544,8 @@ static FAR cJSON *pkg_metadata_manifest_to_json( FAR const struct pkg_manifest_s *manifest) { FAR cJSON *root; + FAR cJSON *launch_args; + size_t i; root = cJSON_CreateObject(); if (root == NULL) @@ -410,51 +561,55 @@ static FAR cJSON *pkg_metadata_manifest_to_json( cJSON_AddStringToObject(root, "sha256", manifest->sha256); cJSON_AddStringToObject(root, "type", pkg_manifest_type_str(manifest->type)); - return root; -} -/**************************************************************************** - * Public Functions - ****************************************************************************/ - -int pkg_metadata_load_index(FAR struct pkg_index_s *index) -{ - FAR cJSON *root; - FAR cJSON *packages; - FAR cJSON *item; - FAR char *text; - char path[PATH_MAX]; - size_t count = 0; - size_t textlen; - int ret; - - if (index == NULL) + if (manifest->description[0] != '\0') { - return -EINVAL; + cJSON_AddStringToObject(root, "description", manifest->description); } - memset(index, 0, sizeof(*index)); - - ret = pkg_store_format_index_path(path, sizeof(path)); - if (ret < 0) + if (manifest->category[0] != '\0') { - return ret; + cJSON_AddStringToObject(root, "category", manifest->category); } - pkg_info("loading index from %s", path); - - ret = pkg_store_read_text(path, &text); - if (ret < 0) + if (manifest->launch_argc > 0) { - return ret; + launch_args = cJSON_AddArrayToObject(root, "launch_args"); + if (launch_args == NULL) + { + cJSON_Delete(root); + return NULL; + } + + for (i = 0; i < manifest->launch_argc; i++) + { + FAR cJSON *arg; + + arg = cJSON_CreateString(manifest->launch_args[i]); + if (arg == NULL) + { + cJSON_Delete(root); + return NULL; + } + + cJSON_AddItemToArray(launch_args, arg); + } } - textlen = strlen(text); - pkg_info("index read complete (%zu bytes)", textlen); + return root; +} + +static int pkg_metadata_parse_index_text(FAR const char *text, + FAR struct pkg_index_s *index) +{ + FAR cJSON *root; + FAR cJSON *packages; + FAR cJSON *item; + size_t count = 0; + int ret; root = cJSON_Parse(text); pkg_info("cJSON_Parse returned %s", root != NULL ? "success" : "failure"); - free(text); if (root == NULL) { return -EINVAL; @@ -471,15 +626,27 @@ int pkg_metadata_load_index(FAR struct pkg_index_s *index) { if (count >= PKG_INDEX_MAX) { - cJSON_Delete(root); - return -E2BIG; + /* Keep what's already parsed rather than discarding the whole + * index: a catalog that's grown past PKG_INDEX_MAX shouldn't + * make every other package unavailable too. + */ + + pkg_error("index has more than %d packages, truncating", + PKG_INDEX_MAX); + break; } ret = pkg_metadata_parse_manifest(item, &index->manifests[count]); if (ret < 0) { - cJSON_Delete(root); - return ret; + /* Skip a malformed entry instead of discarding the entire + * index: one bad/malicious package definition shouldn't make + * every other, otherwise-valid package unavailable too. + */ + + pkg_error("skipping malformed package entry %zu: %d", count, + ret); + continue; } pkg_info("parsed manifest %s %s", @@ -493,6 +660,84 @@ int pkg_metadata_load_index(FAR struct pkg_index_s *index) return 0; } +/**************************************************************************** + * Public Functions + ****************************************************************************/ + +int pkg_metadata_load_index_path(FAR const char *path, + FAR struct pkg_index_s *index) +{ + FAR char *text; + size_t textlen; + int ret; + + if (path == NULL || index == NULL) + { + return -EINVAL; + } + + memset(index, 0, sizeof(*index)); + + pkg_info("loading index from %s", path); + + ret = pkg_store_read_text(path, &text); + if (ret < 0) + { + return ret; + } + + textlen = strlen(text); + pkg_info("index read complete (%zu bytes)", textlen); + + ret = pkg_metadata_parse_index_text(text, index); + pkg_free(text); + return ret; +} + +int pkg_metadata_load_index(FAR struct pkg_index_s *index) +{ + char path[PATH_MAX]; + int ret; + + ret = pkg_store_format_index_path(path, sizeof(path)); + if (ret < 0) + { + return ret; + } + + return pkg_metadata_load_index_path(path, index); +} + +int pkg_metadata_load_manifest_path(FAR const char *path, + FAR struct pkg_manifest_s *manifest) +{ + FAR cJSON *root; + FAR char *text; + int ret; + + if (path == NULL || manifest == NULL) + { + return -EINVAL; + } + + ret = pkg_store_read_text(path, &text); + if (ret < 0) + { + return ret; + } + + root = cJSON_Parse(text); + pkg_free(text); + if (root == NULL) + { + return -EINVAL; + } + + ret = pkg_metadata_parse_manifest(root, manifest); + cJSON_Delete(root); + return ret; +} + FAR const struct pkg_manifest_s * pkg_metadata_find_latest(FAR const struct pkg_index_s *index, FAR const char *name) @@ -573,7 +818,7 @@ int pkg_metadata_load_installed(FAR struct pkg_installed_db_s *db) } root = cJSON_Parse(text); - free(text); + pkg_free(text); if (root == NULL) { return -EINVAL; @@ -590,15 +835,23 @@ int pkg_metadata_load_installed(FAR struct pkg_installed_db_s *db) { if (count >= PKG_INSTALLED_MAX) { - cJSON_Delete(root); - return -E2BIG; + pkg_error("installed db has more than %d entries, truncating", + PKG_INSTALLED_MAX); + break; } ret = pkg_metadata_parse_installed_entry(item, &db->entries[count]); if (ret < 0) { - cJSON_Delete(root); - return ret; + /* A single corrupted entry (plausible after a crash mid-write, + * despite the atomic-write mechanism) must not make every + * other installed package look uninstalled - that would drive + * needless reinstalls for everything else. + */ + + pkg_error("skipping malformed installed entry %zu: %d", count, + ret); + continue; } count++;