Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

buildGo{Module,Package}: Support PIE mode on Linux #344325

Open
wants to merge 1 commit into
base: staging
Choose a base branch
from
Open

buildGo{Module,Package}: Support PIE mode on Linux #344325

wants to merge 1 commit into from

Conversation

chivay
Copy link
Member

@chivay chivay commented Sep 24, 2024 •
edited
Loading

Description of changes

In order to support PIE builds on Linux CGO_ENABLED must be set and -linkmode=external added to ldflags, otherwise generated binaries are broken and depend on interpreter paths such as /lib64/ld-linux-x86-64.so.2.

This is a part of a larger effort (treewide PIE) documented at #252310

Reproducer:

$ CGO_ENABLED=0 go build -buildmode=pie test.go

$ file test
test: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, Go BuildID=my3pP23M4JFJ_7Ae-mCr/HxVEDoplFn0--pK_qJP6/jXZx9JN4CCLlMgD9XW61/SA-rP2uTLzlPPj243UmO, with debug_info, not stripped

$ CGO_ENABLED=1 go build -buildmode=pie -ldflags=-linkmode=external test.go

$ file test
test: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /nix/store/3dyw8dzj9ab4m8hv5dpyx7zii8d0w6fi-glibc-2.39-52/lib/ld-linux-x86-64.so.2, for GNU/Linux 3.10.0, Go BuildID=QQ3cSgZ3iTfBlspGeTL8/tObSc2zUB7g-T2pTJDil/jXZx9JN4CCLlMgD9XW61/n44KpOU6t6YmePjqwRk5, with debug_info, not stripped

Things done

Enabled those two options for PIE/Linux/Glibc builds.

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@chivay
buildGo{Module,Package}: Support PIE mode on Linux
5b41d62 
In order to support PIE builds on Linux CGO_ENABLED must be set
and -linkmode=external added to ldflags, otherwise generated binaries
are broken and depend on interpreter paths such as
/lib64/ld-linux-x86-64.so.2.
@zowoq
Copy link
Contributor

zowoq commented Sep 24, 2024

nixpkgs/pkgs/build-support/go/module.nix

Lines 201 to 204 in 24959f9

# this will respect the `hardening{Disable,Enable}` flags if set
if [[ $NIX_HARDENING_ENABLE =~ "pie" ]]; then
export GOFLAGS="-buildmode=pie $GOFLAGS"
fi

@chivay
Copy link
Member Author

chivay commented Sep 24, 2024

nixpkgs/pkgs/build-support/go/module.nix

Lines 201 to 204 in 24959f9

# this will respect the `hardening{Disable,Enable}` flags if set
if [[ $NIX_HARDENING_ENABLE =~ "pie" ]]; then
export GOFLAGS="-buildmode=pie $GOFLAGS"
fi

Yes, I'm aware. But this is not enough. For example:

$ nix build --impure --expr  '(import <nixpkgs> {}).fzf.overrideAttrs (oldAttrs: { hardeningEnable = ["pie"]; })'
error: builder for '/nix/store/db99q432j9z6dabwd5391d5rj30iyzj9-fzf-0.55.0.drv' failed with exit code 1;
       last 10 log lines:
       > Building subPackage .
       > Building subPackage ./src
       > Building subPackage ./src/algo
       > Building subPackage ./src/protector
       > Building subPackage ./src/tui
       > Building subPackage ./src/util
       > Running phase: checkPhase
       > fork/exec /build/go-build3493889823/b001/src.test: no such file or directory
       > FAIL	github.com/junegunn/fzf/src	0.001s
       > FAIL
       For full logs, run 'nix log /nix/store/db99q432j9z6dabwd5391d5rj30iyzj9-fzf-0.55.0.drv'.

zowoq
zowoq reviewed Sep 25, 2024
View reviewed changes
pkgs/build-support/go/module.nix
@@ -63,6 +63,11 @@ let
GO111MODULE = "on";
GOTOOLCHAIN = "local";

# When building PIE binaries for Linux, we must set CGO_ENABLED and enable external linking.
# Otherwise the emitted binary still depends on ld.so but at wrong path
linuxGnuPie = builtins.elem "pie" stdenv.cc.bintools.defaultHardeningFlags
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this respect hardening{Disable,Enable} that could be set in a package?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are usually set at runtime in shell. There is currently a check for musl that enables pie. Maybe it can also disable pie if it's not in NIX_HARDENING_ENABLE

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That being sad on musl we now would add the flag twice:

    if [[ $NIX_HARDENING_ENABLE =~ "pie" ]]; then
        export GOFLAGS="-buildmode=pie $GOFLAGS"
    fi

So maybe we should drop or adapt this.

pkgs/build-support/go/module.nix
inherit CGO_ENABLED enableParallelBuilding GO111MODULE GOTOOLCHAIN;
inherit enableParallelBuilding GO111MODULE GOTOOLCHAIN;

CGO_ENABLED = linuxGnuPie || CGO_ENABLED;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't evaluate, bool and int.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

..or string. CGO_ENABLED = "0" is also common

pkgs/build-support/go/package.nix

GO111MODULE = "off";
GOTOOLCHAIN = "local";
GOFLAGS = GOFLAGS ++ lib.optional (!allowGoReference) "-trimpath" ;

GOARM = toString (lib.intersectLists [(stdenv.hostPlatform.parsed.cpu.version or "")] ["5" "6" "7"]);

# If not set to an explicit value, set the buildid empty for reproducibility.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment shouldn't be removed.

katexochen
katexochen reviewed Sep 25, 2024
View reviewed changes
pkgs/build-support/go/package.nix
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert the changes on buildGoPackage, which is deprecated and will be removed shortly after the 24.11 branch off.

katexochen
katexochen reviewed Sep 25, 2024
View reviewed changes
pkgs/build-support/go/module.nix
inherit CGO_ENABLED enableParallelBuilding GO111MODULE GOTOOLCHAIN;
inherit enableParallelBuilding GO111MODULE GOTOOLCHAIN;

CGO_ENABLED = linuxGnuPie || CGO_ENABLED;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really not sure about this one. As far as I understand, you plan is to enable PIE by default in stdenv, right? So that would mean we are always building with CGO_ENABLED as default. I don't think that's worth it. I'd only enable PIE in case CGO is enabled but respect when CGO is disabled. The security benefit of PIE in Go is minimal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go programs can make use of C dependencies that are vulnerable in ways that relocation mitigates even if it's hard to write exploitable go code.

This seems important and makes it worth supporting PIE properly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is explicitly about enabling CGO to enable PIE (which the line aims to do according to my understanding). I'm not opposing to add PIE for CGO enabled binaries.

However, if CGO_ENABLED = 0, the Go binary cannot link against C libraries. So I would like to not enable PIE in this case.

Copy link
Member

@LunNova LunNova Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I misunderstood this badly.

Let me rephrase what you're suggesting to check if I understand:

pie on in stdenv + CGO_ENABLED: binary is compiled with relocation support, linked C libs are protected this way
pie on in stdenv + !CGO_ENABLED: binary is compiled without relocation support, no C libs can be present to be vulnerable
pie off in stdenv: no changes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable 👍

@katexochen

This comment was marked as resolved.

Sign in to view
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zowoq zowoq zowoq left review comments

@katexochen katexochen katexochen left review comments

@LunNova LunNova LunNova left review comments

@kalbasit kalbasit Awaiting requested review from kalbasit

@Mic92 Mic92 Awaiting requested review from Mic92

Assignees
No one assigned
Labels
6.topic: golang
Projects
Go team
Status: In progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@chivay @zowoq @katexochen @Mic92 @LunNova