[RFC PATCH 0/1] Anti rollback protection for FIT Images

[Thirupathaiah Annapureddy]

Hi Rasmus,

Thanks for the review. Please see in-line:

On 9/2/2020 12:58 AM, Rasmus Villemoes wrote:
> On 01/09/2020 22.48, Thirupathaiah Annapureddy wrote:
>> Anti rollback protection is required when there is a need to retire
>> previous versions of FIT images due to security flaws in them.
>> Currently U-Boot Verified boot does not have rollback protection to
>> protect against known security flaws.
> 
> This is definitely something we've had on our todo-list/wishlist. But we
> haven't had the time to sit down and work out the semantics and
> implementation, so thanks for doing this.
> 
> I think most of this cover letter should be in the commit log instead.
Yes, will do in the formal patch proposal.

> 
>> This RFC introduces a proposal to add anti-rollback protection for
>> FIT images. This protection feature prevents U-Boot from accepting
>> an image if it has ever successfully loaded an image with a larger
>> anti-rollback version number.
>>
>> Each sub-image node inside /images node has an
>> anti-rollback version number(arbvn) similar to rollback_index in
>> Android Verified Boot. This version number is part of signed data and
>> it is incremented as security flaws are discovered and fixed.
>> U-Boot stores the last seen arbvn for a given image type in platform
>> specific tamper-evident storage.
> 
> Hmm. What is the purpose of per-image-type version numbers? That seems
> to be wrong. For example, we use the the "loadables" property in the
> U-Boot FIT to get the SPL to load some firmware blob to a known memory
> address where U-Boot proper then knows it to be. If we happened to have
> two such blobs for different pieces of hardware, they'd have the same
> "type" in the FIT image, but that doesn't mean they should follow the
> same versioning.
I received similar feedback internally to enhance the board specific hooks
to take sub-image node name as another input argument. Board specific code
can use this additional input to distinguish different images of the same
type.

> 
> The way I imagined rollback protection to work (but I really haven't
> given this too much thought) was to have an extra property in the
> configuration:
> 
> 	configurations {
> 		default = "conf-1";
> 		conf-1 {
> 			description = "...";
> 			kernel = "kernel-1";
> 			fdt = "fdt-1";
> 			ramdisk = "ramdisk-1";
> 			arvbn = "arvbn"; /* <-- */
> 
> 			hash-1 {
> 				algo = "sha1";
> 			};
> 			signature-foo {
> 				algo = "sha1,rsa2048";
> 				sign-images = "kernel", "fdt", "ramdisk", "arvbn";
> 				key-name-hint = "foo";
> 			};
> 		};
> 	};
> 
> with arvbn simply being a small extra "image" node
> 
> 		arvbn {
> 			description = "BSP version";
> 			data = <0x02 0x03 0x01>;
> 		}
> 
> (which should of course not be loaded to memory). This requires use of
> signed configurations rather than individually signed images, but that's
> anyway required for proper security.
The current proposal applies to both signed images and signed configurations.

> 
> The board callbacks would simply be given a pointer to the data part of
> that node; that would make the versioning scheme rather flexible instead
> of being limited to a single monotonically increasing u32 (hence also
> the comparison logic should be in the board callbacks, instead of a
The comparison logic should be in the common code to enforce the checks in
a consistent way. 

> "get/set" interface). For example, one could have a version composed as
> $major.$minor;$security, the board logic could prevent both downgrading
> to another major version and another security version. I.e., suppose
> we've had several BSP releases (in chronological order):
A BSP could be composed of multiple images and each of them will have their
own normal versioning scheme. Anti-rollback version number(arbvn) is an
additional complimentary version that can be incremented as 
security flaws are discovered and fixed in a given image. This does not replace
existing versioning scheme for a given image. 

