Add memory protections for secret values #108

zmre · 2020-04-17T23:07:02Z

These changes should prevent private keys from getting written to disk by the operating system. I went through and looked for any memory that we were zeroing out and added the protections there. All unit tests continue to work well.

Do we run unit tests in a Windows environment? If so, I'll attempt to add Windows support to this.

One ugly: in memlock.rs I have conditional compilation so this only happens on unix platforms, but there are empty no-op functions on non-unix. But I have to annotate each function with the conditional compilation. I initially used mod memlock { ... } and had two of those blocks each with an annotation. So then the file only had two of those, which was nice. But Clippy whined about inception. The use would be use crate::internal::memlock::memlock which is ugly. I couldn't find a way around this. Any ideas? I went for the thing I think is least ugly there.

One other ugly: there's no way to call out to these functions without unsafe code. I don't believe it's dirty in this case; it just is what it is. We don't mutate the data values.

skeet70 · 2020-04-20T16:45:26Z

src/api.rs

@@ -1042,20 +1052,38 @@ impl Hashable32 for PrivateKey {
    }
 }

+/// Avoid accidental logging of secrets
+impl fmt::Debug for PrivateKey {


This stuff seems to have a different goal than memory protection, and I think we should have a debug here that is distinguishing in at least some way. Probably just don't want to have a Display at all if the goal is to avoid accidental {}.

#110 is where we can discuss that further. This should be removed for now and we can keep this change non-breaking.

coltfred · 2020-04-21T00:02:13Z

src/api.rs

@@ -1042,20 +1052,38 @@ impl Hashable32 for PrivateKey {
    }
 }

