[PATCH 0/2] topic: meson: add more compiler hardening flags

[PATCH 0/2] topic: meson: add more compiler hardening flags

[Daniel P. Berrangé]

This brings more compiler hardening flags to the default QEMU
build process. The proposed flags have already been adopted by
default in the kernel build process. At some point it is hoped
that distros might enable them globally, as they've done in
the past with things like _FORTIFY_SOURCE. Meanwhile they are
easy things to enable in QEMU which have negligible cost and
clear benefits to hardening. Considering QEMU shows no signs
of stoppping the flow of guest triggerable CVEs, investing in
hardening is worthwhile. See the respective commit messages
for details

I also tested enabling -ftrapv, to change signed integer
overflow from wrapping, to trapping instead. This exposed a
bug in the string-input-visitor which overflows when parsing
ranges, and exposed the test-int128 code as (harmlessly)
overflowing during its testing. Both can be fixed, but I'm
not entirely sure whether -ftrapv is viable or not. I was
wondering about TCG and whether it has a need to intentionally
allow integer overflow for any of its instruction emulation
requirements ?  "make check" passes qtest but that's not
sufficiently broad to make me comfortable. Thus I've not
included an -ftrapv patch here.

Daniel P. Berrangé (2):
  meson: mitigate against ROP exploits with -fzero-call-used-regs
  meson: mitigate against use of uninitialize stack for exploits

 meson.build | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

-- 
2.41.0

[PATCH 1/2] meson: mitigate against ROP exploits with -fzero-call-used-regs

[Daniel P. Berrangé]

To quote wikipedia:

  "Return-oriented programming (ROP) is a computer security exploit
   technique that allows an attacker to execute code in the presence
   of security defenses such as executable space protection and code
   signing.

   In this technique, an attacker gains control of the call stack to
   hijack program control flow and then executes carefully chosen
   machine instruction sequences that are already present in the
   machine's memory, called "gadgets". Each gadget typically ends in
   a return instruction and is located in a subroutine within the
   existing program and/or shared library code. Chained together,
   these gadgets allow an attacker to perform arbitrary operations
   on a machine employing defenses that thwart simpler attacks."

QEMU is by no means perfect with an ever growing set of CVEs from
flawed hardware device emulation, which could potentially be
exploited using ROP techniques.

Since GCC 11 there has been a compiler option that can mitigate
against this exploit technique:

    -fzero-call-user-regs

To understand it refer to these two resources:

   https://www.jerkeby.se/newsletter/posts/rop-reduction-zero-call-user-regs/
   https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552262.html

I used two programs to scan qemu-system-x86_64 for ROP gadgets:

  https://github.com/0vercl0k/rp
  https://github.com/JonathanSalwan/ROPgadget

When asked to find 8 byte gadgets, the 'rp' tool reports:

  A total of 440278 gadgets found.
  You decided to keep only the unique ones, 156143 unique gadgets found.

While the ROPgadget tool reports:

  Unique gadgets found: 353122

With the --ropchain argument, the latter attempts to use the found
gadgets to product a chain that can execute arbitrary syscalls. With
current QEMU it succeeds in this task, which is an undesirable
situation.

With QEMU modified to use -fzero-call-user-regs=used-gpr the 'rp' tool
reports

  A total of 528991 gadgets found.
  You decided to keep only the unique ones, 121128 unique gadgets found.

This is 22% fewer unique gadgets

While the ROPgadget tool reports:

  Unique gadgets found: 328605

This is 7% fewer unique gadgets. Crucially though, despite this more
modest reduction, the ROPgadget tool is no longer able to identify a
chain of gadgets for executing arbitrary syscalls. It fails at the
very first step, unable to find gadgets for populating registers for
a future syscall. Having said that, more advanced tools do still
manage to put together a viable ROP chain.

Also this only takes into account QEMU code. QEMU links to many 3rd
party shared libraries and ideally all of them would be compiled with
this same hardening. That becomes a distro policy question though.

In terms of performance impact, TCG was used as an evaluation test
case. We're not interested in protecting TCG since it isn't designed
to provide a security barrier, but it is performance sensitive code,
so useful as a guide to how other areas of QEMU might be impacted.
With the -fzero-call-user-regs=used-gpr argument present, using the
real world test of booting a linux kernel and having init immediately
poweroff, there is a ~1% slow down in performance under TCG. The QEMU
binary size also grows by approximately 1%.

