F39 Proposal: LIBFFI34 static trampolines (System-Wide Change)

[Aoife Moloney]

https://fedoraproject.org/wiki/Changes/LIBFFI34_static_trampolines

This document represents a proposed Change. As part of the Changes
process, proposals are publicly announced in order to receive
community feedback. This proposal will only be implemented if approved
by the Fedora Engineering Steering Committee.


== Summary ==
Libffi is currently configured to use dynamic trampolines, which
require some source of memory which is both writable and executable.
This is an obvious security issue, and selinux and system defaults
have made it more and more difficult to safely provide this memory to
libffi clients.  With this change, libffi will be configured to use
static trampolines, which do not require such memory, and will not
pose those security and administrative risks.

== Owner ==

* Name: [[User:djdelorie| DJ Delorie]]
* Email: dj(a)redhat.com



== Detailed Description ==
The change itself is simple - libffi will no longer be configured with
--disable-exec-static-tramp, which changes how closures are generated.
Closures, and libffi, are used in many interpreted languages.  There
are about 145 packages that depend, directly or indirectly, on libffi.
I ran the Mass Prebuilder.  Of those 145, 10 already FTBFS, and 1 saw
a new failure.

The Mass PreBuilder has indicated that cjs (Javascript Bindings for
Cinnamon) will fail to build with static trampolines.  We debugged
this a bit and noted that cjs's upstream seems to be behind the gjs
upstream (the gjs package builds OK) it tracks, and is missing at
least two closure-related changes (although simply adding those two
changes proved nontrivial).

== Feedback ==


== Benefit to Fedora ==

This change is needed to fully secure Fedora systems against attacks
that use self-modifying code.

The cjs build failure affects the Cinnamon spin.

== Scope ==
* Proposal owners:
The change is a single line in the spec file.

* Other developers:

The cjs package will need to be fixed to build with the new libffi.

Other libffi users will need to be aware of this change if they make
closure-related changes to their packages, but should not need to take
any actions at this time.

