boot: bootutil: swap_offset: Abort attempted swap on inaccessible image

[vengaer]

I recently ran into a double bit flash error when writing firmware to the secondary image slot while preparing for a firmware upgrade. When using the swap using offset algorithm, such an error occurring in the secondary slot causes the boot_read_swap_state call for said slot in swap_status_source to fail with BOOT_EFLASH, triggering the subsequent assertion. When resetting the system, the problem - unsurprisingly - persists, causing the bootloader to trigger the same assertion over and over again. The only way out appears to be to erase the secondary slot using a programmer, something that can prove rather cumbersome should the device be far away.

This PR alters the behavior of swap_status_source such that, rather than triggering the assertion, it falls back on booting the primary image should the secondary prove inaccessible. This should allow users to reattempt the DFU using their upload method of choice.

[nordicjm]

Actually an inaccessible upgrade slot would be an issue, it is by design that it does not boot as it likely indicates an attacker is trying something. Have you tried disabling assertions? They should not be enabled in production firmware

[vengaer]

Actually an inaccessible upgrade slot would be an issue, it is by design that it does not boot as it likely indicates an attacker is trying something. Have you tried disabling assertions? They should not be enabled in production firmware

I have not tried it with assertions disabled, no. To my eye, the behavior doesn't look quite right even should the assertion be gone though. Looking at only the use of state_secondary_slot in swap_status_source, the declaration of and all subsequent accesses to the former are shown below

    struct boot_swap_state state_secondary_slot;
    
   /* .... */

    rc = boot_read_swap_state(state->imgs[image_index][BOOT_SLOT_SECONDARY].area,
                              &state_secondary_slot);
    assert(rc == 0);
    BOOT_LOG_SWAP_STATE("Secondary image", &state_secondary_slot);

    if (state_primary_slot.magic == BOOT_MAGIC_GOOD &&
        state_primary_slot.copy_done == BOOT_FLAG_UNSET &&
        state_secondary_slot.magic != BOOT_MAGIC_GOOD) {

whereas the first part of boot_read_swap_state looks as follows

int
boot_read_swap_state(const struct flash_area *fap,
                     struct boot_swap_state *state)
{
    uint8_t magic[BOOT_MAGIC_SZ];
    uint32_t off;
    uint8_t swap_info;
    int rc;

    off = boot_magic_off(fap);
    rc = flash_area_read(fap, off, magic, BOOT_MAGIC_SZ);
    if (rc < 0) {
        return BOOT_EFLASH;
    }

Should the flash_area_read call fail - which was what I encountered due to the double bit error - and assertions be disabled, subsequent accesses to state_secondary_slot in swap_status_source end up reading garbage. That's something I'd be looking to exploit if I were an attacker, especially seeing as all that is required to reliably trigger the read is write access to the secondary image slot. I don't mean to cry wolf but if this were Linux - which, of course, it is not, but still - such a thing would probably even be considered a CVE.

Assuming asserts are disabled, accesses to state_primary_slot suffer from the same issue.

That above does, of course, assume you don't use -ftrivial-auto-var-init or something along those lines. If you do, please accept my apologies for wasting your time.

Actually an inaccessible upgrade slot would be an issue, it is by design that it does not boot as it likely indicates an attacker is trying something

Just to make sure I understand this part correctly, is it the intended behavior to refuse to boot even a valid primary image should the secondary one prove to be corrupted/inaccessible/whatever? If so, this PR is certainly not the correct way of addressing the issue.

All of that said, I'm not quite sure I understand why booting the primary image would be inadmissible even in the presence of a malicious upgrade counterpart so long as mcuboot verifies the signature of the former before booting it. Someone might be up to something, sure, but so long as the signature of the primary image is valid, wouldn't it be safe to boot it? (I'm not trying to argue, I'm just curious).

[nordicjm]

Should the flash_area_read call fail - which was what I encountered due to the double bit error - and assertions be disabled, subsequent accesses to state_secondary_slot in swap_status_source end up reading garbage. That's something I'd be looking to exploit if I were an attacker, especially seeing as all that is required to reliably trigger the read is write access to the secondary image slot. I don't mean to cry wolf but if this were Linux - which, of course, it is not, but still - such a thing would probably even be considered a CVE.

Yes having the variables initialised to 0 would be a good step, but I don't see how this would be a CVE, all it would do is say there is an image there, image validation would still fail

Just to make sure I understand this part correctly, is it the intended behavior to refuse to boot even a valid primary image should the secondary one prove to be corrupted/inaccessible/whatever? If so, this PR is certainly not the correct way of addressing the issue.

MCUboot has been built around the idea that flash devices are accessible, reads/writes/erases should work, the idea that a flash read fails because a vendor uses ECC was probably not part of the consideration when it was implemented. But it expects that devices to exist and be readable, corruption would be detected by reading and verifying the image data - not relying on ECC checks (there was another vendor that would fire a fault if you attempted to read from flash that had a failed ECC, so nothing is really uniform in how vendors do that).

All of that said, I'm not quite sure I understand why booting the primary image would be inadmissible even in the presence of a malicious upgrade counterpart so long as mcuboot verifies the signature of the former before booting it. Someone might be up to something, sure, but so long as the signature of the primary image is valid, wouldn't it be safe to boot it? (I'm not trying to argue, I'm just curious).

If a product ships with an external flash for the secondary slot, the idea is that it would be a security vulnerability if the secondary slot cannot be accessed since a malicious user could just de-solder one of the chip's I/O lines and permanently prevent firmware updates, it averts that issue (mostly, I dare say someone could alter the application with a flaw and get it to erase then permanently enable write protection on those areas to do something equivalent but that is out of scope for MCUboot)

[vengaer]

Sorry for the somewhat late reply, seems Github were having problems yesterday.

Yes having the variables initialised to 0 would be a good step, but I don't see how this would be a CVE, all it would do is say there is an image there, image validation would still fail

You are probably right in that altering control flow should not in itself be considered a CVE. That said, I do believe that it at least in theory could be used as part of a larger exploit (which would of course require at least another vulnerability). Be that as it may, it seems to me that initializing the variables to 0 should have the same practical effect as checking the return value and returning early here, albeit in a perhaps slightly more roundabout fashion.

You have worked far more on MCUboot than I have. If you prefer memset over this PR, I will not object.

MCUboot has been built around the idea that flash devices are accessible, reads/writes/erases should work, the idea that a flash read fails because a vendor uses ECC was probably not part of the consideration when it was implemented. But it expects that devices to exist and be readable, corruption would be detected by reading and verifying the image data - not relying on ECC checks (there was another vendor that would fire a fault if you attempted to read from flash that had a failed ECC, so nothing is really uniform in how vendors do that).

That seems a reasonable assumption. Thank you for the clarification.

If a product ships with an external flash for the secondary slot, the idea is that it would be a security vulnerability if the secondary slot cannot be accessed since a malicious user could just de-solder one of the chip's I/O lines and permanently prevent firmware updates, it averts that issue (mostly, I dare say someone could alter the application with a flaw and get it to erase then permanently enable write protection on those areas to do something equivalent but that is out of scope for MCUboot)

That is very true. So, provided I have read your comments correctly, MCUboot should not boot the system at all in such a situation. Correct me if I'm wrong but would not zero-initializing the boot state variables have the same effect as this PR - i.e. having MCUboot boot the primary image - should the secondary slot prove inaccessible?