By comparison, using the more aggressive -fzero-call-user-regs=all,
results in a slowdown of over 25% in TCG, which is clearly not an
acceptable impact, and a binary size increase of 5%.

Considering that 'used-gpr' succesfully stopped ROPgadget assembling
a chain, this more targetted protection is a justifiable hardening
/ performance tradeoff.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 meson.build | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/meson.build b/meson.build
index 20ceeb8158..2003ca1ba4 100644
--- a/meson.build
+++ b/meson.build
@@ -435,6 +435,17 @@ if get_option('fuzzing')
   endif
 endif
 
+# Check further flags that make QEMU more robust against malicious parties
+
+hardening_flags = [
+    # Zero out registers used during a function call
+    # upon its return. This makes it harder to assemble
+    # ROP gadgets into something usable
+    '-fzero-call-used-regs=used-gpr',
+]
+
+qemu_common_flags += cc.get_supported_arguments(hardening_flags)
+
 add_global_arguments(qemu_common_flags, native: false, language: all_languages)
 add_global_link_arguments(qemu_ldflags, native: false, language: all_languages)
 
-- 
2.41.0

Re: [PATCH 1/2] meson: mitigate against ROP exploits with -fzero-call-used-regs

[Thomas Huth]

On 05/10/2023 19.38, Daniel P. Berrangé wrote:
> To quote wikipedia:
> 
>    "Return-oriented programming (ROP) is a computer security exploit
>     technique that allows an attacker to execute code in the presence
>     of security defenses such as executable space protection and code
>     signing.
> 
>     In this technique, an attacker gains control of the call stack to
>     hijack program control flow and then executes carefully chosen
>     machine instruction sequences that are already present in the
>     machine's memory, called "gadgets". Each gadget typically ends in
>     a return instruction and is located in a subroutine within the
>     existing program and/or shared library code. Chained together,
>     these gadgets allow an attacker to perform arbitrary operations
>     on a machine employing defenses that thwart simpler attacks."
> 
> QEMU is by no means perfect with an ever growing set of CVEs from
> flawed hardware device emulation, which could potentially be
> exploited using ROP techniques.
> 
> Since GCC 11 there has been a compiler option that can mitigate
> against this exploit technique:
> 
>      -fzero-call-user-regs
> 
> To understand it refer to these two resources:
> 
>     https://www.jerkeby.se/newsletter/posts/rop-reduction-zero-call-user-regs/
>     https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552262.html
> 
> I used two programs to scan qemu-system-x86_64 for ROP gadgets:
> 
>    https://github.com/0vercl0k/rp
>    https://github.com/JonathanSalwan/ROPgadget
> 
> When asked to find 8 byte gadgets, the 'rp' tool reports:
> 
>    A total of 440278 gadgets found.
>    You decided to keep only the unique ones, 156143 unique gadgets found.
> 
> While the ROPgadget tool reports:
> 
>    Unique gadgets found: 353122
> 
> With the --ropchain argument, the latter attempts to use the found
> gadgets to product a chain that can execute arbitrary syscalls. With
> current QEMU it succeeds in this task, which is an undesirable
> situation.
> 
> With QEMU modified to use -fzero-call-user-regs=used-gpr the 'rp' tool
> reports
> 
>    A total of 528991 gadgets found.
>    You decided to keep only the unique ones, 121128 unique gadgets found.
> 
> This is 22% fewer unique gadgets
> 
> While the ROPgadget tool reports:
> 
>    Unique gadgets found: 328605
> 
> This is 7% fewer unique gadgets. Crucially though, despite this more
> modest reduction, the ROPgadget tool is no longer able to identify a
> chain of gadgets for executing arbitrary syscalls. It fails at the
> very first step, unable to find gadgets for populating registers for
> a future syscall. Having said that, more advanced tools do still
> manage to put together a viable ROP chain.
> 
> Also this only takes into account QEMU code. QEMU links to many 3rd
> party shared libraries and ideally all of them would be compiled with
> this same hardening. That becomes a distro policy question though.
> 
> In terms of performance impact, TCG was used as an evaluation test
> case. We're not interested in protecting TCG since it isn't designed
> to provide a security barrier, but it is performance sensitive code,
> so useful as a guide to how other areas of QEMU might be impacted.
> With the -fzero-call-user-regs=used-gpr argument present, using the
> real world test of booting a linux kernel and having init immediately
> poweroff, there is a ~1% slow down in performance under TCG. The QEMU
> binary size also grows by approximately 1%.
> 
> By comparison, using the more aggressive -fzero-call-user-regs=all,
> results in a slowdown of over 25% in TCG, which is clearly not an
> acceptable impact, and a binary size increase of 5%.
> 
> Considering that 'used-gpr' succesfully stopped ROPgadget assembling
> a chain, this more targetted protection is a justifiable hardening
> / performance tradeoff.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   meson.build | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/meson.build b/meson.build
> index 20ceeb8158..2003ca1ba4 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -435,6 +435,17 @@ if get_option('fuzzing')
>     endif
>   endif
>   
> +# Check further flags that make QEMU more robust against malicious parties
> +
> +hardening_flags = [
> +    # Zero out registers used during a function call
> +    # upon its return. This makes it harder to assemble
> +    # ROP gadgets into something usable
> +    '-fzero-call-used-regs=used-gpr',
> +]
> +
> +qemu_common_flags += cc.get_supported_arguments(hardening_flags)

