Security considerations around swupdate checksum validation

[Dominique MARTINET]

Hi,

I've started discussions around this with Stefano earlier this month,
but thought it might benefit being discussed more publicly.
(I've forwarded the mails we sent, but will recap here anyway and most
shouldn't need to read it all)
I was holding up for our monthly release which has a few fixes in the
way we build SWU files (nothing fixed in swupdate) and a libarchive
update, as that covers most of the issues we were facing for our
boards updates.

Sorry for the long mail.



Recap of how SWUs validation works
==================================

SWU integrity is checked as follow:
- we have a sw-description file with checksums
- that sw-description file is signed, swupdate checks that early
- for files without installed_directly, they're saved to TMPDIR while
verifying the checksum matches (which was signed, so the file is
correct) and then it's processed by its handler
- for files with installed_directly set, we're streaming them to the
handler which processes the files directly; most will be using
copyimage()/copyfile() or a variant which streams to a fd while
verifying the checksum -- sending a bad file will eventually fail when
the checksum is finalized and it doesn't match, but until then all sort
of things can happen.

That means that if I can get my hand on a valid SWU, replace a file that
has installed_directly with a proper payload and somehow trigger an
install I can make the handler process arbitrary data before the update
fails. (How likely that is depends on how you distribute your updates
and who your customers are, unfortunately for us it is a quite realistic
threat with the archive handler)


Attack examples
===============

There's probably more but here are a few examples of things that can go
wrong with this:

1/ We're feeding unvalidated data to libarchive (exploits)
I was talking about a libarchive update earlier, because CVE-2024-26256
came out earlier and was only fixed this month for our distro.
The details don't overlay matter but basically it's supposedly a remote
code execution when processing a bad .rar file, and libarchive
automatically guesses the archive format so I was able to replace a
tar.zst (decompressed by swupdate) file with a rar.zst file with no
issue until the checksum failed, meaning that someone with the proper
payload could make swupdate run anything on this affected systems.

This CVE has been fixed, but there will be others - for the archive
handler, anything impacting libarchive[1] or any of its dependencies (on my
system, libcrypto/libacl, expat (what is a xar file?...), lzma, zstd,
lz4, bz2, libz) ; for other handlers whatever it is they use, and
if using compression at swupdate level the zstd compression is also run
in parallel to checksumming so any bug in ztsd[2] also impacts updates
without installed_directly.

[1] https://osv.dev/list?ecosystem=&q=libarchive
[2] https://osv.dev/list?ecosystem=&q=zstd


2/ We're feeding unvalidated data to libarchive (single copy directories)
Likewise, if the update isn't 100% double-copy and the archive handler
destination path contains any single-copy directory, you can make an
archive that will write anything to that partition and it's there to
stay.
It's not recommended to unpack anything to a single-copy directory
anyway because a failing update can leave it in a bad state, but the
presence of such a directory should not need to be a security
consideration, or if it is we need to much more strongly advertise this
problem.

These first two problems can be mitigated by enabling encryption, but
AES CBC is still vulnerable to some specific modifications so while it
makes such attacks extremely unlikely it might not be enough on its own.


3/ The checksum doesn't fail until the file is over.
File sizes aren't written in sw-description, so it's possible to make an
absurdly large SWU (thanksfully? limited to 4GB per file as we only
support newc/crc) and make the handler work quite a bit before the
update fails. Without install_directly, if TMPDIR isn't on its own
partition it's possible to fill it and impact other users of the
partition.

This could be handled by adding a size attribute to each file/image in
the sw-description, but while some items have a size there is no generic
way of doing this right now.



Going forward
=============

After thinking it off and on the past month, I'm back to my first idea
that we want to validate the data as much as possible before processing
it -- or at least a new way to make that possible.


Issue 2 above (single-copy partition accessible) can be addressed by
improving the documentation, I'll send some documentation patch this
month.

For the rest I think we'll need some improvements in the implementation,
if you dig through the attached mails we currently have two suggestions:
- Stefano suggested using the delta handler as a pass-through handler
(delta data is chunked with zchunk, so it can validate a bit at a time
without processing the whole thing, then forward to another handler)

I don't like that it's not compatible (new SWUs won't be installable on
older versions of swupdate), but it's a solid approach;
it'll require adding a way of embedding the chunks in the swu itself and
I'm not sure how the file in cpio <-> handler mapping in sw-description
will work but I'm sure we can figure that out.
It might need to also add size fields for not-installed directly, and if
we go that way I'll want to fuzz zchunk a bit (not sure it's been
audited from a security perspective...), but that's definitely a
possible way forward.

- I had suggested implementing some chunked checksuming ourselves by
replacing our sha256 property by a sha256_chunks array property;
mostly because this can be made retrocompatible (the new SWU's
sw-description just need to contain both the sha256 field and the new
chunks array; older swupdate versions will use the checksum as before
and newer versions will use the array).
This would allow us to store each chunk (in TMPDIR-file backed memory?)
while computing *only* the checksum, then we can go on to decompress and
feed to the handler. It'll also solve the size problem, as we'll know
the file was too big if there is no chunk left to validate.

Implementation wise I had looked at it a bit and was quite annoyed that
we're "closing" the config before starting the installs, so we can't
fetch the checksum chunks from config while extracting the files
currently and would need to allocate a dyanmic array upfront, which
might lock some memory for very large SWUs (I counted 32KB/GB of update
for 1MB chunks, so for my use case it's actually acceptable, but it's
not negligible).
But thinking about this again now I don't see why we couldn't change the
config parsing to leave the handle open and close it when cleaning up
the update, so I'm tempted to try that.



If someone on the list has opinions, another idea, or anything to share
please do!

I'm still overworked, but writing this mail rekindled my motivation
here, unless there's a reason not to I'll finish my poc for chunked
transfer next week; I think it shouldn't be a too large patch and
having a concrete implementation might give better grounds for
discussions.
(And if the zchunk approach seems better we can still throw it away, but
right now even ignoring the backwards compatibility I don't quite see
how to make the files in the cpio archive match up cleanly)


Thanks,
--
Dominique

[James Hilliard]

There's some additional ideas in this thread that may be relevant such
as using a merkle tree data structure and only storing the root hash in
the sw-description.

https://groups.google.com/g/swupdate/c/DSbJo83oc3c/m/dTuwuXqIAAAJ

I had also come across this library which might give some good ideas
on how to implement the data structures efficiently for chunk validation.

https://github.com/IAIK/secure-block-device

>
> I'm still overworked, but writing this mail rekindled my motivation
> here, unless there's a reason not to I'll finish my poc for chunked
> transfer next week; I think it shouldn't be a too large patch and
> having a concrete implementation might give better grounds for
> discussions.
> (And if the zchunk approach seems better we can still throw it away, but
> right now even ignoring the backwards compatibility I don't quite see
> how to make the files in the cpio archive match up cleanly)
>
>
> Thanks,
> --
> Dominique
>

