On Mon, Sep 30, 2024 at 6:00 AM Mikulas Patocka <mpatocka@xxxxxxxxxx> wrote:
>
>
>
> On Fri, 27 Sep 2024, Sami Tolvanen wrote:
>
> > > See for example openssh, the function read_config_file_depth. There is:
> > >
> > > while (getline(&line, &linesize, f) != -1) {
> > > ... process_config_line_depth
> > > }
> > > free(line);
> > > fclose(f)
> > > if (bad_options > 0)
> > > fatal("%s: terminating, %d bad configuration options",
> > > filename, bad_options);A
> > > return 1;
> > >
> > > So, the function doesn't distinguish between error and eof. If reading the
> > > config file returns -EIO, the function exits with 1 as if the file was
> > > empty.
> >
> > Sounds like OpenSSH's threat model doesn't include an attacker who can
> > trigger arbitrary I/O errors. If you want dm-verity to protect against
> > this, why not add a new restart_on_errors flag instead of changing the
> > semantics of the restart_on_corruption flag and risk breaking existing
> > users?
> >
> > Sami
>
> The dm-verity behavior was reported as a security bug, so by default, it
> should behave in the secure way - i.e. restart or panic on I/O error.
>
> Do you intend to use dm-verity in Android and ChromeOS in the less-secure
> way where it returns -EIO? Have you audited the Android and ChromeOS
> codebase so that -EIO can't cause security breach? If yes, I can make a
> configuration switch for you that will enable the old behavior.
tl;dr don't change the default behavior, but adding a reboot-on-eio is nice.
I'll dig through the archives, but I think this was discussed back when we
first worked on upstreaming (and at linux plumbers).
IO errors are _outside_ dm-verity's threat model. dm-verity verifies that a
block that is read in matches (or can be rebuilt via FEC to) the integrity hash
that it can safely authenticate back to its root hash. Historically, systems
running on raw nand often had to deal with -EIOs in the field that were
transient or could be worked around in other layers of the system.
However, even if a block is read and cannot be authenticated, dm-verity is
not violating any security invariants by not rebooting -- it just must not pass
along invalid data as valid data. Rebooting on invalid data and rebooting on
EIO are system-wide policies which may or may not relate to the security
stance of the system.
wrt this thread, a flag which enables reboot-on-eio seems like a nice feature,
but it shouldn't be conflated with a global security stance or improvement.
Just because someone reports something as a security bug doesn't make it
a security bug -- it just means the behavior didn't match their expectations
and may have a security impact on _their_ deployment.
Both CrOS and Android deploy dm-verity with awarness that the underlying
block device stack is attack surface regardless of whether the device
mapper layer reboots on EIO or not. Unless there is a specific security
problem in the EIO handling code paths or subsequent dm/dm-verity
behavior, then at best it's a late symptom of a problem.
If this behavior change does happen, please make sure dm-verity is still
useful without forced reboots on EIO.
thanks and hth!
will