Linux kernel uses the same flag and talks about similar performance costs:

  https://github.com/torvalds/linux/commit/a82adfd5c7cb4b

So I think this should be fine fine to be used in QEMU, too.

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 2/2] meson: mitigate against use of uninitialize stack for exploits

[Daniel P. Berrangé]

When variables are used without being initialized, there is potential
to take advantage of data that was pre-existing on the stack from an
earlier call, to drive an exploit.

It is good practice to always initialize variables, and the compiler
can warn about flaws when -Wuninitialized is present. This warning,
however, is by no means foolproof with its output varying depending
on compiler version and which optimizations are enabled.

The -ftrivial-auto-var-init option can be used to tell the compiler
to always initialize all variables. This increases the security and
predictability of the program, closing off certain attack vectors,
reducing the risk of unsafe memory disclosure.

While the option takes several possible values, using 'zero' is
considered to be the  option that is likely to lead to semantically
correct or safe behaviour[1]. eg sizes/indexes are not likely to
lead to out-of-bounds accesses when initialized to zero. Pointers
are less likely to point something useful if initialized to zero.

Even with -ftrivial-auto-var-init=zero set, GCC will still issue
warnings with -Wuninitialized if it discovers a problem, so we are
not loosing diagnostics for developers, just hardening runtime
behaviour and making QEMU behave more predictably in case of hitting
bad codepaths.

[1] https://lists.llvm.org/pipermail/cfe-dev/2020-April/065221.html
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 meson.build | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/meson.build b/meson.build
index 2003ca1ba4..19faea8d30 100644
--- a/meson.build
+++ b/meson.build
@@ -442,6 +442,11 @@ hardening_flags = [
     # upon its return. This makes it harder to assemble
     # ROP gadgets into something usable
     '-fzero-call-used-regs=used-gpr',
+
+    # Initialize all stack variables to zero. This makes
+    # it harder to take advantage of uninitialized stack
+    # data to drive exploits
+    '-ftrivial-var-auto-init=zero',
 ]
 
 qemu_common_flags += cc.get_supported_arguments(hardening_flags)
-- 
2.41.0

Re: [PATCH 2/2] meson: mitigate against use of uninitialize stack for exploits

[Thomas Huth]