> --
> You received this message because you are subscribed to the Google Groups "swupdate" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to swupdate+u...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/swupdate/ZlmB1OIxf4KbVPXA%40atmark-techno.com.
>
>
>
> ---------- Forwarded message ----------
> From: Dominique MARTINET <dominique...@atmark-techno.com>
> To: Stefano Babic <stefan...@swupdate.org>
> Cc:
> Bcc:
> Date: Tue, 7 May 2024 14:02:11 +0900
> Subject: [security RFC] install_directly and checksum validation - chunkify the checksum?
> Hi Stefano,
>
> sorry for the direct mail; I've started writing this for the list but
> since I consider this to be a security issue for us I'd rather get your
> feedback first; happy to forward all our mails to the list once some
> basic patches are ready (doesn't necessarily have to be merged yet as I
> don't think it's necessarily a swupdate problem per se, more of a
> problem with how I'm using it; I just selfishly want at least some
> partial solution first before going public)
>
>
> This is rather obvious in hindsight and I'm not sure why I didn't
> consider this a problem at first, but using installed_directly we're
> basically trusting handlers to be no-op (or fully reversible) if someone
> keeps an existing sw-description and its signature but swaps one of the
> files inside the swu archive:
>
> - swupdate checks sw-description signature matches (ok)
> - file X is fed to the handler while verifying the checksum in parallel
> - verification fails, making handler ultimately fail terminating the
> installation
>
> For most handlers the worst that can happen is useless writes to disk
> (e.g. filling up an inactive partition that cannot be used), but for
> example the archive handler could easily be abused if there is a
> "shared" partition (we unfortuantely have such a "state" partition, so
> e.g. logs or databases can be updated during a new install and rebooting
> into the update will not lose anything -- it's mounted during the update
> if the user need to interact with it directly e.g. cleanup obsolete data)
>
> Encryption somewhat mitigates the issue because it's harder to mess with
> encrypted data, but while we suggest encryption for our users'
> application update, we also distribute "official OS" updates for all
> our users and these are not encrypted.
>
>
> With the background out of the way, that leaves a few possibilities:
> - Stop mounting such a partition; I'm not considering this further for
> two reasons: 1/ it's breaking compatibility, which I'd rather avoid if
> possible, 2/ even if we don't mount the partition directly, swupdate can
> also write to different btrfs subvolumes (we're using snapshots for
> cheap A/B copy), so an attacker could still cause trouble by filling the
> fs at the very least and I'd like to protect from that as well.
>
> As a poor workaround I could ensure the shared partition isn't mounted
> for our official updates, but I'd like to think we can do better.
>
> - Make the archive handler buffer the whole archive so we can verify
> the checksum before starting extraction, this is brutish and might not
> work for large archives; we're also not setting the archive compressed
> size (checksum target; I'm not sure it's possible in a generic way?), so
> this does not address filling $TMPDIR.
>
> In our particular case we already accept large-ish images in TMPDIR
> before extracting them (podman doesn't support non-seekable container
> archive input so we store a copy there), so I'd consider this acceptable
> for us for previously generated archives if there is no better solution,
> but I wouldn't suggest this for swupdate upstream.
>
> - Modify the checksum in sw-description to not be a single value, but
> e.g. a list of checksums for each 1MB chunks; __swupdate_copy could then
> temporarily buffer these chunks in TMPDIR then feed the data to handlers
> after the chunk is validated
> (chunk size to be discussed; the current BUFF_SIZE is 16KB which does
> not seem practical so I suggested 1MB but I'm open to suggestions.
> I know swupdate doesn't/shouldn't use dynamically allocated memory for
> updates and reserving a full MB isn't realistic, so for example using a
> mmap'ed buffer in TMPDIR would allow the kernel to flush to disk if
> there is memory pressure, yet never touch disk if there is none; but
> we can just really write it down 16KB at a time and read it back if you
> prefer, I don't want to get lost in details yet)
>
> That's obviously the direction I prefer, but it requires allowing a new
> way of writing checksums in the config so it's obviously something to
> consider.
> iirc swupdate ignores unknown fields so we could keep the full checksum
> and the chunked checksums, old versions of swupdate would work as they
> previously did and new versions would only use the chunked one if
> present.
>
>
> What do you think?
> --
> Dominique
>
>
>
> ---------- Forwarded message ----------
> From: Stefano Babic <stefan...@swupdate.org>
> To: Dominique MARTINET <dominique...@atmark-techno.com>
> Cc:
> Bcc:
> Date: Tue, 7 May 2024 10:00:30 +0200
> Subject: Re: [security RFC] install_directly and checksum validation - chunkify the checksum?
> Hi Dominique,
>
> On 07.05.24 07:02, Dominique MARTINET wrote:
> > Hi Stefano,
> >
> > sorry for the direct mail; I've started writing this for the list but
> > since I consider this to be a security issue for us I'd rather get your
> > feedback first; happy to forward all our mails to the list once some
> > basic patches are ready (doesn't necessarily have to be merged yet as I
> > don't think it's necessarily a swupdate problem per se, more of a
> > problem with how I'm using it; I just selfishly want at least some
> > partial solution first before going public)
>
> Sure - then a post to ML is useful and it will be tracked for the
> future. You could also think about if it makes sense to write something
> into documentation - I collect some times ago some suggestions into a
> Best Practices chapter (or where you think it is better).
>
> >
> >
> > This is rather obvious in hindsight and I'm not sure why I didn't
> > consider this a problem at first, but using installed_directly we're
> > basically trusting handlers to be no-op (or fully reversible) if someone
> > keeps an existing sw-description and its signature but swaps one of the
> > files inside the swu archive:
> >
> > - swupdate checks sw-description signature matches (ok)
> > - file X is fed to the handler while verifying the checksum in parallel
> > - verification fails, making handler ultimately fail terminating the
> > installation
>
> Yes - but this is exactly how it works to allow streaming. It is exactly
> the same when we have a single-copy approach: something were installed,
> but the update is considered FAILED and shouldn't taken. SWUodate
> reports this via progress interface and bootloader variable ("recovery"
> and "ustate"), and it is duty of the integrator to do something if required.
>
>
> >
> > For most handlers the worst that can happen is useless writes to disk
> > (e.g. filling up an inactive partition that cannot be used), but for
> > example the archive handler could easily be abused if there is a
> > "shared" partition (we unfortuantely have such a "state" partition, so
> > e.g. logs or databases can be updated during a new install and rebooting
> > into the update will not lose anything -- it's mounted during the update
> > if the user need to interact with it directly e.g. cleanup obsolete data)
>
> You have then a mixed between double-copy and single-copy. All data on
> your shared partition are updated in a single copy concept, and should
> be considered invalid until SWUpdate will be successful.
>
> I generally avoid to mix the concepts - if I go for a double-copy, I try
> to have double-copy for all artifacts. In this case, if space is enough,
> I had tried to duplicate the directory where the archive handler
> extracts data, and then in a later handler (postinstall) to switch the
> names.
>
> I have recently (well, not so recently, but it is not yet in an official
> release, I have to push a 2024.05) "post-failure" scripts. The main goal
> was to restart containers that were previously stopped, but it can fit
> in your use case. You could use to drop what the archive has installed
> before the failure.
>
> If the shared partition is just mounted but not updated (or no files are
> updated), I do not see issues, because sw-description is verified before
> doing anything.
>
> >
> > Encryption somewhat mitigates the issue because it's harder to mess with
> > encrypted data, but while we suggest encryption for our users'
> > application update, we also distribute "official OS" updates for all
> > our users and these are not encrypted.
>
> It could mitigate, but the update should be already clean without
> encryption.
>
> >
> >
> > With the background out of the way, that leaves a few possibilities:
> > - Stop mounting such a partition; I'm not considering this further for
> > two reasons: 1/ it's breaking compatibility, which I'd rather avoid if
> > possible, 2/ even if we don't mount the partition directly, swupdate can
> > also write to different btrfs subvolumes (we're using snapshots for
> > cheap A/B copy), so an attacker could still cause trouble by filling the
> > fs at the very least and I'd like to protect from that as well.
> >
> > As a poor workaround I could ensure the shared partition isn't mounted
> > for our official updates, but I'd like to think we can do better.
> >
> > - Make the archive handler buffer the whole archive so we can verify
> > the checksum before starting extraction, this is brutish and might not
> > work for large archives; we're also not setting the archive compressed
> > size (checksum target; I'm not sure it's possible in a generic way?), so
> > this does not address filling $TMPDIR.
> >
>
> This drops the streaming feature. In fact, the issue you report is more
> about what the integrator should do else an issue in side the code, as
> you have reported, too.
>
> I am not sure if your thoughts are due because you are updating the
> shared partition, and then yes, data are not valid, or this partition is
> not updated but it is still present, and you think to unmount it. But as
> no rule is in sw-description, I do not see any issue with it.
>
> > In our particular case we already accept large-ish images in TMPDIR
> > before extracting them (podman doesn't support non-seekable container
> > archive input so we store a copy there),
>
> I know this, someone has patched podman to skip the hash check and let
> SWUpdate doing this (but I haven't these patches for podman).
>
> > so I'd consider this acceptable
> > for us for previously generated archives if there is no better solution,
> > but I wouldn't suggest this for swupdate upstream.
> >
> > - Modify the checksum in sw-description to not be a single value, but
> > e.g. a list of checksums for each 1MB chunks; __swupdate_copy could then
> > temporarily buffer these chunks in TMPDIR then feed the data to handlers
> > after the chunk is validated
> > (chunk size to be discussed; the current BUFF_SIZE is 16KB which does
> > not seem practical so I suggested 1MB but I'm open to suggestions.
> > I know swupdate doesn't/shouldn't use dynamically allocated memory for
> > updates and reserving a full MB isn't realistic, so for example using a
> > mmap'ed buffer in TMPDIR would allow the kernel to flush to disk if
> > there is memory pressure, yet never touch disk if there is none; but
> > we can just really write it down 16KB at a time and read it back if you
> > prefer, I don't want to get lost in details yet)
> >
>
> This is quite what the delta handler is doing, but with the "delta"
> packed into the SWU.
>
> A file for delta is in zchunk format, that means split into chunks based
> on rolling-hash algorithms. The result of the delta handler is then
> streamed again to another handler, and it could be the archive handler.
>
> This is not implemented and it sounds a little weird as "delta", but
> splitting into chunks (and in a format already recognized and supported
> like zck) is done. So the pipeline is:
>
> tarball in ZCK format ==> modified delta ==> archive handler
>
> The delta handler verifies each chunk before forwarding to the chained
> handler using a pipe.
>
> However, I will first suggest you if the post-failure scripts are not
> enough for you. They run just in case of failure, and you can also track
> what happened before if they are in Lua because the Lua context is
> extended to be per update instead of per script.
>
> > That's obviously the direction I prefer, but it requires allowing a new
> > way of writing checksums in the config so it's obviously something to
> > consider.
>
> I won't create an own method for this if there is already a standard
> way. I chose zchunk for sevral reason, and one of them is that is wide
> supported in distro like Fedora (but not only).
>
> > iirc swupdate ignores unknown fields
>
> Right - this is a "feature" that I often use, for example when some
> information should be sent to the application but ignored by SWUpdate.
>
> > so we could keep the full checksum
> > and the chunked checksums, old versions of swupdate would work as they
> > previously did and new versions would only use the chunked one if
> > present.
> >
> >
> > What do you think?
>
> Best regards,
> Stefano
>
>
>
>
> ---------- Forwarded message ----------
> From: Dominique MARTINET <dominique...@atmark-techno.com>
> To: Stefano Babic <stefan...@swupdate.org>
> Cc:
> Bcc:
> Date: Tue, 7 May 2024 18:30:09 +0900
> Subject: Re: [security RFC] install_directly and checksum validation - chunkify the checksum?
> Thanks for the quick reply
>
> Stefano Babic wrote on Tue, May 07, 2024 at 10:00:30AM +0200:
> > Sure - then a post to ML is useful and it will be tracked for the
> > future. You could also think about if it makes sense to write something
> > into documentation - I collect some times ago some suggestions into a
> > Best Practices chapter (or where you think it is better).
>
> Yes, that'll be even better.
> I'll forward our discussion and suggest an update to
> doc/source/swupdate-best-practise.rst once we're done.
>
> > > For most handlers the worst that can happen is useless writes to disk
> > > (e.g. filling up an inactive partition that cannot be used), but for
> > > example the archive handler could easily be abused if there is a
> > > "shared" partition (we unfortuantely have such a "state" partition, so
> > > e.g. logs or databases can be updated during a new install and rebooting
> > > into the update will not lose anything -- it's mounted during the update
> > > if the user need to interact with it directly e.g. cleanup obsolete data)
> >
> > You have then a mixed between double-copy and single-copy. All data on
> > your shared partition are updated in a single copy concept, and should
> > be considered invalid until SWUpdate will be successful.
> >
> > I generally avoid to mix the concepts - if I go for a double-copy, I try
> > to have double-copy for all artifacts. In this case, if space is enough,
> > I had tried to duplicate the directory where the archive handler
> > extracts data, and then in a later handler (postinstall) to switch the
> > names.
>
> Yes, in hindsight I definitely agree with this; I'm just stuck here
> because of backwards compatibility as I'm not the final integrator;
> but we have many very small users which just use small variations of the
> examples I provide so it's hard to change anything.
>
> I'm not giving any example that write in the single-copy partition
> (because even without this security consideration, it's always dangerous
> to write somewhere the live system can access), but it's a directory that
> we've documented as writeable so it'd be a large user-facing change.
>
>
> > I have recently (well, not so recently, but it is not yet in an official
> > release, I have to push a 2024.05) "post-failure" scripts. The main goal
> > was to restart containers that were previously stopped, but it can fit
> > in your use case. You could use to drop what the archive has installed
> > before the failure.
> >
> > If the shared partition is just mounted but not updated (or no files are
> > updated), I do not see issues, because sw-description is verified before
> > doing anything.
>
> sw-description is verified, but the way that "single-copy" partition is
> mounted doesn't allow me to check if it is written or not easily
> (e.g. my updates will have path = "/target", which is a double-copy
> partition, but the single-copy partition is available under
> /target/var/app/volumes so if the archive is modified to include files
> like /var/app/volumes/foo it will be written to the single-copy
> partition.
>
> A post-failure script also won't be able to know which files have been
> written by the update and what has been modified by the live system, so
> it'll be hard to undo - I have the same problem if I want to protect the
> live system and take a snapshot of this subvolume, I won't know which
> files to keep from which side at the end of the update, as the whole
> point is to keep logs/database entries from the running system while the
> update is running.
> It's also not possible to undo a file being written on both sides.
>
> At this point the right thing to do would be to just not mount this
> partition beneath the update path, but we're back to breaking
> compabitility; I'd rather avoid that if possible.
>
> > > - Make the archive handler buffer the whole archive so we can verify
> > > the checksum before starting extraction, this is brutish and might not
> > > work for large archives; we're also not setting the archive compressed
> > > size (checksum target; I'm not sure it's possible in a generic way?), so
> > > this does not address filling $TMPDIR.
> >
> > This drops the streaming feature. In fact, the issue you report is more
> > about what the integrator should do else an issue in side the code, as
> > you have reported, too.
>
> Yes, I definitely wouldn't suggest this for swupdate upstream.
> That would be a local workaround for otherwise vulnerable updates only.
>
> Once again I am bitten by users building their own updates; for our
> updates we could just rotate the signing key (that is planned at some
> point), but our users manage their own keys as well and many will not
> even have a second key ready on devices...
>
>
> > I am not sure if your thoughts are due because you are updating the
> > shared partition, and then yes, data are not valid, or this partition is
> > not updated but it is still present, and you think to unmount it. But as
> > no rule is in sw-description, I do not see any issue with it.
>
> This is quite murky, let me try to recap:
> - the normal OS updates we provide do not write to this partition,
> but the partition is a submount of the update target path so an attacker
> could use these updates to write there if they want
> - user updates are allowed to write to this partition, so while just
> unmounting it would address the problem for normal OS updates it will
> break that for user updates.
> user updates could of course use another path that is not beneath the
> normal double-copy target, but I'd rather avoid this if possible as
> users explicitly writing there would also be vulnerable
> - even if I move this mount point, other part of the update (TMPDIR,
> another submount that is double-copy) write to subvolumes of the same
> partition, so filling the partition is also easy and I'd like to try to
> protect from that as well.
>
>
> > > In our particular case we already accept large-ish images in TMPDIR
> > > before extracting them (podman doesn't support non-seekable container
> > > archive input so we store a copy there),
> >
> > I know this, someone has patched podman to skip the hash check and let
> > SWUpdate doing this (but I haven't these patches for podman).
>
> This is off-topic so I'll open another thread on the list directly for
> this patch, I'll be interested as a later optimization.
>
> > > - Modify the checksum in sw-description to not be a single value, but
> > > e.g. a list of checksums for each 1MB chunks; __swupdate_copy could then
> > > temporarily buffer these chunks in TMPDIR then feed the data to handlers
> > > after the chunk is validated
> > > (chunk size to be discussed; the current BUFF_SIZE is 16KB which does
> > > not seem practical so I suggested 1MB but I'm open to suggestions.
> > > I know swupdate doesn't/shouldn't use dynamically allocated memory for
> > > updates and reserving a full MB isn't realistic, so for example using a
> > > mmap'ed buffer in TMPDIR would allow the kernel to flush to disk if
> > > there is memory pressure, yet never touch disk if there is none; but
> > > we can just really write it down 16KB at a time and read it back if you
> > > prefer, I don't want to get lost in details yet)
> > >
> >
> > This is quite what the delta handler is doing, but with the "delta"
> > packed into the SWU.
>
> Yes, I've seen the delta handler and tried it as you pushed it, but
> didn't keep it for two reasons:
> - it currently requires a web server (I didn't see a way to pack the
> delta into the SWU, please correct me if it's possible).
> This is normal for optimizing bandwidth (using RANGE requests to only
> fetch required chunks), but we also need offline updates.
> - if I start generating updates using a new handler, these will not be
> installable on old systems, and that will require quite some preparation
> (shipping systems where swupdate include the current handler, then
> perhaps a year later(!) finally require it).
> Our policy here is way too nice for my taste, but it has to be backwards
> compatible for a (long) while.
>
> > A file for delta is in zchunk format, that means split into chunks based
> > on rolling-hash algorithms. The result of the delta handler is then
> > streamed again to another handler, and it could be the archive handler.
> >
> > This is not implemented and it sounds a little weird as "delta", but
> > splitting into chunks (and in a format already recognized and supported
> > like zck) is done. So the pipeline is:
> >
> > tarball in ZCK format ==> modified delta ==> archive handler
> >
> > The delta handler verifies each chunk before forwarding to the chained
> > handler using a pipe.
>
> I'm not familiar with the ZCK format to reply immediately, but my first
> impression is that it sounds more complex than just chunking checksums,
> and I'm not sure the little code that will be shared this way is worth
> the cognitive burden to understand this pipeline.
>
>
> To give a more concrete example, I'm suggesting something like this:
> - file foo.tar is 30MB after compression
> - compute checksums for it, e.g. replace (pseudoshell)
> sha256 = "`sha256sum foo.tar.zst`"
> with something like (not sure what libconfig lists look like)
> sha256-chunks = [
> `split -b $((1024*1024)) --filter='sha256sum -' foo.tar.zst`
> ]
> which will look like
> sha256-chunks = [
> "eac4aded3727a63f72d8bd641a0f08541c0d3e17f82a274b9d4ddfa48836a880",
> "626e97f31e585646665fc9a6682243965a6c8bf9693fd1a585d7d3dd972615b7",
> ...
> ]
> - use these in copyfile
>
> The other advantage of this is that it makes the input file size
> explicit, so even with no streaming there is no more risk of having
> TMPDIR filled by invalid data (which is the second problem I'm
> referring to -- as long as the file does not end the checksum
> computation never fails and an attacker can write forever)
>
> > > That's obviously the direction I prefer, but it requires allowing a new
> > > way of writing checksums in the config so it's obviously something to
> > > consider.
> >
> > I won't create an own method for this if there is already a standard
> > way. I chose zchunk for sevral reason, and one of them is that is wide
> > supported in distro like Fedora (but not only).
>
> That's not so much a new method as making copyfile use either
> 'sha256-chunks' if available and fall back to 'sha256' (or none) if not,
> but yes, it will be a bit of code;
> the checksum compuation is all in place and can be reused so all that
> will change is moving the HASH_init/final to a subfunction so it can be
> shared from within the loop at regular interval, I don't expect this to
> add much code.
> (there is no advantage of computing both, so if both are present only
> the chunks variant will be checked)
>
> I've run out of time for today, but I can write a quick poc tomorrow if
> that can convince you the implementation is simple enough.
> (I'll re-check what you meant with zck first)
>
> Thanks,
> --
> Dominique
>
>
>
> ---------- Forwarded message ----------
> From: Stefano Babic <stefan...@swupdate.org>
> To: Dominique MARTINET <dominique...@atmark-techno.com>
> Cc:
> Bcc:
> Date: Tue, 7 May 2024 13:37:44 +0200
> Subject: Re: [security RFC] install_directly and checksum validation - chunkify the checksum?
> Hi Dominique,
>
> On 07.05.24 11:30, Dominique MARTINET wrote:
> > Thanks for the quick reply
> >
> > Stefano Babic wrote on Tue, May 07, 2024 at 10:00:30AM +0200:
> >> Sure - then a post to ML is useful and it will be tracked for the
> >> future. You could also think about if it makes sense to write something
> >> into documentation - I collect some times ago some suggestions into a
> >> Best Practices chapter (or where you think it is better).
> >
> > Yes, that'll be even better.
> > I'll forward our discussion and suggest an update to
> > doc/source/swupdate-best-practise.rst once we're done.
> >
>
> Fine.
>
> >>> For most handlers the worst that can happen is useless writes to disk
> >>> (e.g. filling up an inactive partition that cannot be used), but for
> >>> example the archive handler could easily be abused if there is a
> >>> "shared" partition (we unfortuantely have such a "state" partition, so
> >>> e.g. logs or databases can be updated during a new install and rebooting
> >>> into the update will not lose anything -- it's mounted during the update
> >>> if the user need to interact with it directly e.g. cleanup obsolete data)
> >>
> >> You have then a mixed between double-copy and single-copy. All data on
> >> your shared partition are updated in a single copy concept, and should
> >> be considered invalid until SWUpdate will be successful.
> >>
> >> I generally avoid to mix the concepts - if I go for a double-copy, I try
> >> to have double-copy for all artifacts. In this case, if space is enough,
> >> I had tried to duplicate the directory where the archive handler
> >> extracts data, and then in a later handler (postinstall) to switch the
> >> names.
> >
> > Yes, in hindsight I definitely agree with this; I'm just stuck here
> > because of backwards compatibility as I'm not the final integrator;
> > but we have many very small users which just use small variations of the
> > examples I provide so it's hard to change anything.
> >
>
> Ok, got it, but it is difficult to take over the responsibility when
> someone else was in charge and built mistakes.
>
> > I'm not giving any example that write in the single-copy partition
> > (because even without this security consideration, it's always dangerous
>
> +1
>
> > to write somewhere the live system can access), but it's a directory that
> > we've documented as writeable so it'd be a large user-facing change.
>
> Well, writable does not yet mean how it could be upgraded ;-)
>
> >
> >
> >> I have recently (well, not so recently, but it is not yet in an official
> >> release, I have to push a 2024.05) "post-failure" scripts. The main goal
> >> was to restart containers that were previously stopped, but it can fit
> >> in your use case. You could use to drop what the archive has installed
> >> before the failure.
> >>
> >> If the shared partition is just mounted but not updated (or no files are
> >> updated), I do not see issues, because sw-description is verified before
> >> doing anything.
> >
> > sw-description is verified, but the way that "single-copy" partition is
> > mounted doesn't allow me to check if it is written or not easily
> > (e.g. my updates will have path = "/target", which is a double-copy
> > partition, but the single-copy partition is available under
> > /target/var/app/volumes so if the archive is modified to include files
> > like /var/app/volumes/foo it will be written to the single-copy
> > partition.
>
> ok - but at the end, the way in SWUpdate is to consider the update
> successful or not in a whole. A hash mismatch (due to an attacker) will
> lead to a failed update. So at least this information is available, and
> then some decisions can be taken.
>
> >
> > A post-failure script also won't be able to know which files have been
> > written by the update and what has been modified by the live system, so
> > it'll be hard to undo
>
> Yes, but I guess we have always this. Let's say we have a tarball with
> multiple hashes as you proposed. The archive handler is running, some
> files were extracted, some of them not. SWUpdate uses also the
> libarchive in an atomic way, so everything was unpacked or nothing. You
> still have a mix up between extracted files with right hashes.
>
> And if these files can be updated, they can overwrite some files written
> by the live system, too. I mean: I do not know if we can provide a
> solution in SWUpdate if the integrator had not thought to what is
> happening. A tarball can always contain files that overwrite files
> written by live system, even if hash match - and then ?
>
> > - I have the same problem if I want to protect the
> > live system and take a snapshot of this subvolume, I won't know which
> > files to keep from which side at the end of the update, as the whole
> > point is to keep logs/database entries from the running system while the
> > update is running.
> > It's also not possible to undo a file being written on both sides.
> >
> > At this point the right thing to do would be to just not mount this
> > partition beneath the update path, but we're back to breaking
> > compabitility; I'd rather avoid that if possible.
>
> But let's say: what happens if SWUpdate raised an error on a file, that
> is important for the application ?
>
> Anyway, if you are already using btrfs snapshot, why cannot you add a
> snapshot before update is running and restore it with a post-failure
> script ?
>
> >
> >>> - Make the archive handler buffer the whole archive so we can verify
> >>> the checksum before starting extraction, this is brutish and might not
> >>> work for large archives; we're also not setting the archive compressed
> >>> size (checksum target; I'm not sure it's possible in a generic way?), so
> >>> this does not address filling $TMPDIR.
> >>
> >> This drops the streaming feature. In fact, the issue you report is more
> >> about what the integrator should do else an issue in side the code, as
> >> you have reported, too.
> >
> > Yes, I definitely wouldn't suggest this for swupdate upstream.
> > That would be a local workaround for otherwise vulnerable updates only.
> >
> > Once again I am bitten by users building their own updates; for our
> > updates we could just rotate the signing key (that is planned at some
> > point), but our users manage their own keys as well and many will not
> > even have a second key ready on devices...
>
> You are very kindly with your customers :-).
>
> I say my customers that I feel responsible for the integrations I do
> myself. If they are doing on their own, they should solve the problems
> on their own...
>
> It seems to me that we are trying to fix a wrong concept, due to missing
> experience by some users. But you know: we cannot crate something
> foolproof because folls are very creative and circumvents our measures ;-)
>
> >
> >
> >> I am not sure if your thoughts are due because you are updating the
> >> shared partition, and then yes, data are not valid, or this partition is
> >> not updated but it is still present, and you think to unmount it. But as
> >> no rule is in sw-description, I do not see any issue with it.
> >
> > This is quite murky, let me try to recap:
> > - the normal OS updates we provide do not write to this partition,
> > but the partition is a submount of the update target path so an attacker
> > could use these updates to write there if they want
>
> Ok - indeed, this is a failure in the update concept.
>
> > - user updates are allowed to write to this partition, so while just
> > unmounting it would address the problem for normal OS updates it will
> > break that for user updates.
>
> Got it.
>
> > user updates could of course use another path that is not beneath the
> > normal double-copy target,
>
> Exactly
>
> > but I'd rather avoid this if possible as
> > users explicitly writing there would also be vulnerable
> > - even if I move this mount point, other part of the update (TMPDIR,
> > another submount that is double-copy) write to subvolumes of the same
> > partition, so filling the partition is also easy and I'd like to try to
> > protect from that as well.
> >
> >
> >>> In our particular case we already accept large-ish images in TMPDIR
> >>> before extracting them (podman doesn't support non-seekable container
> >>> archive input so we store a copy there),
> >>
> >> I know this, someone has patched podman to skip the hash check and let
> >> SWUpdate doing this (but I haven't these patches for podman).
> >
> > This is off-topic so I'll open another thread on the list directly for
> > this patch, I'll be interested as a later optimization.
>
> Ok
>
> >
> >>> - Modify the checksum in sw-description to not be a single value, but
> >>> e.g. a list of checksums for each 1MB chunks; __swupdate_copy could then
> >>> temporarily buffer these chunks in TMPDIR then feed the data to handlers
> >>> after the chunk is validated
> >>> (chunk size to be discussed; the current BUFF_SIZE is 16KB which does
> >>> not seem practical so I suggested 1MB but I'm open to suggestions.
> >>> I know swupdate doesn't/shouldn't use dynamically allocated memory for
> >>> updates and reserving a full MB isn't realistic, so for example using a
> >>> mmap'ed buffer in TMPDIR would allow the kernel to flush to disk if
> >>> there is memory pressure, yet never touch disk if there is none; but
> >>> we can just really write it down 16KB at a time and read it back if you
> >>> prefer, I don't want to get lost in details yet)
> >>>
> >>
> >> This is quite what the delta handler is doing, but with the "delta"
> >> packed into the SWU.
> >
> > Yes, I've seen the delta handler and tried it as you pushed it, but
> > didn't keep it for two reasons:
> > - it currently requires a web server (I didn't see a way to pack the
> > delta into the SWU, please correct me if it's possible).
>
> In fact, what I have written is that the ZCK file should be part of the
> SWU instead of being retrieved from an external Webserver. This is of
> course not done, because then we haven't any "delta" anymore. So you can
> just see it as another format for the artifact (ZCK format).
>
> In the build, you should have your tarball for archive, say my.tar, pass
> it to create a ZCK file and deliver this file into the SWU. It is not a
> delta anymore, chunks are simply read by the SWU instead of be compared
> and with the live copy and downloaded from an external source.
>
>
> > This is normal for optimizing bandwidth (using RANGE requests to only
> > fetch required chunks), but we also need offline updates.
>
> See above.
>
> > - if I start generating updates using a new handler, these will not be
> > installable on old systems,
>
> Everything we will do, including adding multiple hashes, won't be
> compatible with the past. It is in the matter of things.
>
> The only way to fix and made backward compatible is to change the
> concept, that avoiding to extract a tarball into the path above. Using a
> new partition, uploading an image instead of tarball, etc..
>
> Proposals here will change SWUpdate code, and then customers need to
> update at least once to have the changes in place.
>
> > and that will require quite some preparation
> > (shipping systems where swupdate include the current handler, then
> > perhaps a year later(!) finally require it).
>
> Sure, that depends on your customers. But well, if they are the product
> owner, they should decide.
>
> > Our policy here is way too nice for my taste, but it has to be backwards
> > compatible for a (long) while.
>
> ok, I am not sure what can be done when the concept is wrong. At the
> end, we can do a lot of nasty things by writing a bad sw-description.
>
> >
> >> A file for delta is in zchunk format, that means split into chunks based
> >> on rolling-hash algorithms. The result of the delta handler is then
> >> streamed again to another handler, and it could be the archive handler.
> >>
> >> This is not implemented and it sounds a little weird as "delta", but
> >> splitting into chunks (and in a format already recognized and supported
> >> like zck) is done. So the pipeline is:
> >>
> >> tarball in ZCK format ==> modified delta ==> archive handler
> >>
> >> The delta handler verifies each chunk before forwarding to the chained
> >> handler using a pipe.
> >
> > I'm not familiar with the ZCK format to reply immediately, but my first
> > impression is that it sounds more complex than just chunking checksums,
>
> It is more complex, it could be a pain to implement, but well, someone
> else (me) has already done and it is working. It has a nice methods to
> split into chunks with mathematical algorithms, and each chunk is
> compressed via zst.
>
> > and I'm not sure the little code that will be shared this way is worth
> > the cognitive burden to understand this pipeline.
> >
> >
> > To give a more concrete example, I'm suggesting something like this:
> > - file foo.tar is 30MB after compression
> > - compute checksums for it, e.g. replace (pseudoshell)
> > sha256 = "`sha256sum foo.tar.zst`"
> > with something like (not sure what libconfig lists look like)
> > sha256-chunks = [
> > `split -b $((1024*1024)) --filter='sha256sum -' foo.tar.zst`
> > ]
>
> But let's say: your customer can already split the big tarball in a set
> of smaller tarballs, and each of them has its own hash. If they do this,
> they minimize the risks.
>
> Even with the changes above, an attacker can replace a chunk (then it
> depends on the size of the chunk..) and replace some important files. As
> usual, the bug itself due t othe wrong concept cannot be solved, we are
> trying to find some work-arounds, that can work or not.
>
> > which will look like
> > sha256-chunks = [
> > "eac4aded3727a63f72d8bd641a0f08541c0d3e17f82a274b9d4ddfa48836a880",
> > "626e97f31e585646665fc9a6682243965a6c8bf9693fd1a585d7d3dd972615b7",
> > ...
> > ]
> > - use these in copyfile
> >
> > The other advantage of this is that it makes the input file size
> > explicit, so even with no streaming there is no more risk of having
> > TMPDIR filled by invalid data (which is the second problem I'm
> > referring to -- as long as the file does not end the checksum
> > computation never fails and an attacker can write forever)
>
> But well, this is a separate issue. Some handlers have already a "size"
> attribute. It could be extended and checked with the CPIO header to
> avoid this.
>
> Issue is known, but it does not lead that manipulated software is
> installed - SWUpdate will stop with OOM and files are erased. Of course,
> this in case installed-directly is not set, else the device size will be
> effective.
>
> >
> >>> That's obviously the direction I prefer, but it requires allowing a new
> >>> way of writing checksums in the config so it's obviously something to
> >>> consider.
> >>
> >> I won't create an own method for this if there is already a standard
> >> way. I chose zchunk for sevral reason, and one of them is that is wide
> >> supported in distro like Fedora (but not only).
> >
> > That's not so much a new method as making copyfile use either
> > 'sha256-chunks' if available and fall back to 'sha256' (or none) if not,
> > but yes, it will be a bit of code;
> > the checksum compuation is all in place and can be reused so all that
> > will change is moving the HASH_init/final to a subfunction so it can be
> > shared from within the loop at regular interval, I don't expect this to
> > add much code.
> > (there is no advantage of computing both, so if both are present only
> > the chunks variant will be checked)
> >
> > I've run out of time for today, but I can write a quick poc tomorrow if
> > that can convince you the implementation is simple enough.
> > (I'll re-check what you meant with zck first)
> >
> > Thanks,
>
> Best regards,
> Stefano
>
>
>
>
> ---------- Forwarded message ----------
> From: Dominique MARTINET <dominique...@atmark-techno.com>
> To: Stefano Babic <stefan...@swupdate.org>
> Cc:
> Bcc:
> Date: Wed, 8 May 2024 11:02:07 +0900
> Subject: Re: [security RFC] install_directly and checksum validation - chunkify the checksum?
> Stefano Babic wrote on Tue, May 07, 2024 at 01:37:44PM +0200:
> > > A post-failure script also won't be able to know which files have been
> > > written by the update and what has been modified by the live system, so
> > > it'll be hard to undo
> >
> > Yes, but I guess we have always this. Let's say we have a tarball with
> > multiple hashes as you proposed. The archive handler is running, some
> > files were extracted, some of them not. SWUpdate uses also the
> > libarchive in an atomic way, so everything was unpacked or nothing. You
> > still have a mix up between extracted files with right hashes.
> >
> > And if these files can be updated, they can overwrite some files written
> > by the live system, too. I mean: I do not know if we can provide a
> > solution in SWUpdate if the integrator had not thought to what is
> > happening. A tarball can always contain files that overwrite files
> > written by live system, even if hash match - and then ?
>
> Yes, as said above I do not think a post failure script could handle
> this in a generic way.
>
> > > - I have the same problem if I want to protect the
> > > live system and take a snapshot of this subvolume, I won't know which
> > > files to keep from which side at the end of the update, as the whole
> > > point is to keep logs/database entries from the running system while the
> > > update is running.
> > > It's also not possible to undo a file being written on both sides.
> > >
> > > At this point the right thing to do would be to just not mount this
> > > partition beneath the update path, but we're back to breaking
> > > compabitility; I'd rather avoid that if possible.
> >
> > But let's say: what happens if SWUpdate raised an error on a file, that
> > is important for the application ?
>
> Yes, even if there is no ill intent, if a real error occurs after the
> single-copy partition has been updated then what has been written is
> done and cannot be taken back.
> I'll definitely also update our own user documentation to strongly
> discourage writing to this partition (assuming we keep the possibility)
>
> > Anyway, if you are already using btrfs snapshot, why cannot you add a
> > snapshot before update is running and restore it with a post-failure
> > script ?
>
> Because that would lose data written during the update.
> This partition is akin to /var for a normal system - database &
> logs; it shouldn't be rolled back.
> It's the same reason we don't snapshot before writing to it: in case of
> update success the partition would lose writes that happened during the
> update itself.
>
> It really should not have been mounted in the first place...
>
> > > Once again I am bitten by users building their own updates; for our
> > > updates we could just rotate the signing key (that is planned at some
> > > point), but our users manage their own keys as well and many will not
> > > even have a second key ready on devices...
> >
> > You are very kindly with your customers :-).
> >
> > I say my customers that I feel responsible for the integrations I do
> > myself. If they are doing on their own, they should solve the problems
> > on their own...
> >
> > It seems to me that we are trying to fix a wrong concept, due to missing
> > experience by some users. But you know: we cannot crate something
> > foolproof because folls are very creative and circumvents our measures ;-)
>
> Right, I think we can definitely say "these invalid use cases will stop
> working", but in our case the integration boundary is pretty blurry and
> while I do not necessary want to support 100% I'd like to at least try
> to not change how things work too much as it can get very confusing
>
> > > > This is quite what the delta handler is doing, but with the "delta"
> > > > packed into the SWU.
> > >
> > > Yes, I've seen the delta handler and tried it as you pushed it, but
> > > didn't keep it for two reasons:
> > > - it currently requires a web server (I didn't see a way to pack the
> > > delta into the SWU, please correct me if it's possible).
> >
> > In fact, what I have written is that the ZCK file should be part of the
> > SWU instead of being retrieved from an external Webserver. This is of
> > course not done, because then we haven't any "delta" anymore. So you can
> > just see it as another format for the artifact (ZCK format).
> >
> > In the build, you should have your tarball for archive, say my.tar, pass
> > it to create a ZCK file and deliver this file into the SWU. It is not a
> > delta anymore, chunks are simply read by the SWU instead of be compared
> > and with the live copy and downloaded from an external source.
> >
> > > This is normal for optimizing bandwidth (using RANGE requests to only
> > > fetch required chunks), but we also need offline updates.
> >
> > See above.
>
> Yes, I was talking about delta handler in general.
> For the archive case actually it would probably make sense to just use
> delta directly and have it extract files with zero pre-existing
> knowledge (that should behave as libarchive?), but we'd still need to
> implement this packing into SWU feature.
>
> > > - if I start generating updates using a new handler, these will not be
> > > installable on old systems,
> >
> > Everything we will do, including adding multiple hashes, won't be
> > compatible with the past. It is in the matter of things.
>
> My suggestion is compatible. It won't magically fix old swu to be
> non-vulnerable, but they'll still be installable:
> * old swu, old swupdate: current behaviour
> * old swu, new swupdate: it'll find the sha256 field and check full
> file hash as per current behaviour. Optionally as a local kludge I
> can disable the streaming, but for upstream we probably want to just
> keep current behaviour.
> * new swu with both sha256 and sha256-chunks, old swupdate:
> it'll find the sha256 field and work as per current, ignoring the chunks
> * (at some point integrator can say they no longer support older
> swupdate and stop putting in the sha256, in which case install will fail
> because no hash was found for these older swupdate versions)
> * new swu and new swupdate: it'll only use the chunks
>
> Whether that's good or not is up to discussion, but it's definitely
> possible.
>
> > The only way to fix and made backward compatible is to change the
> > concept, that avoiding to extract a tarball into the path above. Using a
> > new partition, uploading an image instead of tarball, etc..
> >
> > Proposals here will change SWUpdate code, and then customers need to
> > update at least once to have the changes in place.
>
> Right.
> We can mitigate the issue within the SWU itself (e.g. umount the
> partition before any archive extraction when it's not used), but old swu
> files will always have the problem.
> (thanksfully version check will make these not installable if user is up
> to date)
>
> > > and that will require quite some preparation
> > > (shipping systems where swupdate include the current handler, then
> > > perhaps a year later(!) finally require it).
> >
> > Sure, that depends on your customers. But well, if they are the product
> > owner, they should decide.
>
> Yes, that can definitely be made an option for user generated updates;
> we set the pace for OS updates though.
>
> > > > A file for delta is in zchunk format, that means split into chunks based
> > > > on rolling-hash algorithms. The result of the delta handler is then
> > > > streamed again to another handler, and it could be the archive handler.
> > > >
> > > > This is not implemented and it sounds a little weird as "delta", but
> > > > splitting into chunks (and in a format already recognized and supported
> > > > like zck) is done. So the pipeline is:
> > > >
> > > > tarball in ZCK format ==> modified delta ==> archive handler
> > > >
> > > > The delta handler verifies each chunk before forwarding to the chained
> > > > handler using a pipe.
> > >
> > > I'm not familiar with the ZCK format to reply immediately, but my first
> > > impression is that it sounds more complex than just chunking checksums,
> >
> > It is more complex, it could be a pain to implement, but well, someone
> > else (me) has already done and it is working. It has a nice methods to
> > split into chunks with mathematical algorithms, and each chunk is
> > compressed via zst.
>
> So the only two things that'd require implementing are:
> - allowing packing ZCK files into SWU itself
> - allowing chaining another handler piped from the delta handler
>
> as both of these do not look possible to me right now?
>
> > > To give a more concrete example, I'm suggesting something like this:
> > > - file foo.tar is 30MB after compression
> > > - compute checksums for it, e.g. replace (pseudoshell)
> > > sha256 = "`sha256sum foo.tar.zst`"
> > > with something like (not sure what libconfig lists look like)
> > > sha256-chunks = [
> > > `split -b $((1024*1024)) --filter='sha256sum -' foo.tar.zst`
> > > ]
> >
> > But let's say: your customer can already split the big tarball in a set
> > of smaller tarballs, and each of them has its own hash. If they do this,
> > they minimize the risks.
>
> There is no risk management there: the archive size is not in
> sw-description, so the archive in the cpio can be as big as an attacker
> wants, and they can overwrite as much data as they want as the hash will
> only fail when the file has been fully streamed.
>
> > Even with the changes above, an attacker can replace a chunk (then it
> > depends on the size of the chunk..) and replace some important files. As
> > usual, the bug itself due t othe wrong concept cannot be solved, we are
> > trying to find some work-arounds, that can work or not.
>
> No, because I am suggesting that for the chunks variant the chunks get
> checked before passing the data on to the handler (if possible even
> before decompression, as there are occasional vulnerabilities on
> decompressors as well[1][2])
>
> [1] https://osv.dev/vulnerability/OSV-2020-691
> [2] https://osv.dev/vulnerability/OSV-2020-405
>
> This really fixes the problem by saying no untrusted data is ever
> processed by swupdate:
> - having a larger file in the cpio archive will cause errors because
> there are not enough chunks available
> - any invalid data will be caught before causing harm
>
> What is still possible in the single-copy case is to overwrite partial
> data, e.g. archive contains files A and B in different chunks, if you
> break the archive at B you can get swupdate to update only A before
> erroring out.
> I am sure in some specific scenario this can also cause problems, but at
> least it is no longer possible to write a file in a single-copy
> partition if there was not such a write in the original update, so this
> can be left to final integrators to just not use the single copy
> partition and it can be left present for people who really want to use
> it.
>
>
> But that made me check the delta handler code, and it's checking the
> hash while feeding the pipe through copybuffer (in non-local copy case),
> despite all the data being available in a local buffer.
> I'm not sure if it's redundant with a check zck already did before (the
> digest comes from it in the first place..), but if not this isn't a good
> solution for the reason you described: even if it's a single chunk, an
> attacker could overwrite files with it.
> OTOH the data is fully present in buffer, so that's an easy change (just
> check before calling copybuffer) and won't increase overhead.
>
> > > The other advantage of this is that it makes the input file size
> > > explicit, so even with no streaming there is no more risk of having
> > > TMPDIR filled by invalid data (which is the second problem I'm
> > > referring to -- as long as the file does not end the checksum
> > > computation never fails and an attacker can write forever)
> >
> > But well, this is a separate issue. Some handlers have already a "size"
> > attribute. It could be extended and checked with the CPIO header to
> > avoid this.
> >
> > Issue is known, but it does not lead that manipulated software is
> > installed - SWUpdate will stop with OOM and files are erased. Of course,
> > this in case installed-directly is not set, else the device size will be
> > effective.
>
> Yes, it's somewhat separate, but I believe both can be addressed with
> the same solution. "Two birds with one stone".
>
>
> So we have three paths forward:
> * keep swupdate as is, and just make it clear single-copy updates are
> dangerous in docs (will be done anyway)
> * update delta handler to accept zck files embedded in swu & stream to
> another handler, eventually use this for new swu
> ** merit: re-uses the delta handler code for chunking, chunks themselves
> are in a well supported format with tools to generate them
> * add chunked checksums and use these if present
> ** merit: possible to generate SWUs compatible with both format, generic
> for all handlers, check hash before piping to next handler, also address
> the "cpio size" problem even if install directly is off.
>
> I'm still thinking the chunked checksums is more straightforward (and
> thus easier to understand for new integrators), especially since tools
> like swugenerator could "just do it" without having to think about how
> handlers should be chained or other subtle install-directly
> implications, so I had a quick look at the code this morning, and the
> hard part would be config parsing...
> Which makes me want to reconsider zck a bit (feel free to skip details
> below)
>
> ----chunked hash implementation details-------------
> * __swupdate_copy easy side first: If single hash keep current
> behaviour, if hash list add an extra leading step that'll buffer a
> full chunk and check before passing on to the next step (re-chunking
> by decompressor so handler won't be called with too large buffers)
>
> * __swupdate_copy/copyfile/copybuffer have too many arguments, but
> ultimately we'll only check either the full file hash or the chunks, so
> we can probably re-use the hash field...
> I'm not looking forward to updating all callers, but it should be
> mechanical enough and having the hash type change will make it easy to
> catch any missed caller.
>
> * which leaves the "chunked hash" type / extraction from config.
> For common parser/parser.c (libconfig/json) ideally the chunked version
> could return a pointer to the chunks hash node so that the copy step
> could fetch directly from config one at a time, but we're
> freeing/destroying the config at the end of parse_cfg/parse_json so
> that's not possible.
>
> That means pre-loading all hashes in img_type, which isn't optimal for
> very large images... (even assuming large-ish 1MB chunks, having a 512MB
> image would consume 512*32 = 16KB ; that's probably acceptable for most
> systems nowadays but more importantly it's not possible to have a static
> img_type; and using a list would have even more overhead, so I'd still
> favor a malloc from get_array_length() * SHA256_HASH_LENGTH)
>
> (for parse_external if we go that way it doesn't look overly difficult
> to fetch values in a similar way, but likewise I don't see how to make
> it work with a callback to fetch hashes as they are needed...)
>
> I guess that's probably not be acceptable and we'd want something a bit
> better here; having hashes in yet another file doesn't sound great to me
> either (that's what zck is doing with the zck index, at this point might
> as well go full delta handler)...
> ---------------------------------------------------
>
> I've got other urgent work (ugh) so I'll give it a day or two to think
> it through further; I really liked the idea of having something generic
> for all handlers though so I'm not fully giving up but I'd rather not
> add to the pile of not-upstreamable patches I'm running...
>
>
> Thanks,
> --
> Dominique
>
>
>
> ---------- Forwarded message ----------
> From: Dominique MARTINET <dominique...@atmark-techno.com>
> To: Stefano Babic <stefan...@swupdate.org>
> Cc:
> Bcc:
> Date: Thu, 9 May 2024 16:35:04 +0900
> Subject: Re: [security RFC] install_directly and checksum validation - chunkify the checksum?
> Dominique MARTINET wrote on Wed, May 08, 2024 at 11:02:07AM +0900:
> > > Anyway, if you are already using btrfs snapshot, why cannot you add a
> > > snapshot before update is running and restore it with a post-failure
> > > script ?
> >
> > Because that would lose data written during the update.
> > This partition is akin to /var for a normal system - database &
> > logs; it shouldn't be rolled back.
> > It's the same reason we don't snapshot before writing to it: in case of
> > update success the partition would lose writes that happened during the
> > update itself.
> >
> > It really should not have been mounted in the first place...
>
> I think I can make something not-too-distruptive that'll only mount it
> if the user requests it, and print a warning that it probably shouldn't
> be accessed; so I'll fix up a workaround for our users first then let's
> discuss a cleaner follow up on the list (best practice update first and
> then something about chunking or at least adding size parameter at cpio
> level)
>
> Our update will go out at the end of the month, so hopefully I'll have
> some time to work on the workaround until then and I'll follow up early
> June.
>
> (Until then feel free to reply anyway, we can keep discussing if you
> have ideas, but I probably won't have much time to work on anything
> concrete)
>
> Cheers,
> --
> Dominique
>
>
>
> ---------- Forwarded message ----------
> From: Stefano Babic <stefan...@swupdate.org>
> To: Dominique MARTINET <dominique...@atmark-techno.com>
> Cc:
> Bcc:
> Date: Fri, 10 May 2024 07:53:07 +0200
> Subject: Re: [security RFC] install_directly and checksum validation - chunkify the checksum?
> Hi Dominique,
>
> On 09.05.24 09:35, Dominique MARTINET wrote:
> > Dominique MARTINET wrote on Wed, May 08, 2024 at 11:02:07AM +0900:
> >>> Anyway, if you are already using btrfs snapshot, why cannot you add a
> >>> snapshot before update is running and restore it with a post-failure
> >>> script ?
> >>
> >> Because that would lose data written during the update.
> >> This partition is akin to /var for a normal system - database &
> >> logs; it shouldn't be rolled back.
> >> It's the same reason we don't snapshot before writing to it: in case of
> >> update success the partition would lose writes that happened during the
> >> update itself.
> >>
> >> It really should not have been mounted in the first place...
> >
> > I think I can make something not-too-distruptive that'll only mount it
> > if the user requests it, and print a warning that it probably shouldn't
> > be accessed; so I'll fix up a workaround for our users first then let's
> > discuss a cleaner follow up on the list (best practice update first and
> > then something about chunking or at least adding size parameter at cpio
> > level)
>
> Fine.
>
> >
> > Our update will go out at the end of the month, so hopefully I'll have
> > some time to work on the workaround until then and I'll follow up early
> > June.
> >
> > (Until then feel free to reply anyway, we can keep discussing if you
> > have ideas,
>
> Sure, I find this discussion very constructive - then we will switch to
> ML if we want to go on with the thread.
>
> Regards,
> Stefano
>
> but I probably won't have much time to work on anything
> > concrete)
> >
> > Cheers,

[Storm, Christian]

Hi,

first of all: Thanks for thinking about this! :)

> Recap of how SWUs validation works
> ==================================
>
> SWU integrity is checked as follow:
> - we have a sw-description file with checksums
> - that sw-description file is signed, swupdate checks that early
> - for files without installed_directly, they're saved to TMPDIR while
> verifying the checksum matches (which was signed, so the file is
> correct) and then it's processed by its handler
> - for files with installed_directly set, we're streaming them to the
> handler which processes the files directly; most will be using
> copyimage()/copyfile() or a variant which streams to a fd while
> verifying the checksum -- sending a bad file will eventually fail when
> the checksum is finalized and it doesn't match, but until then all sort
> of things can happen.

Well, just things that the handler allows to happen. If it writes this data to /dev/null, you just wasted electricity. If it executes this data by a root shell, hell may break loose. And there are intermediate scenarios when data is parsed / interpreted ...

> That means that if I can get my hand on a valid SWU, replace a file that
> has installed_directly with a proper payload and somehow trigger an
> install I can make the handler process arbitrary data before the update
> fails.

Yes. Assuming the handlers do not execute / interpret data but just store it, on the update failing, this data has to be discarded. In double-copy scenarios, this is easy. In single-copy scenarios, one has to come up with a double-copy work-alike. In case of data interpretation, you're at the mercy of the library you're probably using for handling the data.

> (How likely that is depends on how you distribute your updates
> and who your customers are, unfortunately for us it is a quite realistic
> threat with the archive handler)
>
>
> Attack examples
> ===============
>
> There's probably more but here are a few examples of things that can go
> wrong with this:
>
> 1/ We're feeding unvalidated data to libarchive

Well, regardless of libarchive and true for any library you pass that data to, the question is about input validation / sanitation and this is a very hard problem to solve.

> (exploits)
> I was talking about a libarchive update earlier, because CVE-2024-26256
> came out earlier and was only fixed this month for our distro.
> The details don't overlay matter but basically it's supposedly a remote
> code execution when processing a bad .rar file, and libarchive
> automatically guesses the archive format so I was able to replace a
> tar.zst (decompressed by swupdate) file with a rar.zst file with no
> issue until the checksum failed, meaning that someone with the proper
> payload could make swupdate run anything on this affected systems.
>
> This CVE has been fixed, but there will be others - for the archive
> handler, anything impacting libarchive[1] or any of its dependencies (on my
> system, libcrypto/libacl, expat (what is a xar file?...),

xar is used by Apple/macOS, I have seen that there at least...

> lzma, zstd,
> lz4, bz2, libz) ; for other handlers whatever it is they use, and
> if using compression at swupdate level the zstd compression is also run
> in parallel to checksumming so any bug in ztsd[2] also impacts updates
> without installed_directly.
>
> [1] https://osv.dev/list?ecosystem=&q=libarchive
> [2] https://osv.dev/list?ecosystem=&q=zstd
>
>
> 2/ We're feeding unvalidated data to libarchive (single copy directories)
> Likewise, if the update isn't 100% double-copy and the archive handler
> destination path contains any single-copy directory, you can make an
> archive that will write anything to that partition and it's there to
> stay.

Correct.

> It's not recommended to unpack anything to a single-copy directory
> anyway because a failing update can leave it in a bad state, but the
> presence of such a directory should not need to be a security
> consideration, or if it is we need to much more strongly advertise this
> problem.

Yes, this is an inherent problem with the single-copy approach. Usually, one would want to make single-copy work-alike double-copy by using, e.g., block device/filesystem snapshots, A/B location with symlink-switching/chroot, versioning that can be rolled-back, ... That said, raising awareness of this problem in terms of documentation surely cannot hurt.

> These first two problems can be mitigated by enabling encryption, but
> AES CBC is still vulnerable to some specific modifications so while it
> makes such attacks extremely unlikely it might not be enough on its own.

Let alone key material handling / revoking etc. pp. which creates a substitute problem that then needs to be properly solved, generically.

> 3/ The checksum doesn't fail until the file is over.

Right, in particular in the streaming case, you cannot have that information prior to having seen the whole file.

> File sizes aren't written in sw-description, so it's possible to make an
> absurdly large SWU (thanksfully? limited to 4GB per file as we only
> support newc/crc) and make the handler work quite a bit before the
> update fails.

Yes, but even in smaller chunks you need to make sure nothing touches the disk until that chunk has been verified. Doing that for small (whatever specific size that is) chunks has less impact than waiting for the whole 4GB file to pass by, granted.

But even then, how do you roll-back in the single-copy case if chunks 1..n have passed and n+1 fails verification? Chunking "just" gives you the (nice) property of fail-earl{y,ier}.

> Without install_directly, if TMPDIR isn't on its own
> partition it's possible to fill it and impact other users of the
> partition.

Then, you got DoS'ed.

Hm, if we do pipe through zchunk unconditionally, we may also question using the CPIO format as transport and see if there's something that ticks all the needed feature boxes? This would be a big & breaking change though...

> - I had suggested implementing some chunked checksuming ourselves by
> replacing our sha256 property by a sha256_chunks array property;
> mostly because this can be made retrocompatible (the new SWU's
> sw-description just need to contain both the sha256 field and the new
> chunks array; older swupdate versions will use the checksum as before
> and newer versions will use the array).
> This would allow us to store each chunk (in TMPDIR-file backed memory?)
> while computing *only* the checksum, then we can go on to decompress and
> feed to the handler. It'll also solve the size problem, as we'll know
> the file was too big if there is no chunk left to validate.

That always requires temporary storage and hence defeats the idea of streaming updates, doesn't it? Or did I get this wrong? Then again, this doesn't solve the single-copy n+1 chunk failing, does it?

> Implementation wise I had looked at it a bit and was quite annoyed that
> we're "closing" the config before starting the installs, so we can't
> fetch the checksum chunks from config while extracting the files
> currently and would need to allocate a dyanmic array upfront, which
> might lock some memory for very large SWUs (I counted 32KB/GB of update
> for 1MB chunks, so for my use case it's actually acceptable, but it's
> not negligible).
> But thinking about this again now I don't see why we couldn't change the
> config parsing to leave the handle open and close it when cleaning up
> the update, so I'm tempted to try that.
>
>
>
> If someone on the list has opinions, another idea, or anything to share
> please do!
>
> I'm still overworked, but writing this mail rekindled my motivation
> here, unless there's a reason not to I'll finish my poc for chunked
> transfer next week; I think it shouldn't be a too large patch and
> having a concrete implementation might give better grounds for
> discussions.
> (And if the zchunk approach seems better we can still throw it away, but
> right now even ignoring the backwards compatibility I don't quite see
> how to make the files in the cpio archive match up cleanly)
>
>
> Thanks,
> --
> Dominique

Thanks!

Kind regards,
Christian

--
Dr. Christian Storm
Siemens AG, Technology, T CED OES-DE
Otto-Hahn-Ring 6, 81739 Munich, Germany

[Stefano Babic]

Hi,

On 03.06.24 16:36, 'Storm, Christian' via swupdate wrote:
> Hi,
>
> first of all: Thanks for thinking about this! :)
>
>
>> Recap of how SWUs validation works
>> ==================================
>>
>> SWU integrity is checked as follow:
>> - we have a sw-description file with checksums
>> - that sw-description file is signed, swupdate checks that early
>> - for files without installed_directly, they're saved to TMPDIR while
>> verifying the checksum matches (which was signed, so the file is
>> correct) and then it's processed by its handler
>> - for files with installed_directly set, we're streaming them to the
>> handler which processes the files directly; most will be using
>> copyimage()/copyfile() or a variant which streams to a fd while
>> verifying the checksum -- sending a bad file will eventually fail when
>> the checksum is finalized and it doesn't match, but until then all sort
>> of things can happen.
>
> Well, just things that the handler allows to happen. If it writes this data to /dev/null, you just wasted electricity. If it executes this data by a root shell, hell may break loose. And there are intermediate scenarios when data is parsed / interpreted ...
>
>
>> That means that if I can get my hand on a valid SWU, replace a file that
>> has installed_directly with a proper payload and somehow trigger an
>> install I can make the handler process arbitrary data before the update
>> fails.
>
> Yes. Assuming the handlers do not execute / interpret data but just store it, on the update failing, this data has to be discarded. In double-copy scenarios, this is easy. In single-copy scenarios, one has to come up with a double-copy work-alike. In case of data interpretation, you're at the mercy of the library you're probably using for handling the data.
>

Sure - but SWUpdate state machine splits between installation and
activation. In most use cases running in single-copy, for example
running SWUpdate from a rescue, the new software is invalid until update
is successful, and this avoids to use (activate) a manipulated artifact.

Dominique's case is really a mixed, mainly from his users, and some
artifacts are updated in single copy - so the concept has flaws here.
The manipulation happens because the artifact is automatically validated
when it is installed.

>
>> (How likely that is depends on how you distribute your updates
>> and who your customers are, unfortunately for us it is a quite realistic
>> threat with the archive handler)
>>
>>
>> Attack examples
>> ===============
>>
>> There's probably more but here are a few examples of things that can go
>> wrong with this:
>>
>> 1/ We're feeding unvalidated data to libarchive
>
> Well, regardless of libarchive and true for any library you pass that data to, the question is about input validation / sanitation and this is a very hard problem to solve.

Exactly. It is questionable, too, which part can be considered
authenticated. If instead of a tarball for libarchive we have a
database, there is no way to consider some chunks of it as valid.

+1

>
>
>> It's not recommended to unpack anything to a single-copy directory
>> anyway because a failing update can leave it in a bad state, but the
>> presence of such a directory should not need to be a security
>> consideration, or if it is we need to much more strongly advertise this
>> problem.
>
> Yes, this is an inherent problem with the single-copy approach. Usually, one would want to make single-copy work-alike double-copy by using, e.g., block device/filesystem snapshots, A/B location with symlink-switching/chroot, versioning that can be rolled-back,

Right, with symlinking, or using filesystems features (like snapshot in
btrfs). In all cases, there is a rollback.

> ... That said, raising awareness of this problem in terms of documentation surely cannot hurt.

Patches welcome :-)

>
>
>> These first two problems can be mitigated by enabling encryption, but
>> AES CBC is still vulnerable to some specific modifications so while it
>> makes such attacks extremely unlikely it might not be enough on its own.
>
> Let alone key material handling / revoking etc. pp. which creates a substitute problem that then needs to be properly solved, generically.

but still a DoS cannot be avoided. An attacker can replace some part of
an artifact, and the first part can be decrypted successfully. This
leads again to the corruption of the original data.

>
>
>> 3/ The checksum doesn't fail until the file is over.
>
> Right, in particular in the streaming case, you cannot have that information prior to having seen the whole fil >
>

>> File sizes aren't written in sw-description, so it's possible to make an
>> absurdly large SWU (thanksfully? limited to 4GB per file as we only
>> support newc/crc) and make the handler work quite a bit before the
>> update fails.
>
> Yes, but even in smaller chunks you need to make sure nothing touches the disk until that chunk has been verified. Doing that for small (whatever specific size that is) chunks has less impact than waiting for the whole 4GB file to pass by, granted.

Right

>
> But even then, how do you roll-back in the single-copy case if chunks 1..n have passed and n+1 fails verification? Chunking "just" gives you the (nice) property of fail-earl{y,ier}.

Yes, it just fails earlier. But what should be done when it fails ? The
handler reports the failure - theoretically, we need a rollback. In
single-copy this is realized outside SWUpdate (symlink, snapshot, etc.).

Of course it is possible, but it raises big compatibility issues. One
point for CPIO is it is an easy format, and anyone can check a SWU
simply double clicking on it, even on not-Linux systems.

>
>
>> - I had suggested implementing some chunked checksuming ourselves by
>> replacing our sha256 property by a sha256_chunks array property;
>> mostly because this can be made retrocompatible (the new SWU's
>> sw-description just need to contain both the sha256 field and the new
>> chunks array; older swupdate versions will use the checksum as before
>> and newer versions will use the array).
>> This would allow us to store each chunk (in TMPDIR-file backed memory?)
>> while computing *only* the checksum, then we can go on to decompress and
>> feed to the handler. It'll also solve the size problem, as we'll know
>> the file was too big if there is no chunk left to validate.
>
> That always requires temporary storage and hence defeats the idea of streaming updates, doesn't it? Or did I get this wrong? Then again, this doesn't solve the single-copy n+1 chunk failing, does it?

That is the major point - what should be done when n chunks are ok, n+1
has a wrong checksum ? And in a generic way, that means for all handlers
? What about if the handler is the remote handler, and the artifact (n
chunks) were transferred to another entity ?

[Dominique MARTINET]

(Replying to all at once)

James Hilliard wrote on Mon, Jun 03, 2024 at 03:52:25AM -0600:
> > If someone on the list has opinions, another idea, or anything to share
> > please do!
>
> There's some additional ideas in this thread that may be relevant such
> as using a merkle tree data structure and only storing the root hash in
> the sw-description.
>
> https://groups.google.com/g/swupdate/c/DSbJo83oc3c/m/dTuwuXqIAAAJ

Thanks for pointing me to this thread, I had missed it!

Merkle tree is definitely a possibility, at least that'd solve the
problem of storing the full list in sw-description and keeping it in
memory all the time as we would only store the root hash in
sw-description and the rest can be sent just before each file...

I think swupdate errors out if there are unknown files in the cpio
archive though so would need to think of some way to describe the tree
"files" in a way that'd be ignored by old versions, but I'm not sure
anyone else but me cares about that so that might not need to be a
primary goal.


Another thing is that just having chunked checksums is easy to compute,
the example I gave yesterday just added this to my sw-description
generation script:
echo 'chunked_hashes = ('
split -b $((1024*512)) --filter='sha256sum -' "$file_out" | sed -e 's/^/"/' -e 's/\s*-$/",/'
echo ');'

Simplicity of generation of the sw-description/hash data certainly isn't
a hard requirement, but the easier it is to generate the more likely
people will update their code to use the new format.

> I had also come across this library which might give some good ideas
> on how to implement the data structures efficiently for chunk validation.
>
> https://github.com/IAIK/secure-block-device

It seems to be aimed at "Data-at-Rest" more than streaming, but it does
implement merkle trees -- I'll have a closer look this week.

Storm, Christian wrote on Mon, Jun 03, 2024 at 02:36:24PM +0000:
> first of all: Thanks for thinking about this! :)

Thanks for the thorough reply!

> > 1/ We're feeding unvalidated data to libarchive
>

> Well, regardless of libarchive and true for any library you pass that
> data to, the question is about input validation / sanitation and this
> is a very hard problem to solve.

Yes -- my thinking is that we cannot validate all our dependencies;
there will always be new such CVEs poping up, so not handing a handler
any unvalidated data is probably a better design than what we currently
have.

(Stefano wrote)

> Exactly. It is questionable, too, which part can be considered
> authenticated. If instead of a tarball for libarchive we have a
> database, there is no way to consider some chunks of it as valid.

Right, I think the handlers of such files will have to cache it all to
disk to use it anyway (some databases like parquet can handle fetching
ranges, but I'm not sure how much use streaming would be anyway...);
in these cases I don't expect any difference except that the full size
is known.

> > This CVE has been fixed, but there will be others - for the archive
> > handler, anything impacting libarchive[1] or any of its dependencies (on my
> > system, libcrypto/libacl, expat (what is a xar file?...),
>

> xar is used by Apple/macOS, I have seen that there at least...

Thanks! My point was more along the lines of "that certainly isn't a
format I'll ever use, but it being supported increases the attack
surface if unvalidated data is piped in"

> > It's not recommended to unpack anything to a single-copy directory
> > anyway because a failing update can leave it in a bad state, but the
> > presence of such a directory should not need to be a security
> > consideration, or if it is we need to much more strongly advertise this
> > problem.
>

> Yes, this is an inherent problem with the single-copy
> approach. Usually, one would want to make single-copy work-alike
> double-copy by using, e.g., block device/filesystem snapshots, A/B
> location with symlink-switching/chroot, versioning that can be
> rolled-back, ... That said, raising awareness of this problem in terms
> of documentation surely cannot hurt.

Yes, I'll send a documentation update when we're done discussing.

> > These first two problems can be mitigated by enabling encryption, but
> > AES CBC is still vulnerable to some specific modifications so while it
> > makes such attacks extremely unlikely it might not be enough on its own.
>

> Let alone key material handling / revoking etc. pp. which creates a
> substitute problem that then needs to be properly solved,
> generically.

The asymmetric sw-description encryption that Michael sent to the list a
while ago[1] solves this pretty nicely, since the content aes key will be
written in the sw-description it can be changed for every update, and
the sw-description key itself can be rotated more easily (because a swu
can be signed for multiple target keys)

I still need to test it more seriously, but this came up first and I'd
need another head to deal with all the other small things that come in,
my current realistic schedule is to integrate this by next year... One
problem at a time.

[1] https://groups.google.com/g/swupdate/c/YgvtIDTC8b8/m/0kJd0t9jAQAJ

> > File sizes aren't written in sw-description, so it's possible to make an
> > absurdly large SWU (thanksfully? limited to 4GB per file as we only
> > support newc/crc) and make the handler work quite a bit before the
> > update fails.
>

> Yes, but even in smaller chunks you need to make sure nothing touches
> the disk until that chunk has been verified. Doing that for small
> (whatever specific size that is) chunks has less impact than waiting
> for the whole 4GB file to pass by, granted.
>
> But even then, how do you roll-back in the single-copy case if chunks
> 1..n have passed and n+1 fails verification? Chunking "just" gives you
> the (nice) property of fail-earl{y,ier}.

In the single-copy case there can be all sort of failures, even with a
valid SWU (e.g. disk full, transient network error); the update system
will need to deal with these anyway.
I don't think it's a new problem, and at least not one we need to tackle
here all at once (an improvement is still an improvement)

My situation is somewhat particular in that I'm not a final integrator,
but we're providing a set of primitives for updates using swupdate, and
out of the mostly double-copy update target there is just one directory
that is single copy and we recommend not to touch on updates.
But even if our user respect this, if an attacker can swap an archive,
they can write to that single-copy directory.

The idea here isn't to just chunk, it's that chunk gives us the ability
to swap the validation in before we give it to the handler, so the
attacker doesn't have a chance to alter the extraction directory.

... And if a library has an attack pattern where just truncating a file
could lead to weird behaviour, then I think that's a problem for that
library to handle better and/or users not to use that library; the worst
that should happen is failing the update.

> > Without install_directly, if TMPDIR isn't on its own
> > partition it's possible to fill it and impact other users of the
> > partition.
>

> Then, you got DoS'ed.

Depending on the thread model that could a valid security issue, if the
solution works without install_directly as well I don't think anybody
would complain :)
Anyway, the only thing that improves here in this case is just we'd
limit the write to the size that was written in the sw-description, but
it's still a step forward.

> > I don't like that it's not compatible (new SWUs won't be installable on
> > older versions of swupdate), but it's a solid approach;
> > it'll require adding a way of embedding the chunks in the swu itself and
> > I'm not sure how the file in cpio <-> handler mapping in sw-description
> > will work but I'm sure we can figure that out.
> > It might need to also add size fields for not-installed directly, and if
> > we go that way I'll want to fuzz zchunk a bit (not sure it's been
> > audited from a security perspective...), but that's definitely a
> > possible way forward.
>

> Hm, if we do pipe through zchunk unconditionally, we may also question
> using the CPIO format as transport and see if there's something that
> ticks all the needed feature boxes? This would be a big & breaking
> change though...

That's an interesting possibility!

Stefano replied that cpio being easily introspectable is a feature, but
if we add encryption to the mix I'm not sure how viable it is anyway.

In practice, we've been using SWU as our update transport for about 2.5
years and people around me rarely extract SWU files to inspect their
content.
Checking the sw-description signature manually or decrypting the archive
content are things I do from time to time but it all requires highly
specific commands, so I don't think it matters if we're not using cpio.

(and the 4GB file limit has actually come up a couple of times for large
container images our users could want to ship...)

My main worry is that we'd be introducing another layer of potential
vulnerabilities, cpio at least is dead simple and there isn't much room
for mistakes; if the new format handles chunks or other high level
features how much trust can we place into it? And can the chunks be
signed in a way that'd allow upfront verification? (we cannot use the
cpio crc for verification, not only because crc is weak, but because
we're packing into cpio after we're done generating the sw-description
and signed it)


I think it's still worth considering, even if we end up keeping the same
format; perhaps start with a checklist of what we'd like to see?

Is there something that checks all these boxes?
- multiple files that can be read one at a time from a stream (linear
http download), or single file on disk (installing from .swu file)
- chunked validation before extracting (this topic)
- individual files larger than 4GB
- seekable? (cpio has file size so we could skip a single file, but
cannot skip directly an arbitrary number of files)
- the format itself is simple enough to be auditable

> > - I had suggested implementing some chunked checksuming ourselves by
> > replacing our sha256 property by a sha256_chunks array property;
> > mostly because this can be made retrocompatible (the new SWU's
> > sw-description just need to contain both the sha256 field and the new
> > chunks array; older swupdate versions will use the checksum as before
> > and newer versions will use the array).
> > This would allow us to store each chunk (in TMPDIR-file backed memory?)
> > while computing *only* the checksum, then we can go on to decompress and
> > feed to the handler. It'll also solve the size problem, as we'll know
> > the file was too big if there is no chunk left to validate.
>

> That always requires temporary storage and hence defeats the idea of
> streaming updates, doesn't it? Or did I get this wrong?

It's a halfway measure: it requires enough storage or memory for one
chunk of data.
So if you have a 1GB update, without installed_directly you'd need 1GB
of temporary storage, but with the patches I sent yesterday (512KB
chunks) you'd need 512KB of disk and and extra 64KB of ram (32 bytes per
chunk); it'd also make the sw-description 140KB longer (around 70 bytes
per chunk in string representation).

Having a better chunking implementation would allow fetching the chunks
checksum just in time and remove the ram requirement and possibly the
sw-description requirement (if we only have a root hash in the
sw-description for example); but for my devices I'm thinking that even
this rough implementation is a trade-off that's worth not feeding
libarchive unvalidated data.

> Then again, this doesn't solve the single-copy n+1 chunk failing, does it?

Yes, I don't think we need to address this here; I don't see how that
would be possible without caching the whole file to disk or downloading
the data twice (and then how would you know that you've downloaded the
same data the second time around?)

(Stefano wrote)

> That is the major point - what should be done when n chunks are ok, n+1
> has a wrong checksum ? And in a generic way, that means for all handlers
> ? What about if the handler is the remote handler, and the artifact (n
> chunks) were transferred to another entity ?

I think just making copyimage() fail in the handler is fine:
- the handler decides how to cleanup its own data, including forwarding
the error to whatever remote mechanism if required,
- the handler itself fails, and swupdate / higher level integration
scripts decide how to clean up the failed update.

We've not fed the handler any unvalidated data, so it's exactly the same
as a network failure, swupdate needs to be able to handle this
regardless of the security considerations here.


Thank you three again for the replies :)
--
Dominique

[Stefano Babic]

On 04.06.24 03:11, Dominique MARTINET wrote:
> (Replying to all at once)
>
> James Hilliard wrote on Mon, Jun 03, 2024 at 03:52:25AM -0600:
>>> If someone on the list has opinions, another idea, or anything to share
>>> please do!
>>
>> There's some additional ideas in this thread that may be relevant such
>> as using a merkle tree data structure and only storing the root hash in
>> the sw-description.
>>
>> https://groups.google.com/g/swupdate/c/DSbJo83oc3c/m/dTuwuXqIAAAJ
>
> Thanks for pointing me to this thread, I had missed it!
>
> Merkle tree is definitely a possibility, at least that'd solve the
> problem of storing the full list in sw-description and keeping it in
> memory all the time as we would only store the root hash in
> sw-description and the rest can be sent just before each file...
>
> I think swupdate errors out if there are unknown files

Correct. It is often used as "feature" to deploy some additional file,
like a README for the release or something else. SWUpdate ignore them.

> in the cpio
> archive though so would need to think of some way to describe the tree
> "files" in a way that'd be ignored by old versions, but I'm not sure
> anyone else but me cares about that so that might not need to be a
> primary goal.
>
>
> Another thing is that just having chunked checksums is easy to compute,
> the example I gave yesterday just added this to my sw-description
> generation script:
> echo 'chunked_hashes = ('
> split -b $((1024*512)) --filter='sha256sum -' "$file_out" | sed -e 's/^/"/' -e 's/\s*-$/",/'
> echo ');'
>
> Simplicity of generation of the sw-description/hash data certainly isn't
> a hard requirement, but the easier it is to generate the more likely
> people will update their code to use the new format.

Generation of SWU is already abstracted and the integrator / user
shouldn't bother about it. The paths are:

OE ==> meta-swupdate
Debian, Buildroot ==> SWUgenerator

Other ways are unsupported.

A major complexity can be implemented in the above tools - so if
generation is becoming more complex is not a problem at all.

Just to avoid any effort: the nice series above has hit the issue that
support for crypto engines in SWUpdate is quite messy. I have already a
design to make it modular, and let the integrator to just pick up the
crypto feature (RSA, pkcs#7, etc.) independently from the underlying
crypto library. I have started to check this. Until this is not cleaned
up, I won't merge changes regarding crypto (verifying, decription, etc.)
because it just increments mess and #ifconfig.

>
> I still need to test it more seriously, but this came up first and I'd
> need another head to deal with all the other small things that come in,
> my current realistic schedule is to integrate this by next year... One
> problem at a time.
>
> [1] https://groups.google.com/g/swupdate/c/YgvtIDTC8b8/m/0kJd0t9jAQAJ
>
>>> File sizes aren't written in sw-description, so it's possible to make an
>>> absurdly large SWU (thanksfully? limited to 4GB per file as we only
>>> support newc/crc) and make the handler work quite a bit before the
>>> update fails.
>>
>> Yes, but even in smaller chunks you need to make sure nothing touches
>> the disk until that chunk has been verified. Doing that for small
>> (whatever specific size that is) chunks has less impact than waiting
>> for the whole 4GB file to pass by, granted.
>>
>> But even then, how do you roll-back in the single-copy case if chunks
>> 1..n have passed and n+1 fails verification? Chunking "just" gives you
>> the (nice) property of fail-earl{y,ier}.
>
> In the single-copy case there can be all sort of failures, even with a
> valid SWU (e.g. disk full, transient network error); the update system
> will need to deal with these anyway.
> I don't think it's a new problem, and at least not one we need to tackle
> here all at once (an improvement is still an improvement)

In fact, it is not a new problem. A single-copy schema has its
drawbacks. The reliable way used by SWUpdate is to consider the all
update unsuccessful.

>
> My situation is somewhat particular in that I'm not a final integrator,
> but we're providing a set of primitives for updates using swupdate, and
> out of the mostly double-copy update target there is just one directory
> that is single copy and we recommend not to touch on updates.
> But even if our user respect this, if an attacker can swap an archive,
> they can write to that single-copy directory.

Ok, but they can write because sw-description will allow this, that
means your users are not respecting your suggestions..... :-)

If sw-description has no entry for the file in the directory, the file
is simple skipped.

>
> The idea here isn't to just chunk, it's that chunk gives us the ability
> to swap the validation in before we give it to the handler, so the
> attacker doesn't have a chance to alter the extraction directory.

But the attacker can alter a file in the extraction directory, so it
seems to me it is just moving the problem. Of course, in the specif use
case (archive to be extracted), there are some advantages, but taking as
generic (chunk sent to any handler), I do not see a lot.

>
> ... And if a library has an attack pattern where just truncating a file
> could lead to weird behaviour, then I think that's a problem for that
> library to handle better and/or users not to use that library; the worst
> that should happen is failing the update.

Sure.

>
>>> Without install_directly, if TMPDIR isn't on its own
>>> partition it's possible to fill it and impact other users of the
>>> partition.
>>
>> Then, you got DoS'ed.
>
> Depending on the thread model that could a valid security issue, if the
> solution works without install_directly as well I don't think anybody
> would complain :)
> Anyway, the only thing that improves here in this case is just we'd
> limit the write to the size that was written in the sw-description, but
> it's still a step forward.

