[2/6] crypto: clear out buffer after timing pbkdf algorithm

Commit Message

Daniel P. Berrangé Sept. 8, 2016, 4:27 p.m. UTC

The 'out' buffer will hold a key derived from master
password, so it is best practice to clear this buffer
when no longer required.

Signed-off-by: Daniel P. Berrange <[e-mail]>
---
 crypto/pbkdf.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Comments

Eric Blake Sept. 8, 2016, 5:47 p.m. UTC | #1

On 09/08/2016 11:27 AM, Daniel P. Berrange wrote:
> The 'out' buffer will hold a key derived from master
> password, so it is best practice to clear this buffer
> when no longer required.
> 
> Signed-off-by: Daniel P. Berrange <[e-mail]>
> ---
>  crypto/pbkdf.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)

Reviewed-by: Eric Blake <[e-mail]>

It still doesn't prevent the memory from being copied elsewhere (such as
the stack being paged out), unless we go to extraordinary lengths to
explicitly request volatile memory that can't be paged out.  I don't
know if we need to worry about that, though.  Do any of our crypto
libraries provide APIs for allocating local-use-only memory for
sensitive data?

Daniel P. Berrangé Sept. 9, 2016, 9:35 a.m. UTC | #2

On Thu, Sep 08, 2016 at 12:47:43PM -0500, Eric Blake wrote:
> On 09/08/2016 11:27 AM, Daniel P. Berrange wrote:
> > The 'out' buffer will hold a key derived from master
> > password, so it is best practice to clear this buffer
> > when no longer required.
> > 
> > Signed-off-by: Daniel P. Berrange <[e-mail]>
> > ---
> >  crypto/pbkdf.c | 15 ++++++++++-----
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> Reviewed-by: Eric Blake <[e-mail]>
> 
> It still doesn't prevent the memory from being copied elsewhere (such as
> the stack being paged out), unless we go to extraordinary lengths to
> explicitly request volatile memory that can't be paged out.  I don't
> know if we need to worry about that, though.  Do any of our crypto
> libraries provide APIs for allocating local-use-only memory for
> sensitive data?

AFAICT, while gcrypt uses such APIs internally, it doesn't expose them
to users. Nettle avoids malloc entirely in its API. So if we wanted
that we'd basically need to roll our own.  I don't think this is a big
deal, but I just wanted to add the memset() as a sanity check really.

Regards,
Daniel

Patch

diff --git a/crypto/pbkdf.c b/crypto/pbkdf.c
index 695cc35..35dccc2 100644
--- a/crypto/pbkdf.c
+++ b/crypto/pbkdf.c
@@ -67,13 +67,14 @@  int qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
                                const uint8_t *salt, size_t nsalt,
                                Error **errp)
 {
+    int ret = -1;
     uint8_t out[32];
     long long int iterations = (1 << 15);
     unsigned long long delta_ms, start_ms, end_ms;
 
     while (1) {
         if (qcrypto_pbkdf2_get_thread_cpu(&start_ms, errp) < 0) {
-            return -1;
+            goto cleanup;
         }
         if (qcrypto_pbkdf2(hash,
                            key, nkey,
@@ -81,10 +82,10 @@  int qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
                            iterations,
                            out, sizeof(out),
                            errp) < 0) {
-            return -1;
+            goto cleanup;
         }
         if (qcrypto_pbkdf2_get_thread_cpu(&end_ms, errp) < 0) {
-            return -1;
+            goto cleanup;
         }
 
         delta_ms = end_ms - start_ms;
@@ -103,8 +104,12 @@  int qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
     if (iterations > INT32_MAX) {
         error_setg(errp, "Iterations %lld too large for a 32-bit int",
                    iterations);
-        return -1;
+        goto cleanup;
     }
 
-    return iterations;
+    ret = iterations;
+
+ cleanup:
+    memset(out, 0, sizeof(out));
+    return ret;
 }