search

ostreedev / ostree

Operating system and container binary deployment and upgrades
https://ostreedev.github.io/ostree/
Other
1.31k stars 300 forks source link

FS_IOC_MEASURE_VERITY ioctl can read into an unaligned buffer #3339

Closed smcv closed 2 weeks ago

smcv commented 2 weeks ago

While updating from 2024.8 to 2024.9 in Debian, I noticed that the composefs code saves the result of an FS_IOC_MEASURE_VERITY ioctl into char buf[sizeof (struct fsverity_digest) + OSTREE_SHA256_DIGEST_LEN], and dereferences that pointer via (struct fsverity_digest *)&buf.

If I understand alignment requirements correctly, that cast is not necessarily safe, because struct fsverity_digest starts with a __u16 which (on at least some architectures) requires "natural" 2-byte alignment, but a char[] is not guaranteed to have any specific alignment.

This might also violate Standard C aliasing rules (I think optimizing compilers are allowed to assume that a struct fsverity_digest * does not alias an array of a different type), but I'm not 100% sure whether the exception that is made in the standard for char * applies here.

Possible solutions that I can see:

  1. use a union { struct fsverity_digest; char[...]; } which is aligned to the stricter requirement of the struct fsverity_digest but has enough space for the larger char[], which solves both the alignment and the aliasing
  2. use the char[], but with alignas(struct fsverity_digest) to ensure it is correctly aligned; this does not solve the aliasing but perhaps that isn't a problem because char * has special rules
  3. have the ioctl write into a char[], then memcpy out the struct fsverity_digest into a correctly-aligned variable

The union seems simplest and most obviously-correct.

cgwalters commented 2 weeks ago

While updating from 2024.8 to 2024.9 in Debian, I noticed that the composefs code

But were you running a static analyzer? Just perusing git diff?

smcv commented 2 weeks ago

Just perusing git diff?

Yes. This is a dependency of the component I'm actually interested in (Flatpak), so I try to be a responsible maintainer, but there's a rather small limit to how much time and resources I can spend on libostree - especially the parts that Flatpak doesn't make use of.

libcompose is an optional dependency of a dependency of the component I'm actually interested in, and as far as I can tell is only useful for use-cases that are orthogonal to the one I'm interested in, so I don't have the time and resources available to package it for Debian. Sorry, I wish I had more hours in a day, but I don't!

cgwalters commented 2 weeks ago

Yes. This is a dependency of the component I'm actually interested in (Flatpak), so I try to be a responsible maintainer,

And thank you for all your work there!

libcompose is an optional dependency of a dependency of the component I'm actually interested in, and as far as I can tell is only useful for use-cases that are orthogonal to the one I'm interested in, so I don't have the time and resources available to package it for Debian. Sorry, I wish I had more hours in a day, but I don't!

I need to take this one to the flatpak issues, but basically composefs is very intentionally designed to be useful there too! It gives us a way to have dm-verity style on-disk integrity for flatpak apps, the same way we're aiming to do it for bootable hosts and OCI. Anyways, a distinct discussion but something to keep in mind.