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

[Rasmus Villemoes]

On 15/09/2020 07.22, Thirupathaiah Annapureddy wrote:
> Hi Rasmus,
> 
>>
>>> 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.

OK. Are those node-names tamper-proof? I.e., can I take a valid signed
FIT, modify node names (and the references to those) and make one
LOADABLE image appear to be another LOADABLE from the POV of the board hook?

>>
>> 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.

Perhaps, but did you read the "required for proper security" part? Why
not mandate use of signed configurations and move towards getting rid of
possibility of mix-n-match attacks once and for all?

>>
>> 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. 

Well, I disagree. Storage/retrieval of the rollback version in a secure
and robust way has to be done in a board-specific manner anyway, so I
don't think leaving the comparison logic to board code adds much risk of
getting the whole thing wrong.

>> "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/.

That was just a simple example (which is actually somwhat semver-like, I
just dropped the .bugfix to keep it simple), and in any case I'm a
consultant, whatever versioning scheme any given customer uses is
completely out of my hands and is usually not (only) decided on
technical merits.

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.

No, and I definitely want it to work that way; that's why I separated
the "security version" from the normal one by a semi-colon above, to
stress its orthogonality. We're completely in agreement on that point.

My point is that there is currently no simple, or at least standard, way
to enforce the "you can't downgrade from one major version to a lower
one" - simply because there's no concept of versioning in current FIT
images. IMO, having semi-generic code doing the "can't downgrade due to
CVE2020-12345" and having some board_preboot_os do another version check
is silly - especially as wherever the latter version might be stored in
the FIT is almost certainly not going to be automatically signed,
opening up all kinds of questions of "can we actually trust that version
number".

So I'm asking you to consider hitting two flies with one swat, and make
this thing a generic "version", that can be used for whatever purpose
the current product might want to enforce regarding downgrades. Which,
as I said, in no way prevents your use case, the "version" would just be
your arbvn, and the comparison is just your comparison of two u32s.

>>
>> 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.

See above, I don't think it's more complicated to allow use cases beyond
merely security weaknesses. But once you set "arbvn is one monotonically
increasing u32" in stone, getting those other use cases implemented and
upstreamable becomes much much harder.

>> 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.

Except that such a policy cannot actually be implemented currently.

>>> 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
No, of course not, what I meant was that before control is handed over
to linux (or whatever OS is loaded), there should be another loop over
the loaded images and have their arbvn written back.

But here we actually see another argument for the rollback version
applying to the FIT image (the chosen configuration) as a whole:
atomicity. If storing the new version number fails for one of the later
seen images, we've already updated the arbvn for some of the previous
images, which is very likely to brick the device.

> Board specific code can still have the flexibility to defer and write new
> version number at a later time ex: board_preboot_os(). 

So you would have board_set_arbvn() be a mostly empty hook that perhaps
just stashes the new version in some temporary location, for each
image/image type, and then have a "now really write out the new
versions" called from board_preboot_os()? That could work, and could
perhaps also partially mitigate the atomicity problem (by having the
"write things out" function also be able to attempt to revert
already-changed values) but I think that should be part of the
architecture of this rather than something random board developers might
remember to implement.

>>> 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.

OK, I was mostly confused by this being tied to the image type, which
seemed to ignore that a FIT/configuration can validly contain multiple
images with the same type.

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

And what enforces that e.g. the kernel image has an arbvn? IOW, what
prevents me from skipping that check by deleting that property? The
cover letter says that the arbvn is part of the signed data, but I can't
find where that is actually implemented.

>> 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.

Yes, I can see that in the vast majority of cases (there are also cases
where it would be the combination of one kernel version with one
specific device tree, but that's of course quite rare). And I don't
really care that much if the version lives with each image or in the
conf node (though ideally I'd want to be able to, when using signed
configurations, just having one version apply to that configuration as a
whole - but I guess I could do that by having all configurations include
a pseudo-image which just carries that global version number). What I do
care about is that the "version" format and semantics can be defined by
the individual product.

Rasmus