Implemented artifact integrity checks #89

larsewi · 2024-10-03T12:16:12Z

This was a bit harder than I first anticipated. Enjoy 🍿🍻

$ west flash && west espressif monitor
 ---snip---
Hello World! esp32s3_devkitc/esp32s3/procpu
 ---snip---
[00:01:01.395,000] <dbg> mender: mender_log_print: mender_artifact_check_integrity (223): Checking integrity for artifact file 'header.tar'
[00:01:01.395,000] <dbg> mender: mender_log_print: mender_artifact_check_integrity (223): Checking integrity for artifact file 'data/0000/zephyr.signed.bin'
[00:01:01.395,000] <dbg> mender: mender_log_print: mender_artifact_check_integrity (223): Checking integrity for artifact file 'version'
 ---snip---
[00:01:04.243,000] <inf> mender: Artifact installation done, rebooting
 ---snip---
This shit works baby! esp32s3_devkitc/esp32s3/procpu
 ---snip---
[00:00:20.187,000] <inf> mender: Application has been mark valid and rollback canceled
 ---snip---

Also tested without full artifact parsing

mender-test-bot · 2024-10-03T12:33:24Z

There was an error running your pipeline, see logs for details.

vpodzime

Looks good to me otherwise. Thanks for splitting the work in such nice and small commits! ❤️ 🍻

include/mender-sha.h

platform/sha/generic/mbedtls/src/mender-sha.c

vpodzime · 2024-10-03T13:02:24Z

core/src/mender-client.c

