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

ASLR (pie) disabled in v12 Linux amd64 build from nodejs.org #33425

Closed
johnandersen777 opened this issue May 15, 2020 · 11 comments
Closed

ASLR (pie) disabled in v12 Linux amd64 build from nodejs.org #33425

johnandersen777 opened this issue May 15, 2020 · 11 comments
Labels
build Issues and PRs related to build files or the CI. feature request Issues that request new features to be added to Node.js. good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. stale

Comments

@johnandersen777
Copy link

johnandersen777 commented May 15, 2020

$ pwn checksec ./node/node-v12.16.3-linux-x64/bin/node
[*] '/home/pdxjohnny/Downloads/node/node-v12.16.3-linux-x64/bin/node'
    Arch:     amd64-64-little
    RELRO:    Full RELRO
    Stack:    Canary found
    NX:       NX enabled
    PIE:      No PIE (0x400000)  # <---- ASLR disabled

I see there was some discussion on this back in 2016, but I couldn't really follow, it also looks like -pie still exists in some of the build files. Just wanted to report in case it's off by mistake.

@addaleax addaleax added the build Issues and PRs related to build files or the CI. label May 15, 2020
@mscdex mscdex changed the title ALSR (pie) disabled in v12 Linux amd64 build from nodejs.org ASLR (pie) disabled in v12 Linux amd64 build from nodejs.org May 16, 2020
@bnoordhuis
Copy link
Member

I can't find the issue but I remember it having been discussed. IIRC, the conclusion was to keep PIE disabled because of the sizable performance hit. It was something like 5 or 10%.

PIE is enabled on macos but that's not a performance-critical platform like linux is. People don't run production servers on their macbooks. (I hope.)

@johnandersen777
Copy link
Author

Lol, ya let's hope. I saw some comments that talked about performance when I went digging through previous PRs and pull requests but I don't remember seeing anywhere where it was decided definitively to disable because of any specific reasoning. If figured if it was intentional then this issue could just be closed and become an easy place to point to with the definitive response. And if it wasn't intentional then it'll get switched on.

@bnoordhuis
Copy link
Member

bnoordhuis commented May 20, 2020

I think the big blocker was ia32/i386: position-independent code is expensive on that architecture because it's register starved.

But we dropped support for ia32 and x64/amd64/x86_64 doesn't have that problem (rip-relative addressing and plenty of registers) so we could turn on PIE if someone is willing to investigate the performance impact.

edit: comparing the benchmark/ benchmark numbers would be a good start. Light testing of native modules (add-ons) would be good too, to check if there are no compatibility issues.

@bnoordhuis bnoordhuis added feature request Issues that request new features to be added to Node.js. good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. labels May 20, 2020
@woodfairy
Copy link
Contributor

Hey, is this still available to work on?

@woodfairy
Copy link
Contributor

woodfairy commented Oct 18, 2020

PIE is enabled on macos but that's not a performance-critical platform like linux is. People don't run production servers on their macbooks. (I hope.)

Bildschirmfoto 2020-10-18 um 20 31 16

I compiled Node based on the Markdown file instructions and then checked the flags with otool -hv. But there is no PIE flag set, neither under my freshly compiled binary, nor under the binary saved via the official macOS installer. Did I miss something here, because you said that PIE is enabled under macOS?

@woodfairy
Copy link
Contributor

woodfairy commented Oct 18, 2020

I have now run some benchmarks with PIE enabled and disabled. As @bnoordhuis already mentioned, you can expect a performance penalty of 5-10%. Here are some results as text file:
nopie_1.txt
nopie_2.txt
nopie_3.txt
pie_1.txt
pie_2.txt
pie_3.txt

Is this too much cost for PIE being enabled? I think at least on OS X this can be activated. I will make a pull request which removes the -no_pie flags.

woodfairy added a commit to woodfairy/node that referenced this issue Oct 19, 2020
After conducting several benchmarks, I noticed performance losses of
5-10%. As OS X is not a performance critical platform, as already
mentioned by @bnoordhuis, I have removed the -no_pie flag at least for
this platform. I'd love to enable PIE for other platforms if the 5-10%
speed loss is not too high. I would be happy to hear your opinion on
this.

Refs: nodejs#33425
nodejs-github-bot pushed a commit that referenced this issue Nov 17, 2020
After conducting several benchmarks, I noticed performance losses of
5-10%. As OS X is not a performance critical platform, as already
mentioned by @bnoordhuis, I have removed the -no_pie flag at least for
this platform. I'd love to enable PIE for other platforms if the 5-10%
speed loss is not too high. I would be happy to hear your opinion on
this.

Refs: #33425

PR-URL: #35704
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@johnandersen777
Copy link
Author

Should this be closed? The issue was about amd64 on Linux

@Mesteery Mesteery reopened this Feb 17, 2022
@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Aug 17, 2022
@github-actions
Copy link
Contributor

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@jvoisin
Copy link

jvoisin commented Oct 10, 2024

The lack of ASLR on the .text segment of the nodejs binary was exploited by SonarSource recently.

@johnandersen777
Copy link
Author

johnandersen777 commented Oct 11, 2024

“feature request” my ass! We all knew this was a vuln waiting to happen!!

Why is this issue still closed???

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. feature request Issues that request new features to be added to Node.js. good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. stale
Projects
None yet
Development

No branches or pull requests

6 participants