Implemented artifact integrity checks #89
+455
−37
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,6 +129,10 @@ static mender_err_t mender_artifact_read_data(mender_artifact_ctx_t *ctx, mender | |
| */ | ||
| static mender_err_t mender_artifact_drop_file(mender_artifact_ctx_t *ctx); | ||
|
|
||
| #ifdef CONFIG_MENDER_FULL_PARSE_ARTIFACT | ||
| static mender_err_t mender_artifact_read_header(mender_artifact_ctx_t *ctx); | ||
| #endif /* CONFIG_MENDER_FULL_PARSE_ARTIFACT */ | ||
|
|
||
| /** | ||
| * @brief Shift data after parsing | ||
| * @param ctx Artifact context | ||
|
|
@@ -150,6 +154,108 @@ static size_t mender_artifact_round_up(size_t length, size_t incr); | |
| */ | ||
| static mender_artifact_ctx_t *mender_artifact_ctx = NULL; | ||
|
|
||
| #ifdef CONFIG_MENDER_FULL_PARSE_ARTIFACT | ||
| /** | ||
| * @brief Get checksum entry for a file in the context | ||
| * @param ctx The mender artifact context | ||
| * @param filename The name of the file in the artifact | ||
| * @return The checksum entry or NULL on error. | ||
| * @note Since other files may be parsed before the manifest file, we need to | ||
| * create these entries in a lazy fashion. | ||
| */ | ||
| static mender_artifact_checksum_t * | ||
| mender_artifact_checksum_get_or_create(mender_artifact_ctx_t *ctx, const char *filename) { | ||
| assert(NULL != ctx); | ||
| assert(NULL != filename); | ||
|
|
||
| /* See if we already have an entry for this file */ | ||
| mender_artifact_checksum_t *checksum; | ||
| for (checksum = ctx->artifact_info.checksums; NULL != checksum; checksum = checksum->next) { | ||
| if (StringEqual(checksum->filename, filename)) { | ||
|
There was a problem hiding this comment. I am not sure about using the filename as unique ID. A tar archive does not guarantee uniqueness of filenames, so in theory we could have more than entry with same filename but different checksum. Our Can you elaborate on the motivation? Can't we use good old indexes? It is a list after all. There was a problem hiding this comment. The goal is to compare the computed checksums to those in the manifest. The manifest contains pairs of checksums and a filenames. No reference to indexes. So I'm not sure how we could do this without changing the artifact format. |
||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (NULL == checksum) { | ||
| /* Create new if entry not found */ | ||
| checksum = (mender_artifact_checksum_t *)calloc(1, sizeof(mender_artifact_checksum_t)); | ||
| if (NULL == checksum) { | ||
| mender_log_error("Unable to allocate memory"); | ||
| return NULL; | ||
| } | ||
| checksum->filename = strdup(filename); | ||
| if (NULL == checksum->filename) { | ||
| mender_log_error("Unable to allocate memory"); | ||
| free(checksum); | ||
| return NULL; | ||
| } | ||
| checksum->next = ctx->artifact_info.checksums; | ||
| ctx->artifact_info.checksums = checksum; | ||
|
|
||
| /* Start SHA-256 checksum computation (if not already started) */ | ||
|
There was a problem hiding this comment. What do you mean with "if not already started"? At this point it is starting it unconditionally There was a problem hiding this comment. Good catch! It's some leftovers from an earlier iteration of trying to get this thing to work. I will remove the last part ;) |
||
| if (MENDER_OK != mender_sha256_begin(&(checksum->context))) { | ||
| mender_log_error("Failed to start checksum"); | ||
|
There was a problem hiding this comment.
|
||
| return NULL; | ||
| } | ||
| } | ||
|
|
||
| return checksum; | ||
| } | ||
| #endif /* CONFIG_MENDER_FULL_PARSE_ARTIFACT */ | ||
|
|
||
| mender_err_t | ||
| mender_artifact_check_integrity(mender_artifact_ctx_t *ctx) { | ||
| assert(NULL != ctx); | ||
|
|
||
| #ifdef CONFIG_MENDER_FULL_PARSE_ARTIFACT | ||
| for (mender_artifact_checksum_t *checksum = ctx->artifact_info.checksums; NULL != checksum; checksum = checksum->next) { | ||
| unsigned char computed[MENDER_DIGEST_BUFFER_SIZE]; | ||
|
|
||
| if (!StringEqual(checksum->filename, "version") && !StringEqual(checksum->filename, "header.tar") | ||
| && !mender_utils_strbeginwith(checksum->filename, "data/")) { | ||
| /* Whenever we introduce a new file to the artifact manifest, we | ||
| * need to skip integrity checks for that file until it's | ||
| * implemented. Otherwise, the client will get stuck trying to | ||
| * install the artifact which may hold the new implementation. */ | ||
| mender_log_warning("Skipping integrity check for artifact file '%s': Not implemented", checksum->filename); | ||
| continue; | ||
| } | ||
|
Comment on lines
+214
to
+222
There was a problem hiding this comment. This sounds insecure to me, why will it get "stuck"? If we introduce new fields in the manifest, we will in order do
All in all I think we should gracefully fail the deployment. On a breaking change like this, the mender-artifact tool will provide a way to produce "legacy Artifacts" so that you can update your devices in the field to newer clients before sending new Artifacts. There was a problem hiding this comment. Let's discuss this 😄2😄 on Monday with a ☕ |
||
| mender_log_debug("Checking integrity for artifact file '%s'", checksum->filename); | ||
|
|
||
| if (MENDER_OK != mender_sha256_finish(checksum->context, computed)) { | ||
| mender_log_error("Failed to finish checksum for file '%s'", checksum->filename); | ||
| checksum->context = NULL; | ||
| return MENDER_FAIL; | ||
| } | ||
| checksum->context = NULL; | ||
|
|
||
| if (0 != memcmp(checksum->manifest, computed, MENDER_DIGEST_BUFFER_SIZE)) { | ||
| mender_log_error("Computed checksum for file '%s' does not match manifest", checksum->filename); | ||
| #if CONFIG_MENDER_LOG_LEVEL >= MENDER_LOG_LEVEL_DBG | ||
| /* Log the mismatching checksums for debugging */ | ||
| char checksum_str[(MENDER_DIGEST_BUFFER_SIZE * 2) + 1]; | ||
|
|
||
| for (int i = 0; i < MENDER_DIGEST_BUFFER_SIZE; i++) { | ||
| if (2 != snprintf(checksum_str + (i * 2), 3, "%02hhx", checksum->manifest[i])) { | ||
| break; | ||
| } | ||
| } | ||
| mender_log_debug("%s: '%s' (manifest)", checksum->filename, checksum_str); | ||
|
|
||
| for (int i = 0; i < MENDER_DIGEST_BUFFER_SIZE; i++) { | ||
| if (2 != snprintf(checksum_str + (i * 2), 3, "%02hhx", computed[i])) { | ||
| break; | ||
| } | ||
| } | ||
| mender_log_debug("%s: '%s' (computed)", checksum->filename, checksum_str); | ||
| #endif /* CONFIG_MENDER_LOG_LEVEL >= MENDER_LOG_LEVEL_DBG */ | ||
| return MENDER_FAIL; | ||
| } | ||
| } | ||
| #endif /* CONFIG_MENDER_FULL_PARSE_ARTIFACT */ | ||
| return MENDER_OK; | ||
| } | ||
|
|
||
| mender_artifact_ctx_t * | ||
| mender_artifact_create_ctx(void) { | ||
|
|
||
|
|
@@ -270,7 +376,10 @@ mender_artifact_process_data(mender_artifact_ctx_t *ctx, | |
|
|
||
| /* Drop data, file is not relevant */ | ||
| ret = mender_artifact_drop_file(ctx); | ||
| #ifdef CONFIG_MENDER_FULL_PARSE_ARTIFACT | ||
| } else if (StringEqual(ctx->file.name, "header.tar")) { | ||
| ret = mender_artifact_read_header(ctx); | ||
| #endif /* CONFIG_MENDER_FULL_PARSE_ARTIFACT */ | ||
| } else { | ||
|
|
||
| /* Nothing to do */ | ||
|
|
@@ -325,7 +434,10 @@ mender_artifact_release_ctx(mender_artifact_ctx_t *ctx) { | |
| #ifdef CONFIG_MENDER_FULL_PARSE_ARTIFACT | ||
| mender_utils_free_linked_list(ctx->artifact_info.provides); | ||
| mender_utils_free_linked_list(ctx->artifact_info.depends); | ||
| for (mender_artifact_checksum_t *checksum = ctx->artifact_info.checksums; NULL != checksum; checksum = checksum->next) { | ||
| free(checksum->filename); | ||
| mender_sha256_finish(checksum->context, NULL); | ||
| } | ||
| #endif | ||
| free(ctx); | ||
| } | ||
|
|
@@ -440,6 +552,21 @@ mender_artifact_read_version(mender_artifact_ctx_t *ctx) { | |
| return MENDER_OK; | ||
| } | ||
|
|
||
| #ifdef CONFIG_MENDER_FULL_PARSE_ARTIFACT | ||
| /* Get checksum entry (create one if needed) */ | ||
| mender_artifact_checksum_t *checksum; | ||
| if (NULL == (checksum = mender_artifact_checksum_get_or_create(ctx, "version"))) { | ||
| /* Error already logged */ | ||
| return MENDER_FAIL; | ||
| } | ||
|
|
||
| /* Update SHA-256 checksum */ | ||
| if (MENDER_OK != mender_sha256_update(checksum->context, ctx->input.data, ctx->file.size)) { | ||
| mender_log_error("Failed to update update checksum"); | ||
| return MENDER_FAIL; | ||
| } | ||
| #endif /* CONFIG_MENDER_FULL_PARSE_ARTIFACT */ | ||
|
|
||
| /* Check version file */ | ||
| if (NULL == (object = cJSON_ParseWithLength(ctx->input.data, ctx->file.size))) { | ||
| mender_log_error("Unable to allocate memory"); | ||
|
|
@@ -532,31 +659,40 @@ mender_artifact_read_manifest(mender_artifact_ctx_t *ctx) { | |
| } | ||
| *next = '\0'; | ||
|
|
||
| /* Process line */ | ||
| char *separator = strstr(line, " "); | ||
| if (NULL == separator) { | ||
| mender_log_error("Invalid manifest file"); | ||
| return MENDER_FAIL; | ||
| } | ||
| *separator = '\0'; | ||
|
|
||
| const char *checksum_str = line; | ||
larsewi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| const char *filename = separator + 2; | ||
|
|
||
| /* Useful when debugging artifact integrity check failures */ | ||
| mender_log_debug("%s %s", checksum_str, filename); | ||
|
|
||
| /* Make sure digest is of expected length (two hex per byte) */ | ||
| if ((MENDER_DIGEST_BUFFER_SIZE * 2) != strlen(checksum_str)) { | ||
| mender_log_error("Bad checksum '%s' in manifest for file '%s'", checksum_str, filename); | ||
| return MENDER_FAIL; | ||
| } | ||
|
|
||
| /* Get checksum entry for the file (creates one if not found) */ | ||
| mender_artifact_checksum_t *checksum; | ||
| if (NULL == (checksum = mender_artifact_checksum_get_or_create(ctx, filename))) { | ||
| /* Error already logged */ | ||
| return MENDER_FAIL; | ||
| } | ||
|
|
||
| /* Populate with manifest checksum */ | ||
| for (int i = 0; i < MENDER_DIGEST_BUFFER_SIZE; i++) { | ||
| if (1 != sscanf(checksum_str + (2 * i), "%02hhx", checksum->manifest + i)) { | ||
|
There was a problem hiding this comment. Won't we hit the same problems with There was a problem hiding this comment. Worked on my machine 😉 I used the ESP32-S3-DevKitC. Could be that it is only buggy for some format specifiers. Can you try on the ESP32-Ethernet-Kit? |
||
| mender_log_error("Bad checksum '%s' in manifest for file '%s'", checksum_str, filename); | ||
| return MENDER_FAIL; | ||
| } | ||
| } | ||
|
|
||
| ///* Move to the next line */ | ||
| line = next + 1; | ||
|
|
@@ -699,6 +835,33 @@ mender_artifact_read_header_info(mender_artifact_ctx_t *ctx) { | |
| return ret; | ||
| } | ||
|
|
||
| #ifdef CONFIG_MENDER_FULL_PARSE_ARTIFACT | ||
| static mender_err_t | ||
| mender_artifact_read_header(mender_artifact_ctx_t *ctx) { | ||
| assert(NULL != ctx); | ||
|
|
||
| /* Check if all data have been received */ | ||
| if ((NULL == ctx->input.data) || (ctx->input.length < mender_artifact_round_up(ctx->file.size, MENDER_ARTIFACT_STREAM_BLOCK_SIZE))) { | ||
| return MENDER_OK; | ||
| } | ||
|
|
||
| /* Get checksum entry (create one if needed) */ | ||
| mender_artifact_checksum_t *checksum; | ||
| if (NULL == (checksum = mender_artifact_checksum_get_or_create(ctx, "header.tar"))) { | ||
| /* Error already logged */ | ||
| return MENDER_FAIL; | ||
| } | ||
|
|
||
| /* Update SHA-256 checksum */ | ||
| if (MENDER_OK != mender_sha256_update(checksum->context, ctx->input.data, ctx->file.size)) { | ||
| mender_log_error("Failed to update update checksum"); | ||
| return MENDER_FAIL; | ||
| } | ||
|
|
||
| return MENDER_DONE; | ||
| } | ||
| #endif /* CONFIG_MENDER_FULL_PARSE_ARTIFACT */ | ||
|
|
||
| #ifdef CONFIG_MENDER_FULL_PARSE_ARTIFACT | ||
| static mender_err_t | ||
| mender_artifact_read_type_info(mender_artifact_ctx_t *ctx) { | ||
|
|
@@ -910,6 +1073,37 @@ mender_artifact_read_data(mender_artifact_ctx_t *ctx, mender_err_t (*callback)(c | |
| size_t length | ||
| = ((ctx->file.size - ctx->file.index) > MENDER_ARTIFACT_STREAM_BLOCK_SIZE) ? MENDER_ARTIFACT_STREAM_BLOCK_SIZE : (ctx->file.size - ctx->file.index); | ||
|
|
||
| #ifdef CONFIG_MENDER_FULL_PARSE_ARTIFACT | ||
| mender_artifact_checksum_t *checksum; | ||
| { | ||
| /* The filename will be something like | ||
| * 'data/0000.tar/zephyr.signed.bin'. But the manifest will hold | ||
| * 'data/0000/zephyr.signed.bin'. Hence, we need to remove the | ||
| * '.tar' extension from the string. | ||
| */ | ||
| char filename[strlen(ctx->file.name) + 1]; | ||
| strcpy(filename, ctx->file.name); | ||
|
|
||
| for (char *ch = strstr(filename, ".tar"); (NULL != ch) && (*ch != '\0'); ch++) { | ||
| /* Don't worry! The call to strlen() on a static string should | ||
| * be optimized out by the compiler */ | ||
|
|
||
| *ch = ch[strlen(".tar")]; | ||
| } | ||
|
|
||
| /* Get checksum entry (create one if needed) */ | ||
| if (NULL == (checksum = mender_artifact_checksum_get_or_create(ctx, filename))) { | ||
| /* Error already logged */ | ||
| return MENDER_FAIL; | ||
| } | ||
| } | ||
|
|
||
| /* Update SHA-256 checksum */ | ||
| if (MENDER_OK != mender_sha256_update(checksum->context, ctx->input.data, length)) { | ||
| mender_log_error("Failed to update update checksum"); | ||
| return MENDER_FAIL; | ||
| } | ||
| #endif /* CONFIG_MENDER_FULL_PARSE_ARTIFACT */ | ||
|
|
||
| /* Invoke callback */ | ||
| if (MENDER_OK | ||
| != (ret = callback(ctx->payloads.values[index].type, | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I know this is totally out of the current PR, fell free to ignore. It is Friday and I am feeling creative...)
Did we land on a naming convention for static (local) functions? I don't have an opinion on which convention, but I have an opinion on having one convention.
In this very file we already have static functions with no
mender_artifactprefix, for exampleis_compressed.If we want to keep the
mender_artifactin the name, some ideas could be:mender_artifact_checksum_get_or_create_s_mender_artifact_checksum_get_or_createmender_artifact__checksum_get_or_createThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With static symbols, we don't have to worry about conflicting names. Thus we can safely omit the
mender_prefix. I can fix this in a separate commit. I agree that we should stick to one convention.