Serial recovery: decryption result discarded and SMP success sent before boot_handle_enc_fw() (encrypted image, primary-slot upload)

[JayToltTech]

Description

In serial recovery, bs_upload() (boot/boot_serial/src/boot_serial.c) sends the SMP upload success response before the encrypted image is decrypted, and then discards the return value of boot_handle_enc_fw(). A decryption failure is therefore both unreported to the client and unhandled — the primary slot is left containing an undecryptable image.

Affected code (upstream main, commit 0fae8920)

bs_upload() is static void. Its out: label:

out:
    ...
    zcbor_map_end_encode(cbor_state, 10);

    boot_serial_output();          // (1) success rc=0 + offset sent to client HERE

#ifdef MCUBOOT_ENC_IMAGES
#if !defined(MCUBOOT_SERIAL_DIRECT_IMAGE_UPLOAD)
    if (flash_area_id_from_multi_image_slot(img_num, 0) == FLASH_AREA_IMAGE_PRIMARY(0))
#else
    if (flash_area_id_from_direct_image(img_num) == FLASH_AREA_IMAGE_PRIMARY(0))
#endif
    {
        if (curr_off == img_size) {
            rc = boot_handle_enc_fw(fap);   // (2) decrypt-in-place; rc never read again
        }
    }
#endif

    flash_area_close(fap);          // function returns; rc discarded
}

Two coupled problems:

1. Success reported before decryption. boot_serial_output() emits rc=0 and the accepted offset at (1), before the in-place decryption at (2). The client is told the final chunk succeeded regardless of the decryption outcome.
2. Decryption result discarded. boot_handle_enc_fw() → decrypt_image_inplace() (boot/boot_serial/src/boot_serial_encryption.c) can fail, but rc is assigned at (2) and never read (the function is void and only flash_area_close() follows). A failed or partial decrypt is silently swallowed, leaving an undecryptable image in the primary slot.

Impact

This is only safe under CONFIG_BOOT_VALIDATE_SLOT0=y, where the boot-time signature check rejects the bad image (and, with CONFIG_BOOT_SERIAL_NO_APPLICATION=y, re-enters recovery). With CONFIG_BOOT_VALIDATE_SLOT0=n — a supported configuration — there is no boot-time check, so a corrupt or partially-decrypted primary image can be executed on the next boot. Independently of that Kconfig, reporting SMP success for an install that then fails to decrypt is a protocol/robustness defect.

The same assumption is visible in the disabled pre-decrypt validation inside decrypt_image_inplace():

#if 0 //Skip this step?, the image will just not boot if it's not decrypted properly
     /* First check if the encrypted image is a good image before decrypting */
    FIH_CALL(boot_image_validate_encrypted, fih_rc, state, fa_p, &_hdr, tmpbuf, BOOT_TMPBUF_SZ);

Steps to reproduce

1. Build MCUboot for serial recovery with CONFIG_MCUBOOT_SERIAL=y, CONFIG_BOOT_ENCRYPT_IMAGE=y, and CONFIG_BOOT_VALIDATE_SLOT0=n.
2. Enter serial recovery and upload an encrypted image whose decryption will fail (e.g. encrypted to a key the bootloader does not hold, or a corrupt ciphertext that still completes the chunked upload).
3. Observe: the SMP client reports the upload succeeded (rc=0), and on the next boot MCUboot jumps to the undecrypted/garbage primary image rather than reporting failure or re-entering recovery.

Suggested fix

• Move the response encoding / boot_serial_output() to after boot_handle_enc_fw(), and reflect its result in the SMP rc so the client learns of decrypt failures.
• On decryption failure, erase the primary slot header/trailer so a partially-decrypted image cannot be booted (matters for CONFIG_BOOT_VALIDATE_SLOT0=n).
• At minimum, stop discarding boot_handle_enc_fw()'s return value.

Version

Observed on main @ 0fae8920c4e5acb792b3fe766c89c668f42be6ee and on the v2.3.0 line (v2.3.0-34-g41b043d4).

[nordicjm]

This is expected, decryption takes a long time and would be a command timeout. The user is expected to send the image, wait for decryption, then check the image state using that command before rebooting (or reuploading/logging an error if it has failed)