size can be added and core could check this with CPIO header to detect a
manipulation.

>
>>> I don't like that it's not compatible (new SWUs won't be installable on
>>> older versions of swupdate), but it's a solid approach;
>>> it'll require adding a way of embedding the chunks in the swu itself and
>>> I'm not sure how the file in cpio <-> handler mapping in sw-description
>>> will work but I'm sure we can figure that out.
>>> It might need to also add size fields for not-installed directly, and if
>>> we go that way I'll want to fuzz zchunk a bit (not sure it's been
>>> audited from a security perspective...), but that's definitely a
>>> possible way forward.
>>
>> Hm, if we do pipe through zchunk unconditionally, we may also question
>> using the CPIO format as transport and see if there's something that
>> ticks all the needed feature boxes? This would be a big & breaking
>> change though...
>
> That's an interesting possibility!
>
> Stefano replied that cpio being easily introspectable is a feature, but
> if we add encryption to the mix I'm not sure how viable it is anyway.

Of course, but everything here is configurable. A lot of users don't use
encryption, it is duty of the integrator to decide this.

>
> In practice, we've been using SWU as our update transport for about 2.5
> years and people around me rarely extract SWU files to inspect their
> content.
> Checking the sw-description signature manually or decrypting the archive
> content are things I do from time to time but it all requires highly
> specific commands, so I don't think it matters if we're not using cpio.
>

