swupd-client icon indicating copy to clipboard operation
swupd-client copied to clipboard

Metadata belongs in Manifests, not tar files

Open icarus-sparry opened this issue 6 years ago • 7 comments

There are a number of security issues that could be resolved if we didn't need to do various operations as root. In particular if we unpacked tarballs as a dedicated user we wouldn't need to worry if files are going to be overwritten via symlink attacks.

There are also issues of additional attributes associated with files that do not have standard representations in tar files, Access control lists are an example.

Currently we can't do this unpacking as a dedicated user, because the only place that ownership and permission information is stored is in the tar headers.

This information ought instead to be in the Manifests, where it is more directly accessible to swupd. The information is then part of the hash of the Manifest, and hence covered by the chain of trust.

The most straight forward way of handling this would be to add things like the the permissions, owner and group as new columns in the body of the Manifest. To support this, and to make the data self describing we could add a new header, e.g. Fields or Columns, to the manifest header which describes what the tab separated columns are. Unknown columns should be ignored, to make the system (once it has this header) backward and forward compatible.

icarus-sparry avatar Feb 27 '18 20:02 icarus-sparry

In particular if we unpacked tarballs as a dedicated user we wouldn't need to worry if files are going to be overwritten via symlink attacks.

It is possible to validate the entire contents of the tarball, and extract the metadata, without having to write anything to disk as root.

There are also issues of additional attributes associated with files that do not have standard representations in tar files, Access control lists are an example.

Do we have concrete use cases for those?

This information ought instead to be in the Manifests, where it is more directly accessible to swupd.

Why? Are there examples where we need the metadata before having the file at hand. That is a scenario that justifies bringing them to the manifest. Another scenario is if libarchive is causing us trouble (efficiency or functionality) to collect this data.

The information is then part of the hash of the Manifest, and hence covered by the chain of trust.

The metadata is used to create the hash of the individual files, so it is covered by the chain of trust. With my current understanding, this argument is not a valid one.

The most straight forward way of handling this would be to add things like the the permissions, owner and group as new columns in the body of the Manifest.

I really don't think this proposal is compelling. It seems to me we can achieve almost everything mentioned by making a better use of libarchive (with the exception of ACLs).

cmarcelo avatar Mar 01 '18 17:03 cmarcelo

Note: I'm not against changing the Manifest or anything, and my first intuition when I was learning swupd was that would be better if the hashes referred to file contents and not the metadata. I just want to make sure that the "upsides" and downsides of the changes are clear.

cmarcelo avatar Mar 01 '18 17:03 cmarcelo

OK, I was exaggerating a little when I said we can't unpack as a dedicated user, we clearly can and then we can look at the tar headers to see what the metadata ought to be.

I think a better question to ask is "why are we using a tarball in the first place?"

I believe the answer is historical, it was a quick and easy way for swupd-client to avoid having to worry about file ownership and permissions, just ship the tarball and unpack it and everything is preserved.

The downside is that now you have to worry about the security of the tar program, and you have to run as root in order to preserve the ownership (bsd based unix made chown a privileged system call when they added support for disk quotas). This opened up an attack by sending a crafted tarball (here is a new /etc/passwd), or two (here is a symlink from foo to /etc/passwd, now here is new contents for foo). So the fix was to switch to libarchive rather than tar to unpack, as it has better facilities to prevent this kind of thing.

If we just sent the file (probably compressed), and had the metadata elsewhere then we don't have to worry about this whole class of bugs leading to security problems.

At the moment the tar files are not part of the chain of trust, some of the contents of the tarball are part of the chain.

There are additional problems that us having control over the metadata solves. When I first started I was using a container which was using inherited extended attributes. swupd couldn't validate this because it didn't know to ignore the extended attributes. If we control the metadata we can say which extended attributes are of interest to swupd and filter out others.

icarus-sparry avatar Mar 01 '18 19:03 icarus-sparry

I believe the answer is historical, it was a quick and easy way for swupd-client to avoid having to worry about file ownership and permissions, just ship the tarball and unpack it and everything is preserved.

Correct.

tmarcu avatar Mar 02 '18 19:03 tmarcu

It seems to me all your issues are related to using the 'tar' program, which we are not using anymore: client uses libarchive, and server uses Go archive/tar package (assuming all goes well next weeks). E.g.

you have to run as root in order to preserve the ownership

That is not necessary. You can read the tar headers and gather the metadata, as well as the contents. The class of bugs you talk about is really about trusting the files before verifying them. The file in question would not pass the hash check.

My contention with the proposal is that it will be a large change and I couldn't clearly see what the benefits are. It involves changing both client and server, and changing almost all our assets. Compare to our previous change that took about 2x the expected time and had the goal of not changing the assets (except for new rename detection).

I suggest doing another iteration in the proposal trying to be more specific about those concrete benefits of moving to this new approach. I'm not working directly on swupd-client anymore, so you and others feel free to put less weight on my contention and suggestion when deciding how to proceed.

cmarcelo avatar Mar 05 '18 19:03 cmarcelo

"It seems to me all your issues are related to using the 'tar' program, which we are not using anymore: client uses libarchive, and server uses Go archive/tar package".

Sorry, I think you are missing the point.

"That is not necessary. You can read the tar headers and gather the metadata, as well as the contents" - yes of course you can. the point is that you have to implement a lot of things that you don't need which increases the attack surface.

"I suggest doing another iteration in the proposal trying to be more specific" - there is already a multi-page document describing the problem and the security issues. This github issue is just a placeholder.

icarus-sparry avatar Mar 05 '18 20:03 icarus-sparry

Why does check-update require root? I need this removed in our environment so I don't need to run a service as root, but can instead limit it's privileges using capabilities such as CAP_SYS_ADMIN for mounting devices, etc.

aaronovz1 avatar Sep 30 '18 18:09 aaronovz1