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

Add -fstack-protector support to wasi-libc #351

Merged
merged 1 commit into from
Dec 6, 2022

Conversation

kateinoigakukun
Copy link
Contributor

@kateinoigakukun kateinoigakukun commented Dec 5, 2022

Inlcude __stack_chk_fail.c and initialize __stack_chk_guard in ctor.
This allows userland stack smash protection.

$ cat main.c
char input[] = "0123456789012345";
int main(void) {
    char buf[8];

    for (char *sp = input, *dp = buf; *sp != '\0'; sp++, dp++) {
        *dp = *sp;
    }
    return 0;
}

$ clang main.c -fstack-protector
$ wasmtime ./a.out
Error: failed to run main module `./a.out`

Caused by:
    0: failed to invoke command default
    1: wasm trap: wasm `unreachable` instruction executed
       wasm backtrace:
           0:  0x258 - <unknown>!__stack_chk_fail
           1:  0x21e - <unknown>!__original_main
           2:   0xca - <unknown>!_start

Inlcude `__stack_chk_fail.c` and initialize `__stack_chk_guard` in ctor.

```
$ cat main.c
char input[] = "0123456789012345";
int main(void) {
    char buf[8];

    for (char *sp = input, *dp = buf; *sp != '\0'; sp++, dp++) {
        *dp = *sp;
    }
    return 0;
}

$ clang main.c -fstack-protector
$ wasmtime ./a.out
Error: failed to run main module `./a.out`

Caused by:
    0: failed to invoke command default
    1: wasm trap: wasm `unreachable` instruction executed
       wasm backtrace:
           0:  0x258 - <unknown>!__stack_chk_fail
           1:  0x21e - <unknown>!__original_main
           2:   0xca - <unknown>!_start
```
@sbc100
Copy link
Member

sbc100 commented Dec 5, 2022

Seems reasonable.

IIUC -fstack-protector is about detecting cases of overflowing the individual stack frame. Does it detect stack overflow too? i.e. valid programs with too-deep recursion?

Is suppose the security benefits of -fstack-protector in the wasm world are fairly minimal (compared to native platforms)? But it can still be useful for detect program bugs?

@pchickey
Copy link
Collaborator

pchickey commented Dec 5, 2022

This uses wasi random_get at initialization time, which could make it impossible to run wizer on programs which expect to run a bunch of "constant" initialization code. Is there a benefit to using randomness for stack protection on the wasm platform? I can understand how that would be desirable on platforms where you want to randomize virtual memory layout, but I don't know what it is used for here.

@TerrorJack
Copy link
Contributor

Would be nice to make it optional via a Makefile flag; I see value in -fstack-protector support, by I'd also like to make it easy to produce sysroots without the extra overhead in non-debug builds.

@kateinoigakukun
Copy link
Contributor Author

kateinoigakukun commented Dec 6, 2022

@sbc100

IIUC -fstack-protector is about detecting cases of overflowing the individual stack frame. Does it detect stack overflow too? i.e. valid programs with too-deep recursion?

It doesn't detect stack overflow. wasm-ld's --stack-first can be used for the purpose, but...

Is suppose the security benefits of -fstack-protector in the wasm world are fairly minimal (compared to native platforms)? But it can still be useful for detect program bugs?

Yes, I agree that the security benefit would be small. TBH, my main motivation is just reducing if(wasm) in toolchains like this. But I think it's still useful to detect BOF bugs in C/C++ on Wasm.

@pchickey

This uses wasi random_get at initialization time, which could make it impossible to run wizer on programs which expect to run a bunch of "constant" initialization code.

Good point. To be precise, the ctor is included only when stack protector is enabled in user code, so it can be an issue when wizer and -fstack-protector are used for an executable.

Is there a benefit to using randomness for stack protection on the wasm platform? I can understand how that would be desirable on platforms where you want to randomize virtual memory layout, but I don't know what it is used for here.

The random seed is used for randomizing the canary. I think it's still useful without ASLR to prevent bypassing guards by overwriting statically known canary.

@TerrorJack
Currently, the __stack_chk_fail.c is always compiled in wasi-libc build system, but it's linked only when -fstack-protector is enabled. Do you think we still need additional flag in Makefile?

@sbc100
Copy link
Member

sbc100 commented Dec 6, 2022

The Makefile already supports EXTRA_CFALGS=xxx for folks that might want to compile libc itself with -fstack-protector enabled.

@TerrorJack
Copy link
Contributor

Ah, I just experimented compiling with this patch and discovered there's no extra overhead if none of libc or user code is compiled with -fstack-protector, so it seems as long as the __stack_chk_fail symbol is not reachable, the object file will not be linked so that extra ctor will not come into play. In that case, no need for any extra Makefile logic then!

@kateinoigakukun
Copy link
Contributor Author

@TerrorJack Yes! Thank you for experimenting on your side :)

@sunfishcode
Copy link
Member

@TerrorJack Thanks for doing that experiment! This looks good.

@sunfishcode sunfishcode merged commit 8b7148f into WebAssembly:main Dec 6, 2022
@kateinoigakukun kateinoigakukun deleted the stack-protector branch December 6, 2022 18:30
john-sharratt pushed a commit to john-sharratt/wasix-libc that referenced this pull request Mar 6, 2023
Inlcude `__stack_chk_fail.c` and initialize `__stack_chk_guard` in ctor.

```
$ cat main.c
char input[] = "0123456789012345";
int main(void) {
    char buf[8];

    for (char *sp = input, *dp = buf; *sp != '\0'; sp++, dp++) {
        *dp = *sp;
    }
    return 0;
}

$ clang main.c -fstack-protector
$ wasmtime ./a.out
Error: failed to run main module `./a.out`

Caused by:
    0: failed to invoke command default
    1: wasm trap: wasm `unreachable` instruction executed
       wasm backtrace:
           0:  0x258 - <unknown>!__stack_chk_fail
           1:  0x21e - <unknown>!__original_main
           2:   0xca - <unknown>!_start
```
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 this pull request may close these issues.

None yet

5 participants