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

Enable pointer authentication on ARM64 #42888

Closed
jgowdy opened this issue Apr 27, 2022 · 8 comments · Fixed by #51256
Closed

Enable pointer authentication on ARM64 #42888

jgowdy opened this issue Apr 27, 2022 · 8 comments · Fixed by #51256
Labels
feature request Issues that request new features to be added to Node.js. stale

Comments

@jgowdy
Copy link
Contributor

jgowdy commented Apr 27, 2022

What is the problem this feature will solve?

ARM64v8.3 supports Pointer Authentication with the PACIASP and AUTIASP instructions which are interpreted as NOP instructions on pre 8.3 architectures. These instructions sign the stack pointer and validate the stack pointer prior to return to mitigate return oriented programming.

GCC supports these options on arm64 / aarch64. The legacy option was -msign-return-address=[all | non-leaf | none] and the modern option is -mbranch-protection=none|standard|pac-ret[+leaf+b-key]|bti

I would like to suggest that the arm64 build be modified to include -mbranch-protection=pac-ret with the -march being set to ARMv8.2 or earlier or not configured, so that GCC will generate PACIASP and AUTIASP instructions. It is critical that -march=armv8.3 or higher not be passed or the non-backwards compatible RETAA instruction will be generated.

What is the feature you are proposing to solve the problem?

The benefit of enabling pointer authentication for the stack pointer on ARM64 would be to mitigate return oriented programming attacks against the Node.js runtime.

What alternatives have you considered?

Presently we are pursuing custom compiles of the Node.js runtime for the new Graviton3 CPUs that support pointer authentication in AWS.

@jgowdy jgowdy added the feature request Issues that request new features to be added to Node.js. label Apr 27, 2022
@RaisinTen
Copy link
Contributor

Will this affect performance?
What will happen if this setting is enabled and someone still attempts to take advantage of an already present buffer overflow?

@jgowdy
Copy link
Contributor Author

jgowdy commented May 5, 2022

The performance impact is very small, we've had difficulty measuring it versus variance in runs of our benchmark suite. It's an additional assembly language instruction at the beginning and end of each method that does a small amount of math in hardware. If the CPU doesn't support PAC, the instructions are treated as NOPs.

If the stack pointer is modified by an attacker, when the function is about to return it executes the AUTIASP instruction which will detect the modified stack pointer and the process will signal and abort.

@RaisinTen
Copy link
Contributor

Hmm, I don't immediately see any problem with enabling this. Would you like to send a PR? We could have more reviews there.

@nxhack
Copy link

nxhack commented Aug 26, 2022

This modification results in an error in the case of cross-compilation. (#43200)

'host_arch': 'x64'
'target_arch': 'arm64',

g++: error: unrecognized command line option '-msign-return-address=all'

sunoru added a commit to sunoru/Yggdrasil that referenced this issue Nov 15, 2022
giordano pushed a commit to JuliaPackaging/Yggdrasil that referenced this issue Nov 15, 2022
…nd mac builds. (#5859)

* Update for building NodeJS v18.12.1. Add scripts for w64 and mac.

* Try building with trap-handler on aarch64-linux.

* chmod +x

* Disable pointer authentication on arm 64. (nodejs/node#42888)

* Include a header to use `memalign`.
sparist added a commit to Distributive-Network/node that referenced this issue Dec 1, 2022
@josh-hemphill
Copy link

Is there a workaround for this unrecognized command line option '-msign-return-address=all' error?

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Dec 6, 2022
Commit 938212f added -msign-return-address=all to _all_ cflags but that
is wrong when cross-compiling, it should only be added to the target's
cflags.

Fixes: nodejs#42888
@bnoordhuis
Copy link
Member

Untested, but #45756 hopefully fixes it. Please test.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 5, 2023

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 Jun 5, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2023

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.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 5, 2023
sparist added a commit to Distributive-Network/node that referenced this issue Aug 29, 2023
sparist added a commit to Distributive-Network/node that referenced this issue Sep 10, 2023
sparist added a commit to Distributive-Network/node that referenced this issue Sep 13, 2023
targos added a commit to targos/node that referenced this issue Dec 22, 2023
Commit 938212f added -msign-return-address=all to _all_ cflags but that
is wrong when cross-compiling, it should only be added to the target's
cflags.

The flag being deprecated, it is also changed to
`-mbranch-protection=standard`.

Fixes: nodejs#42888
Co-Authored-By: Michaël Zasso <[email protected]>
nodejs-github-bot pushed a commit that referenced this issue Dec 27, 2023
Commit 938212f added -msign-return-address=all to _all_ cflags but that
is wrong when cross-compiling, it should only be added to the target's
cflags.

The flag being deprecated, it is also changed to
`-mbranch-protection=standard`.

Fixes: #42888
Co-Authored-By: Michaël Zasso <[email protected]>
PR-URL: #51256
Fixes: nodejs/build#3319
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
RafaelGSS pushed a commit that referenced this issue Jan 2, 2024
Commit 938212f added -msign-return-address=all to _all_ cflags but that
is wrong when cross-compiling, it should only be added to the target's
cflags.

The flag being deprecated, it is also changed to
`-mbranch-protection=standard`.

Fixes: #42888
Co-Authored-By: Michaël Zasso <[email protected]>
PR-URL: #51256
Fixes: nodejs/build#3319
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Commit 938212f added -msign-return-address=all to _all_ cflags but that
is wrong when cross-compiling, it should only be added to the target's
cflags.

The flag being deprecated, it is also changed to
`-mbranch-protection=standard`.

Fixes: #42888
Co-Authored-By: Michaël Zasso <[email protected]>
PR-URL: #51256
Fixes: nodejs/build#3319
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
marco-ippolito pushed a commit that referenced this issue May 2, 2024
Commit 938212f added -msign-return-address=all to _all_ cflags but that
is wrong when cross-compiling, it should only be added to the target's
cflags.

The flag being deprecated, it is also changed to
`-mbranch-protection=standard`.

Fixes: #42888
Co-Authored-By: Michaël Zasso <[email protected]>
PR-URL: #51256
Fixes: nodejs/build#3319
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. stale
Projects
None yet
5 participants