+/// Avoid accidental logging of secrets
+impl fmt::Debug for PrivateKey {


#110 is where we can discuss that further. This should be removed for now and we can keep this change non-breaking.

coltfred · 2020-04-21T00:05:35Z

src/internal/memlock.rs

+// Which was generously released into the public domain.
+// The functions were modified to deal with structs rather than slices.
+
+#[cfg(all(unix, not(target_arch = "wasm32")))]


This file could benefit from use of https://docs.rs/cfg-if/0.1.10/cfg_if/, which would make it more clear which things are mutually exclusive of each other.

giarc3

Looks pretty good to me, though I admit I don't understand all the details of the actual "memlock.rs", so others should review there more in depth

README.md

giarc3 · 2020-04-21T17:03:48Z

src/internal/memlock.rs

+cfg_if! {
+    //If we are targeting wasm or not unix/windows we need to set the lock and unlock functions
+    //to empty. This will also happen if you've enabled "disable_memlock" feature flag
+    if #[cfg(any(feature="disable_memlock", target_arch = "wasm32", all(not(unix), not(windows))))] {


all(not(unix), not(windows))

what target_family does this cover?

It's just a safety since we cove the others explicitly. If there were other target_famlies defined, the if would be true and the code would at least compile.

src/internal/memlock.rs

clintfred

I mentioned this for Plaintext, but I think it would be good to minimize the places that we are creating locks. Ideally we'd have one lock call and one unlock for each type. Drop makes the unlock side of that easy, but there are often multiple ways to create a type. Maybe there are performance reasons to not route everything through a single path, but we should consider it.

It might also be worth calling out somewhere in internal docs what types we do mlock for and why.

README.md

src/api.rs

src/internal/ed25519.rs

clintfred · 2020-04-21T18:47:12Z

src/internal/memlock.rs

+    //If we are targeting wasm or not unix/windows we need to set the lock and unlock functions
+    //to empty. This will also happen if you've enabled "disable_memlock" feature flag
+    if #[cfg(any(feature="disable_memlock", target_arch = "wasm32", all(not(unix), not(windows))))] {
+        #[inline(always)]


I don't think this compiler hint makes sense as no code will be omitted for an empty function with no return.

I wasn't convinced that a function call to something empty wouldn't emit nothing, but I was convinced that inlining nothing would yield nothing.

I did look at the ASM for both and I couldn't ever make it emit any code for an empty function.

I think it's great if calls to these functions are no-op and have no code. These functions are needed to allow compile to work in certain cases and nothing more, right?

src/internal/memlock.rs

clintfred · 2020-04-21T18:58:10Z

src/internal/memlock.rs

+// The memlock code below is inspired by the secstr project
+// https://github.com/myfreeweb/secstr
+// Which was generously released into the public domain.
+// The functions were modified to deal with structs rather than slices.


It looks like goal of the mlock code is two fold.

Stop the marked memory from being swapped out

Stop the marked memory from appearing in a core dump

Is this correct?
We also think this will work for Linux (including Android), *BSD (including OSX and iOS), and Windows?

Unfortunately, mlock is different on different platforms, but generally speaking, it just prevents memory from going to swap. On linux, madvise is what prevents parts of memory from going into a core dump.

mlock is supported on Linux and *BSD including OSX. I don't know for sure about iOS or Android, but I expect it would work fine there as libc should be available. For Windows, we use a winapi call of VirtualLock, which also just pins memory in memory and prevents swap. I've tested it on Mac, Linux, and Windows.

I added some comments to the module that I hope reflects reality. Feel free to review.

src/internal/memlock.rs

clintfred · 2020-04-21T19:27:27Z

I would like to see this building (and testing?) on windows before we merge it. We could add that to Travis or do it as part of #111.

zmre · 2020-04-21T21:08:40Z

One thing that I didn't do that we should consider: check the return value from mlock and munlock. Different systems have different error codes, but across all of them, zero is success. I don't imagine the caller will care if it succeeds or not, but it would be something we could unit test for and maybe detect hidden issues when we cross compile. Thoughts?

github-actions · 2020-04-23T23:20:37Z

Benchmark results comparing with base:

group                               base                                   current
-----                               ----                                   -------
256-bit compute public key          1.02  1292.7±33.11µs        ? B/sec    1.00  1263.5±37.18µs        ? B/sec
256-bit decrypt (level 0)           1.05     11.3±0.26ms        ? B/sec    1.00     10.7±0.26ms        ? B/sec
256-bit decrypt (level 1)           1.08     41.2±1.24ms        ? B/sec    1.00     38.2±1.09ms        ? B/sec
256-bit decrypt (level 2)           1.04     69.1±1.63ms        ? B/sec    1.00     66.4±2.00ms        ? B/sec
256-bit derive symmetric key        1.00      3.5±0.39µs        ? B/sec    1.65      5.8±0.55µs        ? B/sec
256-bit generate ed25519 keypair    1.00     26.5±1.17µs        ? B/sec    1.14     30.1±1.07µs        ? B/sec
256-bit generate key pair           1.00  1275.7±22.38µs        ? B/sec    1.02  1304.2±30.95µs        ? B/sec
256-bit generate transform key      1.03     26.4±0.59ms        ? B/sec    1.00     25.6±0.71ms        ? B/sec
256-bit transform (level 1)         1.06     32.4±1.09ms        ? B/sec    1.00     30.6±1.12ms        ? B/sec
256-bit transform (level 2)         1.05     73.3±0.82ms        ? B/sec    1.00     70.0±1.46ms        ? B/sec

clintfred · 2020-04-24T17:36:14Z

@coltfred will you rebase this on master, please?

- Remove sizeof_slice function when not necessary

### Public API changes - [[#115](#115)] - Add explicit 32-bit and 64-bit backends as features: `u64_backend`, `u32_backend` ### Notable internal changes - [[#108](#108)] - Add memory protections for secret values (disable with `disable_memlock`)

zmre added 2 commits April 17, 2020 16:59

Addresses #107 and adds memory protections

5fdfd5f

fixed linux and windows (and wasm?)

d2548f1

zmre linked an issue Apr 18, 2020 that may be closed by this pull request

Secure memory storage of private keys #107

Closed

fix rust format issues

2d90cc4

skeet70 requested changes Apr 20, 2020

View reviewed changes

coltfred requested changes Apr 21, 2020

View reviewed changes

coltfred added 5 commits April 21, 2020 08:21

Use cfg_if

997c86c

No need for extern crates

24c2085

Trim down if/else clauses to be more readable

15439b1

Remove unused crate from dependency list

fcca1dd

Get rid of unused code warning, general cleanup

3445e1d

giarc3 approved these changes Apr 21, 2020

View reviewed changes

clintfred suggested changes Apr 21, 2020

View reviewed changes

clintfred changed the title ~~Addresses #107 and adds memory protections~~ Add memory protections for secret values Apr 21, 2020

Fix comments and readme

62bbb54

coltfred added 2 commits April 23, 2020 17:03

Remove double mlocking

c9fde15

Merge branch 'master' into lock_mem

aa160a1

cjyar mentioned this pull request Apr 24, 2020

.github: Remove benchmarks. #117

Merged

coltfred and others added 2 commits April 24, 2020 11:42

Merge branch 'master' into lock_mem

0ab0d9f

- Add comments about OS specific behavior of memlock.rs

7c3378e

- Remove sizeof_slice function when not necessary

clintfred approved these changes Apr 24, 2020

View reviewed changes

coltfred merged commit f7183c4 into master Apr 27, 2020

giarc3 deleted the lock_mem branch November 19, 2021 16:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add memory protections for secret values #108

Add memory protections for secret values #108

zmre commented Apr 17, 2020 •

edited by clintfred

Loading

skeet70 Apr 20, 2020

coltfred Apr 21, 2020

coltfred Apr 21, 2020

coltfred Apr 21, 2020

giarc3 left a comment

giarc3 Apr 21, 2020

coltfred Apr 21, 2020

clintfred left a comment

clintfred Apr 21, 2020

coltfred Apr 21, 2020

clintfred Apr 21, 2020

zmre Apr 21, 2020

clintfred Apr 21, 2020

zmre Apr 21, 2020

clintfred Apr 24, 2020

clintfred commented Apr 21, 2020

zmre commented Apr 21, 2020

github-actions bot commented Apr 23, 2020

clintfred commented Apr 24, 2020

Add memory protections for secret values #108

Add memory protections for secret values #108

Conversation

zmre commented Apr 17, 2020 • edited by clintfred Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giarc3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintfred left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintfred commented Apr 21, 2020

zmre commented Apr 21, 2020

github-actions bot commented Apr 23, 2020

clintfred commented Apr 24, 2020

zmre commented Apr 17, 2020 •

edited by clintfred

Loading