Re: [Quilt-dev] setup command

[Andreas Grünbacher]

Am Mo., 12. Feb. 2024 um 09:21 Uhr schrieb Jean Delvare <jdelvare@suse.de>:
> On Mon, 12 Feb 2024 00:02:42 +0100, Andreas Grünbacher wrote:
> > Am So., 11. Feb. 2024 um 23:11 Uhr schrieb Jean Delvare <jdelvare@suse.de>:
> > > On Sat, 10 Feb 2024 22:24:48 +0100, Andreas Grünbacher wrote:
> > > > I'm all for better documentation. But the setup command is severely
> > > > broken right now. Executing the %prep section in the current working
> > > > directory is just insane. (...)
> > >
> > > Because...?
> >
> > Because it may leave additional artifacts around that have nothing to
> > do with what the setup command is expected to do, and it may destroy
> > existing files. The code in the %prep section doesn't expect to be run
> > in a directory that already contains random stuff, and it's allowed to
> > do arbitrary things in that directory. Isn't that obvious enough?
>
> Doesn't seem to be a problem in practice, and if it is, then again you
> can use option -d to work in a dedicated, initially empty directory.

You have created a time bomb there. Sooner or later, it will go off
and someone will lose data, and it will be entirely your fault. Then
it will very much be a practical problem.

The old setup command was designed not to overwrite arbitrary data in
the working directory, and that didn't happen by accident. The
current, broken implementation totally ignores that. This is
completely unacceptable. The setup command needs to be safe by
default. I've shown how the setup command can be made safe again
without slowing things down too much; see the tar-wrapper-experiment
branch.

The fact that the -d option can be used to tell quilt to work in a
different directory doesn't change anything. I don't want quilt to run
arbitrary code in the current directory, and I don't want it to run
arbitrary code in a different directory, either.

> The code in the %prep section can do anything the running user can do,
> including modifying or deleting files outside of both the current
> directory and the target directory, so you simply shouldn't run "quilt
> setup" (or rpmbuild for that matter) on a spec file which you do not
> trust. This is actually something we should document.

Yes, we should document that this can be a security risk. But note
that we're talking about a malicious spec file here, and without
creating an isolated build environment and running %prep inside there,
we cannot really protect against that. What I'm mostly complaining
about is that right now, a perfectly valid and reasonable %prep
section can destroy data because the %prep code expects to be run in
an empty directory but "quit setup" runs it in a directory that may
contain relevant data. There is absolutely no excuse for that.

> Note that even the old, slow setup method can overwrite files in the
> current directory, if the unpacked archive contains files at the root
> level. And it can also create, modify or delete any file in the home
> directory of the user, for example, due to the insecure design of rpm
> spec files.

Yes, malicious spec files again. That's a risk, and we should document
that the risk exists, but we cannot reasonably protect against that.

> > > Note that you can always use option -d if you want to execute the %prep
> > > section in a different directory. The reason why it defaults to the
> > > current directory is because this is where the original (slow)
> > > implementation was ultimately preparing the working tree, and I wanted
> > > to make the transition to the new (fast) method as smooth as possible.
> >
> > The original implementation only expanded the tarball(s) in the
> > working directory, it didn't execute random code there. That's a very
> > significant difference.
>
> Indeed, and one effect of this was that the resulting working tree
> wasn't always what people expected, because the effects of some
> commands run in the temporary directory were never reflected to the
> working tree. This is a limitation my new implementation lifted.

This is complete and utter nonsense. Let's assume that someone is
trying to work on a package, but without quilt as the tool of choice.
What do you think they will do? Unpack the tarballs, apply the
patches, fix things up, and regenerate whatever patches need to be
changed. In your bizarre world view, this will no longer work and the
patches will no longer apply --- because they were generated against a
modified source tree, not the original source tree from the tarball.
In order to work on those patches, people will have to modify the
source tree first.

You are forcing people into a nonsensical workflow that will make
things extremely hard for others. Not only that, but the benefit is
exactly zero; it's only an ill side effect of the current, broken
implementation.

> > > (Actually, there was a report about "quilt setup" being unsafe:
> > >  https://savannah.nongnu.org/bugs/?56969
> > >  However my understanding is that this is caused by rpmbuild allowing
> > >  arbitrary commands in spec files, and not related to slow mode vs.
> > >  fast mode implementation details.)
> >
> > Protecting against malicious code in %prep would indeed be much harder
> > ... it would require creating an environment that contains all the
> > tools that %prep expects, and that can differ from package to package.
> > I don't think we can do that.
>
> The %prep section of spec files can contain anything. If you don't
> trust a given spec file, don't run "quilt setup" on it, period. I see
> no point in adding complexity and slowing things down to deal with
> hypothetical situations when the whole thing is inherently unsafe in
> the first place.

See above about the difference between handling perfectly valid and
reasonable %prep sections well vs. protecting against malicious code.

Thanks,
Andreas