Bug 29072 - ld silently make the program stack area executable if nested function is used
Summary: ld silently make the program stack area executable if nested function is used
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Nick Clifton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-04-19 09:40 UTC by Rui Ueyama
Modified: 2022-04-28 04:14 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2022-04-19 00:00:00


Attachments
Proposed Patch (8.83 KB, patch)
2022-04-20 12:37 UTC, Nick Clifton
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rui Ueyama 2022-04-19 09:40:56 UTC
GCC's nested function (https://gcc.gnu.org/onlinedocs/gcc/Nested-Functions.html) depends on the executable stack, so the feature has a huge implication on a generated program's security. Essentially, using the nested function feature makes the entire program vulnerable to a simple buffer overflow attack.

GNU ld makes the stack area executable if at least one object file contains a `.note.GNU-stack` section with `SHF_EXECINSTR`. GCC emits such section if the nested function feature is used.

I think this surprises users. If you link against an object file that contains such note section, the program's entire executable becomes executable without any notice. Frankly, this looks very dangerous to me.

Can we make a change to GNU ld so that it at least print out a warning message for the executable stack? If a user explicitly requests the executable stack by passing `-z execstack`, then we can mute the warning.
Comment 1 Andreas Schwab 2022-04-19 10:47:18 UTC
Nested functions by itself don't require executable stacks, only when the address of such a function is passed outside its scope.
Comment 2 Andreas Schwab 2022-04-19 11:20:18 UTC
More often, it's the lack of the stack note in an assembler source that inadvertently makes the stack executable, on platforms where it is the default.
Comment 3 Rui Ueyama 2022-04-19 12:42:00 UTC
Right. Unless you know the default behavior of GNU ld, it is very hard to foresee that adding a benign assembler file to your project could make it significantly vulnerable to the traditional stack overflow attack. It can be used for the supply chain attack. If you can sneak in an assembly file, the last thing to create a remote vulnerability is to find a buffer overflow bug.
Comment 4 Nick Clifton 2022-04-19 13:22:36 UTC
Working on a fix.  It adds two new warnings to the linker:

  warning: enabling an executable stack because of an executable .note.GNU-stack section in <object>

and:

  warning: enabling an executable stack because <object> does not have a .note.GNU-stack section

Testing is taking longer than expected because that second warning is triggered by a lot of tests in the linker testsuite....
Comment 5 Ian Lance Taylor 2022-04-19 15:49:50 UTC
I want to mention that the gold linker supports a --warn-execstack (and --no-warn-execstack) option.  It would be helpful if GNU ld used the same spelling.
Comment 6 Nick Clifton 2022-04-20 12:37:28 UTC
Created attachment 14072 [details]
Proposed Patch
Comment 7 cvs-commit@gcc.gnu.org 2022-04-20 12:39:12 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=65daf5bed68f3e792e80f7c9a12871fd71da32a2

commit 65daf5bed68f3e792e80f7c9a12871fd71da32a2
Author: Nick Clifton <nickc@redhat.com>
Date:   Wed Apr 20 13:37:51 2022 +0100

    Add linker warning for when it creates an executable stack.
    
       PR 29072
Comment 8 Nick Clifton 2022-04-20 12:44:37 UTC
Hi Guys,

  Right - I have applied a patch (also uploaded to this PR) which adds 3 new
  warnings to the linker:

warning: <obj>: requires executable stack (because the .note.GNU-stack section is executable)

warning: <obj>: missing .note.GNU-stack section implies executable stack

warning: enabling an executable stack because of -z execstack command line option

  The first two are enabled by default, but the third one is only generated
  if the new --warn-execstack command line option is used.  The first two
  can also be suppressed by using the --no-warn-execstack option.

  The wording and option names were chosen to be similar to the ones used/
  generated by the GOLD linker.  (Although GOLD does not generate the third
  type of warning message).

Cheers
  Nick
