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

Use-After-Free (UAF) Analysis First Iteration #1050

Merged
merged 54 commits into from
Jul 7, 2023

Conversation

mrstanb
Copy link
Member

@mrstanb mrstanb commented May 17, 2023

To sum up the PR infos:

  • Added a UAF analysis to Goblint
  • For this, I also had to add free as a supported library function (similar to malloc)
  • Added a simple C regression test file with a UAF bug

Note: Currently the analysis doesn't seem to be really working properly, hence it's only a first iteration. I'm debugging my way through it to try and find out what's going on and what's wrong.

@michael-schwarz feel free to do a review round

@michael-schwarz michael-schwarz self-requested a review May 26, 2023 14:22
@michael-schwarz
Copy link
Member

Something we should also write tests for: Developers like to have custom wrappers around calls to memory-related functions such as free or malloc to potentially plugin-in a custom allocator. I think we will be precise in presence of such usages, as we are context-sensitive in the pointer argument that is passed to free, but it might still be worth having tests for this.

@mrstanb
Copy link
Member Author

mrstanb commented Jun 23, 2023

There are two ways how to fix this: We immediately go for a solution to this problem in the multi-threaded context, or we add a simple heuristics to basically make the analysis warn whenever something is accessed in multi-threaded mode, and improve precision in a separate PR.

I think I'd go for the second option and add proper multi-threaded support in a separate PR, if we'd prefer to keep this PR rather lean and not too big. What do you think?

@mrstanb
Copy link
Member Author

mrstanb commented Jul 1, 2023

Something we should also write tests for: Developers like to have custom wrappers around calls to memory-related functions such as free or malloc to potentially plugin-in a custom allocator. I think we will be precise in presence of such usages, as we are context-sensitive in the pointer argument that is passed to free, but it might still be worth having tests for this.

Should be now covered by 9e5cd7a

@mrstanb
Copy link
Member Author

mrstanb commented Jul 1, 2023

Thank you for the PR, and sorry I am only now getting around to reviewing it in detail.

Here's a first issue: As expected, this does not soundly account for UAF in the concurrent setting yet:

//PARAM: --set ana.activated[+] useAfterFree
#include <stdlib.h>
#include <stdio.h>
#include <pthread.h>

int* gptr;

// Mutex to ensure we don't get race warnings, but the UAF warnings we actually care about
pthread_mutex_t mtx = PTHREAD_MUTEX_INITIALIZER;

void *t_other(void* p) {
    pthread_mutex_lock(&mtx);
    free(gptr); //WARN
    pthread_mutex_unlock(&mtx);
}

int main() {
    gptr = malloc(sizeof(int));
    *gptr = 42;

    pthread_t thread;
    pthread_create(&thread, NULL, t_other, NULL);

    pthread_mutex_lock(&mtx);
    *gptr = 43; //WARN
    free(gptr); //WARN
    pthread_mutex_unlock(&mtx);

    return 0;
}

There are two ways how to fix this: We immediately go for a solution to this problem in the multi-threaded context, or we add a simple heuristics to basically make the analysis warn whenever something is accessed in multi-threaded mode, and improve precision in a separate PR.

As a idea: We might want to record the thread ids of threads (and potentially which threads have already been joined at that point) that free the memory in a global invariant. Then, upon a potentially multi-threaded access, I need to warn if the thread ids do indicate that the free must have happen only after the current function.

I now added this example C program as a regression test for UAF in a multi-threaded context (cf. a00814f)

I also added a simple heuristic that warns for a potential UAF occuring whenver the program is definitely not single-threaded (cf. 9545618)

As already discussed, the proper multi-threaded solution for UAF detection will be implemented in a follow-up PR

@michael-schwarz michael-schwarz marked this pull request as ready for review July 7, 2023 10:38
@michael-schwarz michael-schwarz merged commit 3af00a8 into goblint:master Jul 7, 2023
@michael-schwarz michael-schwarz linked an issue Jul 8, 2023 that may be closed by this pull request
@sim642 sim642 added this to the v2.2.0 milestone Sep 11, 2023
sim642 added a commit to sim642/opam-repository that referenced this pull request Sep 13, 2023
CHANGES:

* Add `setjmp`/`longjmp` analysis (goblint/analyzer#887, goblint/analyzer#970, goblint/analyzer#1015, goblint/analyzer#1019).
* Refactor race analysis to lazy distribution (goblint/analyzer#1084, goblint/analyzer#1089, goblint/analyzer#1136, goblint/analyzer#1016).
* Add thread-unsafe library function call analysis (goblint/analyzer#723, goblint/analyzer#1082).
* Add mutex type analysis and mutex API analysis (goblint/analyzer#800, goblint/analyzer#839, goblint/analyzer#1073).
* Add interval set domain and string literals domain (goblint/analyzer#901, goblint/analyzer#966, goblint/analyzer#994, goblint/analyzer#1048).
* Add affine equalities analysis (goblint/analyzer#592).
* Add use-after-free analysis (goblint/analyzer#1050, goblint/analyzer#1114).
* Add dead code elimination transformation (goblint/analyzer#850, goblint/analyzer#979).
* Add taint analysis for partial contexts (goblint/analyzer#553, goblint/analyzer#952).
* Add YAML witness validation via unassume (goblint/analyzer#796, goblint/analyzer#977, goblint/analyzer#1044, goblint/analyzer#1045, goblint/analyzer#1124).
* Add incremental analysis rename detection (goblint/analyzer#774, goblint/analyzer#777).
* Fix address sets unsoundness (goblint/analyzer#822, goblint/analyzer#967, goblint/analyzer#564, goblint/analyzer#1032, goblint/analyzer#998, goblint/analyzer#1031).
* Fix thread escape analysis unsoundness (goblint/analyzer#939, goblint/analyzer#984, goblint/analyzer#1074, goblint/analyzer#1078).
* Fix many incremental analysis issues (goblint/analyzer#627, goblint/analyzer#836, goblint/analyzer#835, goblint/analyzer#841, goblint/analyzer#932, goblint/analyzer#678, goblint/analyzer#942, goblint/analyzer#949, goblint/analyzer#950, goblint/analyzer#957, goblint/analyzer#955, goblint/analyzer#954, goblint/analyzer#960, goblint/analyzer#959, goblint/analyzer#1004, goblint/analyzer#558, goblint/analyzer#1010, goblint/analyzer#1091).
* Fix server mode for abstract debugging (goblint/analyzer#983, goblint/analyzer#990, goblint/analyzer#997, goblint/analyzer#1000, goblint/analyzer#1001, goblint/analyzer#1013, goblint/analyzer#1018, goblint/analyzer#1017, goblint/analyzer#1026, goblint/analyzer#1027).
* Add documentation for configuration JSON schema and OCaml API (goblint/analyzer#999, goblint/analyzer#1054, goblint/analyzer#1055, goblint/analyzer#1053).
* Add many library function specifications (goblint/analyzer#962, goblint/analyzer#996, goblint/analyzer#1028, goblint/analyzer#1079, goblint/analyzer#1121, goblint/analyzer#1135, goblint/analyzer#1138).
* Add OCaml 5.0 support (goblint/analyzer#1003, goblint/analyzer#945, goblint/analyzer#1162).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generalized Use-After-Free in a Multithreaded setting
3 participants