Because you're a developer....:-)

End users do not bother at it, and application developers as well. That
means OEM developers, too. They just require that their software is
bundled into the SWU.

> (and the 4GB file limit has actually come up a couple of times for large
> container images our users could want to ship...)

When this will become a requirement, it will be fixed.

>
> My main worry is that we'd be introducing another layer of potential
> vulnerabilities, cpio at least is dead simple

That was the reason for the choice. And it is not really a full CPIO
format: files are stored flat, and just few fields from the header are used.

> and there isn't much room
> for mistakes; if the new format handles chunks or other high level
> features how much trust can we place into it? And can the chunks be
> signed in a way that'd allow upfront verification?

Changing in the ZCK format should then discussed and verified with the
Zchunk's maintainer (Jonathan Dieter), as I did in the past when I
introduced the delta update.

> (we cannot use the
> cpio crc for verification, not only because crc is weak, but because
> we're packing into cpio after we're done generating the sw-description
> and signed it)

This is currently an advantage that packaging and signing are orthogonal.

>
>
> I think it's still worth considering, even if we end up keeping the same
> format; perhaps start with a checklist of what we'd like to see?

There is a doc/source/roadmap.rst. It will be nice that new ideas or
proposal will be stored there, even if they won't be implemented in
future. Probably changing the name from "roadmap", it isn't already a
roadmap. This allows to track ideas instead of digging into ML's archive.

