[riot-notifications] [RIOT-OS/RIOT] sys/riotboot: add initial image digest verification (#11805)

Francisco notifications at github.com
Mon Jul 8 16:56:41 CEST 2019


fjmolinas requested changes on this pull request.

I tested the use case in #11818. As for all I can see the returned value is incorrect and the digest validation is failing.

I'm getting this:

```
sha256_digest:82015820a72ebb4ef82107d3de4b9586f0d18cd692aeb0c7cef27547be0a238f
digest:       a72ebb4ef82107d3de4b9586f0d18cd692aeb0c7cef27547be0a238fcaf99704
```



> +{
+    char digest[SHA256_DIGEST_LENGTH];
+
+    sha256_context_t sha256;
+
+    if (img_len < 4) {
+        LOG_INFO("riotboot: verify_sha256(): image too small\n");
+        return -1;
+    }
+
+    uint8_t *img_start = (uint8_t *)riotboot_slot_get_hdr(target_slot);
+
+    LOG_INFO("riotboot: verifying digest at %p (img at: %p size: %u)\n", sha256_digest, img_start, img_len);
+
+    sha256_init(&sha256);
+    sha256_update(&sha256, "RIOT", 4);

Is this `RIOTBOOT_MAGIC`? If it is I would replace "RIOT" by RIOTBOOT_MAGIC, and 4 by `sizeof(RIOTBOOT_MAGIC)` to avoid magic numbers.

> +    char digest[SHA256_DIGEST_LENGTH];
+
+    sha256_context_t sha256;
+
+    if (img_len < 4) {
+        LOG_INFO("riotboot: verify_sha256(): image too small\n");
+        return -1;
+    }
+
+    uint8_t *img_start = (uint8_t *)riotboot_slot_get_hdr(target_slot);
+
+    LOG_INFO("riotboot: verifying digest at %p (img at: %p size: %u)\n", sha256_digest, img_start, img_len);
+
+    sha256_init(&sha256);
+    sha256_update(&sha256, "RIOT", 4);
+    sha256_update(&sha256, img_start + 4, img_len - 4);

Same as in upper comment.

> +
+    if (img_len < 4) {
+        LOG_INFO("riotboot: verify_sha256(): image too small\n");
+        return -1;
+    }
+
+    uint8_t *img_start = (uint8_t *)riotboot_slot_get_hdr(target_slot);
+
+    LOG_INFO("riotboot: verifying digest at %p (img at: %p size: %u)\n", sha256_digest, img_start, img_len);
+
+    sha256_init(&sha256);
+    sha256_update(&sha256, "RIOT", 4);
+    sha256_update(&sha256, img_start + 4, img_len - 4);
+    sha256_final(&sha256, digest);
+
+    return memcmp(sha256_digest, digest, SHA256_DIGEST_LENGTH) == 0;

This  return is wrong, should be:

`memcmp(sha256_digest, digest, SHA256_DIGEST_LENGTH)`

as it is now it returns 0 for invalid digests and in #11818 it is used as if 0 meant the verify happened OK.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11805#pullrequestreview-258917649
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190708/ea23b149/attachment.html>


More information about the notifications mailing list