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

build: add '-z relro -z now' linker flags #20513

Closed
wants to merge 1 commit into from

Conversation

tingshao
Copy link
Contributor

@tingshao tingshao commented May 4, 2018

These flags could make some sections and the GOT entries of node
process read only to avoid being modified after dynamic linking is
done, thus the security could be enhanced.

Fixes: #20122

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

These flags could make some sections and the GOT entries of node
process read only to avoid being modified after dynamic linking is
done, thus the security could be enhanced.

Fixes: nodejs#20122
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

@bnoordhuis bnoordhuis added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 4, 2018
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 5, 2018
@tingshao
Copy link
Contributor Author

tingshao commented May 8, 2018

@bnoordhuis Thanks for trigger the CI test.
It's quite strange that it failed on AIX+PPC64, as I think this change should be irrelevant to AIX.
Previously, I thought I can have a try on AIX+PPC with Qemu, but found AIX is not freeware. Do you have some good suggestions?

@bnoordhuis
Copy link
Member

@tingshao Looks like a flaky test, probably nothing to worry about. citgm looks okay too, insofar as I can decode the results.

@addaleax
Copy link
Member

Landed in 2d4dd10 🎉

@addaleax addaleax closed this May 14, 2018
addaleax pushed a commit that referenced this pull request May 14, 2018
These flags could make some sections and the GOT entries of node
process read only to avoid being modified after dynamic linking is
done, thus the security could be enhanced.

Fixes: #20122
PR-URL: #20513
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build: Add Data Relocation and Protection flags (-z relro -z now) to address potential security issue
8 participants