Bug 252629

Summary: Need infrastructure to set ELF feature note flags via elfctl
Product: Ports & Packages Reporter: Ed Maste <emaste>
Component: Ports FrameworkAssignee: Port Management Team <portmgr>
Status: New ---    
Severity: Affects Only Me CC: dch, dewayne, kevans, lwhsu, manu, ports-bugs
Priority: ---    
Version: Latest   
Hardware: Any   
OS: Any   
See Also: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=239873
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=259968
Bug Depends on: 252689, 271838, 273036, 239873    
Bug Blocks:    

Description Ed Maste freebsd_committer freebsd_triage 2021-01-13 03:15:08 UTC
[PR for issue tracking; I would like to discuss the desired approach before starting implementation]

We have elfctl(1) which can set ELF feature flags, to indicate that a binary is incompatible with certain vulnerability mitigations.

The current list of flags is:
% elfctl -l
Known features are:
aslr            Disable ASLR
protmax         Disable implicit PROT_MAX
stackgap        Disable stack gap
wxneeded        Requires W+X mappings

I would like to have some canonical way to set these flags in ports, but I am not sure what the right interface is.  It could be done in an ad-hoc manner in individual post-install targets, but I believe we'll want common infrastructure as different sets of flags will be available on different releases. Within an individual port we may have sets of files with different flags to be set.
Comment 1 Ed Maste freebsd_committer freebsd_triage 2021-01-13 03:33:15 UTC
Although maybe we can allow elfctl to silently ignore unknown flags and use the same invocation across all releases - see https://reviews.freebsd.org/D28130.
Comment 2 Ed Maste freebsd_committer freebsd_triage 2021-01-13 03:34:08 UTC
My world was out of date, currently known flags are:

Known features are:
aslr            Disable ASLR
protmax         Disable implicit PROT_MAX
stackgap        Disable stack gap
wxneeded        Requires W+X mappings
la48            amd64: Limit user VA to 48bit
aslrstkgap      Disable ASLR stack gap
Comment 3 commit-hook freebsd_committer freebsd_triage 2021-01-13 05:12:37 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=f6d95a01103a49a94c876d5a51bb4be25c06d964

commit f6d95a01103a49a94c876d5a51bb4be25c06d964
Author:     Ed Maste <emaste@FreeBSD.org>
AuthorDate: 2021-01-13 03:24:52 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2021-01-13 05:10:13 +0000

    elftcl: add -i flag to ignore unknown flags

    This may allow an identical elfctl invocation to be used on multiple
    FreeBSD versions, with features not implemented on older releases being
    silently ignored.

    PR:             252629 (related)
    Reviewed by:    kib
    MFC after:      1 week
    Sponsored by:   The FreeBSD Foundation
    Differential Revision: https://reviews.freebsd.org/D28130

 usr.bin/elfctl/elfctl.1 |  6 +++++-
 usr.bin/elfctl/elfctl.c | 13 ++++++++++---
 2 files changed, 15 insertions(+), 4 deletions(-)
Comment 4 Emmanuel Vadot freebsd_committer freebsd_triage 2021-01-13 08:54:36 UTC
Adding support in pkg(8) might be the way.
Then adding some keyworks support in the plist like 
@elfctl aslr /path/to/bin
@elfctl wxneeded /path/to/other/bin
Comment 5 Emmanuel Vadot freebsd_committer freebsd_triage 2021-01-13 08:59:53 UTC
While here, I think that it's a bit strange to do
elfctl +aslr /path/to/bin to disable it
Comment 6 Emmanuel Vadot freebsd_committer freebsd_triage 2021-01-13 09:05:10 UTC
After looking at elfctl it seems that if we can set multiple features at a time we cannot do something like
elfctl -e +aslr,-wxneeded /path/to/bin
I think that would be good to have.
For the keywords something like :
@elfctl set <featurelist> /path/to/bin
@elfctl clear <featurelist> /path/to/bin

