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.

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

cmd/link/internal/ld: Passing incorrect offset to codesign.Sign #59555

Closed
oxisto opened this issue Apr 11, 2023 · 5 comments
Closed

cmd/link/internal/ld: Passing incorrect offset to codesign.Sign #59555

oxisto opened this issue Apr 11, 2023 · 5 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.21

Comments

@oxisto
Copy link
Contributor

oxisto commented Apr 11, 2023 •
edited

What version of Go are you using (go version)?

go version devel go1.21-da879c6e6a Tue Apr 11 20:56:35 2023 +0000 darwin/arm64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/oxisto/Library/Caches/go-build"
GOENV="/Users/oxisto/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/oxisto/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/oxisto/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/oxisto/Repositories/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/oxisto/Repositories/go/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="devel go1.21-fd7fa96b85 Tue Apr 11 22:38:11 2023 +0200"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/oxisto/Repositories/go/src/go.mod"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/7x/03y_vsbn2ylbn8cq3w_zz_dc0000gn/T/go-build1784480665=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

The internal function asmbMacho which creates the mach-o header passes incorrect values to the codesign package here:

go/src/cmd/link/internal/ld/macho.go

Line 871 in 5a962bf

codesign.Sign(ldr.Data(cs), bytes.NewReader(data), "a.out", codesigOff, int64(Segtext.Fileoff), int64(Segtext.Filelen), ctxt.IsExe() || ctxt.IsPIE())

What did you expect to see?

codesign.Sign expects the offset and size of the executable segment.

What did you see instead?

Instead, only the values of the __text section are passed.

It seems this gets sort of fixed in the end because codesign.Sign gets executed again in the Rewrite function of cmd/internal/buildid and this time, the correct values are passed.

go/src/cmd/internal/buildid/rewrite.go

Lines 107 to 112 in 1635205

text := f.Segment("__TEXT")
cs := make([]byte, cmd.Datasize)
codesign.Sign(cs, w.(io.Reader), "a.out", int64(cmd.Dataoff), int64(text.Offset), int64(text.Filesz), f.Type == macho.TypeExec)
if _, err := w.WriteAt(cs, int64(cmd.Dataoff)); err != nil {
return err
}

Possible fix

The fix is quite easy, the local variable v within asmbMacho already holds the exec file size and the offset should be 0.

--- a/src/cmd/link/internal/ld/macho.go
+++ b/src/cmd/link/internal/ld/macho.go
@@ -868,7 +868,7 @@ func asmbMacho(ctxt *Link) {
                if int64(len(data)) != codesigOff {
                        panic("wrong size")
                }
-               codesign.Sign(ldr.Data(cs), bytes.NewReader(data), "a.out", codesigOff, int64(Segtext.Fileoff), int64(Segtext.Filelen), ctxt.IsExe() || ctxt.IsPIE())
+               codesign.Sign(ldr.Data(cs), bytes.NewReader(data), "a.out", codesigOff, 0, v, ctxt.IsExe() || ctxt.IsPIE())
                ctxt.Out.SeekSet(codesigOff)
                ctxt.Out.Write(ldr.Data(cs))
        }

I am not an expert on the buildid, but does this rewrite always happen? Or just under special circumstances?
@oxisto oxisto mentioned this issue Apr 12, 2023
Closed
oxisto added a commit to oxisto/go that referenced this issue Apr 12, 2023
@oxisto
cmd/link/internal/ld: Passing correct offset to codesign.Sign
0e972b2 
Previously, codesign.Sign was called with Segtext.Fileoff and Segtext.Filelen. However,
both variables are being changed in the course of asmbMacho and point to the __text
section at the time codesign.Sign is called, instead of the whole __TEXT segment.
Therefore, a stored local variable, which already existed and holds the correct length is
used. The offset is always 0.

Fixes golang#59555
@gopherbot
Copy link

Change https://go.dev/cl/484015 mentions this issue: cmd/link/internal/ld: Passing correct offset to codesign.Sign

@dr2chase
Copy link
Contributor

@golang/compiler - already has a proposed fix, just needs to be eyeballed to be sure it is really a bug.

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 13, 2023
@cherrymui
Copy link
Member

What did you expect to see?
codesign.Sign expects the offset and size of the executable segment.

What did you see instead?
Instead, only the values of the __text section are passed.

The code is actually intended to use the segment's values, not the section's, as Segtext is the segment. The difference is whether the header and the padding is included. It doesn't seem to actually matter (the header and the padding should not be executed anyway).

The buildid command usually runs if you build with go build. But other tools, or building manually with go tool link may not run it. So we want the linker to generate a correct binary even without the buildid command. I verified that an output binary directly from go tool link runs correctly and passes the system's code signature check.

For consistency, maybe we'll change the linker to use the values matching the Mach-O segment, one day.

Thanks.

@cherrymui cherrymui closed this as not planned Won't fix, can't repro, duplicate, stale Apr 18, 2023
oxisto added a commit to oxisto/go that referenced this issue Apr 19, 2023
@oxisto
cmd/link/internal/ld: Passing correct offset to codesign.Sign
37a048d 
Previously, codesign.Sign was called with Segtext.Fileoff and Segtext.Filelen. However,
both variables do not contain the complete __TEXT segment, as it excludes padding and header.
Therefore, we now store a reference to the complete segment in mstext when it is created and
pass its offset (which should always be 0) and filesize to codesign.Sign.

Fixes golang#59555
@oxisto
Copy link
Contributor Author

oxisto commented May 25, 2023

As requested per @dmitshur: There was an ongoing discussion about this issue in https://go-review.googlesource.com/c/go/+/484015 and the consensus was that the values were changed to match the Mach-O segment.

@dmitshur
Copy link
Contributor

Reopening to track submission of CL 484015 which will resolve this.

@dmitshur dmitshur reopened this May 25, 2023
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 25, 2023
@dmitshur dmitshur modified the milestones: Backlog, Go1.21 May 25, 2023
@dmitshur dmitshur added the compiler/runtime Issues related to the Go compiler and/or runtime. label May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.21
Development

Successfully merging a pull request may close this issue.

cmd/link/internal/ld: pass correct offset to codesign.Sign oxisto/go
5 participants
@dmitshur @dr2chase @gopherbot @oxisto @cherrymui