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

fix: crash in postject_find_resource() on Linux #77

Merged
merged 5 commits into from
Mar 3, 2023

Conversation

RaisinTen
Copy link
Contributor

@RaisinTen RaisinTen commented Feb 27, 2023

The program headers base address values returned by getauxval(AT_PHDR) and dl_iterate_phdr() are identical only on
g++ (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0. However, the values are totally different on clang version 10.0.0-4ubuntu1 and g++ (GCC) 8.5.0 20210514 (Red Hat 8.5.0-16).
Since the dl_iterate_phdr() approach seems to work for all the 3 compilers, I think we should proceed with that.

Turns out, this happens because PIE is not enabled by default on the compilers which produce binaries that are crashing.

Fixes: #70
Refs: #76 (actually Fixes: but I'll add Fixes: on the postject version upgrade PR in Node.js)

(Node.js PR to test this - nodejs/node#46868)

The program headers base address values returned by `getauxval(AT_PHDR)`
and `dl_iterate_phdr()` are identical only on
`g++ (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0`. However, the values are
totally different on `clang version 10.0.0-4ubuntu1` and
`g++ (GCC) 8.5.0 20210514 (Red Hat 8.5.0-16)`.
Since the `dl_iterate_phdr()` approach seems to work for all the 3
compilers, I think we should proceed with that.

Fixes: #70
Refs: #76
Signed-off-by: Darshan Sen <[email protected]>
postject-api.h Outdated Show resolved Hide resolved
Copy link
Contributor

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing a changes requested on this because I don't think this is the right approach for the issue we're seeing and want to fix.

In my initial testing, the differences and crashes are arising from Position Independent Executable (PIE) vs non-PIE output. I'm seeing on Ubuntu 22.04 the crash case from #70 does not repro on either g++ or clang++, and file reports that both executables are PIE. On Ubuntu 20.04, clang++ is reporting "ELF 64-bit LSB executable" (non-PIE), while g++ is, confusingly, reporting "ELF 64-bit LSB shared object" - and shared objects are Position Independent Code (PIC) by default, AFAIK.

The crash from #70 can be repro'd with g++ by telling it to not link as PIE (g++ -no-pie test.cc), at which point the output file also reports as "ELF 64-bit LSB executable" and the crash repros.

I'm still further investigating, but I don't believe this is necessarily the right solution to the problem. It changes behavior since it will iterate through all shared objects loaded for the program, and would find notes in shared libraries, which could lead to buggy behavior, and/or open security concerns.

The resource gets injected in the main executable, so there is no need
to iterate the other shared libraries that are loaded by the program.
This also resolves a security concern.

Refs: #77 (review)
Signed-off-by: Darshan Sen <[email protected]>
@RaisinTen
Copy link
Contributor Author

RaisinTen commented Mar 2, 2023

It is indeed a PIE vs non-PIE issue, good catch!

It changes behavior since it will iterate through all shared objects loaded for the program, and would find notes in shared libraries, which could lead to buggy behavior, and/or open security concerns.

From my testing, it looks like running the callback only on the program headers of the current executable works and I think it addresses your blocking concern. I've pushed a commit for that.

@RaisinTen RaisinTen requested a review from dsanders11 March 2, 2023 06:29
@robertgzr
Copy link
Member

robertgzr commented Mar 2, 2023

unless we just want to avoid the callback mechanism here, using the approach from this PR looks better.

I was playing around with the parsing code and read the elf spec again, specifically how it behaves when PIE come into play.

phdr->p_vaddr will be relative when compiling with -pie which is the default, and absolute when -no-pie, but the original implementation unconditionally adds base_addr to it :P

so we could fix it by checking if (p_vaddr - p_offset) == 0 and only then adding base_addr, but it's nice that dl_iterate_phdr already takes PIE/non-PIE into account

Copy link
Contributor

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having dug into this further, I'm good with the change to use dl_iterate_phdr now that it has been scoped to only get the struct dl_phdr_info for the main program.

I think the current version of this PR makes it more concise what is being changed - we're changing from getauxval to dl_iterate_phdr for determining the three main values we need on Linux: pointer to the start of the program header, the number of program headers, and the base address in memory.

Should be more robust using dl_iterate_phdr since that takes into account the relocation address for PIEs, while getauxval(AT_PHDR) did not, which resulted in these crashes.

There's also an upstream Linux kernel bug (https://bugzilla.kernel.org/show_bug.cgi?id=197921) that suggests AT_PHDR may be incorrect sometimes, so more reason to move away from it.

From a portability perspective, I don't think we're any worse off using dl_iterate_phdr vs getauxval - if anything it might be better. https://gnu.org/ lists portability issues with both of them, but the list for dl_iterate_phdr seems better than the list for getauxval.

I think follow-up research is warranted on the usage of #define _GNU_SOURCE, and we should document that as needed depending on what we find.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@RaisinTen RaisinTen merged commit e6f79ac into main Mar 3, 2023
@RaisinTen RaisinTen deleted the fix-crash-in-postject_find_resource-on-linux branch March 3, 2023 05:59
@ShenHongFei
Copy link

I verified it with the latest Node.js 19.8.1 version, run postject from the windows system, inject my-app.js into the linux node executable file, generate my-app, and then run it successfully under the linux system, no segfault

@targos
Looks like this issue has been successfully fixed
#76

[root@vm ~]# ./my-app
{
  node: '19.8.1',
  acorn: '8.8.2',
  ada: '1.0.4',
  ares: '1.19.0',
  brotli: '1.0.9',
  cldr: '42.0',
  icu: '72.1',
  llhttp: '8.1.0',
  modules: '111',
  napi: '8',
  nghttp2: '1.52.0',
  nghttp3: '0.7.0',
  ngtcp2: '0.8.1',
  openssl: '3.0.8+quic',
  simdutf: '3.2.2',
  tz: '2022g',
  undici: '5.21.0',
  unicode: '15.0',
  uv: '1.44.2',
  uvwasi: '0.0.16',
  v8: '10.8.168.25-node.12',
  zlib: '1.2.13'
}
[ '/root/my-app', './my-app' ]
(node:394) ExperimentalWarning: Single executable application is an experimental feature and might change at any time
(Use `my-app --trace-warnings ...` to show where the warning was created)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calling postject_find_resource() segfaults on rhel8-ppc64le
6 participants