>
> Is there something that checks all these boxes?
> - multiple files that can be read one at a time from a stream (linear
> http download), or single file on disk (installing from .swu file)

Not clear what you mind - yes, they are two use cases, both supported.
Selecting individual files from a stream, in a way using Byte Request
like delta update is doing, is something I proposed to customers some
times ago, and I called it "Selective downloading!". But interest was
n't so big and I got no sponsors for this topic. I removed this from
roadmap with commit a7ab76a, but you can still find it.

> - chunked validation before extracting (this topic)
> - individual files larger than 4GB
> - seekable? (cpio has file size so we could skip a single file, but
> cannot skip directly an arbitrary number of files)

This was done at the early beginning, and I had two different paths for
local and remote update. If you check in earlier code, you see that
SWUpdate used seek() to pass the right offset to the handler.

Anyway, the advantage (just skipping not required file in case of local
update) are small compared to the disadvantages. I had always to test
separately a local and remote update for any change.

> - the format itself is simple enough to be auditable
>
>>> - I had suggested implementing some chunked checksuming ourselves by
>>> replacing our sha256 property by a sha256_chunks array property;
>>> mostly because this can be made retrocompatible (the new SWU's
>>> sw-description just need to contain both the sha256 field and the new
>>> chunks array; older swupdate versions will use the checksum as before
>>> and newer versions will use the array).
>>> This would allow us to store each chunk (in TMPDIR-file backed memory?)
>>> while computing *only* the checksum, then we can go on to decompress and
>>> feed to the handler. It'll also solve the size problem, as we'll know
>>> the file was too big if there is no chunk left to validate.
>>
>> That always requires temporary storage and hence defeats the idea of
>> streaming updates, doesn't it? Or did I get this wrong?
>
> It's a halfway measure: it requires enough storage or memory for one
> chunk of data.
> So if you have a 1GB update, without installed_directly you'd need 1GB
> of temporary storage, but with the patches I sent yesterday (512KB
> chunks) you'd need 512KB of disk and and extra 64KB of ram (32 bytes per
> chunk); it'd also make the sw-description 140KB longer (around 70 bytes
> per chunk in string representation).

