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

Secure usage of Emscripten heap #495

Merged
merged 8 commits into from
Jul 11, 2019
Merged

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Jul 9, 2019

In order to call Themis Core functions we need to transfer key material and messages from JavaScript memory into Emscripten heap. Operations results are similarly transferred back to JavaScript memory. We do this by explicitly allocating a buffer on Emscripten heap and using Emscripten helpers for transfers.

One important point here is that all data is still retained for some time on Emscripten heap after free() is called. This includes sensitive data which theoretically may be accessed by an adversary. Generally it is recommended to wipe the memory that contains sensitive data after use. We can't control JavaScript memory, but we can at least ensure that Emscripten heap does not leave any unwanted traces.

Introduce some functions to help us with data transfer between JavaScript and Emscripten heaps. They are designed to be used together so any new code will naturally use them instead of reaching for C functions and Emscripten runtime directly.


Provide some wrapper functions to hide all the intricacies of dealing
with Emscrpten heap. We should use these functions instead of calling
C directly.

Note that they ensure that allocated memory is zeroed out before use
as well as after use. This makes it easier to debug initialization
issues, and reduces the likelyhood of sensitive data leakage through
Emscripten heap.
Instead of calling malloc() and free() directly, use our new helpers
to deal with passing byte arrays between JavaScript and Emscripten.
@ilammy ilammy added the W-WasmThemis 🌐 Wrapper: WasmThemis, JavaScript API, npm packages label Jul 9, 2019
@ilammy
Copy link
Collaborator Author

ilammy commented Jul 9, 2019

Currently this covers only key generation. I'll update other cryptosystems in this PR once they are merged.

Instead of calling malloc() and free() directly, use our new helpers
to deal with passing byte arrays between JavaScript and Emscripten.
Instead of calling malloc() and free() directly, use our new helpers
to deal with passing byte arrays between JavaScript and Emscripten.
Instead of calling malloc() and free() directly, use our new helpers
to deal with passing byte arrays between JavaScript and Emscripten.
Instead of calling malloc() and free() directly, use our new helpers
to deal with passing byte arrays between JavaScript and Emscripten.
@ilammy
Copy link
Collaborator Author

ilammy commented Jul 9, 2019

I've converted all allocations to use heapAlloc and heapFree. Now we should not be leaving any leaked data in Emscripten heap after Themis calls are complete. Obviously, the data will still be present there during the operation time, but it has to be somewhere in memory.

Copy link
Contributor

@vixentael vixentael left a comment

Choose a reason for hiding this comment

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

Makes 100% sense.

If I also understood correctly, we plan to use utils.heapFree(...) on errors?

@ilammy
Copy link
Collaborator Author

ilammy commented Jul 10, 2019

If I also understood correctly, we plan to use utils.heapFree(...) on errors?

We already do it in this PR. These calls are placed in finally blocks so they are executed in both the happy path and when an exception is thrown due to an error.

@vixentael
Copy link
Contributor

Oh, i missed that we wrap everything in try blocks :)

heapAlloc: if malloc() fails then do not zero out the result

heapFree: make this function a no-op to allow its usage in failure
paths where the pointer is null (e.g., returned by failed heapAlloc)

We use a weird check "!!buffer" because JavaScript has weird ideas
about what is null and what is zero. malloc() results are generally
checked with "!ptr" so we invert this check.
@ilammy
Copy link
Collaborator Author

ilammy commented Jul 10, 2019

Your comment made me realize that we handle memory allocation errors poorly in the these utility functions. I've added a minor fix which allows using headFree on NULL values returned by failed heapAlloc invocation.

@vixentael
Copy link
Contributor

that's actually makes sense! 💪

@ilammy ilammy merged commit 258d0aa into cossacklabs:master Jul 11, 2019
@ilammy ilammy deleted the wasm-zeroization branch July 26, 2019 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
W-WasmThemis 🌐 Wrapper: WasmThemis, JavaScript API, npm packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants