Re: Needed: tooling to detect references to buggy */stable packages (was: Re: [PATCHES] ImageMagick security updates without grafting)

[Mark H Weaver]

Hi Maxime,

Maxime Devos <maximedevos@telenet.be> writes:

> On Sun, 2021-03-28 at 18:33 -0400, Mark H Weaver wrote:
>> Earlier, I wrote:
>> > One thing to be very careful about is to only use 'gtk-doc/stable',
>> > 'dblatex/stable', and 'imagemagick/stable' in native-inputs, and
>> > moreover to make sure that no references to these */stable packages
>> > remain in any package outputs.
>> > 
>> > Of course, if any package retains references to its 'native-inputs',
>> > that's always a bug, but I wouldn't be surprised if such bugs exist in
>> > Guix.  Such bugs might be relatively harmless now (except when
>> > cross-compiling), but they could become a security bug if a package
>> > retains a reference to 'imagemagick/stable'.
>
> It just occurred to me: could we automatically add all native-inputs
> to #:disallowed-references when cross-compiling?  This shouldn't break
> any packages, except possibly when cross-compiling.
>
> Or stronger, add all native-inputs to #:disallowed-references (unless
> they are also in inputs or propagated-inputs), even when compiling
> natively?

Yes, I think this is a good idea and worth considering.  However, we'd
need to consider the case where the same package is in both 'inputs' and
'native-inputs'.  When cross-compiling, it's no problem, because the
same package in those two fields will lead to distinct output paths,
since they are compiled for different systems.  However, when compiling
natively, the outputs of packages occurring in both 'inputs' and
'native-inputs' should *not* be implicitly included in
#:disallowed-references.

> Problems include:
> * I guess a world rebuild, as the derivations would be different.

Indeed!

> * In some places we have the following pattern:
>
>     (native-inputs
>      `(("autoconf" ,autoconf)
>        ("automake" ,automake)
>        ("pkg-config" ,pkg-config)
>        ,@(if (%current-target-system)
>              `(("guile" ,guile-3.0))   ;for 'guild compile' and 'guile-3.0.pc'
>              '())))
>     (inputs
>      `(("guile" ,guile-3.0)
>        ("lzlib" ,lzlib)))
>     (synopsis "Guile bindings to lzlib")
>
>   The (if (%current-target-system) ...) would need to be made unconditional.

I don't understand this.  Why would it need to be made unconditional?

> * I guess an option to disable this behaviour might be useful.
>
>> It occurs to me that we will need some tooling to ensure that no
>> references to these buggy "*/stable" packages end up in package outputs
>> that users actually use.  Otherwise, it is likely that sooner or later,
>> a runtime reference to one of these buggy packages will sneak in to our
>> systems.
>> 
>> An initial idea is that these "*/stable" packages could have a package
>> property (perhaps named something like 'build-time-only') that indicates
>> that references to its outputs should not occur within the outputs of
>> any other package that does not have that same property.
>
> Would this be (a) something enforced by the build process (using
> #:disallowed-references or #:allowed-references), or (b) a linter?

I would prefer option (a) above.
  
>> We'd also need to somehow ensure that users don't install these
>> 'build-time-only' packages directly, at least not without an additional
>> option (e.g. --force-unsafe-build-time-only) to override it.
>
> What about a developer running "guix environment eom"?  IIUC, this would
> make the developer vulnerable (at least, once I've gotten around replacing
> the 'gtk-doc' input with 'gtk-doc/stable'), so it might make sense to
> replace /stable -> unstable packages here.
>
> However, now the developer ends up with a different set of packages than
> wil be seen in the build environment ...

That's an excellent point, for which I don't have any good answer.
I'm open to suggestions.

> Is this something you will be writing "guix lint" checkers (or other
> checkers) for yourself?

At the present time, I'm more inclined to add machinery to automatically
add _implicit_ #:disallowed-references, to enforce this checking at
package build time.  This would require rebuilding everything that
depends on a '*/stable' package, which means that this kind of tooling
could not be applied directly to 'master', but would need to go through
'staging'.

What do you think?

    Thanks,
      Mark