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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions pkgs/build-support/go/module.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

&& stdenv.hostPlatform.isLinux && stdenv.hostPlatform.isGnu;

in
(stdenv.mkDerivation (finalAttrs:
args
Expand Down Expand Up @@ -175,10 +180,14 @@ in
(lib.optional (!finalAttrs.proxyVendor) "-mod=vendor")
++ lib.warnIf (builtins.elem "-trimpath" GOFLAGS) "`-trimpath` is added by default to GOFLAGS by buildGoModule when allowGoReference isn't set to true"
(lib.optional (!allowGoReference) "-trimpath");
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

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 👍


# If not set to an explicit value, set the buildid empty for reproducibility.
ldflags = ldflags ++ lib.optional (!lib.any (lib.hasPrefix "-buildid=") ldflags) "-buildid=";
ldflags = ldflags
++ lib.optional (!lib.any (lib.hasPrefix "-buildid=") ldflags) "-buildid="
++ lib.optional linuxGnuPie "-linkmode=external";

configurePhase = args.configurePhase or (''
runHook preConfigure
Expand Down
15 changes: 12 additions & 3 deletions pkgs/build-support/go/package.nix
View file
Open in desktop
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.

Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ let

goPath = if goDeps != null then importGodeps { depsFile = goDeps; } ++ extraSrcs
else extraSrcs;

# 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
&& stdenv.hostPlatform.isLinux && stdenv.hostPlatform.isGnu;

package = stdenv.mkDerivation (
(builtins.removeAttrs args [ "goPackageAliases" "disabled" "extraSrcs"]) // {

Expand All @@ -87,16 +93,19 @@ let
GOHOSTARCH = go.GOHOSTARCH or null;
GOHOSTOS = go.GOHOSTOS or null;

inherit CGO_ENABLED enableParallelBuilding;
inherit enableParallelBuilding;

CGO_ENABLED = linuxGnuPie || CGO_ENABLED;

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.

ldflags = ldflags ++ lib.optional (!lib.any (lib.hasPrefix "-buildid=") ldflags) "-buildid=";
ldflags = ldflags
++ lib.optional (!lib.any (lib.hasPrefix "-buildid=") ldflags) "-buildid="
++ lib.optional linuxGnuPie "-linkmode=external";

configurePhase = args.configurePhase or (''
runHook preConfigure
Expand Down