@@ -990,24 +990,26 @@ mender_client_update_work_function(void) {
                    /* Get artifact context if artifact download succeeded */
                    if ((NULL != mender_update_module) && (MENDER_OK == (ret = mender_artifact_get_ctx(&mender_artifact_ctx)))) {
 #ifdef CONFIG_MENDER_FULL_PARSE_ARTIFACT
-                        if (MENDER_OK == (ret = mender_check_artifact_requirements(mender_artifact_ctx, deployment))) {
+                        if (MENDER_OK == (ret = mender_artifact_check_integrity(mender_artifact_ctx))) {
+                            if (MENDER_OK == (ret = mender_check_artifact_requirements(mender_artifact_ctx, deployment))) {


I feel like those two should be in a single function, a new one I guess.

core/src/mender-artifact.c

vpodzime · 2024-10-03T13:09:46Z

core/src/mender-artifact.c

+
+            for (char *ch = strstr(filename, ".tar"); *ch != '\0'; ch++) {
+                /* Don't worry! The call to strlen() on a static string should
+                 * be optimized out by the compiler */


core/src/mender-artifact.c

Ticket: MEN-7483 Changelog: None Signed-off-by: Lars Erik Wik <[email protected]>

Ticket: None Changelog: None Signed-off-by: Lars Erik Wik <[email protected]>

The client now performs integrity checks on the version file in the artifact by comparing its computed checksum to the one in the artifact manifest. Ticket: MEN-7483 Changelog: Commit Signed-off-by: Lars Erik Wik <[email protected]>

The client now performs integrity checks on the header.tar file in the artifact by comparing its computed checksum to the one in the artifact manifest. Ticket: MEN-7483 Changelog: Commit Signed-off-by: Lars Erik Wik <[email protected]>

Added logging of mismatching checksums during artifact integrity checks in debug mode. Ticket: None Changelog: None Signed-off-by: Lars Erik Wik <[email protected]>

The client now performs integrity checks on the data files in the artifact by comparing its computed checksum to the one in the artifact manifest. Ticket: MEN-7483 Changelog: Commit Signed-off-by: Lars Erik Wik <[email protected]>

mender-test-bot · 2024-10-03T14:29:55Z

Merging these commits will result in the following changelog entries:

Changelogs

mender-mcu (sha256)

New changes in mender-mcu since main:

Features

added integrity checks for artifact data files

The client now performs integrity checks on the data files in the
artifact by comparing its computed checksum to the one in the artifact
manifest.
(MEN-7483)
added integrity checks for artifact header.tar file

The client now performs integrity checks on the header.tar file in the
artifact by comparing its computed checksum to the one in the artifact
manifest.
(MEN-7483)
added integrity checks for artifact version file

The client now performs integrity checks on the version file in the
artifact by comparing its computed checksum to the one in the artifact
manifest.
(MEN-7483)

larsewi · 2024-10-03T14:34:14Z

I feel like those two should be in a single function, a new one I guess.

Thanks for the thorough review @vpodzime. I will follow up on ^^ in a separate commit 🍻

lluiscampos

Great work 🚀

See my comments below. Nothing really big but some discussions will be needed.

Before my final approval, I will now compile your work locally and try few things as I suspect another bug when parsing multiple files.

lluiscampos · 2024-10-04T07:01:54Z

platform/sha/generic/mbedtls/src/mender-sha.c

+    }
+
+    mbedtls_sha256_init(ctx);
+    mbedtls_sha256_starts(ctx, 0); /* Use SHA-256, not SHA-224 */


Check return code

The version of MbedTLS we compile with returns void for all of these functions.

That is not the case.

Zephyr v3.7.0 comes with mbedtls revision 2f24831ee13d399ce019c4632b0bcd440a713f7c 👉 you can see this both in the manifest file and from your local workspace with cd modules/crypto/mbedtls/ && git log. Maybe you are using an untracked version of it? Or what am I missing?

lluiscampos · 2024-10-04T07:02:39Z

platform/sha/generic/mbedtls/src/mender-sha.c

+    assert(NULL != context);
+
+    mbedtls_sha256_context *ctx = context;
+    mbedtls_sha256_update(ctx, input, length);


Check return code here too

lluiscampos · 2024-10-04T07:03:16Z

platform/sha/generic/mbedtls/src/mender-sha.c

+    mbedtls_sha256_context *ctx = context;
+    if (NULL != ctx) {
+        if (NULL != output) {
+            mbedtls_sha256_finish(ctx, output);


And here 🫠

lluiscampos · 2024-10-04T07:05:01Z

platform/sha/generic/mbedtls/src/mender-sha.c

+    if (NULL != ctx) {
+        if (NULL != output) {


I think these two should be assert instead. Otherwise we are just returning MENDER_OK while really doing nothing.

See doc string in the declaration ;)

Yes, but why would we allow this use case?

include/mender-sha.h

lluiscampos · 2024-10-04T07:24:48Z

core/src/mender-artifact.c

+    /* 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)) {


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 mender-artifact cli tool however will prevent this to happen, so we are only talking about a manually crafted Artifact or a 3rd party tool creating the Artifact.

Can you elaborate on the motivation? Can't we use good old indexes? It is a list after all.

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.

lluiscampos · 2024-10-04T07:27:00Z

core/src/mender-artifact.c

+        checksum->next               = ctx->artifact_info.checksums;
+        ctx->artifact_info.checksums = checksum;
+
+        /* Start SHA-256 checksum computation (if not already started) */


What do you mean with "if not already started"? At this point it is starting it unconditionally

Good catch! It's some leftovers from an earlier iteration of trying to get this thing to work. I will remove the last part ;)

lluiscampos · 2024-10-04T07:27:31Z

core/src/mender-artifact.c

+
+        /* Start SHA-256 checksum computation (if not already started) */
+        if (MENDER_OK != mender_sha256_begin(&(checksum->context))) {
+            mender_log_error("Failed to start checksum");


... for entry %s, filename

lluiscampos · 2024-10-04T07:33:11Z

core/src/mender-artifact.c

+
+        /* 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)) {


Won't we hit the same problems with sscanf on ESP32?

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?

lluiscampos · 2024-10-04T07:57:07Z

core/src/mender-artifact.c

+        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;
+        }


This sounds insecure to me, why will it get "stuck"?

If we introduce new fields in the manifest, we will in order do

Update Artifact format specification, likely publishing a new version

Implement the changes in the client for the new spec.

Assume the client is updated if you are sending a Artifact with the new spec

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.

Let's discuss this 😄2😄 on Monday with a ☕

larsewi marked this pull request as ready for review October 3, 2024 12:29

larsewi requested review from lluiscampos, vpodzime, danielskinstad and pasinskim October 3, 2024 12:29

vpodzime reviewed Oct 3, 2024

View reviewed changes

larsewi force-pushed the sha256 branch 2 times, most recently from f9b7064 to 0c12ac1 Compare October 3, 2024 14:20

larsewi added 7 commits October 3, 2024 16:29

feat: added SHA-256 interface and MbedTLS implementation

e7c277b

Ticket: MEN-7483 Changelog: None Signed-off-by: Lars Erik Wik <[email protected]>

chore: fixed typos in comments

f38d6d3

Ticket: None Changelog: None Signed-off-by: Lars Erik Wik <[email protected]>

docs: fixed misleading doc string for mender_artifact_get_ctx

8bfcf65

Ticket: None Changelog: None Signed-off-by: Lars Erik Wik <[email protected]>

chore: added logging of mismatching checksums

9130d79

Added logging of mismatching checksums during artifact integrity checks in debug mode. Ticket: None Changelog: None Signed-off-by: Lars Erik Wik <[email protected]>

feat: added integrity checks for artifact data files

6c1a829

The client now performs integrity checks on the data files in the artifact by comparing its computed checksum to the one in the artifact manifest. Ticket: MEN-7483 Changelog: Commit Signed-off-by: Lars Erik Wik <[email protected]>

larsewi force-pushed the sha256 branch from 0c12ac1 to 6c1a829 Compare October 3, 2024 14:29

lluiscampos reviewed Oct 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented artifact integrity checks #89

Implemented artifact integrity checks #89

larsewi commented Oct 3, 2024 •

edited

Loading

mender-test-bot commented Oct 3, 2024

vpodzime left a comment

vpodzime Oct 3, 2024

vpodzime Oct 3, 2024

mender-test-bot commented Oct 3, 2024 •

edited by jira bot

Loading

larsewi commented Oct 3, 2024

lluiscampos left a comment

lluiscampos Oct 4, 2024

larsewi Oct 4, 2024

lluiscampos Oct 4, 2024

lluiscampos Oct 4, 2024

lluiscampos Oct 4, 2024

lluiscampos Oct 4, 2024

larsewi Oct 4, 2024

lluiscampos Oct 4, 2024

lluiscampos Oct 4, 2024

larsewi Oct 4, 2024

lluiscampos Oct 4, 2024

larsewi Oct 4, 2024 •

edited

Loading

lluiscampos Oct 4, 2024

lluiscampos Oct 4, 2024

larsewi Oct 4, 2024 •

edited

Loading

lluiscampos Oct 4, 2024

larsewi Oct 4, 2024

Implemented artifact integrity checks #89

Are you sure you want to change the base?

Implemented artifact integrity checks #89

Conversation

larsewi commented Oct 3, 2024 • edited Loading

mender-test-bot commented Oct 3, 2024

vpodzime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mender-test-bot commented Oct 3, 2024 • edited by jira bot Loading

Changelogs

mender-mcu (sha256)

Features

larsewi commented Oct 3, 2024

lluiscampos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larsewi Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larsewi Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larsewi commented Oct 3, 2024 •

edited

Loading

mender-test-bot commented Oct 3, 2024 •

edited by jira bot

Loading

larsewi Oct 4, 2024 •

edited

Loading

larsewi Oct 4, 2024 •

edited

Loading