Would be nicer to what I've said earlier.
Comment 7 Emmanuel Vadot freebsd_committer freebsd_triage 2021-01-13 09:14:13 UTC
So what's needed is :

 - in pkg(8) add a lua callable function pkg.elfctl that takes a list of features and a path
 - In the portstree add a Keywords/elfctl.ucl that calls this lua function
 - Then in the plist of the ports just @elfctl <featurelist> /path/to/bin

I would prefer if the featurelist is more intuitive that what's in elfctl
"aslr" would enable it while "noaslr" would disable it etc ...
Comment 8 Baptiste Daroussin freebsd_committer freebsd_triage 2021-01-13 09:17:35 UTC
I agree with manu this is the simplest way and the cleanest way
Comment 9 Ed Maste freebsd_committer freebsd_triage 2021-01-13 15:25:19 UTC
(In reply to Emmanuel Vadot from comment #6)
> elfctl -e +aslr,-wxneeded /path/to/bin

For tagging at build time I expect we'd use = and just set the bits we want. In general I think there would not be a need to turn off bits in ports builds.
Comment 10 dewayne 2021-01-13 21:52:24 UTC
(In reply to Emmanuel Vadot from comment #7)
Please bear in mind that lua may not be functional in some environments.  In src.conf, I have
WITHOUT_LOADER_LUA=YES
which I suspect is the cause of pkg being unable to process lua scripts (on FreeBSD12.2 r368820M).

Is lua a prerequisite for pkg use on FreeBSD?
Comment 11 Kyle Evans freebsd_committer freebsd_triage 2021-01-13 21:53:56 UTC
(In reply to dewayne from comment #10)

pkg has its own lua, this knob is irrelevant.
Comment 12 commit-hook freebsd_committer freebsd_triage 2021-01-19 02:50:30 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=369a4023e671c35390574d42b1b409b55946faf8

commit 369a4023e671c35390574d42b1b409b55946faf8
Author:     Ed Maste <emaste@FreeBSD.org>
AuthorDate: 2021-01-13 03:24:52 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2021-01-19 02:49:55 +0000

    elftcl: add -i flag to ignore unknown flags

    This may allow an identical elfctl invocation to be used on multiple
    FreeBSD versions, with features not implemented on older releases being
    silently ignored.

    PR:             252629 (related)
    Reviewed by:    kib
    Sponsored by:   The FreeBSD Foundation
    Differential Revision: https://reviews.freebsd.org/D28130

    (cherry picked from commit f6d95a01103a49a94c876d5a51bb4be25c06d964)

 usr.bin/elfctl/elfctl.1 |  6 +++++-
 usr.bin/elfctl/elfctl.c | 13 ++++++++++---
 2 files changed, 15 insertions(+), 4 deletions(-)
Comment 13 Ed Maste freebsd_committer freebsd_triage 2021-01-22 17:31:19 UTC
Another proposed change to allow flags to be set by value: https://reviews.freebsd.org/D28284

then we could do e.g.

NT_FREEBSD_FCTL_ASLR_DISABLE=0x1
NT_FREEBSD_FCTL_PROTMAX_DISABLE=0x2
NT_FREEBSD_FCTL_STKGAP_DISABLE=0x4

elfctl -e =${NT_FREEBSD_FCTL_ASLR_DISABLE},${NT_FREEBSD_FCTL_STKGAP_DISABLE} /path/to/file
Comment 14 commit-hook freebsd_committer freebsd_triage 2021-01-22 19:41:32 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=86f33b5fcf6087bf4439881011b920ff99e6e300

commit 86f33b5fcf6087bf4439881011b920ff99e6e300
Author:     Ed Maste <emaste@FreeBSD.org>
AuthorDate: 2021-01-22 17:22:35 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2021-01-22 19:38:52 +0000

    elfctl: allow features to be specified by value

    This will allow elfctl on older releases to set bits that are not yet
    known there, so that the binary will have the correct settings applied
    if run on a later FreeBSD version.

    PR:             252629 (related)
    Suggested by:   kib
    Reviewed by:    gbe (manpage, earlier), kib
    MFC after:      3 days
    Sponsored by:   The FreeBSD Foundation
    Differential Revision: https://reviews.freebsd.org/D28284

 usr.bin/elfctl/elfctl.1 | 15 +++++++++++++--
 usr.bin/elfctl/elfctl.c | 25 ++++++++++++++++++++++---
 2 files changed, 35 insertions(+), 5 deletions(-)
Comment 15 commit-hook freebsd_committer freebsd_triage 2021-01-26 14:47:54 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=d7e23b5cdd8465bd50f88b1e38cb695a361a26f5

commit d7e23b5cdd8465bd50f88b1e38cb695a361a26f5
Author:     Ed Maste <emaste@FreeBSD.org>
AuthorDate: 2021-01-22 17:22:35 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2021-01-26 14:46:00 +0000

    elfctl: allow features to be specified by value

    This will allow elfctl on older releases to set bits that are not yet
    known there, so that the binary will have the correct settings applied
    if run on a later FreeBSD version.

    PR:             252629 (related)
    Suggested by:   kib
    Reviewed by:    gbe (manpage, earlier), kib
    Sponsored by:   The FreeBSD Foundation
    Differential Revision: https://reviews.freebsd.org/D28284

    (cherry picked from commit 86f33b5fcf6087bf4439881011b920ff99e6e300)

    elfctl: fix typo from last-minute refactoring

    Reported by:    jkim

    (cherry picked from commit f302fd1aa6730facd53a3f761e0a57302731b03e)

    elfctl: Fix type errors.

    Target value for val has uint32_t type, not uint, adjust used constant.
    Change val type to unsigned so that left and right sides of comparision
    operator do not expose different signed types of same range [*].

    Switch to unsigned long long and strtoll(3) so that 0x80000000 is
    accepted by conversion function [**].

    Reported by:    kargl [*]
    Noted by:       emaste [**]
    Reviewed by:    emaste
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D28301

    (cherry picked from commit 9940ac808de7b7d4ed0408c3e739f667dca06d3b)

 usr.bin/elfctl/elfctl.1 | 15 +++++++++++++--
 usr.bin/elfctl/elfctl.c | 26 +++++++++++++++++++++++---
 2 files changed, 36 insertions(+), 5 deletions(-)
Comment 16 commit-hook freebsd_committer freebsd_triage 2021-01-27 00:26:40 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=f2b5999b2166579a2ebd4b5e117a17574f18d50f

commit f2b5999b2166579a2ebd4b5e117a17574f18d50f
Author:     Ed Maste <emaste@FreeBSD.org>
AuthorDate: 2021-01-22 17:22:35 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2021-01-26 14:44:12 +0000

    elfctl: allow features to be specified by value

    This will allow elfctl on older releases to set bits that are not yet
    known there, so that the binary will have the correct settings applied
    if run on a later FreeBSD version.

    PR:             252629 (related)
    Suggested by:   kib
    Reviewed by:    gbe (manpage, earlier), kib
    Sponsored by:   The FreeBSD Foundation
    Differential Revision: https://reviews.freebsd.org/D28284

    (cherry picked from commit 86f33b5fcf6087bf4439881011b920ff99e6e300)

    elfctl: fix typo from last-minute refactoring

    Reported by:    jkim

    (cherry picked from commit f302fd1aa6730facd53a3f761e0a57302731b03e)

    elfctl: Fix type errors.

    Target value for val has uint32_t type, not uint, adjust used constant.
    Change val type to unsigned so that left and right sides of comparision
    operator do not expose different signed types of same range [*].

    Switch to unsigned long long and strtoll(3) so that 0x80000000 is
    accepted by conversion function [**].

    Reported by:    kargl [*]
    Noted by:       emaste [**]
    Reviewed by:    emaste
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D28301

    (cherry picked from commit 9940ac808de7b7d4ed0408c3e739f667dca06d3b)

 usr.bin/elfctl/elfctl.1 | 15 +++++++++++++--
 usr.bin/elfctl/elfctl.c | 26 +++++++++++++++++++++++---
 2 files changed, 36 insertions(+), 5 deletions(-)