crypto/helper: Add secure wipe function #10219

bergzand · 2018-10-21T18:34:25Z

Contribution description

Adds a cryptographically secure wipe function to wipe structs with
sensitive data. Works by first casting the pointer to a volatile
pointer to ensure that the compiler doesn't optimize the "memset" away.

Testing procedure

For now it is mostly if this compiles and if the source makes sense

Issues/PRs references

None

basilfx · 2018-10-22T10:53:17Z

What about data remanence attacks? We should either list that this isn't taken care of, or implement a more secure wipe (multiple times, random data, memory type dependent).

I guess the first is the easiest option :-)

bergzand · 2018-10-22T11:21:49Z

Yeah, let's go with the first option.

Most crypto libraries I know of handle a "secure" wipe this way and only protect against normal memory reads and not against other memory black magic :)

bergzand · 2018-10-22T11:23:02Z

I'm thinking of renaming it to crypto_secure_wipe to prevent name collisions with at least Monocypher. Other suggestions are welcome

Marking as WIP to prevent merging before the rename

bergzand · 2018-10-23T09:17:26Z

@basilfx I've added a small note to the function. I've also renamed the function to hopefully avoid naming collision.

basilfx · 2018-10-23T09:34:41Z

sys/include/crypto/helper.h

+ * This wipe function zeros the supplied buffer in a way that the compiler is
+ * not allowed to optimize. This can be used to erase secrets from memory.
+ *
+ * Note that this function on its owm could be insufficient. It is outside the


basilfx · 2018-10-23T09:36:07Z

sys/include/crypto/helper.h

+ * This wipe function zeros the supplied buffer in a way that the compiler is
+ * not allowed to optimize. This can be used to erase secrets from memory.
+ *
+ * Note that this function on its owm could be insufficient. It is outside the


maybe add:

.. could be insufficient against (data remanence) attacks.

This clarifies why it is insufficient.

Fixed, thanks for the suggestion

basilfx · 2018-10-23T11:40:43Z

I'm OK with the changes, looks good. Haven't tested it though.

bergzand · 2018-10-23T11:43:19Z

I can add a quick unittest for this, only issue is that verifying the result is a read operation which also prevents the compiler from optimizing the wipe away. But it would test that the memory region is zero'd out.

basilfx · 2018-10-23T11:53:14Z

Would make sense, since a unit test verifies the code, not the compiler specifics ;-)

bergzand · 2018-10-23T12:08:18Z

Added a simple test to the crypto unittest.

jcarrano · 2018-10-23T15:23:28Z

It is not being optimized away (at least not now).

native:

00005233 <test_crypto_wipe>:
    5233:       55                      push   ebp
    5234:       89 e5                   mov    ebp,esp
    5236:       53                      push   ebx
    5237:       83 ec 34                sub    esp,0x34
    523a:       e8 01 c3 ff ff          call   1540 <__x86.get_pc_thunk.bx>
    523f:       81 c3 c1 cd 00 00       add    ebx,0xcdc1
    5245:       83 ec 04                sub    esp,0x4
    5248:       6a 14                   push   0x14
    524a:       68 aa 00 00 00          push   0xaa
    524f:       8d 83 34 bf 00 00       lea    eax,[ebx+0xbf34]
    5255:       50                      push   eax
    5256:       e8 d5 be ff ff          call   1130 <memset@plt>
    525b:       83 c4 10                add    esp,0x10
    525e:       83 ec 08                sub    esp,0x8
    5261:       6a 13                   push   0x13
    5263:       8d 83 34 bf 00 00       lea    eax,[ebx+0xbf34]
    5269:       50                      push   eax
    526a:       e8 3f 2e 00 00          call   80ae <crypto_secure_wipe>
    526f:       83 c4 10                add    esp,0x10
..............

arm:

