feat: use `objcopy` instead of `LIEF` #64

RaisinTen · 2022-11-03T08:36:03Z

Fixes: #63
Signed-off-by: Darshan Sen raisinten@gmail.com

Fixes: #63 Signed-off-by: Darshan Sen <raisinten@gmail.com>

RaisinTen · 2022-11-03T08:46:41Z

test/cli.mjs

@@ -95,7 +112,6 @@ describe("postject CLI", () => {
          console.log(err.message);
        }
        if (codesignFound) {
-          execSync(`codesign --sign - ${filename}`);


For some reason, code signing the binary after running objcopy --add-section on macOS causes the binary to crash:

$ ./node [1] 5366 killed ./node $ echo $? 137

and I can't even lldb into it:

lldb -- ./node (lldb) target create "./node" Current executable set to '/Users/raisinten/Desktop/git/postject/node' (x86_64). (lldb) run error: Cannot allocate memory (lldb) ^D

However, it seems to be possible to run objcopy after code signing and not invalidate the code signature! I also tried to check if the new section was being added after the LC_CODE_SIGNATURE section that @bnoordhuis mentioned in nodejs/node#45066 (comment) and it seems like objcopy adds new sections at the end, even after LC_CODE_SIGNATURE but the codesign tool thinks that this is valid:

$ otool -l ./node | tail -n29 Load command 18 cmd LC_CODE_SIGNATURE cmdsize 16 dataoff 84875008 datasize 663232 Load command 19 cmd LC_SEGMENT_64 cmdsize 152 segname __POSTJECT vmaddr 0x00000001057d4000 vmsize 0x0000000000001000 fileoff 68509696 filesize 4096 maxprot 0x00000007 initprot 0x00000007 nsects 1 flags 0x0 Section sectname __NODE_JS_CODE segname __POSTJECT addr 0x00000001057d4000 size 0x0000000000000014 offset 68509696 align 2^0 (1) reloff 0 nreloc 0 flags 0x00000000 reserved1 0 reserved2 0

$ codesign -v ./node $ echo $? 0

Any clue what's happening here?

For some reason, code signing the binary after running objcopy --add-section on macOS causes the binary to crash

A binary with segments after LC_CODE_SIGNATURE is not valid for code signing. There are certain requirements for a Mach-O executable being signable, usually when they're not met the codesign tool you get the error "main executable failed strict validation", not sure why it's not happening in this case.

[1] 5366 killed ./node

If you check dmesg you can get more information on why it was killed. On macOS when an executable is killed like that immediately on start (killed, not crashing) then it is probably the kernel killing it, and there will be a message logged as to why it killed it.

I wasn't able to reproduce this case (I can see the segment is after LC_CODE_SIGNATURE) in a quick test, but if you can reliably reproduce, check dmesg to find more information on why it was killed.

robertgzr · 2022-11-03T10:43:35Z

src/api.js

-      }
-      break;
+      execSync(
+        `/usr/local/opt/llvm/bin/llvm-objcopy --add-section ${machoSegmentName},__${resourceName}=${resource} ${filename}`


can we just use llvm-objcopy from $PATH?

brew install llvm does not add it to the path

hmm. this will probably not work on linux, on my system the llvm install is located under /usr/lib/llvm/14/

correct, the path (even the binary name) is different on linux. I haven't implemented that part yet, will do that soon.

src/api.js

jviotti · 2022-11-03T14:05:11Z

@RaisinTen I thought the agreement from the last call is that we would NOT do this on Postject.

RaisinTen · 2022-11-03T14:25:41Z

@jviotti we didn't consider a point during the meeting. I posted about it yesterday in the issue - #63 (comment).

Refs: #64 (comment) Signed-off-by: Darshan Sen <raisinten@gmail.com>

jviotti · 2022-11-04T13:14:25Z

I thought the conclusion was:

We want Postject to inject across platforms (you raised valid use cases like M1 not being available on CI, etc)
The first point is a must for integrating Postject to i.e. PKG
My though was to consider the LLVM approach only for testing purposes within Node.js core as an alternative to integrating Postject under test/fixtures

cc @tony-go @mhdawson in case you can confirm whether I got it wrong or not

RaisinTen · 2022-11-04T13:18:47Z

Correct but I forgot to mention a point during the meeting - #63 (comment):

the cross-platform binary creation might actually not even work if your project has native bindings

i.e., while testing out pkg I found that if it produces an ELF file when you're packaging an app with native dependencies on macOS, the .node files would still be Mach-O binaries, so it won't run on Linux

Does this change anything?

jviotti · 2022-11-04T14:45:44Z

Yeah, that's very interesting. If so, I'm fine with the objcopy approach, as users will need to build the CLI on each platform anyway to cover all possible edge cases (i.e. native add-ons).

dsanders11

Left some comments about the implementation on this PR, but I’m generally -1 on this change, see my comments on #63.

Another general comment on this PR, the commit message isn’t accurate for what the PR does - first it should specifically mention llvm-objcopy, and second it should specify that this PR is macOS only.

dsanders11 · 2022-11-08T23:33:35Z

src/api.js

-        }
-      }
-      break;
+      await execFile("/usr/local/opt/llvm/bin/llvm-objcopy", [


This path shouldn't be hardcoded, since it depends on the user's system. For example, on my system after doing brew install llvm, I do not have that path, it says it didn't link it since there's already system binaries which it would conflict with.

The more robust option would be to try to find the binary, and also allow an option or environment variable to override the path.

dsanders11 · 2022-11-08T23:54:02Z

test/cli.mjs

@@ -95,7 +112,6 @@ describe("postject CLI", () => {
          console.log(err.message);
        }
        if (codesignFound) {
-          execSync(`codesign --sign - ${filename}`);


For some reason, code signing the binary after running objcopy --add-section on macOS causes the binary to crash

A binary with segments after LC_CODE_SIGNATURE is not valid for code signing. There are certain requirements for a Mach-O executable being signable, usually when they're not met the codesign tool you get the error "main executable failed strict validation", not sure why it's not happening in this case.

[1] 5366 killed ./node

If you check dmesg you can get more information on why it was killed. On macOS when an executable is killed like that immediately on start (killed, not crashing) then it is probably the kernel killing it, and there will be a message logged as to why it killed it.

I wasn't able to reproduce this case (I can see the segment is after LC_CODE_SIGNATURE) in a quick test, but if you can reliably reproduce, check dmesg to find more information on why it was killed.

dsanders11 · 2022-11-08T23:56:35Z

src/api.js

-        if (result === postject.InjectResult.kAlreadyExists) {
-          throw new Error(
-            `Segment and section with that name already exists: ${machoSegmentName}/${sectionName}\n` +
-              "Use --overwrite to overwrite the existing content"
-          );
-        }


The changes in this PR break --overwrite since it doesn't check if the section exists.

I believe you'd want to use llvm-objcopy --update-section to get the overwrite functionality.

dsanders11 · 2022-11-09T00:01:06Z

src/postject.cpp

-      // Create the segment and mark it read-only
-      LIEF::MachO::SegmentCommand new_segment(segment_name);
-      new_segment.max_protection(
-          static_cast<uint32_t>(LIEF::MachO::VM_PROTECTIONS::VM_PROT_READ));
-      new_segment.init_protection(
-          static_cast<uint32_t>(LIEF::MachO::VM_PROTECTIONS::VM_PROT_READ));
-      new_segment.add_section(section);


This PR doesn't mark the segment as read-only, instead the segment will get the default protections, read/write/execute. Part of the security model of Postject is that the resources are injected as read-only to ensure they aren't at risk of being modified in a running executable, which the OS providing that automatically since the loaded segments are read-only and so the virtual memory pages will be read-only.

There is a --set-section-flags option which could be used simultaneously to make the segment read-only, but trying to use it on a Mach-O executable tells me: llvm-objcopy: error: option is not supported for MachO.

feat: use objcopy instead of LIEF

4441c2e

Fixes: #63 Signed-off-by: Darshan Sen <raisinten@gmail.com>

RaisinTen requested review from dsanders11, jviotti and robertgzr as code owners November 3, 2022 08:36

RaisinTen mentioned this pull request Nov 3, 2022

Internally use objcopy --add-section on macOS and Linux #63

Closed

RaisinTen commented Nov 3, 2022

View reviewed changes

robertgzr reviewed Nov 3, 2022

View reviewed changes

addaleax reviewed Nov 3, 2022

View reviewed changes

src/api.js Outdated Show resolved Hide resolved

fix: stop using template string for exec

bb3bed8

Refs: #64 (comment) Signed-off-by: Darshan Sen <raisinten@gmail.com>

dsanders11 requested changes Nov 9, 2022

View reviewed changes

RaisinTen closed this Nov 9, 2022

RaisinTen deleted the use-objcopy-instead-of-LIEF branch November 9, 2022 07:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use `objcopy` instead of `LIEF` #64

feat: use `objcopy` instead of `LIEF` #64

RaisinTen commented Nov 3, 2022

RaisinTen Nov 3, 2022

dsanders11 Nov 8, 2022

robertgzr Nov 3, 2022

RaisinTen Nov 3, 2022

robertgzr Nov 3, 2022 •

edited

RaisinTen Nov 3, 2022

jviotti commented Nov 3, 2022

RaisinTen commented Nov 3, 2022

jviotti commented Nov 4, 2022

RaisinTen commented Nov 4, 2022

jviotti commented Nov 4, 2022

dsanders11 left a comment

dsanders11 Nov 8, 2022

dsanders11 Nov 8, 2022

dsanders11 Nov 8, 2022

dsanders11 Nov 9, 2022

feat: use objcopy instead of LIEF #64

feat: use objcopy instead of LIEF #64

Conversation

RaisinTen commented Nov 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertgzr Nov 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jviotti commented Nov 3, 2022

RaisinTen commented Nov 3, 2022

jviotti commented Nov 4, 2022

RaisinTen commented Nov 4, 2022

jviotti commented Nov 4, 2022

dsanders11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: use `objcopy` instead of `LIEF` #64

feat: use `objcopy` instead of `LIEF` #64

robertgzr Nov 3, 2022 •

edited