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

[oxisto]

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?

[gopherbot]

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

[dr2chase]

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

[cherrymui]

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.

[oxisto]

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]

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