On 05/10/2023 19.38, Daniel P. Berrangé wrote:
> When variables are used without being initialized, there is potential
> to take advantage of data that was pre-existing on the stack from an
> earlier call, to drive an exploit.
> 
> It is good practice to always initialize variables, and the compiler
> can warn about flaws when -Wuninitialized is present. This warning,
> however, is by no means foolproof with its output varying depending
> on compiler version and which optimizations are enabled.
> 
> The -ftrivial-auto-var-init option can be used to tell the compiler
> to always initialize all variables. This increases the security and
> predictability of the program, closing off certain attack vectors,
> reducing the risk of unsafe memory disclosure.
> 
> While the option takes several possible values, using 'zero' is
> considered to be the  option that is likely to lead to semantically
> correct or safe behaviour[1]. eg sizes/indexes are not likely to
> lead to out-of-bounds accesses when initialized to zero. Pointers
> are less likely to point something useful if initialized to zero.
> 
> Even with -ftrivial-auto-var-init=zero set, GCC will still issue
> warnings with -Wuninitialized if it discovers a problem, so we are
> not loosing diagnostics for developers, just hardening runtime
> behaviour and making QEMU behave more predictably in case of hitting
> bad codepaths.
> 
> [1] https://lists.llvm.org/pipermail/cfe-dev/2020-April/065221.html
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   meson.build | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/meson.build b/meson.build
> index 2003ca1ba4..19faea8d30 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -442,6 +442,11 @@ hardening_flags = [
>       # upon its return. This makes it harder to assemble
>       # ROP gadgets into something usable
>       '-fzero-call-used-regs=used-gpr',
> +
> +    # Initialize all stack variables to zero. This makes
> +    # it harder to take advantage of uninitialized stack
> +    # data to drive exploits
> +    '-ftrivial-var-auto-init=zero',
>   ]

I was a little bit torn about using =zero when I first read your patch, but 
after looking at [1], I tend now also tend to agree that =zero is likely the 
best choice. So from my side:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 2/2] meson: mitigate against use of uninitialize stack for exploits

[Thomas Huth]

On 09/10/2023 09.44, Thomas Huth wrote:
> On 05/10/2023 19.38, Daniel P. Berrangé wrote:
>> When variables are used without being initialized, there is potential
>> to take advantage of data that was pre-existing on the stack from an
>> earlier call, to drive an exploit.
>>
>> It is good practice to always initialize variables, and the compiler
>> can warn about flaws when -Wuninitialized is present. This warning,
>> however, is by no means foolproof with its output varying depending
>> on compiler version and which optimizations are enabled.
>>
>> The -ftrivial-auto-var-init option can be used to tell the compiler
>> to always initialize all variables. This increases the security and
>> predictability of the program, closing off certain attack vectors,
>> reducing the risk of unsafe memory disclosure.
>>
>> While the option takes several possible values, using 'zero' is
>> considered to be the  option that is likely to lead to semantically
>> correct or safe behaviour[1]. eg sizes/indexes are not likely to
>> lead to out-of-bounds accesses when initialized to zero. Pointers
>> are less likely to point something useful if initialized to zero.
>>
>> Even with -ftrivial-auto-var-init=zero set, GCC will still issue
...
>> +    '-ftrivial-var-auto-init=zero',
>>   ]

There is something fishy here: In the commit description, you write about 
"-ftrivial-auto-var-init" but in the code you use "-ftrivial-var-auto-init" 
... that looks wrong to me, please fix!

> I was a little bit torn about using =zero when I first read your patch, but 
> after looking at [1], I tend now also tend to agree that =zero is likely the 
> best choice.

Thinking about this twice: What about using -ftrivial-var-auto-init=pattern 
for --enable-debug builds, and only use the "zero" init for non-debug 
builds? ... that would prevent that people blindly rely on this "language 
extension".

  Thomas

Re: [PATCH 2/2] meson: mitigate against use of uninitialize stack for exploits

[Daniel P. Berrangé]