{
    13dc:       b513            push    {r0, r1, r4, lr}
    memset(secret, VALUE, sizeof(secret));
    13de:       4c12            ldr     r4, [pc, #72]   ; (1428 <test_crypto_wipe+0x4c>)
    13e0:       2214            movs    r2, #20
    13e2:       21aa            movs    r1, #170        ; 0xaa
    13e4:       0020            movs    r0, r4
    13e6:       f001 ff4d       bl      3284 <memset>
    /* Wipe everything except the last byte */
    crypto_secure_wipe(secret, sizeof(secret) - 1);
    13ea:       2113            movs    r1, #19
    13ec:       0020            movs    r0, r4
    13ee:       f001 f83d       bl      246c <crypto_secure_wipe>
.....

bergzand · 2018-10-23T15:33:38Z

It is not being optimized away (at least not now).

@jcarrano Thanks for checking. Is this with or without the test assert reading the values after the call?

jcarrano · 2018-10-23T15:49:11Z

Oops! It was with the read. I commented it and dissassembled it again. The code is still there, so it is still secure.

It is declared as volatile, so it should get written eventually. The user just needs to be aware of what reorderings are possible (there is a C standard for that).

bergzand · 2018-11-07T06:09:07Z

@basilfx @jcarrano Ready to squash here?

basilfx · 2018-11-07T08:36:09Z

I would say yes. I fully trust @jcarrano checks!

Adds a cryptographically secure wipe function to wipe structs with sensitive data. Works by first casting the pointer to a `volatile` pointer to ensure that the compiler doesn't optimize the "memset" away.

bergzand · 2018-11-07T08:38:34Z

squashed!

The test added for crypto_secure_wipe wipes a buffer with a secret in it. Only the last byte is kept as it was. The last byte is used to check that the function doesn't write outside the supplied buffer.

bergzand · 2018-11-07T17:27:54Z

Fixed the murdock issue with an amend

bergzand · 2018-11-30T09:29:42Z

@jcarrano Mind pressing the big green button here? :)

bergzand · 2018-11-30T10:38:17Z

@jcarrano Thanks!

bergzand added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: crypto Area: Cryptographic libraries labels Oct 21, 2018

bergzand mentioned this pull request Oct 21, 2018

chacha20poly1305: Initial implementation of AEAD cipher. #10220

Merged

bergzand added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 21, 2018

bergzand added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Oct 22, 2018

basilfx reviewed Oct 23, 2018

View reviewed changes

bergzand removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Oct 23, 2018

basilfx approved these changes Oct 23, 2018

View reviewed changes

crypto/helper: Add secure wipe function

fa64817

Adds a cryptographically secure wipe function to wipe structs with sensitive data. Works by first casting the pointer to a `volatile` pointer to ensure that the compiler doesn't optimize the "memset" away.

bergzand force-pushed the pr/crypt/helper_add_wipe branch from 57fbdde to 43b481a Compare November 7, 2018 08:38

crypto/helper: Add test for crypto_secure_wipe

7302869

The test added for crypto_secure_wipe wipes a buffer with a secret in it. Only the last byte is kept as it was. The last byte is used to check that the function doesn't write outside the supplied buffer.

bergzand force-pushed the pr/crypt/helper_add_wipe branch from 43b481a to 7302869 Compare November 7, 2018 17:27

jcarrano merged commit 77c9cc4 into RIOT-OS:master Nov 30, 2018

bergzand deleted the pr/crypt/helper_add_wipe branch November 30, 2018 10:38

aabadie added this to the Release 2019.01 milestone Dec 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto/helper: Add secure wipe function #10219

crypto/helper: Add secure wipe function #10219

bergzand commented Oct 21, 2018

basilfx commented Oct 22, 2018

bergzand commented Oct 22, 2018

bergzand commented Oct 22, 2018

bergzand commented Oct 23, 2018

basilfx Oct 23, 2018

bergzand Oct 23, 2018

basilfx Oct 23, 2018

bergzand Oct 23, 2018

basilfx commented Oct 23, 2018

bergzand commented Oct 23, 2018

basilfx commented Oct 23, 2018

bergzand commented Oct 23, 2018

jcarrano commented Oct 23, 2018

bergzand commented Oct 23, 2018

jcarrano commented Oct 23, 2018

bergzand commented Nov 7, 2018

basilfx commented Nov 7, 2018

bergzand commented Nov 7, 2018

bergzand commented Nov 7, 2018

bergzand commented Nov 30, 2018

bergzand commented Nov 30, 2018

crypto/helper: Add secure wipe function #10219

crypto/helper: Add secure wipe function #10219

Conversation

bergzand commented Oct 21, 2018

Contribution description

Testing procedure

Issues/PRs references

basilfx commented Oct 22, 2018

bergzand commented Oct 22, 2018

bergzand commented Oct 22, 2018

bergzand commented Oct 23, 2018

basilfx Oct 23, 2018

Choose a reason for hiding this comment

bergzand Oct 23, 2018

Choose a reason for hiding this comment

basilfx Oct 23, 2018

Choose a reason for hiding this comment

bergzand Oct 23, 2018

Choose a reason for hiding this comment

basilfx commented Oct 23, 2018

bergzand commented Oct 23, 2018

basilfx commented Oct 23, 2018

bergzand commented Oct 23, 2018

jcarrano commented Oct 23, 2018

bergzand commented Oct 23, 2018

jcarrano commented Oct 23, 2018

bergzand commented Nov 7, 2018

basilfx commented Nov 7, 2018

bergzand commented Nov 7, 2018

bergzand commented Nov 7, 2018

bergzand commented Nov 30, 2018

bergzand commented Nov 30, 2018