There shouln't be any restriction anymore for sw-description's size.
Anyway, the size of the chunk is a challenge. Too small, too many
hashes. Too big, no real advantage. The delta handler with ZCK looks for
chunks of variable length using a rolling hash function - most suitable,
but of course much more complicated.

Regarding the storage: a big advantage of the streaming technique
(installed-directly set) is that the buffer (currently small) is
allocated oin the heap. This increases security, because the data are
allocated dynamically on the heap, and it is more difficult for an
attacker to find it and change it. If the chunks are verified and saved
on TMPDIR, there is a time window and attacker could (theoretically)
change the data on the filesystem (easy to find) before the handler has
processed.

>
> Having a better chunking implementation would allow fetching the chunks
> checksum just in time and remove the ram requirement and possibly the
> sw-description requirement (if we only have a root hash in the
> sw-description for example); but for my devices I'm thinking that even
> this rough implementation is a trade-off that's worth not feeding
> libarchive unvalidated data.
>
>> Then again, this doesn't solve the single-copy n+1 chunk failing, does it?
>
> Yes, I don't think we need to address this here; I don't see how that
> would be possible without caching the whole file to disk or downloading
> the data twice (and then how would you know that you've downloaded the
> same data the second time around?)

Exactly, so I do not think it could be solved. The only way is to have
twice, in some way. Even updater working in single-copy have really a
second copy, and the original data are not overwritten.

