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

Memory hardening #2729

Open
arr2036 opened this issue Jun 4, 2019 · 5 comments
Open

Memory hardening #2729

arr2036 opened this issue Jun 4, 2019 · 5 comments
Labels
feature enhancement category: a new feature (an extension of functionality) refactoring & design category: related to rework of a feature or internal API v4.0.x meta: relates to the v4.0.x branch

Comments

@arr2036
Copy link
Member

arr2036 commented Jun 4, 2019

For FR_TYPE_SECRET, buffers should be allocated from an mlocked pool, which should secure memset the objects to zero when they're freed.

The pool should be page aligned and reuse the same code as the mprotected pages.

Related thoughts

  • It also might be a good idea to implement a new API which allows allocation of memory fragments, and tracks the allocations in a linked list, as phase 0 then, allocates a page aligned talloc pool to hold all those allocations, and copies them over. This would allow us to get rid of the explicit mprotect pool size, which was always a dumb hack, and not require an explicit pool size for secret memory.

  • We can coarsely intercept memory allocations in OpenSSL that may be dealing with things like private keys. Other allocations may get caught in this, but that's not a huge issue.

  • We need page aligned memory for mprotect, madvise, mlock, mmap, mremap.

  • We might want to use madvise on the pages used for the ring buffers for packet processing, to allow aggressive readahead.

@arr2036
Copy link
Member Author

arr2036 commented Jun 4, 2019

@terryburton this relates to some of the things we were talking about. Do you have any other thoughts on memory hardening strategies, other than secure memset on free and making sure sensitive memory doesn't get paged? Might be worth having a look at SME, and seeing if we can enable that on the relevant pages, I think it'd prevent cold boot attacks...

There's also memguard in go, but I'm not sure what mechanism it's actually using to do memory encryption and whether it's go specific.

@arr2036 arr2036 added feature enhancement category: a new feature (an extension of functionality) refactoring & design category: related to rework of a feature or internal API labels Jun 6, 2019
@FreeRADIUS FreeRADIUS deleted a comment from Enrique922 Nov 20, 2019
@FreeRADIUS FreeRADIUS deleted a comment from Enrique922 Nov 20, 2019
@alandekok
Copy link
Member

We could:

  • add flags->secret to dict.h
  • set it in the RADIUS code, for encrypted attributes
  • update _fr_pair_free() to check for the flags
  • if set, use memset_s() to wipe the secret data

That works for VALUE_PAIR. We would need to add something similar for CONF_PAIR:

  • add bool secret, which is set when the CONF_PARSER has FR_TYPE_SECRET
  • add _cf_pair_free() function as a talloc destructor
  • if cp->secret, call memset_s()

realistically, that's much less useful than the VALUE_PAIR changes. The CONF_PAIRs are generally never freed while the server is running. Because the server usually needs continuous access to the data.

@arr2036
Copy link
Member Author

arr2036 commented Sep 3, 2020

Sure. I added some functions in talloc.c to add an arbitrary number of destructors to a talloc chunk using the linking ctx stuff. We could just add another generic function in that source file to add a destructor which calls memset_s on the chunk when it's freed.

That way it's really easy to sprinkle around the code, and we can apply it at a per-talloc-chunk level, without needing to add explicit destructor functions anywhere.

I think talloc_get_size would work for this.

@arr2036
Copy link
Member Author

arr2036 commented Sep 3, 2020

Function is called talloc_destructor_add

@arr2036
Copy link
Member Author

arr2036 commented Sep 3, 2020

Looks like there's a vestigial declaration in talloc.h for fr_talloc_destructor_t *talloc_add_destructor(TALLOC_CTX *chunk, fr_talloc_free_func_t func, void const *uctx); that should be removed

@alandekok alandekok added the v4.0.x meta: relates to the v4.0.x branch label Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature enhancement category: a new feature (an extension of functionality) refactoring & design category: related to rework of a feature or internal API v4.0.x meta: relates to the v4.0.x branch
Projects
None yet
Development

No branches or pull requests

2 participants