Gerrit Bot has uploaded this change for review.
cmd/link: support full relro
Most Linux distributions today enable PIE and full RELRO on all binaries
to make exploitation harder. When buildmode=pie is used we enable full
relro as that is probably what most people want regardless.
This introduces a negligible startup time for binaries.
https://fedoraproject.org/wiki/Changes/Harden_All_Packages
https://www.redhat.com/en/blog/hardening-elf-binaries-using-relocation-read-only-relro
Related #44480
Change-Id: I2ff8d39f095978a2524946d95de23793ca3064e1
GitHub-Last-Rev: 20ab0e5c756a056a44337ba12c199d183964bc9e
GitHub-Pull-Request: golang/go#45681
---
M src/cmd/go/internal/work/security.go
M src/cmd/go/internal/work/security_test.go
M src/cmd/link/doc.go
M src/cmd/link/internal/ld/lib.go
M src/cmd/link/internal/ld/main.go
5 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/src/cmd/go/internal/work/security.go b/src/cmd/go/internal/work/security.go
index 36bbab3..e9638aa 100644
--- a/src/cmd/go/internal/work/security.go
+++ b/src/cmd/go/internal/work/security.go
@@ -206,7 +206,7 @@
re(`-Wl,--(no-)?warn-([^,]+)`),
re(`-Wl,-?-wrap[=,][^,@\-][^,]*`),
re(`-Wl,-z,(no)?execstack`),
- re(`-Wl,-z,relro`),
+ re(`-Wl,-z,relro(,-z,now)?`),
re(`[a-zA-Z0-9_/].*\.(a|o|obj|dll|dylib|so)`), // direct linker inputs: x.o or libfoo.so (but not -foo.o or @foo.o)
re(`\./.*\.(a|o|obj|dll|dylib|so)`),
diff --git a/src/cmd/go/internal/work/security_test.go b/src/cmd/go/internal/work/security_test.go
index 4f2e0eb..2708c71 100644
--- a/src/cmd/go/internal/work/security_test.go
+++ b/src/cmd/go/internal/work/security_test.go
@@ -149,6 +149,8 @@
{"-Wl,--just-symbols,foo"},
{"-Wl,--warn-error"},
{"-Wl,--no-warn-error"},
+ {"-Wl,-z,relro"},
+ {"-Wl,-z,relro,-z,now"},
{"foo.so"},
{"_世界.dll"},
{"./x.o"},
@@ -224,6 +226,7 @@
{"-Wl,-R,foo,bar"},
{"-Wl,-R,@foo"},
{"-Wl,--just-symbols,@foo"},
+ {"-Wl,-z,relro,-z,nottoday"},
{"../x.o"},
}
diff --git a/src/cmd/link/doc.go b/src/cmd/link/doc.go
index 604675c..06ca5db 100644
--- a/src/cmd/link/doc.go
+++ b/src/cmd/link/doc.go
@@ -85,6 +85,8 @@
instead of $GOROOT/pkg/$GOOS_$GOARCH.
-k symbol
Set field tracking symbol. Use this flag when GOEXPERIMENT=fieldtrack is set.
+ -l
+ Disable Full RELRO.
-libgcc file
Set name of compiler support library.
This is only used in internal link mode.
diff --git a/src/cmd/link/internal/ld/lib.go b/src/cmd/link/internal/ld/lib.go
index c840e5e..c743d4c 100644
--- a/src/cmd/link/internal/ld/lib.go
+++ b/src/cmd/link/internal/ld/lib.go
@@ -1299,6 +1299,17 @@
return argv
}
+ // Enables Full/Partial RELRO.
+ addRELROargs := func(argv []string) []string {
+ relro := "-Wl,-z,relro"
+ // Enable Full RELRO
+ if !*FlagL {
+ relro += ",-z,now"
+ }
+ argv = append(argv, relro)
+ return argv
+ }
+
switch ctxt.BuildMode {
case BuildModeExe:
if ctxt.HeadType == objabi.Hdarwin {
@@ -1315,7 +1326,7 @@
default:
// ELF.
if ctxt.UseRelro() {
- argv = append(argv, "-Wl,-z,relro")
+ argv = addRELROargs(argv)
}
argv = append(argv, "-pie")
}
@@ -1324,7 +1335,7 @@
argv = append(argv, "-dynamiclib")
} else {
if ctxt.UseRelro() {
- argv = append(argv, "-Wl,-z,relro")
+ argv = addRELROargs(argv)
}
argv = append(argv, "-shared")
if ctxt.HeadType == objabi.Hwindows {
@@ -1341,7 +1352,7 @@
}
case BuildModeShared:
if ctxt.UseRelro() {
- argv = append(argv, "-Wl,-z,relro")
+ argv = addRELROargs(argv)
}
argv = append(argv, "-shared")
case BuildModePlugin:
@@ -1349,7 +1360,7 @@
argv = append(argv, "-dynamiclib")
} else {
if ctxt.UseRelro() {
- argv = append(argv, "-Wl,-z,relro")
+ argv = addRELROargs(argv)
}
argv = append(argv, "-shared")
}
diff --git a/src/cmd/link/internal/ld/main.go b/src/cmd/link/internal/ld/main.go
index 52dfe919..462309c 100644
--- a/src/cmd/link/internal/ld/main.go
+++ b/src/cmd/link/internal/ld/main.go
@@ -85,6 +85,7 @@
flagN = flag.Bool("n", false, "dump symbol table")
FlagS = flag.Bool("s", false, "disable symbol table")
FlagW = flag.Bool("w", false, "disable DWARF generation")
+ FlagL = flag.Bool("l", false, "disable full RELRO")
flag8 bool // use 64-bit addresses in symbol table
flagInterpreter = flag.String("I", "", "use `linker` as ELF dynamic linker")
FlagDebugTramp = flag.Int("debugtramp", 0, "debug trampolines")
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Patch set 1:Run-TryBot +1
2 comments:
Patchset:
Thanks.
Does the cmg/go change need to be part of this CL? I think that is about user writing flags in cgo code, vs. this one being flags added to the linker. That could be a separate CL.
File src/cmd/link/internal/ld/main.go:
Patch Set #1, Line 88: FlagL = flag.Bool("l", false, "disable full RELRO")
I think it is a longer flag. Maybe -fullrelro ?
Also, it may be better to have positive flag (that enables something) than a negative one (that disables something).
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
1 comment:
File src/cmd/link/internal/ld/main.go:
Patch Set #1, Line 88: FlagL = flag.Bool("l", false, "disable full RELRO")
I think it is a longer flag. Maybe -fullrelro ?
I mean, it may be better to use a longer flag.
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Bryan C. Mills, Morten Linderud, Jay Conrod, Ian Lance Taylor, Russ Cox, Matthew Dempsky, Michael Matloob.
1 comment:
File src/cmd/link/internal/ld/main.go:
Patch Set #1, Line 88: FlagL = flag.Bool("l", false, "disable full RELRO")
A longer flag can be done. […]
So this is changing the default behavior? Per Ian's comment on issue #44480 we probably don't want to do that...
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Bryan C. Mills, Morten Linderud, Jay Conrod, Ian Lance Taylor, Russ Cox, Matthew Dempsky, Michael Matloob.
1 comment:
File src/cmd/link/internal/ld/main.go:
Patch Set #1, Line 88: FlagL = flag.Bool("l", false, "disable full RELRO")
>That said, I think it would be reasonable to make -z,now the default but we might want some way to […]
Okay, thanks. I'll let Ian decide.
We can still have it as a positive flag. It can have default value true (use -fullrelro=false to turn it off).
Attention is currently required from: Bryan C. Mills, Morten Linderud, Jay Conrod, Russ Cox, Matthew Dempsky, Michael Matloob, Cherry Zhang.
1 comment:
File src/cmd/link/internal/ld/main.go:
Patch Set #1, Line 88: FlagL = flag.Bool("l", false, "disable full RELRO")
Okay, thanks. I'll let Ian decide. […]
I'm OK with having "-z now" be the default if there is a way to turn it off. I wouldn't call the option to turn it off "full relro", although I see that some people do refer to it that way. "-z now" precedes "-z relro"; it means "resolve all symbols at programs startup."
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Bryan C. Mills, Jay Conrod, Ian Lance Taylor, Russ Cox, Matthew Dempsky, Michael Matloob, Cherry Zhang.
2 comments:
Patchset:
Thanks. […]
I don't think it needs to be, so this could probably be removed.
File src/cmd/link/internal/ld/main.go:
Patch Set #1, Line 88: FlagL = flag.Bool("l", false, "disable full RELRO")
> I think it is a longer flag. Maybe -fullrelro ? […]
A longer flag can be done. However the second request is harder because it is painful for a distribution to pass linker options to the go compiler through build systems.
https://github.com/golang/go/issues/38522
I think it makes sense to have full relro when -buildmode=pie is requested, and rather disable it if you don't want it in PIE mode. Else the result is not better then the status quo today.
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Bryan C. Mills, Jay Conrod, Ian Lance Taylor, Russ Cox, Matthew Dempsky, Michael Matloob, Cherry Zhang.
1 comment:
File src/cmd/link/internal/ld/main.go:
Patch Set #1, Line 88: FlagL = flag.Bool("l", false, "disable full RELRO")
Okay, thanks. I'll let Ian decide. […]
Yes, that sounds good. I'll do a revision with the mentioned changes within a day or two while waiting on review from Ian and others. Thank you!
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Bryan C. Mills, Jay Conrod, Ian Lance Taylor, Russ Cox, Matthew Dempsky, Michael Matloob, Cherry Zhang.
1 comment:
File src/cmd/link/internal/ld/main.go:
Patch Set #1, Line 88: FlagL = flag.Bool("l", false, "disable full RELRO")
So this is changing the default behavior? Per Ian's comment on issue #44480 we probably don't want t […]
>That said, I think it would be reasonable to make -z,now the default but we might want some way to turn it off.
https://github.com/golang/go/issues/44480#issuecomment-785152628
I interpreted that as having "-z,relro,-z,now" default. But did they mean "-z,relro" should be swapped for "-z,now"?
Attention is currently required from: Bryan C. Mills, Morten Linderud, Russ Cox, Matthew Dempsky, Michael Matloob, Cherry Zhang.
Bryan C. Mills removed Jay Conrod from this change.
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Bryan C. Mills, Morten Linderud, Russ Cox, Matthew Dempsky, Cherry Zhang.
Bryan C. Mills removed Michael Matloob from this change.
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Morten Linderud, Russ Cox, Matthew Dempsky, Cherry Zhang.
Gerrit Bot uploaded patch set #2 to this change.
cmd/link: support full relro
Most Linux distributions today enable PIE and full RELRO on all binaries
to make exploitation harder. When buildmode=pie is used we enable full
relro as that is probably what most people want regardless.
This introduces a negligible startup time for binaries.
https://fedoraproject.org/wiki/Changes/Harden_All_Packages
https://www.redhat.com/en/blog/hardening-elf-binaries-using-relocation-read-only-relro
Related #44480
Change-Id: I2ff8d39f095978a2524946d95de23793ca3064e1
GitHub-Last-Rev: b80f28467fe1d9fc46704e4404f12a5a68403082
GitHub-Pull-Request: golang/go#45681
---
M src/cmd/link/doc.go
M src/cmd/link/internal/ld/lib.go
M src/cmd/link/internal/ld/main.go
3 files changed, 18 insertions(+), 4 deletions(-)
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Ian Lance Taylor, Russ Cox, Matthew Dempsky, Cherry Zhang.
1 comment:
File src/cmd/link/internal/ld/main.go:
Patch Set #1, Line 88: FlagL = flag.Bool("l", false, "disable full RELRO")
I'm OK with having "-z now" be the default if there is a way to turn it off. […]
The new patch has the flag renamed from "-l" to "-relro={true,false}". If we want more granularity we could implement "-relro={off,partial,full}" as for example rust does.
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cherry Mui, Matthew Dempsky, Russ Cox.
1 comment:
File src/cmd/link/doc.go:
Patch Set #2, Line 89: Enable RELRO (default true).
This doesn't seem to accurately reflect the patch. We already enable RELRO by default where appropriate when linking externally. What this patch does is add "-z now" which is "Mark object for immediate function binding." We shouldn't call this option -relro, because do relro regardless of how this option is set. Perhaps
-bindnow When linking externally and marking variables read-only after relocation, also require immediate function binding (default true)
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cherry Mui, Matthew Dempsky, Russ Cox.
Gerrit Bot uploaded patch set #3 to this change.
cmd/link: support full relro
Most Linux distributions today enable PIE and full RELRO on all binaries
to make exploitation harder. When buildmode=pie is used we enable full
relro as that is probably what most people want regardless.
This introduces a negligible startup time for binaries.
https://fedoraproject.org/wiki/Changes/Harden_All_Packages
https://www.redhat.com/en/blog/hardening-elf-binaries-using-relocation-read-only-relro
Related #44480
Change-Id: I2ff8d39f095978a2524946d95de23793ca3064e1
GitHub-Last-Rev: 54b42669381dd58a242b6d7c0088d44ee522ff81
GitHub-Pull-Request: golang/go#45681
---
M src/cmd/link/doc.go
M src/cmd/link/internal/ld/lib.go
M src/cmd/link/internal/ld/main.go
3 files changed, 41 insertions(+), 4 deletions(-)
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cherry Mui, Ian Lance Taylor, Matthew Dempsky, Russ Cox.
1 comment:
File src/cmd/link/doc.go:
Patch Set #2, Line 89: Enable RELRO (default true).
This doesn't seem to accurately reflect the patch. […]
Did the suggestion and also fixed up the flag description.
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cherry Mui, Matthew Dempsky, Russ Cox.
Patch set 3:Run-TryBot +1Code-Review +1
1 comment:
Patchset:
RELNOTE=yes
Thanks. LGTM. Leaving for linker maintainers to approve.
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cherry Mui, Matthew Dempsky, Russ Cox.
1 comment:
Patchset:
Please rebase up to the current HEAD. Thanks.
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cherry Mui, Ian Lance Taylor, Matthew Dempsky, Russ Cox.
Gerrit Bot uploaded patch set #4 to this change.
cmd/link: support full relro
Most Linux distributions today enable PIE and full RELRO on all binaries
to make exploitation harder. When buildmode=pie is used we enable full
relro as that is probably what most people want regardless.
This introduces a negligible startup time for binaries.
https://fedoraproject.org/wiki/Changes/Harden_All_Packages
https://www.redhat.com/en/blog/hardening-elf-binaries-using-relocation-read-only-relro
Related #44480
Change-Id: I2ff8d39f095978a2524946d95de23793ca3064e1
GitHub-Last-Rev: b85c7f7ebe886d197d998b170a542920d4a02bf7
GitHub-Pull-Request: golang/go#45681
---
M src/cmd/link/doc.go
M src/cmd/link/internal/ld/lib.go
M src/cmd/link/internal/ld/main.go
3 files changed, 41 insertions(+), 4 deletions(-)
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cherry Mui, Matthew Dempsky, Russ Cox.
Patch set 4:Run-TryBot +1Code-Review +1
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Matthew Dempsky, Russ Cox.
4 comments:
Commit Message:
When buildmode=pie is used we enable full
relro as that is probably what most people want regardless.
It seems the CL does more than just buildmode=pie. Maybe adjust the CL description. Also mention that it only affects external linking.
Patchset:
Do we want to make any change for internal linking? I feel that it would be a little weird if internal linking and external linking by default generate semantically different things.
What is the effect for this for the C linker? Whether to use .got.plt section? Whether to use JUMP_SLOT relocations? Anything else? Thanks.
File src/cmd/link/internal/ld/lib.go:
Patch Set #4, Line 1411: if *FlagBindNow {
If the flag is false, should we add flag to explicitly turn it off? Is there a C linker that has -z,now on by default?
Patch Set #4, Line 1419: case BuildModeExe:
Should this also apply to exe mode when external linking?
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cherry Mui, Matthew Dempsky, Russ Cox.
1 comment:
Patchset:
Do we want to make any change for internal linking? I feel that it would be a little weird if intern […]
This has two effects on the C linker:
First, it marks the executable as requiring that all dynamic relocations be processed at program startup. There are three ways to mark this in an ELF dynamic executable, all equivalent:
Second,for most targets, .got.plt becomes a relro section, as it no longer changes after dynamic relocations are applied.
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Ian Lance Taylor, Matthew Dempsky, Russ Cox.
1 comment:
Patchset:
This has two effects on the C linker: […]
Thanks! Do you think we want to add that to internal linking? (I can look into it. But too late for Go 1.19.)
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Ian Lance Taylor, Matthew Dempsky, Russ Cox.
1 comment:
File src/cmd/link/internal/ld/main.go:
Patch Set #4, Line 98: bind function calls when linking externally
Mention "on ELF".
(What do we want to do on other platforms? Mach-O has -bind_at_load. Haven't looked at PE or XCOFF.)
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cherry Mui, Matthew Dempsky, Russ Cox.
1 comment:
Patchset:
Thanks! Do you think we want to add that to internal linking? (I can look into it. […]
Yes, I think you're right that if we add this new flag then we should also honor it when doing internal linking and generating a dynamically linked executable.
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cherry Mui, Russ Cox.
1 comment:
Patchset:
@mcf...@gmail.com Any interest in fixing up this CL for the internal linking case, as suggested in the comments? Thanks.
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cherry Mui, Ian Lance Taylor, Russ Cox.
1 comment:
Patchset:
@mcfoxax@gmail. […]
I don't think I have the knowledge to do the internal linking case without sinking a lot more time into it sadly. Will that be a blocker to get the external linker portion merged?
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cherry Mui, Morten Linderud, Russ Cox.
1 comment:
Patchset:
I don't think I have the knowledge to do the internal linking case without sinking a lot more time i […]
I think we should make sure that we have the internal linking CL ready to go before submitting this one. Otherwise we'll have surprising inconsistent behavior, which seems unwise for a feature whose main purpose is security.
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cherry Mui, Ian Lance Taylor, Morten Linderud, Russ Cox.
1 comment:
Patchset:
Yes, I think you're right that if we add this new flag then we should also honor it when doing inter […]
I believe I have achieved the desired behavior for internal linker in my local test env and will be happy to pick this work up.
A couple of questions:
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cherry Mui, Morten Linderud, Nick Revin, Russ Cox.
1 comment:
Patchset:
I believe I have achieved the desired behavior for internal linker in my local test env and will be […]
I think we should do what other linkers do. Is "full relro" the default for any current Unix linker?
If you cherry pick this CL (312509) you should be able to send a new CL that is based on this one. We can have this CL for external linking and yours for internal linking.
I see that this CL has some open comments, though.
Thanks.
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cherry Mui, Ian Lance Taylor, Morten Linderud, Russ Cox.
1 comment:
Patchset:
I think we should do what other linkers do. […]
I have tested other linkers and here is a gist of observations.
Linkers tested (with gcc, clang and some dummy C program):
1. All tested linkers have "partial relro" enabled by default for linux targets regardless of PIE setting.
2. With -Wl,-z,now lld, mold and gold map .got.plt section to the GNU_RELRO segment whereas ld and bfd merge .got.plt into .got.
3. -z,now is independent from -z,relro meaning one can create an ELF binary without GNU_RELRO header but with DF_BIND_NOW in the DT_FLAGS and DF_1_NOW in the DT_FLAGS_1 dynamic tags.
4. GNU_RELRO segment is always read-only with -Wl,-z,relro for all linkers whereas go's internal linker does not put that flag (see 28541: cmd/link: mark rel.ro segment as PT_GNU_RELRO | https://go-review.googlesource.com/c/go/+/28541)
I suggest we preserve current behavior, make -bindnow flag false by default and decouple non-lazy binding from read-only relocations.
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cherry Mui, Ian Lance Taylor, Russ Cox.
1 comment:
Patchset:
I believe I have achieved the desired behavior for internal linker in my local test env and will be […]
I don't mind handing over this CL to Nick. If they want to rebase this change or do something else that is fine. It's been laying around for too long that I have any strong feelings on the matter :)
Anything you want me to do with this CL Nick?
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cherry Mui, Ian Lance Taylor, Morten Linderud, Russ Cox.
1 comment:
Patchset:
I don't mind handing over this CL to Nick. […]
I have sent a new CL that covers both internal and external linker:
473495: cmd/link: add option to enable full RELRO for ELF | https://go-review.googlesource.com/c/go/+/473495
To view, visit change 312509. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cherry Mui, Morten Linderud, Russ Cox.
Patch set 4:-Code-Review