Comment 9 Fangrui Song 2022-04-21 06:48:02 UTC
I think in 2022 we should consider this https://www.airs.com/blog/archives/518

> These days we could probably change the default: we could probably say that if an object file does not have a .note.GNU-stack section, then it does not require an executable stack.

Only give an executable stack if -z execstack is specified. This is ld.lld's choice and (until one day ago mold's choice). Taking the address of a nested function is so rare that I am unsure having an on-demand state is useful.

FWIW Clang doesn't supported GCC nested functions.
Comment 10 Nick Clifton 2022-04-21 11:30:19 UTC
(In reply to Fangrui Song from comment #9)
 
> > These days we could probably change the default: we could probably say that if an object file does not have a .note.GNU-stack section, then it does not require an executable stack.
 
I agree with the idea.  I have posted a RFC to the binutils list to see if anyone objects to the proposed change:

https://sourceware.org/pipermail/binutils/2022-April/120476.html
Comment 11 cvs-commit@gcc.gnu.org 2022-04-25 11:53:15 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=0d38576a34ec64a1b4500c9277a8e9d0f07e6774

commit 0d38576a34ec64a1b4500c9277a8e9d0f07e6774
Author: Nick Clifton <nickc@redhat.com>
Date:   Mon Apr 25 12:51:31 2022 +0100

    Emit a note warning the user that creating an executable stack because of a missing .note.GNU-stack section is deprecated.
    
            PR 29072
    bfd     * elflink.c (bfd_elf_size_dynamic_sections): Display a note to the
            user that the current ehaviour of creating an executable stack
            because of a missing .note.GNU-stack section is deprecated and
            will be changed in a future release.
    
    binutils* testsuite/lib/binutils-common.exp (prune_warnings_extra): Filter
            out notes about the executable stacjk behaviour beign deprecated.
    
    ld      * testsuite/ld-elf/pr29072.b.warn: Update to include the note
            about the linker's behaviour being depreccated.
Comment 12 H.J. Lu 2022-04-25 14:51:37 UTC
I don't think we should issue a warning in this case:

[hjl@gnu-cfl-1 ld]$ cat /tmp/x.s 
	.text
	.globl	main
main:
	.nops 1
	.section	.note.GNU-stack,"x",@progbits
[hjl@gnu-cfl-1 ld]$ gcc -c /tmp/x.s
[hjl@gnu-cfl-1 ld]$ ./ld-new -e main x.o
./ld-new: warning: x.o: requires executable stack (because the .note.GNU-stack section is executable)
[hjl@gnu-cfl-1 ld]$
Comment 13 H.J. Lu 2022-04-25 15:12:31 UTC
(In reply to H.J. Lu from comment #12)
> I don't think we should issue a warning in this case:
> 
> [hjl@gnu-cfl-1 ld]$ cat /tmp/x.s 
> 	.text
> 	.globl	main
> main:
> 	.nops 1
> 	.section	.note.GNU-stack,"x",@progbits
> [hjl@gnu-cfl-1 ld]$ gcc -c /tmp/x.s
> [hjl@gnu-cfl-1 ld]$ ./ld-new -e main x.o
> ./ld-new: warning: x.o: requires executable stack (because the
> .note.GNU-stack section is executable)
> [hjl@gnu-cfl-1 ld]$

Shouldn't compiler issue the warning instead?
Comment 14 Nick Clifton 2022-04-25 15:17:04 UTC
 
(In reply to H.J. Lu from comment #12)
> I don't think we should issue a warning in this case:

Why not ?  The original point of this PR was that an application can
gain an executable stack without the programmer being aware of it, simply
because an object file in the link requests one.  Providing a warning,
along with the name of the object file to blame, will allow the builder
to decide if they are OK with this, possibly unexpected, behaviour.
  

> Shouldn't compiler issue the warning instead?

Maybe - that is up to the compiler designers.  But the purpose of this
warning is to let application builders know that their app now has a
major security risk, and it may be because of an object file in a library
and nothing at all to do with files that they themselves maintain.
Comment 15 H.J. Lu 2022-04-25 17:26:50 UTC
Is the goal to remove nested functions?
Comment 16 Nick Clifton 2022-04-26 12:11:20 UTC
(In reply to H.J. Lu from comment #15)
> Is the goal to remove nested functions?

No - the goal is to improve the security of programs by letting their builders know that they have a vulnerability.  They *may* chose to address the vulnerability by removing nested functions from their code - if that was the cause - but they may also decide that the vulnerability is acceptable and instead add --no-warn-execstack to the linker command line.  Or just ignore the warning.

The point of the warning is that it gives program builders a prompt to decide what is best for them.  By informing them of the potential security vulnerability - something that they may not have realised was happening to their program - they then have a reason to perform a security review of their code, and can decide what to do.

As for builders who are unaware of the risks of executable stacks and the dangers of nested functions - and hence will be confused by this new warning - I intend to write a blog about the problem and its possible solutions.  It is my hope that a web search will turn up this blog, and so they will be able to find some advice on what to do.
Comment 17 H.J. Lu 2022-04-26 16:25:36 UTC
(In reply to Nick Clifton from comment #16)
> (In reply to H.J. Lu from comment #15)
> > Is the goal to remove nested functions?
> 
> No - the goal is to improve the security of programs by letting their
> builders know that they have a vulnerability.  They *may* chose to address
> the vulnerability by removing nested functions from their code - if that was
> the cause - but they may also decide that the vulnerability is acceptable
> and instead add --no-warn-execstack to the linker command line.  Or just
> ignore the warning.

What should we do about failing nested function tests in GCC testsuite?
Comment 18 Nick Clifton 2022-04-26 16:36:30 UTC
(In reply to H.J. Lu from comment #17)
 
> What should we do about failing nested function tests in GCC testsuite?

Well I believe that Jeff Law is looking into this situation.  But in general
I would say that the gcc folks need to decide if the warning is useful to
them.  If not then it could be pruned in the same way that we do with the
linker tests, or the -Wl,--no-warn-execstack option could be added as an
extra option to the linker command line, or the -Wl,-z,execstack could be
added to indicate that executable stack is indeed intended.

On the other hand if nested function tests are failing in situations where
are executable stack is unexpected, then this would indicate that the test,
or maybe even the compiler, needs to be fixed.  I think however that it is 
unlikely that a situation like this actually exists.
Comment 19 cvs-commit@gcc.gnu.org 2022-04-26 16:45:48 UTC
The master branch has been updated by H.J. Lu <hjl@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=f93c6e0a2ed1ad4f0a9bb8f38e859f3312c25282

commit f93c6e0a2ed1ad4f0a9bb8f38e859f3312c25282
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Tue Apr 26 09:26:36 2022 -0700

    i386: Pass -z noexecstack to linker tests
    
            PR ld/29072
            * testsuite/ld-i386/i386.exp: Pass -z noexecstack to gotpc1
            and property-6.
Comment 20 cvs-commit@gcc.gnu.org 2022-04-27 02:44:57 UTC
The master branch has been updated by Alan Modra <amodra@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=16538271c82db0ba5679344c4107564b70a756a1

commit 16538271c82db0ba5679344c4107564b70a756a1
Author: Alan Modra <amodra@gmail.com>
Date:   Wed Apr 27 09:09:41 2022 +0930

    Revert pr29072 lto test changes
    
    Revert commit 65daf5bed6 testsuite changes in ld-plugin/.  -z isn't
    supported for non-ELF targets, and isn't needed since we now prune the
    exec stack warning (commit 333cd559ba).
    
            PR 29072
Comment 21 H.J. Lu 2022-04-28 04:14:57 UTC
FYI, the linker change caused many regressions in GCC:

https://gcc.gnu.org/pipermail/gcc-regression/2022-April/076526.html