Rust-themis: Allocate with try_reserve

[G1gg1L3s]

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

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

[G1gg1L3s]

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

[vixentael]

@G1gg1L3s take a look?

[G1gg1L3s]

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: https://github.com/cossacklabs/themis/blob/24d4fd0c3c0c0f4b65252c7bdcc967986a763d92/tests/rust/secure_cell.rs#L762-L780

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).

[Lagovas]

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]

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]

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]

Oh, it's harder than I thought :sweat_smile:

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 🙂