> 
> 1.0;0
> 1.1;0
> 2.0;0
> 1.2;0
> 2.1;0
> <major bug found and fixed>
> 1.3;1
> 2.2;1
> 2.3;1
> 1.4;1
> <another one...>
> 2.4;2
> 1.5;2
> 
> With this, a user on 1.3 cannot update to 2.0; he must go at least to
> 2.2, or he would expose himself to a flaw that had already been fixed.
> On the other hand, there should be nothing wrong with downgrading from
> 1.2 to 1.0. A user on 2.0 must not downgrade to 1.x because the old
> major version cannot read the data written by the 2.0 application.
Please revisit your normal versioning scheme. If possible try to follow
https://semver.org/. As I mentioned earlier, 
Anti-rollback version number(arbvn) is an additional version that can be 
incremented as security flaws are discovered and fixed in a given image.
In other words, it does not replace your existing versioning scheme.

> 
> This sort of thing is hard to implement with just a single rollback
> number: At the time 2.0 is released, what rollback number should it get?
> If we give it 10, we can only produce fixes for nine security bugs in
> the 1.x series before that series ends up with a rollback version larger
> than the 2.0 one, after which a user on 2.0 would wrongly be able to
> downgrade to 1.12.
> 
> 
> tl;dr: I'd like this to not only be about rollback protection for
> security bugs, 
But this RFC is for rollback protection from security point of view.

> but also be able to set other policies for rollback. And
> therefore I'd also like it simply to be known as "version" instead of
> the somewhat odd arvbn acronym. 
This is clarified in the previous comments. 
Images will continue to maintain their existing normal versioning scheme
and you can continue to apply existing policies around that.

>> Pseudo code is as follows:
>>
>> ret = board_get_arbvn(type, &plat_arbvn);
>> ...
>> if (image_arbvn < plat_arbvn) {
>> 	return -EPERM;
>> } else if (image_arbvn > plat_arbvn) {
>> 	ret = board_set_arbvn(type, image_arbvn);
>> 	return ret;
>> } else {
>> 	return 0;
>> }
> 
> Hm, I think you should post-pone writing back the new version number
> until all other checks have passed, i.e. until just before we pass
> control to the new image. 
I do not think U-Boot will pass control to each and every image in the 
FIT container. ex: ramdisk.
Board specific code can still have the flexibility to defer and write new
version number at a later time ex: board_preboot_os(). 

>> The following board specific hooks are required to get/set arbvn
>> from platform specific tamper-evident storage.
>> int board_get_arbvn(uint8_t ih_type, uint32_t *arbvn);
>> int board_set_arbvn(uint8_t ih_type, uint32_t arbvn);
>>
>> As an example, consider this FIT:
>> / {
>> 	images {
>> 		kernel-1 {
>> 			data = <data for kernel1>
>> 			arbvn = <1>;
>> 			hash-1 {
>> 				algo = "sha1";
>> 				value = <...kernel hash 1...>
>> 			};
>> 		};
>> 		fdt-1 {
>> 			data = <data for fdt1>;
>> 			hash-1 {
>> 				algo = "sha1";
>> 				value = <...fdt hash 1...>
>> 			};
>> 		};
>> 	};
>> 	configurations {
>> 		default = "conf-1";
>> 		conf-1 {
>> 			kernel = "kernel-1";
>> 			fdt = "fdt-1";
>> 			signature-1 {
>> 				algo = "sha1,rsa2048";
>> 				sign-images = "fdt", "kernel";
>> 				value = <...conf 1 signature...>;
>> 			};
>> 		};
>> 	};
>> };
>>
>> In the above example, kernel-1 image has an arbvn of 1.
>> if plat_arbvn is 1, the system will boot with this FIT image.
>> if plat_arbvn is 2 or more, U-Boot will prevent the system from booting
>> with this FIT image.
> 
> Yeah, this seems a bit arbitrary. 
This is just an example. 

> Why does the kernel have an arvbn, but
> not the device tree or (perhaps more importantly) an initramfs? 
device tree and initramfs can also have their own arbvn. Have you checked
the code patch that came with this RFC? There is nothing in the code that 
prevents these images from having their own arbvn.

> Which images must have such a number?
Any image can have their own arbvn.

> As I said above, I think the version
> number should really be a property of the whole configuration. 
I have considered the above simpler option. But I believe image specific arbvn
is more appropriate as the security features and security bug fixes are implemented
in images than in config sub-nodes.

Best Regards,
Thiru