* Release engineering: [https://pagure.io/releng/issues #Releng issue number]

This change does not affect release engineering or any other packaging
issues.  No mass rebuild is required.

* Policies and guidelines: N/A (not needed for this Change)

* Trademark approval: N/A (not needed for this Change)


* Alignment with Community Initiatives: N/A

== Upgrade/compatibility impact ==

This change does not change ABI but that assumes clients are not
abusing the ABI; the static vs dynamic trampoline change is an
internal implementation.


== How To Test ==

Testing static trampolines is the same as testing libffi, with the
exception that the tests should pass despite having all writable
filesystems locked with selinux such that executable maps cannot be
requested.  A security context that does not allow the memfd_create()
syscall would further isolate this change.


== User Experience ==

Users will be able to follow latest security recommendations without
concern for whether interpreters will stop working.

== Dependencies ==

As noted above, the cjs package requires work before this change can
go through.  They have noted that they'll be rebasing to 5.8.x:
https://bugzilla.redhat.com/show_bug.cgi?id=2193287

Note that cjs has rebased to 5.6.1 in the interim, just to fix this
bug.  I've tested it and it works with static trampolines.


== Contingency Plan ==

Reverting the one line spec file change will revert this change, with
no changes needed elsewhere.  Not completing this change in time does
not negatively impact other package, or Fedora's ability to ship on
time.

* Contingency mechanism: We can revert the change at any time, without
needing to rebuild any other packages.

* Contingency deadline: Any time before shipping.

* Blocks release? No.


== Documentation ==

This is an internal change that should not need nor affect any documentation.

== Release Notes ==

This change should not require any release notes.


--
Aoife Moloney

Product Owner

Community Platform Engineering Team

Red Hat EMEA

Communications House

Cork Road

Waterford

[Zbigniew Jędrzejewski-Szmek]

On Mon, May 08, 2023 at 02:50:21PM +0100, Aoife Moloney wrote:
>  https://fedoraproject.org/wiki/Changes/LIBFFI34_static_trampolines
>  == Summary ==
>  Libffi is currently configured to use dynamic trampolines, which
>  require some source of memory which is both writable and executable.
>  This is an obvious security issue, and selinux and system defaults
>  have made it more and more difficult to safely provide this memory to
>  libffi clients.  With this change, libffi will be configured to use
>  static trampolines, which do not require such memory, and will not
>  pose those security and administrative risks. 
Sounds good. Can you post the list of affected packages here?

Zbyszek

[DJ Delorie]

Zbigniew Jędrzejewski-Szmek <zbyszek(a)in.waw.pl&gt; writes:
>  Sounds good. Can you post the list of affected packages here? 
According to mpb at least:

0ad 
Agda 
Io-language 
Macaulay2 
ShellCheck 
alex 
bench 
brainfuck 
bustle 
cab 
cabal-install 
cabal-rpm 
chromium 
cjs 
cpphs 
darcs 
dhall 
dhall-json 
dl-fedora 
ecl 
fbrnch 
firefox 
gambas3 
gforth 
ghc 
ghc-DAV 
ghc-HaXml 
ghc-aeson-pretty 
ghc-cheapskate 
ghc-clientsession 
ghc-criterion 
ghc-doctest 
ghc-hakyll 
ghc-hgettext 
ghc-highlighting-kate 
ghc-hjsmin 
ghc-hspec-discover 
ghc-pretty-show 
ghc-pretty-simple 
ghc-servant-server 
ghc-tidal 
ghc-vty 
ghc-wai-app-static 
ghc-wai-websockets 
ghc9.0 
ghc9.2 
ghc9.4 
ghc9.6 
ghcid 
git-annex 
git-repair 
gitit 
gjs 
glib2 
gnustep-base 
gobject-introspection 
gtk2hs-buildtools 
guile 
guile22 
guile30 
hadolint 
happy 
haskell-platform 
hedgewars 
hledger 
hledger-ui 
hledger-web 
hlint 
hscolour 
hwk 
icecat 
idris 
ispc 
jna 
koji-tool 
libffi 
libomp 
librep 
lldb 
llvm 
llvm11 
llvm12 
llvm13 
llvm14 
llvm15 
llvm7.0 
llvm8.0 
lsfrom 
lua-lgi 
micropython 
moarvm 
mozjs102 
mozjs78 
mozjs91 
newlisp 
ocaml-ctypes 
ormolu 
p11-kit 
pagure-cli 
pandoc 
patat 
perl-Alien-FFI 
perl-FFI-Platypus 
perl-Glib-Object-Introspection 
php 
pkgtreediff 
polyml 
pygobject2 
pygobject3 
pypy 
pypy3.9 
python-cffi 
python-pyopencl 
python-tox 
python2.7 
python3.10 
python3.11 
python3.12 
python3.6 
python3.7 
python3.8 
python3.9 
qt-creator 
racket 
rakudo 
rhbzquery 
rocm-runtime 
rpmbuild-order 
ruby 
rubygem-ffi 
seamonkey 
shake 
squeak-vm 
thunderbird 
unlambda 
wayland 
xmobar 
xmonad 
yosys

[Jens-Ulrik Petersen]

On Tue, May 9, 2023 at 4:35 AM DJ Delorie <dj(a)redhat.com&gt; wrote:

>  According to mpb at least:
>  
mpb?

The majority of those packages are maintained by me... so I can't say I
thrilled.
I thought ghc 9 was supposed to be okay with static trampolines?
Jens

[Jens-Ulrik Petersen]

On Tue, May 9, 2023 at 12:18 PM Jens-Ulrik Petersen <petersen(a)redhat.com&gt;
wrote:

>  On Tue, May 9, 2023 at 4:35 AM DJ Delorie <dj(a)redhat.com&gt;
> wrote:
>
> > According to mpb at least:
> >
>  
(Okay I found mpb <https://gitlab.com/fedora/packager-tools/mass-prebuild>)
Are the results available?


>  The majority of those packages are maintained by me... so I can't
> say I
>  thrilled.
>  I thought ghc 9 was supposed to be okay with static trampolines?
>

[DJ Delorie]

Jens-Ulrik Petersen <petersen(a)redhat.com&gt; writes:
>   According to mpb at least:
>
>  mpb? 
Mass PreBuilder https://gitlab.com/fedora/packager-tools/mass-prebuild

>  The majority of those packages are maintained by me... so I can't
> say I thrilled.
>  I thought ghc 9 was supposed to be okay with static trampolines? 
MPB uses BuildRequires to collect all packages that *might* be affected
by your change.  It builds all those packages with and without your
change, and lets you know what you broke.  In this case, one broke (cjs,
since fixed), ten didn't build before my change anyway[*], and the rest
were OK.  So if your package doesn't already FTBFS, you're good.

>  Are the results available? 
MPB uses COPR, so..

"before" builds:
https://copr.fedorainfracloud.org/coprs/djdelorie/libffi-3.4.4.checker/
"after" builds: 
https://copr.fedorainfracloud.org/coprs/djdelorie/libffi-3.4.4/

Note: I've done this before, so only look at the builds that are less
than a month old.  There should be an "after" build for all packages
that BuildRequires libffi, and a "before" package for each package that
failed to build "after".

[*] I'm still investigating these, but usually, it's not related to the
change you're making.

[Jens-Ulrik Petersen]

On Wed, May 10, 2023 at 3:13 AM DJ Delorie <dj(a)redhat.com&gt; wrote:

>  Jens-Ulrik Petersen <petersen(a)redhat.com&gt; writes:
>  MPB uses BuildRequires to collect all packages that *might* be affected
>  by your change.  It builds all those packages with and without your
>  change, and lets you know what you broke.  In this case, one broke (cjs,
>  since fixed), ten didn't build before my change anyway[*], and the rest
>  were OK.  So if your package doesn't already FTBFS, you're good.
>  
Okay thanks for the clarification and explanations.
(I got now that by "affected packages" you didn't mean "failed":)

"after" builds:
>  https://copr.fedorainfracloud.org/coprs/djdelorie/libffi-3.4.4/

Okay I was already aware of all the ghc failures: mostly due to the new
sphinx version in Rawhide
(which I had already fixed for ghc9.4). So it looks fine from my pov so
far, thanks.
I have just now pushed fixes for all ghc*, so can you try to rebuild them
again in your repo?
Jens

[DJ Delorie]

Jens-Ulrik Petersen <petersen(a)redhat.com&gt; writes:
>  I have just now pushed fixes for all ghc*, so can you try to rebuild
>  them again in your repo? 
That's a good question, to which I know not the answer.  Fred?  Can MPB
be told to retest a specific set of packages?  Or do I have to start
from scratch and/or do them manually?

[Jens-Ulrik Petersen]

On Thu, May 11, 2023 at 7:27 AM DJ Delorie <dj(a)redhat.com&gt; wrote:

>  Jens-Ulrik Petersen <petersen(a)redhat.com&gt; writes:
>  > I have just now pushed fixes for all ghc*, so can you try to rebuild
>  > them again in your repo?
>
>  That's a good question, to which I know not the answer.  Fred?  Can MPB
>  be told to retest a specific set of packages?  Or do I have to start
>  from scratch and/or do them manually?i18n Engineering - Display Systems 

Well, it is "only" ghc, ghc9.0, ghc9.2, and ghc9.6 (though ideally the
ghc-* and other Haskell packages should also be built against the rebuilt
ghc package).
You could also just build them manually in your djdelorie/libffi-3.4.4 copr
or in a separate copr if that is cleaner.

Jens

[DJ Delorie]

Jens-Ulrik Petersen <petersen(a)redhat.com&gt; writes:
>  I have just now pushed fixes for all ghc*, so can you try to rebuild
>  them again in your repo? 
All succeeded :-)

https://copr.fedorainfracloud.org/coprs/djdelorie/libffi-3.4.4/builds/

[Jarek Prokop]

On 5/8/23 15:50, Aoife Moloney wrote:
>  https://fedoraproject.org/wiki/Changes/LIBFFI34_static_trampolines
>
>  This document represents a proposed Change. As part of the Changes
>  process, proposals are publicly announced in order to receive
>  community feedback. This proposal will only be implemented if approved
>  by the Fedora Engineering Steering Committee.
>
>
>  == Summary ==
>  Libffi is currently configured to use dynamic trampolines, which
>  require some source of memory which is both writable and executable.
>  This is an obvious security issue, and selinux and system defaults
>  have made it more and more difficult to safely provide this memory to
>  libffi clients.  With this change, libffi will be configured to use
>  static trampolines, which do not require such memory, and will not
>  pose those security and administrative risks.
>
>  == Owner ==
>
>  * Name: [[User:djdelorie| DJ Delorie]]
>  * Email:[e-mail]
>
>
>
>  == Detailed Description ==
>  The change itself is simple - libffi will no longer be configured with
>  --disable-exec-static-tramp, which changes how closures are generated.
>  Closures, and libffi, are used in many interpreted languages.  There
>  are about 145 packages that depend, directly or indirectly, on libffi.
>  I ran the Mass Prebuilder.  Of those 145, 10 already FTBFS, and 1 saw
>  a new failure.
>
>  The Mass PreBuilder has indicated that cjs (Javascript Bindings for
>  Cinnamon) will fail to build with static trampolines.  We debugged
>  this a bit and noted that cjs's upstream seems to be behind the gjs
>  upstream (the gjs package builds OK) it tracks, and is missing at
>  least two closure-related changes (although simply adding those two
>  changes proved nontrivial). 
Are the libffi/rebuilt packages available anywhere for us to experiment 
with?

We have a reasonably reliable reproducer in Ruby [0] (also included in 
commit message [1]), but it is not executed as part of test suite,

Moreover, rebuild with current Ruby specfiles won't tell you much as we 
commented out the tests [2]
to have less flaky builds. I'd recommend uncommenting the lines and run 
5 to 10 builds (or just run any of the 2 reproducers).

Regards,
Jarek Prokop

[0] https://bugzilla.redhat.com/show_bug.cgi?id=2040380#c5
[1] 
https://src.fedoraproject.org/rpms/ruby/c/c2026da1750e6d0cc70c7370a084062...
[2] 
https://src.fedoraproject.org/rpms/ruby/blob/9e39fd242a58a5ab286d5da0d541...

[DJ Delorie]

Jarek Prokop <jprokop(a)redhat.com&gt; writes:
>  Are the libffi/rebuilt packages available anywhere for us to
>  experiment with? 
MPB uses COPR, so..

"before" builds:
https://copr.fedorainfracloud.org/coprs/djdelorie/libffi-3.4.4.checker/
"after" builds: 
https://copr.fedorainfracloud.org/coprs/djdelorie/libffi-3.4.4/

>  We have a reasonably reliable reproducer in Ruby [0] (also included
> in
>  commit message [1]), but it is not executed as part of test suite, 
Yes, fork-without-exec case is a known "that should never have worked"
case that only happens to work when your closure's backing store is also
forked, which file-based mappings are *not*.  You need either really old
(rwx mmap, which security disables) or really new (static trampolines,
which are r-x/rw- mmap'ed) libffi to support that.  Hopefully that means
your reproducers should not reproduce with the new libffi.

>  Moreover, rebuild with current Ruby specfiles won't tell you much
> as
>  we commented out the tests [2] to have less flaky builds. I'd
>  recommend uncommenting the lines and run 5 to 10 builds (or just run
>  any of the 2 reproducers). 
Well, if you comment out the tests, I have no way of knowing I broke
anything, so have to rely on posting Change Requests and letting you let
me know ;-)

Not saying you did anything wrong; if you have tests that pass or fail
depending on system configurations outside your control, it's difficult
to reliably test what you want to test.  I'm just saying that when you
disable tests, automated processes have no insight into those failures.

[Jarek Prokop]

On 5/9/23 21:27, DJ Delorie wrote:
>  Jarek Prokop<jprokop(a)redhat.com>  writes:
> > Are the libffi/rebuilt packages available anywhere for us to
> > experiment with?
>  MPB uses COPR, so..
>
>  "before"
> builds:https://copr.fedorainfracloud.org/coprs/djdelorie/libffi-3.4.4.che...
>  "after" builds:https://copr.fedorainfracloud.org/coprs/djdelorie/libffi-3.4.4/
Great! Thanks for the links, I've done manual testing with the reproducer:
~~~
require 'fiddle/closure'
require 'fiddle/struct'

Fiddle::Closure.new(Fiddle::TYPE_VOID, [])

fork { }

GC.start
~~~

And with the copr build of libffi 3.4.4-3 , Ruby indeed no longer 
crashes on this code.

> > We have a reasonably reliable reproducer in Ruby [0] (also included in
> > commit message [1]), but it is not executed as part of test suite,
>  Yes, fork-without-exec case is a known "that should never have worked"
>  case that only happens to work when your closure's backing store is also
>  forked, which file-based mappings are *not*.  You need either really old
>  (rwx mmap, which security disables) or really new (static trampolines,
>  which are r-x/rw- mmap'ed) libffi to support that.  Hopefully that means
>  your reproducers should not reproduce with the new libffi.
>
> > Moreover, rebuild with current Ruby specfiles won't tell you much as
> > we commented out the tests [2] to have less flaky builds. I'd
> > recommend uncommenting the lines and run 5 to 10 builds (or just run
> > any of the 2 reproducers).
>  Well, if you comment out the tests, I have no way of knowing I broke
>  anything, so have to rely on posting Change Requests and letting you let
>  me know ;-) 
And it is great to see :)
>  Not saying you did anything wrong; if you have tests that pass or
> fail
>  depending on system configurations outside your control, it's difficult
>  to reliably test what you want to test.  I'm just saying that when you
>  disable tests, automated processes have no insight into those failures. 
We are
aware of this downside :/, we worked with relevant upstream, but 
a proper fix on the side of rubygem-fiddle would require nontrivial rewrite.
(rubygem-ffi conversely has a closure pool that, AFAICT, prevents the 
issue altogether.)

This was a long-running issue (read: spanning a few Fedora releases) and 
doing a rebuild 10 times to have 1 not segfault and go through the rest 
of the pipeline just for a teeny version rebase got really tiring.

Thanks,
Jarek Prokop