PackagingDrafts/SecureBuildRoot

BuildRoot change Proposal

[[TableOfContents()]

Overview

Problem Space

Currently, the most commonly used BuildRoot settings use a predictable file name in world-writable sticky directory (/var/tmp). This is bad, because it is possible for local users to make builds fail, simply by creating the known directory in advance. It is also problematic from a security perspective, since it is possible for a malicious user to perform a symlink attack and overwrite the arbitrary victim's files (depending on the tools used to unpack the source, possible also during %build).

Solution Overview

Changes to Guidelines

Fixing this in a non-invasive way can be tricky. One obvious way would be to remove the BuildRoot tag and set it to some secure default, but this is not currently available in upstream rpm. The only remaining option is to change the value that Fedora mandates the BuildRoot be defined to. Using mktemp is generally sufficient for this purpose-- and one of our standard BuildRoot values even uses it.

However, just choosing the unguessable name is not enough to solve the problem. We must ensure that we don't leave time for attacker to create it sooner than us. Our problem is that we remove the %buildroot during %install with the good intention of cleaning up the installation root, but after we do so, we effectively revert what mktemp has done for us.

There are two possible solutions for this:

1. Do not clean the buildroot during %install, it is guaranteed to be empty anyways (remember, we're using mktemp). 1. Use one more level of temporary directories, so that when we delete it during %install, the top level directory generated by mktemp is still intact. We'd need to change how we clean up the buildroot for during %clean too.

For the first solution, we could standardize the following BuildRoot form and forbid cleaning the buildroot during %install:

BuildRoot: %(mktemp -d %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)

The second solution would be as follows:

BuildRoot: %(mktemp -d %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)/buildroot

...

%install
rm -rf %{buildroot}

...

%clean
rm -rf $(dirname %{buildroot})

Applying the changes

Changes could be done automatically by a Fedora release engineer. Then in a month or two, a script can be run to ensure that all new packages that were imported during the time this was taking effect are doing the right thing and change them.

In a target time frame (F10?), we should ensure that all packages distributed by Fedora have been rebuilt with these changes, and that no future SRPMS with insecure BuildRoot will go be distributed.

Scope

This would affect practically all of the packages in Fedora.

Discussion Points

1. Which of the two solutions to pick? 1. Would the third solution be to add a big fat warning to the documentation that packages shouldn't be built on systems with untrusted local system?

Comments ?

This problem was mentioned to us a long time ago by the openSUSE folks. Their solution was simple: after the rm -rf %{buildroot}, simply do a mkdir %{buildroot}. The mkdir will fail if someone plants a symlink there. Once the directory is created, using it is safe. This doesn't even require a mktemp at all, it works just fine with the traditional id -u-based buildroots most of us are still using. I have been using this in my packages ever since. Unfortunately, it was rejected as a guideline because the consensus was that whoever is using a shared %{_tmpdir} is just asking to get exploited. I believe the createrpmdevtree tool creates a per-user %{_tmpdir}. -- KevinKofler

• Imho it is not somebody asking to get exploited, because rpm defaults to %{_tmppath} beeing /var/tmp and rpmdev-setuptree does not create per-user %{_tmppath}. I use mkdir after rm in %install, too, btw. -- TillMaas

• Using temporary files is a widely used practice, with known ways to be done in a secure way, not "asking to get exploited" -- LubomirKundrak

• I also don't think we should punish our users for using a shared %{_tmppath}, so this really needs to be fixed. -- KevinKofler

• The SUSE practice is a bad one and makes no sense at all. Unless you consider ability of local malicious user to make your builds fail a feature. -- LubomirKundrak

• It's better than overwriting your files. ;-) But you're right that it isn't the ideal solution either. -- KevinKofler

• I don't recall that reason ever being used. If it was, my apologies, I must have dismissed it because it was so nonsensical. The reason I recall was that the SUSE solution will fail if someone redfines their BuildRoot to be two levels deep like:  %(mktemp -d %{_tmppath}/%{name}-XXXXXX)/buildroot. -- ToshioKuratomi

Both proposals would not work, because the "-u" option removes the directory after creation:

$ LANG=C ls $(mktemp -du "/tmp/foo-XXXXXX")
ls: cannot access /tmp/foo-L21240: No such file or directory

Also mktemp does only create the last directory in case it is possible:

$ mktemp -du "/tmp/foo-XXXXXX/dirname"
mktemp: cannot make temp dir /tmp/foo-XXXXXX/dirname: No such file or directory
$ mkdir "/tmp/foo-XXXXXX"
$ mktemp -du "/tmp/foo-XXXXXX/dirname"
/tmp/foo-XXXXXX/dirname

-- TillMaas

• I did not notice the -u -- you're right, we should not use it. And I fixed it above; thanks!
• noone proposed mktemp -du "/tmp/foo-XXXXXX/dirname, but mktemp -du "/tmp/foo-XXXXXX and creating the subdirectory afterwards. Reread that, and notice the parentheses. -- LubomirKundrak

How will work

$ rpmbuild -bi foo.spec
$ rpmbuild -bl foo.spec

with this proposal? -- EnricoScholz

Owners

LubomirKundrak