On Mon, Oct 09, 2023 at 12:15:17PM +0200, Thomas Huth wrote:
> On 09/10/2023 09.44, Thomas Huth wrote:
> > On 05/10/2023 19.38, Daniel P. Berrangé wrote:
> > > When variables are used without being initialized, there is potential
> > > to take advantage of data that was pre-existing on the stack from an
> > > earlier call, to drive an exploit.
> > > 
> > > It is good practice to always initialize variables, and the compiler
> > > can warn about flaws when -Wuninitialized is present. This warning,
> > > however, is by no means foolproof with its output varying depending
> > > on compiler version and which optimizations are enabled.
> > > 
> > > The -ftrivial-auto-var-init option can be used to tell the compiler
> > > to always initialize all variables. This increases the security and
> > > predictability of the program, closing off certain attack vectors,
> > > reducing the risk of unsafe memory disclosure.
> > > 
> > > While the option takes several possible values, using 'zero' is
> > > considered to be the  option that is likely to lead to semantically
> > > correct or safe behaviour[1]. eg sizes/indexes are not likely to
> > > lead to out-of-bounds accesses when initialized to zero. Pointers
> > > are less likely to point something useful if initialized to zero.
> > > 
> > > Even with -ftrivial-auto-var-init=zero set, GCC will still issue
> ...
> > > +    '-ftrivial-var-auto-init=zero',
> > >   ]
> 
> There is something fishy here: In the commit description, you write about
> "-ftrivial-auto-var-init" but in the code you use "-ftrivial-var-auto-init"
> ... that looks wrong to me, please fix!

Face palm, -ftrivial-auto-var-init is the correct one.

> > I was a little bit torn about using =zero when I first read your patch,
> > but after looking at [1], I tend now also tend to agree that =zero is
> > likely the best choice.
> 
> Thinking about this twice: What about using -ftrivial-var-auto-init=pattern
> for --enable-debug builds, and only use the "zero" init for non-debug
> builds? ... that would prevent that people blindly rely on this "language
> extension".

We can't blindly rely on it, because -Wuninitialized is still going to
do static analysis and warn in most cases, which can't be ignored when
-Werror is set.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/2] topic: meson: add more compiler hardening flags

[Thomas Huth]

On 05/10/2023 19.38, Daniel P. Berrangé wrote:
...
> 
> I also tested enabling -ftrapv, to change signed integer
> overflow from wrapping, to trapping instead. This exposed a
> bug in the string-input-visitor which overflows when parsing
> ranges, and exposed the test-int128 code as (harmlessly)
> overflowing during its testing. Both can be fixed, but I'm
> not entirely sure whether -ftrapv is viable or not. I was
> wondering about TCG and whether it has a need to intentionally
> allow integer overflow for any of its instruction emulation
> requirements ?
I'm not an expert when it comes to this question, but as far as I 
understood, we are using -fwrapv (with "w", not "t") on purpose, see 
meson.build:

# We use -fwrapv to tell the compiler that we require a C dialect where
# left shift of signed integers is well defined and has the expected
# 2s-complement style results. (Both clang and gcc agree that it
# provides these semantics.)

And according to the man-page of gcc:

  The options -ftrapv and -fwrapv override each other,
  so using -ftrapv -fwrapv on the command-line results
  in -fwrapv being effective.

If I got that right, this means you cannot use -ftrapv with QEMU.

  Thomas

Re: [PATCH 0/2] topic: meson: add more compiler hardening flags

[Daniel P. Berrangé]

On Mon, Oct 09, 2023 at 09:21:01AM +0200, Thomas Huth wrote:
> On 05/10/2023 19.38, Daniel P. Berrangé wrote:
> ...
> > 
> > I also tested enabling -ftrapv, to change signed integer
> > overflow from wrapping, to trapping instead. This exposed a
> > bug in the string-input-visitor which overflows when parsing
> > ranges, and exposed the test-int128 code as (harmlessly)
> > overflowing during its testing. Both can be fixed, but I'm
> > not entirely sure whether -ftrapv is viable or not. I was
> > wondering about TCG and whether it has a need to intentionally
> > allow integer overflow for any of its instruction emulation
> > requirements ?
> I'm not an expert when it comes to this question, but as far as I
> understood, we are using -fwrapv (with "w", not "t") on purpose, see
> meson.build:
> 
> # We use -fwrapv to tell the compiler that we require a C dialect where
> # left shift of signed integers is well defined and has the expected
> # 2s-complement style results. (Both clang and gcc agree that it
> # provides these semantics.)
> 
> And according to the man-page of gcc:
> 
>  The options -ftrapv and -fwrapv override each other,
>  so using -ftrapv -fwrapv on the command-line results
>  in -fwrapv being effective.
> 
> If I got that right, this means you cannot use -ftrapv with QEMU.

Opps, I didn't notice we had -fwrapv in our flags, that is clearly
mutually exclusive with -ftrapv, so nothing further to do here.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|