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

Rust-themis: Allocate with try_reserve #1014

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

G1gg1L3s
Copy link
Collaborator

@G1gg1L3s G1gg1L3s commented Jun 27, 2023

While looking at our backlog, I found T1649. In a nutshell, some tests were failing on 32-bit machines, because they corrupt the secure cell tag. This was done in a way that triggers allocation of 2GB from the rust code, which failed. But at that time, it wasn't possible to handle this failure gracefully, so rust wrapper just panicked. It means that on 32-bit platforms it was possible to execute DoS attack with maliciously crafted input.

Since we traversed to the rust 1.58, we now have the try_* methods, including the .try_reserve which allows us to handle failures gracefully.

So, let's use it! I've tested it manually on Pi 4 and the tests pass without issues.

Checklist

  • Change is covered by automated tests
  • The coding guidelines are followed
  • Public API has proper documentation
  • Changelog is updated (in case of notable or breaking changes)

It will allow us using ? with the try_* methods, like try_reserve.
They were skipped because allocation on 32-bit platforms failed
while trying to allocate something bigger than 2GB. This is due to
usage of `.reserve` which panics if it couldn't fulfill the request.

Since we traversed to the rust 1.58, we now have the `try_*` methods,
including the `.try_reserve` which allows us handle panics gracefully.

I've tested it manually on pi4 and it works!
Because we don't have try_with_capacity or something similar.
@G1gg1L3s
Copy link
Collaborator Author

Ah yeah, we should probably merge the #1013 first. It will resolve some rust issues and make the changelog clear to update.

Copy link
Collaborator

@Lagovas Lagovas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we test somehow that we handle it and get an expected error?

@vixentael
Copy link
Contributor

@G1gg1L3s take a look?

@G1gg1L3s
Copy link
Collaborator Author

G1gg1L3s commented Aug 8, 2023

Sorry, I somehow missed this comment:

can we test somehow that we handle it and get an expected error?

Hmm, we have tests that accidantally corrupt the cells in a way that triggers a big allocation, for example this:

#[test]
fn detects_corrupted_token() {
let cell = SecureCell::with_key(SymmetricKey::new())
.unwrap()
.token_protect();
let message = b"Colorless green ideas sleep furiously".as_ref();
let (encrypted, token) = cell.encrypt(&message).unwrap();
// Invert every odd byte, this will surely break the token.
let mut corrupted_token = token;
for (i, b) in corrupted_token.iter_mut().enumerate() {
if i % 2 == 1 {
*b = !*b
}
}
assert!(cell.decrypt(&encrypted, &corrupted_token).is_err());
}

But, it doesn't check that the error is NoMemory. I can write an additional test for checking this on 32 bit machines (because it's currently impossible to trigger similar on x64; it's possible if RAM is limited, sorry).

It was intended only for 32 bit systems, but let's extend it to
x64 as well.
@Lagovas
Copy link
Collaborator

Lagovas commented Aug 8, 2023

actually, it's common approach to limit resources for VM in the cloud. When one server used for huge amount of the containers and they should fit into the expected RAM usage and avoid overusage by one bad container.
So, actually we can test it using docker container limited with [--memory(https://docs.docker.com/config/containers/resource_constraints/) parameter.
Looks like github actions allow to configure container's constraints - https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idcontaineroptions

Does it hard to test it on the VM with 20mb RAM limit, try to allocate 30m and catch NoMemory error?

@G1gg1L3s
Copy link
Collaborator Author

G1gg1L3s commented Aug 10, 2023

Does it hard to test it on the VM with 20mb RAM limit, try to allocate 30m and catch NoMemory error?

I can write a test that will catch precisely that. However, that test would fail on regular machines. In other words, when users pull themis and run cargo test they would get an error. I think we can somehow disable that test to work only in CI (by inspecting env variables for GitHub ones), but I don't know whether it is a good solution. What do you think?

Also, I don't know, maybe it would require adding additional CI step for this particular test or writing a separate Dockerfile, so it should be taken into account as well.

@Lagovas
Copy link
Collaborator

Lagovas commented Aug 10, 2023

maybe let's hide this test under the feature flag turned off by default and pass it in ci env? Found example - https://stackoverflow.com/questions/48583049/run-additional-tests-by-using-a-feature-flag-to-cargo-test

Probably we will need separate workflow for github actions where container will be limited with memory and we will run this only one test for now. To make it more generic, we can pass the ENV variable with the amount of memory of the container and use it in test like allocate(env::get("CONTAINER_RAM_SIZE") + 5*1024*1024) (allocate +5mb over the limit) and test like that. Or if it is easy to get this info from the system, just use system info.

@G1gg1L3s
Copy link
Collaborator Author

G1gg1L3s commented Aug 10, 2023

Oh, it's harder than I thought 😅

Our tests are run using make test_rust, which in turn calls the script tests/rust/run_tests.sh, which calls a bunch of things before calling the cargo test. So, to introduce new CI tests, we need either to run cargo test directly in CI (which is not that beautiful or at least consistent), or change our makefile/script setup to somehow accept additional parameters, if we want to use feature-like approach.

But even with detecting CI environment, adding a new step to CI would require building themis again and maybe setting up caching. Also, we can use the --memory option only if we run the whole job in a container. What container to use in this case is an open question. And that's only for one/a couple of tests.

So, I think that's not worth it 🙂

Copy link
Collaborator

@Lagovas Lagovas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, lets believe that it works

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.

3 participants