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 React Native Shared Objects with Stack Smash Protection #38018

Closed
wants to merge 1 commit into from

Conversation

SparshaSaha
Copy link
Contributor

Summary:

Some of the .so files in React Native does not seem to be stack protected. We figure that out by checking for "__stack_chk_fail" string inside the binary files.

This was probably happening because we were not using the "-fstack-protector" flag.

The fix I have imagined involves adding a "-fstack-protector-all" flag in the compile options in the CMake files.

Changelog:

[ANDROID][ADDED] - Added -fstack-protector-all flag in CMakeLists.txt for ReactAndroid and ReactCommon.

Test Plan:

One of the ways to test this would be to generate the rn-tester app from this branch and following these steps:
#36870 (comment)

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 22, 2023
@SparshaSaha SparshaSaha changed the title Added -f-stack-protector-all Build React Native Shared Objects with Stack Smash Protection Jun 22, 2023
@kelset kelset requested a review from cortinico June 22, 2023 10:36
@kelset kelset added Platform: Android Android applications. p: Microsoft Partner: Microsoft Partner labels Jun 22, 2023
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,036,875 +70,445
android hermes armeabi-v7a 8,359,314 +131,702
android hermes x86 9,551,081 +70,993
android hermes x86_64 9,393,621 +71,064
android jsc arm64-v8a 9,583,699 +56,343
android jsc armeabi-v7a 8,769,028 +103,139
android jsc x86 9,668,085 +56,176
android jsc x86_64 9,916,452 +57,976

Base commit: 8ddb334
Branch: main

Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Sadly this change can't be applied as it is. The -fstack-protector flag is provided the by the Android NDK and we can't just re-apply it. We should fix how we invoke the NDK instead and how we bundle libraries so this is just a workaround fix.

@SparshaSaha
Copy link
Contributor Author

@cortinico I see. Do you suggest that we add this as a cppFlag in build.gradle under externalNativeBuild?

@cortinico
Copy link
Contributor

Nope we need to understand why the NDK is not setting this flag. We don't need to specify it manually

@SparshaSaha
Copy link
Contributor Author

@cortinico I am not sure if NDK toolchain would automatically set the stack-protector flag by default because adding that flag might cause a size increase and might impact perf negatively . I thought developers are supposed to mention these flags as required. Is there any documentation around this which i can read up on?

@cortinico
Copy link
Contributor

@cortinico I am not sure if NDK toolchain would automatically set the stack-protector flag by default because adding that flag might cause a size increase and might impact perf negatively . I thought developers are supposed to mention these flags as required. Is there any documentation around this which i can read up on?

It does. The list of flags that the NDK sets automatically for a release build of 0.72 are:

-g -DANDROID -fdata-sections -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -D_FORTIFY_SOURCE=2 -march=armv7-a -mthumb -Wformat -Werror=format-security   -Os -DNDEBUG -fPIE

More on this here also: android/ndk#1693

So -fstack-protector-strong is there.

@jimmycd
Copy link

jimmycd commented Jul 7, 2023

we are blocked in a huge public program because of the missing stack protection of reactive native. Any forecast by when a version will be available fully supporting this security requirment?

@cortinico
Copy link
Contributor

we are blocked in a huge public program because of the missing stack protection of reactive native. Any forecast by when a version will be available fully supporting this security requirment?

We'll get back to this after the NDK 25 bump: #37974

Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Closing as for #36870 (comment)

@cortinico cortinico closed this Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Microsoft Partner: Microsoft Partner Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants