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

sodium_malloc() does not check return value from sodium_mlock() #1237

Closed
tdammers opened this issue Dec 19, 2022 · 2 comments
Closed

sodium_malloc() does not check return value from sodium_mlock() #1237

tdammers opened this issue Dec 19, 2022 · 2 comments

Comments

@tdammers
Copy link

Specific source code location:

sodium_mlock(unprotected_ptr, unprotected_size);

When sodium_mlock() runs into an OS-imposed mlock limit, it will set errno and return -1, and the memory will remain unlocked. But sodium_malloc() does not check the return value, so we still get the allocated memory back, and, in line to the documentation, we expect it to be mlocked, but it actually isn't.

And because there is no "loud error", the consequences of this may go entirely undetected (after all, the memory itself is still valid, it's just not mlocked), or they may cause spurious errors in code that depends on it being mlocked, like segfaults, assertion failures on line 627, or just silently leaking secrets to swap.

IMO, a fix would amount to this:

  • Capture the return value of sodium_mlock().
  • If it's not 0, free base_ptr, and return NULL. errno should hold the correct error at this point.
@jedisct1
Copy link
Owner

These system calls are intentionally not checked here.

We can't guarantee that sodium_malloc() can lock memory. On some environments, memory locking just doesn't exist. On others, such as WebAssembly, the function is failing right now, but may eventually work (and this will, very likely, also depend on the runtime).

On other systems, the system call may or may not work according to the system configuration and the amount of already locked memory, which is very non-deterministic.

For many application developers, sodium_malloc() would be seen as something that "randomly fails" and would discourage its use.

The reason is was introduced is to mitigate heartbleed-like attacks. It can also be used as a general bound checking mechanism, and it's widely used that way in the test suite. Locking memory is just a bonus step, but without any guarantees. If the documentation states it otherwise or is confusing, that should be clarified.

For cases where memory locking is absolutely required, there is the sodium_mlock() function.

@tdammers
Copy link
Author

The relevant documentation is this:

In addition, sodium_mlock() is called on the region to help avoid it being swapped to disk.

(found here)

I think clarification would be appropriate here, e.g.:

In addition, sodium_mlock() is called on the region to help avoid it being swapped to disk. Note however that sodium_mlock() may fail, in which case sodium_malloc() will return the memory regardless, but it will not be mlocked. If you need to rely on mlocking, consider calling sodium_mlock() explicitly, and checking its return value.

I would also suggest documenting this design decision in the source code, to avoid future misunderstandings of this kind.

tdammers added a commit to IntersectMBO/cardano-base that referenced this issue Dec 19, 2022
See jedisct1/libsodium#1237: sodium_malloc()
calls sodium_mlock() for us, but it ignores any failures reported from
there, so if we run out of our mlock quota, instead of failing loudly
and clearly, it will just give us non-mlocked memory instead, and when
we use it under the assumption that it will be mlocked, all sorts of
spurious / nondeterministic failures will result.
tdammers added a commit to IntersectMBO/cardano-base that referenced this issue Dec 21, 2022
See jedisct1/libsodium#1237: sodium_malloc()
calls sodium_mlock() for us, but it ignores any failures reported from
there, so if we run out of our mlock quota, instead of failing loudly
and clearly, it will just give us non-mlocked memory instead, and when
we use it under the assumption that it will be mlocked, all sorts of
spurious / nondeterministic failures will result.
tdammers added a commit to IntersectMBO/cardano-base that referenced this issue Jan 9, 2023
See jedisct1/libsodium#1237: sodium_malloc()
calls sodium_mlock() for us, but it ignores any failures reported from
there, so if we run out of our mlock quota, instead of failing loudly
and clearly, it will just give us non-mlocked memory instead, and when
we use it under the assumption that it will be mlocked, all sorts of
spurious / nondeterministic failures will result.
tdammers added a commit to IntersectMBO/cardano-base that referenced this issue Jan 19, 2023
See jedisct1/libsodium#1237: sodium_malloc()
calls sodium_mlock() for us, but it ignores any failures reported from
there, so if we run out of our mlock quota, instead of failing loudly
and clearly, it will just give us non-mlocked memory instead, and when
we use it under the assumption that it will be mlocked, all sorts of
spurious / nondeterministic failures will result.
Repository owner locked and limited conversation to collaborators Mar 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants