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

Upstream backed doesn't work with "-fstack-protector" #9780

Closed
VirtualTim opened this issue Nov 5, 2019 · 9 comments · Fixed by #22229
Closed

Upstream backed doesn't work with "-fstack-protector" #9780

VirtualTim opened this issue Nov 5, 2019 · 9 comments · Fixed by #22229

Comments

@VirtualTim
Copy link
Collaborator

VirtualTim commented Nov 5, 2019

I don't know if this is something we care about. This compiles without error in the fastcomp backed (but I haven't actually verified it does anything).
In the upstream backend I get multiple errors like:
wasm-ld: error: CMakeFiles/path/to/file.cpp.o: undefined symbol: __stack_chk_guard

Emscripten 1.39.0

@sbc100
Copy link
Collaborator

sbc100 commented Nov 5, 2019

Sounds like its worth having for completeness. Whole classes of stack smashing attacks are already rendered useless in webassembly due to the fact the control stack is not stored in memory, but I'm sure there are still many others.

@bvibber
Copy link
Collaborator

bvibber commented Nov 5, 2019

Yes, one can still attack the program logic by altering values on stack... including the possibility of overwriting function pointers.

@kripken
Copy link
Member

kripken commented Nov 5, 2019

We did add a SAFE_STACK option, which runs a binaryen pass. Maybe that overlaps with -fstack-protector? Not sure if we looked into the feasibility of using that option.

attilaolah pushed a commit to attilaolah/wasm that referenced this issue Sep 14, 2020
By default, Bazel passes in `-fstack-protector`, which results in build
errors due to emscripten-core/emscripten#9780.

Passing in `--copts=-fno-stack-protector` fixes the issue when compiling
with the Emscripten toolchain.
@stale
Copy link

stale bot commented Nov 5, 2020

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Nov 5, 2020
@sbc100
Copy link
Collaborator

sbc100 commented Nov 5, 2020

We should make this work I think...

@stale
Copy link

stale bot commented Apr 17, 2022

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Apr 17, 2022
@gracicot
Copy link
Contributor

The stalebot is wrong, it would be useful if this one worked

@sbc100 sbc100 removed the wontfix label Jul 11, 2024
@sbc100
Copy link
Collaborator

sbc100 commented Jul 11, 2024

sbc100 added a commit to sbc100/emscripten that referenced this issue Jul 12, 2024
sbc100 added a commit to sbc100/emscripten that referenced this issue Jul 12, 2024
sbc100 added a commit to sbc100/emscripten that referenced this issue Jul 12, 2024
sbc100 added a commit to sbc100/emscripten that referenced this issue Jul 12, 2024
sbc100 added a commit to sbc100/emscripten that referenced this issue Jul 12, 2024
@dschuff
Copy link
Member

dschuff commented Jul 12, 2024

I'm find with @sbc100's change linked above, which enables -fstack-protector in emscripten by linking the necessary runtime functions.

Thinking about this a little bit though, I'm actually kind of surprised this actually works without any LLVM-side work. The documentation suggests that the backend is responsible for placing the stack guard in the right place but I don't recall adding any specific support for this to the wasm backend, and our stack is a bit weird compared to other platforms. It's actually not super obvious what the best place is, since we don't have a return address to protect. I assume the usual/default place would be above all of the locals (i.e. between the locals and the return address). If we do that, we don't protect anything in the current frame, but we do protect the contents of the frames above the current frame.
Another option would be between the statically-sized objects and the dynamically-sized objects in the current frame (see here for a diagram of LLVM's wasm stack frames). If we did that, then we could detect overruns from dynamically-sized objects that clobber other locals but don't go beyond them.
Maybe that's not too useful though; I'd guess that even large stack arrays are mostly statically-sized.
And for completeness, there's not as much point in putting the guard at the bottom: it could detect underruns (which I'm guessing are not as common as overruns?) but there's no live data below to protect.

All that is to say, maybe there's nothing to do in LLVM, other than maybe check where the guard is getting placed, and maybe add a test on the LLVM side.

sbc100 added a commit to sbc100/emscripten that referenced this issue Jul 12, 2024
sbc100 added a commit that referenced this issue Jul 12, 2024
verhovsky pushed a commit to verhovsky/emscripten that referenced this issue Jul 30, 2024
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 a pull request may close this issue.

6 participants