>
> (Stefano wrote)
>> That is the major point - what should be done when n chunks are ok, n+1
>> has a wrong checksum ? And in a generic way, that means for all handlers
>> ? What about if the handler is the remote handler, and the artifact (n
>> chunks) were transferred to another entity ?
>
> I think just making copyimage() fail in the handler is fine:

Agree.

> - the handler decides how to cleanup its own data, including forwarding
> the error to whatever remote mechanism if required,

Agree.

> - the handler itself fails, and swupdate / higher level integration
> scripts decide how to clean up the failed update.

Agree. And there are now postfailure scripts, that give flexibility to
the integrator to decide what to do.

>
> We've not fed the handler any unvalidated data, so it's exactly the same
> as a network failure, swupdate needs to be able to handle this
> regardless of the security considerations here.
>
> > Thank you three again for the replies :)

Thank you for the nice discussion.

Best regards,
Stefano

[Dominique MARTINET]

Stefano Babic wrote on Tue, Jun 04, 2024 at 01:56:25PM +0200:
> > Thanks for pointing me to this thread, I had missed it!
> >
> > Merkle tree is definitely a possibility, at least that'd solve the
> > problem of storing the full list in sw-description and keeping it in
> > memory all the time as we would only store the root hash in
> > sw-description and the rest can be sent just before each file...
> >
> > I think swupdate errors out if there are unknown files
>
> Correct. It is often used as "feature" to deploy some additional file,
> like a README for the release or something else. SWUpdate ignore them.

That wasn't correct then (I said it'd error) ; I've just double-checked
they're skipped alright, thanks!

In that case adding the hashes as a file before each files could also
work, be it merkle tree or plain list.

That's probably more efficient than what I had been doing at the cost of
finding a way to track these special files; I have no strong opinion
either way -- please ask if you think an updated proof of concept with
such intermediate files would be useful for the discussion.

> > Simplicity of generation of the sw-description/hash data certainly isn't
> > a hard requirement, but the easier it is to generate the more likely
> > people will update their code to use the new format.
>
> Generation of SWU is already abstracted and the integrator / user
> shouldn't bother about it. The paths are:
>
> OE ==> meta-swupdate
> Debian, Buildroot ==> SWUgenerator
>
> Other ways are unsupported.

I've written my own tool (mkswu[1]), as SWUgenerator didn't exist back
then, but it's fairly opinionated/specific to our distro so I didn't
bring it forward when discussing SWUgenerator.

I'll consider having mkswu use SWUgenerator as a backend if it gets
difficult to implement, it's currently not worth the refactoring.

[1] https://github.com/atmark-techno/mkswu

But, to get back on topic:

> A major complexity can be implemented in the above tools - so if
> generation is becoming more complex is not a problem at all.

ok for this point, I won't consider this a problem either.

> > The asymmetric sw-description encryption that Michael sent to the list a
> > while ago[1] solves this pretty nicely, since the content aes key will be
> > written in the sw-description it can be changed for every update, and
> > the sw-description key itself can be rotated more easily (because a swu
> > can be signed for multiple target keys)
>
> Just to avoid any effort: the nice series above has hit the issue that
> support for crypto engines in SWUpdate is quite messy. I have already a
> design to make it modular, and let the integrator to just pick up the
> crypto feature (RSA, pkcs#7, etc.) independently from the underlying
> crypto library. I have started to check this. Until this is not cleaned
> up, I won't merge changes regarding crypto (verifying, decription, etc.)
> because it just increments mess and #ifconfig.

Thanks for the heads' up/reminder, I agree it's very complex right now,
this will be very welcome! Thanks!

> > In the single-copy case there can be all sort of failures, even with a
> > valid SWU (e.g. disk full, transient network error); the update system
> > will need to deal with these anyway.
> > I don't think it's a new problem, and at least not one we need to tackle
> > here all at once (an improvement is still an improvement)
>
> In fact, it is not a new problem. A single-copy schema has its
> drawbacks. The reliable way used by SWUpdate is to consider the all
> update unsuccessful.

Yes.

> > My situation is somewhat particular in that I'm not a final integrator,
> > but we're providing a set of primitives for updates using swupdate, and
> > out of the mostly double-copy update target there is just one directory
> > that is single copy and we recommend not to touch on updates.
> > But even if our user respect this, if an attacker can swap an archive,
> > they can write to that single-copy directory.
>
> Ok, but they can write because sw-description will allow this, that
> means your users are not respecting your suggestions..... :-)
>
> If sw-description has no entry for the file in the directory, the file
> is simple skipped.

The "path" target of the archive handler cannot be changed, so any
update that targets a sub-folder away from the single-copy directory
isn't affected, but the problem is the way our swupdate pre-install
script prepares the install target.
When the install actually runs, we had something like this:
/
├── more-doublecopy
├── single-copy
└── target
├── more-doublecopy
└── single-copy

Where target is another partition for /, more-doublecopy is a snapshot
of the /'s one, and single-copy is a bind mount to /single-copy

That means update to /target/foo or /target/more-doublecopy won't be
able to write to /single-copy, but updates targeting /target could write
there by just making the paths in the archive be ./single-copy/badfile

My workaround currently is to stop mounting /target/single-copy unless
it's explicitly requested by the user, so it has to be a conscious
decision from our users. Old SWUs will no longer install if they were
depending on this but that is a necessary evil, and if users respect our
recommendation of not writing to /target/single-copy there is no ill
effect (and nobody complained yet, so it looks like users who updated at
least seem to follow guidelines :P)

> > The idea here isn't to just chunk, it's that chunk gives us the ability
> > to swap the validation in before we give it to the handler, so the
> > attacker doesn't have a chance to alter the extraction directory.
>
> But the attacker can alter a file in the extraction directory, so it
> seems to me it is just moving the problem. Of course, in the specif use
> case (archive to be extracted), there are some advantages, but taking as
> generic (chunk sent to any handler), I do not see a lot.

I do not see how an attacker could alter the file in the extraction
directory -- are you talking about something like this?
- swupdate extracts a chunk to $TMPDIR
- swupdate validates chunk matched the checksum
- attacker modifies the chunk in $TMPDIR
- swupdate reads back data from the chunk to the handler,
which had been altered so is no longer truthful

If that is the case, I do not see how the attacker could modify the
temporary chunk, because it's created by swupdate (root or separate
user) in a safe way (mkstemp ensures that is a new file owner by our
user); an attacker could only modify this chunk if they are root, and if
they are they don't need to mess with swupdate.

> > Anyway, the only thing that improves here in this case is just we'd
> > limit the write to the size that was written in the sw-description, but
> > it's still a step forward.
>
> size can be added and core could check this with CPIO header to detect a
> manipulation.

Yes, we can add a size attribute in sw-description and check it matches
at cpio level.
If the solution we use here does both I do not see the benefit of
implementing both, which is why I brought this in the discussion, but if
chunks aren't implemented or not expected to be used then we should
definitely add this for now.

> > Stefano replied that cpio being easily introspectable is a feature, but
> > if we add encryption to the mix I'm not sure how viable it is anyway.
>
> Of course, but everything here is configurable. A lot of users don't use
> encryption, it is duty of the integrator to decide this.

Agreed.

> > In practice, we've been using SWU as our update transport for about 2.5
> > years and people around me rarely extract SWU files to inspect their
> > content.
> > Checking the sw-description signature manually or decrypting the archive
> > content are things I do from time to time but it all requires highly
> > specific commands, so I don't think it matters if we're not using cpio.
> >
>
> Because you're a developer....:-)
>
> End users do not bother at it, and application developers as well. That
> means OEM developers, too. They just require that their software is
> bundled into the SWU.

Exactly, that means they do not care that double-click opens the SWU;
but your point about adding a README in the cpio is interesting, I had
not thought about it.

> > (and the 4GB file limit has actually come up a couple of times for large
> > container images our users could want to ship...)
>
> When this will become a requirement, it will be fixed.

Our current eMMC are fairly small to reduce cost (~10GB for the bigger
model as it's used in pseudo-SLC mode, which does not leave much room
for >4GB updates when you consider double-copy and compression), so this
does not come regularily yet, but users sometimes try to install some
large container they found so I expect we'll see more of these when MMC
size increase.
For containers we could split by layer, but it's a bit of work that
isn't always possible to automate (what if there was only one layer?)

Anyway, I agree this can wait for our current use case.

> > and there isn't much room
> > for mistakes; if the new format handles chunks or other high level
> > features how much trust can we place into it? And can the chunks be
> > signed in a way that'd allow upfront verification?
>
> Changing in the ZCK format should then discussed and verified with the
> Zchunk's maintainer (Jonathan Dieter), as I did in the past when I
> introduced the delta update.

If we go with ZCK, and changes are required, yes.

> > (we cannot use the
> > cpio crc for verification, not only because crc is weak, but because
> > we're packing into cpio after we're done generating the sw-description
> > and signed it)
>
> This is currently an advantage that packaging and signing are orthogonal.
>
> >
> >
> > I think it's still worth considering, even if we end up keeping the same
> > format; perhaps start with a checklist of what we'd like to see?
>
> There is a doc/source/roadmap.rst. It will be nice that new ideas or
> proposal will be stored there, even if they won't be implemented in
> future. Probably changing the name from "roadmap", it isn't already a
> roadmap. This allows to track ideas instead of digging into ML's archive.

Let's do that, then.

I think a change of format will take time, whereas some manual chunking
can be done quite quickly (the proof of concept I sent on Monday was
done in a single day), so unless an obvious candidate comes up I'd
rather split the discussion and come back to format change after we're
done with chunking.

> > Is there something that checks all these boxes?
> > - multiple files that can be read one at a time from a stream (linear
> > http download), or single file on disk (installing from .swu file)
>
> Not clear what you mind - yes, they are two use cases, both supported.
> Selecting individual files from a stream, in a way using Byte Request
> like delta update is doing, is something I proposed to customers some
> times ago, and I called it "Selective downloading!". But interest was
> n't so big and I got no sponsors for this topic. I removed this from
> roadmap with commit a7ab76a, but you can still find it.

I had in mind that the delta handler doesn't work with just a local .swu
file right now, so just wrote that requirement to make sure swupdate -i
(or swupdate_client) keeps working.

For selective downloading, I hadn't notice it was dropped from the
roadmap; we're still somewhat interested; I hadn't touched it because I
thought you would do it eventually but if that is no longer planned I
might get back to it eventually.

This would allow packing one-size-fits-all large updates, where out of
date devices would install the whole requirements and almost up to date
onces would only get a small part; we currently cannot do this over LTE
to save bandwidth.
Anyway, this will wait.

> > - chunked validation before extracting (this topic)
> > - individual files larger than 4GB
> > - seekable? (cpio has file size so we could skip a single file, but
> > cannot skip directly an arbitrary number of files)
>
> This was done at the early beginning, and I had two different paths for
> local and remote update. If you check in earlier code, you see that
> SWUpdate used seek() to pass the right offset to the handler.
>
> Anyway, the advantage (just skipping not required file in case of local
> update) are small compared to the disadvantages. I had always to test
> separately a local and remote update for any change.

This point was actually one step further (skipping multiple files at
once), it would be a further optimization of selective downloading: With
cpio, the code has to get a range for each file header, so assuming
there is an index with all the files at the start and we skip multiple
files in a row this could avoid getting these intermediate file headers.

It's probably not worth it, just something that came to mind as a
potential wish list item -- let's ignore this.

> > > That always requires temporary storage and hence defeats the idea of
> > > streaming updates, doesn't it? Or did I get this wrong?
> >
> > It's a halfway measure: it requires enough storage or memory for one
> > chunk of data.
> > So if you have a 1GB update, without installed_directly you'd need 1GB
> > of temporary storage, but with the patches I sent yesterday (512KB
> > chunks) you'd need 512KB of disk and and extra 64KB of ram (32 bytes per
> > chunk); it'd also make the sw-description 140KB longer (around 70 bytes
> > per chunk in string representation).
>
> There shouln't be any restriction anymore for sw-description's size.
> Anyway, the size of the chunk is a challenge. Too small, too many
> hashes. Too big, no real advantage. The delta handler with ZCK looks for
> chunks of variable length using a rolling hash function - most suitable,
> but of course much more complicated.

Yes, my proof of concept has a fixed chunk size, but it wouldn't be very
hard to make variable ("just" another extra field in sw-description,
either global or at file level)
I think it could be worth the effort for very large updates or small
updates on more constrained devices, I intended to implement it for a
real patch series.

> Regarding the storage: a big advantage of the streaming technique
> (installed-directly set) is that the buffer (currently small) is
> allocated oin the heap. This increases security, because the data are
> allocated dynamically on the heap, and it is more difficult for an
> attacker to find it and change it. If the chunks are verified and saved
> on TMPDIR, there is a time window and attacker could (theoretically)
> change the data on the filesystem (easy to find) before the handler has
> processed.

As said above I consider swupdate's TMPDIR safe:
- files should not be world-writable (we might want to change umask...);
if they were even files without installed_directly would be problematic.
- for our particular systems, I actually run swupdate in its own TMPDIR
in a locked down subdir of /var/tmp.
This makes swupdate-progress a bit harder to use (need to set TMPDIR),
but I've found this tradeoff to be worth it.

> > > Then again, this doesn't solve the single-copy n+1 chunk failing, does it?
> >

> [...]

> > - the handler itself fails, and swupdate / higher level integration
> > scripts decide how to clean up the failed update.
>
> Agree. And there are now postfailure scripts, that give flexibility to
> the integrator to decide what to do.

Yes, thank you for the postfailure scripts!

--
Dominique

[Stefano Babic]

Hi Dominique,

Thanks for link - it seems to me very targeted to your distro. But just
let me know if there are some features that it is worth to be
implemented in SWUgenerator.

>
> But, to get back on topic:
>> A major complexity can be implemented in the above tools - so if
>> generation is becoming more complex is not a problem at all.
>
> ok for this point, I won't consider this a problem either.

Sure.

>
>>> The asymmetric sw-description encryption that Michael sent to the list a
>>> while ago[1] solves this pretty nicely, since the content aes key will be
>>> written in the sw-description it can be changed for every update, and
>>> the sw-description key itself can be rotated more easily (because a swu
>>> can be signed for multiple target keys)
>>
>> Just to avoid any effort: the nice series above has hit the issue that
>> support for crypto engines in SWUpdate is quite messy. I have already a
>> design to make it modular, and let the integrator to just pick up the
>> crypto feature (RSA, pkcs#7, etc.) independently from the underlying
>> crypto library. I have started to check this. Until this is not cleaned
>> up, I won't merge changes regarding crypto (verifying, decription, etc.)
>> because it just increments mess and #ifconfig.
>
> Thanks for the heads' up/reminder, I agree it's very complex right now,
> this will be very welcome! Thanks!

Hopefully I can go on soon, it depends on if this development will be
sponsored, but chances are good.

So the *bug* and security leak is the bind mount here that drop the
dual-copy concept for this directory. The solution is to avoid this
mount at all.

> That means update to /target/foo or /target/more-doublecopy won't be
> able to write to /single-copy, but updates targeting /target could write
> there by just making the paths in the archive be ./single-copy/badfile
>

Sure - but then, the mount should be avoided.

> My workaround currently is to stop mounting /target/single-copy unless
> it's explicitly requested by the user,

It seems to me the solution, not the work-around.

> so it has to be a conscious
> decision from our users.

Right. If users want to break the device, they will do anyway.

> Old SWUs will no longer install if they were
> depending on this but that is a necessary evil, and if users respect our
> recommendation of not writing to /target/single-copy there is no ill
> effect (and nobody complained yet, so it looks like users who updated at
> least seem to follow guidelines :P)

Your users are listening to you. It does not always happen...:-)

>
>>> The idea here isn't to just chunk, it's that chunk gives us the ability
>>> to swap the validation in before we give it to the handler, so the
>>> attacker doesn't have a chance to alter the extraction directory.
>>
>> But the attacker can alter a file in the extraction directory, so it
>> seems to me it is just moving the problem. Of course, in the specif use
>> case (archive to be extracted), there are some advantages, but taking as
>> generic (chunk sent to any handler), I do not see a lot.
>
> I do not see how an attacker could alter the file in the extraction
> directory -- are you talking about something like this?
> - swupdate extracts a chunk to $TMPDIR
> - swupdate validates chunk matched the checksum
> - attacker modifies the chunk in $TMPDIR
> - swupdate reads back data from the chunk to the handler,
> which had been altered so is no longer truthful
>

Yes - I write *theoretically*. But there were several CVEs able to use a
time window to get more rights. I remember the one sending a trace
signal to a process, able to switch to root right (I do not remember
anymore which kernels were affected). Nevertheless, if an attacker could
find a leak somewhere else and install something on the device, its code
could read via netlink from kernel when a new file is created in a
TMPDIR (not necessary /tmp), and tries to change it before SWUpdate has
worked it.

I do not say this is easy, and with appropriate user rights / umask (we
have Linux, we can use access rights) we can reduce again risks. But
again, it could be theoretically possible, files in TMPDIR are visible,
while buffers on the heap are much harder to be manipulated.

> If that is the case, I do not see how the attacker could modify the
> temporary chunk, because it's created by swupdate (root or separate
> user) in a safe way (mkstemp ensures that is a new file owner by our
> user); an attacker could only modify this chunk if they are root, and if
> they are they don't need to mess with swupdate.

Of course, it depends on user rights, and the attacker should have the
right to write to this file. But if he has the right, he could easier do
it when the chunk is in TMPDIR than if the chunk is in a buffer on
SWUpdate's heap.

>
>>> Anyway, the only thing that improves here in this case is just we'd
>>> limit the write to the size that was written in the sw-description, but
>>> it's still a step forward.
>>
>> size can be added and core could check this with CPIO header to detect a
>> manipulation.
>
> Yes, we can add a size attribute in sw-description and check it matches
> at cpio level.
> If the solution we use here does both I do not see the benefit of
> implementing both, which is why I brought this in the discussion, but if
> chunks aren't implemented or not expected to be used then we should
> definitely add this for now.

Ok

I added support for docker, other virtualization (LXC will be nice)
could be added. This moves how to split a container, loading the images
and creating then the container, supported as well.

Removing the 4 GB limit was also in my "roadmap", but it seems it is not
yet urgent. I know about users upgrading a whole Ubuntu image via
SWUpdate, but they haven't reach the limit as well.

>
> Anyway, I agree this can wait for our current use case.
>

It should be added to the TODO / roadmap.

Well, we are talking about of future development in SWUpdate.

Then, near the delta handler, we can develop a ZCK handler. This handler
will do most of what the delta handler is doing, with the exception that
the chunks of the running software are not computed. So a ZCK handler
will accept a ZCK file that will be stored into the SWU instead of
beeing downloaded, will extract the chunks and pass them to a chained
handler, exactly like the delta handler is doing.

>
> For selective downloading, I hadn't notice it was dropped from the
> roadmap; we're still somewhat interested; I hadn't touched it because I
> thought you would do it eventually but if that is no longer planned I
> might get back to it eventually.

It is not that I haven't ideas how to improve the project, but these
ideas should raise interest. The general (not working) way is:

- write new ideas to "roadmap" or discuss them with customers
- make an estimation of efforts
- find someone to finance the development.

First two points are easier to be done, workflow stops with last point
:-). Then, if after some time I cannot find any interest, I have to
admit that there is no interest and I remove from the roadmap. It looks
like that "roadmap" is a very bad name, it assumes that something is
already planned. Renaming required.

>
> This would allow packing one-size-fits-all large updates, where out of
> date devices would install the whole requirements and almost up to date
> onces would only get a small part; we currently cannot do this over LTE
> to save bandwidth.

The goal for selective downloading is also to pack SWUs for multiple
devices into a single SWU, and a device will just load what it is
needed. This can help in several use cases where the server is not under
full control of the manufacturer.

> Anyway, this will wait.
>
>>> - chunked validation before extracting (this topic)
>>> - individual files larger than 4GB
>>> - seekable? (cpio has file size so we could skip a single file, but
>>> cannot skip directly an arbitrary number of files)
>>
>> This was done at the early beginning, and I had two different paths for
>> local and remote update. If you check in earlier code, you see that
>> SWUpdate used seek() to pass the right offset to the handler.
>>
>> Anyway, the advantage (just skipping not required file in case of local
>> update) are small compared to the disadvantages. I had always to test
>> separately a local and remote update for any change.
>
> This point was actually one step further (skipping multiple files at
> once), it would be a further optimization of selective downloading: With
> cpio, the code has to get a range for each file header, so assuming
> there is an index with all the files at the start and we skip multiple
> files in a row this could avoid getting these intermediate file headers.
>
> It's probably not worth it, just something that came to mind as a
> potential wish list item -- let's ignore this.

I do not think it is worth - this will just remove the need to download
the next CPIO headers. If connection is not dropped, just a few bytes
are downloaded. The advantage is quite negligible.

See above.

>>>> Then again, this doesn't solve the single-copy n+1 chunk failing, does it?
>>>
>> [...]
>>> - the handler itself fails, and swupdate / higher level integration
>>> scripts decide how to clean up the failed update.
>>
>> Agree. And there are now postfailure scripts, that give flexibility to
>> the integrator to decide what to do.
>
> Yes, thank you for the postfailure scripts!
>

Best regards,
Stefano

[Dominique MARTINET]

Hi,

James Hilliard wrote on Mon, Jun 03, 2024 at 03:52:25AM -0600:

> I had also come across this library which might give some good ideas
> on how to implement the data structures efficiently for chunk validation.
>
> https://github.com/IAIK/secure-block-device

So the merkle tree implementation is actually in another repo:
https://github.com/IAIK/merkle-tree

Ultimately, I don't think there is any adantage to this: merkle trees
are mostly useful when you do not want to check the whole file, but
only validate or update part of the data because in this case only part
of the tree will need updating.
If you need to validate the whole thing then validating the intermediate
levels of the tree is just extra work, and it doesn't bring anything -
we might as well just have a plain list and checksum the whole list, as
we need the whole list of checksums to validate the root node anyway
(unless I misunderstood something obvious)

With a tree of signatures (something that, given the root node, could
verify any of its children individually) we could in theory also stream
the checksums and verify them just in time, saving a bit of memory, but
I don't think it's worth the effort/complexitiy; especially considering
we'll need to cache the data anyway if streaming from cpio unless we
litteraly splice it into the input file and I don't see that ending
well.


If we stick with such a hash list as I proof of concept'd, for me the
question would be do we care if this is all in the sw-description (uses
more memory for parsing it and need to keep all the hashes in memory
during the update), or have each file provide its own list to verify
when extracting the next file.

For my own use case, my updates will usually have a few small files with
a single chunk and only one or two large files so there is not such a
big advantage of doing it one file at a time (or perhaps something
intermediate that allows both?), so I'd rather favor simplicity to avoid
coding mistakes before input validation, but I'm open to opinions.

--
Dominique

[Dominique MARTINET]

Stefano Babic wrote on Wed, Jun 05, 2024 at 08:27:28AM +0200:
> > I do not see how an attacker could alter the file in the extraction
> > directory -- are you talking about something like this?
> > - swupdate extracts a chunk to $TMPDIR
> > - swupdate validates chunk matched the checksum
> > - attacker modifies the chunk in $TMPDIR
> > - swupdate reads back data from the chunk to the handler,
> > which had been altered so is no longer truthful
> >
>
> Yes - I write *theoretically*. But there were several CVEs able to use a
> time window to get more rights. I remember the one sending a trace
> signal to a process, able to switch to root right (I do not remember
> anymore which kernels were affected). Nevertheless, if an attacker could
> find a leak somewhere else and install something on the device, its code
> could read via netlink from kernel when a new file is created in a
> TMPDIR (not necessary /tmp), and tries to change it before SWUpdate has
> worked it.
>
> I do not say this is easy, and with appropriate user rights / umask (we
> have Linux, we can use access rights) we can reduce again risks. But
> again, it could be theoretically possible, files in TMPDIR are visible,
> while buffers on the heap are much harder to be manipulated.

While I do agree there have been such kernel failures in the past
(dirtypipe comes to mind), I do not believe it is a problem we should
care about here: if a user can overwrite root-owned files then they can
do whatever they want without trying to target this validation period:
- write files directly without swupdate e.g. /etc/shadow
- if limited to live processes, write files as they are extracted by
the archive handler on an update

By the way, my proof of concept implementation does not leave a file
behind in TMPDIR, we open/unlink/mmap/close it so there is no extra file
left and no fd left to the file.
If we throw away portability we could open with O_TMPFILE on linux.
That would not create a link in the first place so skip the need for
unlink (which could be left behind with an #if bsd); but I do not think
it's worth the code complexity.

> > Anyway, I agree this can wait for our current use case.
>
> It should be added to the TODO / roadmap.

I've sent a separate patch for the roadmap as that's quick to do:
- rename the file to improvement_proposals.rst
- add a point for 4GB files.

I ended up not adding a point about a different format, because the more
I think about it and the more I think having the validation process
independant of the external container (sw-description signature and
chunks validation) has more benefits than whatever we can come up with.

If someone has concrete ideas with clear benefits we can discuss that
and add it to the proposals at this time.

> > For selective downloading, I hadn't notice it was dropped from the
> > roadmap; we're still somewhat interested; I hadn't touched it because I
> > thought you would do it eventually but if that is no longer planned I
> > might get back to it eventually.
>
> It is not that I haven't ideas how to improve the project, but these
> ideas should raise interest. The general (not working) way is:
>
> - write new ideas to "roadmap" or discuss them with customers
> - make an estimation of efforts
> - find someone to finance the development.
>
> First two points are easier to be done, workflow stops with last point
> :-). Then, if after some time I cannot find any interest, I have to
> admit that there is no interest and I remove from the roadmap. It looks
> like that "roadmap" is a very bad name, it assumes that something is
> already planned. Renaming required.

I wish I could pretend to be surprised at this point; it's good that you
can get financing at all.
I'm not in any position to make money move so I'll give some time
instead :)


>>>> So we have three paths forward: [...]

Trying to keep moving forward here, how do you think we should proceed?

The more I think about it and the more I think something along the lines
of the RFC patches I sent is the easiest way forward, but I don't want
to push too hard/too fast -- should I give you some time to have a look
at the patches?
They'll require small fixups but it's honestly very close to something
that could be useable.


Or should I look closer into the ZCK option?
I've just played with the zck/unzck commands verbose mode to understand
the format a bit more and I agree it could work out; I'm not sure I like
chaining handlers but perhaps as an alternative compression type to
gzip/zstd so e.g. you could specify `compress = zck` and have an extra
checksum value for the expected header checksum in sw-description, then
just streaming it out as unzck does should work quite nicely.

It has the advantage that it's not something hand-rolled, and the
complexitiy in swupdate itself will be fairly close to what I have done
in monday's patches;
The inconvenients I see are that I'll need quite a lot of time to audit
the zck code, and that older versions of swupdate won't be able to
extract new updates at all so I won't be able to use it for the
forseeable future (... just had a support ticket with a user still
running the version from jan 2023 over a year ago, will need to provide
these kind of people a paved way to upgrade through an intermediate
version...)

I think compatiblity is going to be a problem with pretty much any other
solution than my initial suggestion to be honest, so if you strongly
prefer zck I'll just have to give up on it...

--
Dominique

[Dominique MARTINET]

Hi,

Coming back after internal discussions & letting it sit for some time.


Thinking about it from a "swupdate project" point of view, I've come to
admit that zck probably makes more sense for upstream inclusion given we
already depend on it for the delta handler (even if I think the
compexity of chaining handlers is too much and it'd be worth having
something like compress=zck at a higher level for files embedded in the
SWU)


Unfortunately our internal discussion put more weight on the forward
compatibility problem (being able to install new SWU files on current
swupdate versions, e.g. even if no updates have been installed in a
couple of years), so while I think it is cleaner I'll still end up
polishing a bit & merging the "manual chunking" way in our tree.

I unfortunately just do not see any other way that'd keep both forward
compatibility and not reinvent a small wheel; we could have the checksum
list being in a separate file rather than directly within the
sw-description but that doesn't bring much benefit vs. implementation
complexity in my opinion, so I'll keep my patches minimal to minimize
rebase pains.
(The worst that can happen is that we decide it's not worth it anymore,
and the extra info in that period's SWU's sw-description is ignored;
that's not too bad.)

On this note, there is a couple more cleanup patches I had sent as RFC
that is a bit more intrusive and didn't send as part of the earlier
cleanups (thanks for merging that!); I'll resend them now for separate
consideration.


If someone has a better idea that preserves forward compatibility
I still have a bit of time to consider & update anything in a way that
has a chance of getting merged (I'm really reluctant to go down this
hole), but unfortunately I just don't see any "good" way of doing it
right now :/

Also, in case it turns out this might not be so bad after all and if
someone wants to look at the whole thing again, I've just rebased my
branch, the patches are identical to what was sent as RFC earlier with
conflicts due to patch order fixed:
https://github.com/atmark-techno/swupdate/tree/chunked_stream
(I can also just resend it if that helps)

There are a couple of possible improvements e.g. configurable chunk
size, but if it's going to stay in my tree I don't plan on implementing
it; I would do it though if this was considered for upstreaming.


Thanks again for the discussions anyway;
and I'll send some documentation update I promised somewhere in July.

Cheers,
--
Dominique