From ed7ba20082b790000215a5ba4545150fab7bb95b Mon Sep 17 00:00:00 2001
From: Didactic Drunk <1479616+didactic-drunk@users.noreply.github.com>
Date: Sun, 1 Sep 2019 10:31:36 -0700
Subject: [PATCH] Sodium::Kdf keep SecureBuffer in noaccess state except when
 in use.

---
 spec/sodium/secure_buffer_spec.cr | 14 ++++++++++++++
 src/sodium/kdf.cr                 | 17 ++++++++++++-----
 src/sodium/lib_sodium.cr          |  6 +++++-
 src/sodium/secure_buffer.cr       | 23 +++++++++++++++--------
 4 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/spec/sodium/secure_buffer_spec.cr b/spec/sodium/secure_buffer_spec.cr
index fb707f7..dba4d09 100644
--- a/spec/sodium/secure_buffer_spec.cr
+++ b/spec/sodium/secure_buffer_spec.cr
@@ -1,6 +1,9 @@
 require "../spec_helper"
 require "../../src/sodium/secure_buffer"
 
+class FakeError < Exception
+end
+
 describe Sodium::SecureBuffer do
   it "allocates empty" do
     buf = Sodium::SecureBuffer.new 5
@@ -58,11 +61,22 @@ describe Sodium::SecureBuffer do
 
     buf.readwrite
     buf.@state.should eq Sodium::SecureBuffer::State::Readwrite
+    buf.readonly { }
+    buf.@state.should eq Sodium::SecureBuffer::State::Readwrite
 
     buf.wipe
     buf.@state.should eq Sodium::SecureBuffer::State::Wiped
   end
 
+  it "temporarily transitions correctly with exceptions" do
+    buf = Sodium::SecureBuffer.new(5).noaccess
+    begin
+      buf.readonly { raise FakeError.new }
+    rescue FakeError
+    end
+    buf.@state.should eq Sodium::SecureBuffer::State::Noaccess
+  end
+
   it "can wipe more than once" do
     buf = Sodium::SecureBuffer.new 5
     3.times { buf.wipe }
diff --git a/src/sodium/kdf.cr b/src/sodium/kdf.cr
index 94ba779..806952d 100644
--- a/src/sodium/kdf.cr
+++ b/src/sodium/kdf.cr
@@ -11,6 +11,10 @@ module Sodium
   # subkey_id = 0
   # output_size = 16
   # subkey = kdf.derive "8bytectx", subkey_id, output_size
+  #
+  # Memory for this class is held in a sodium guarded page with noaccess.
+  # Readonly access is temporarily enabled when deriving keys.
+  # Calling #to_slice marks the page readonly permanently.
   # ```
   class Kdf
     include Wipe
@@ -30,7 +34,7 @@ module Sodium
         raise ArgumentError.new("bytes must be #{KEY_SIZE}, got #{bytes.bytesize}")
       end
 
-      @sbuf = SecureBuffer.new bytes, erase
+      @sbuf = SecureBuffer.new(bytes, erase).noaccess
     end
 
     # Use an existing KDF SecureBuffer key.
@@ -38,14 +42,14 @@ module Sodium
       if @sbuf.bytesize != KEY_SIZE
         raise ArgumentError.new("bytes must be #{KEY_SIZE}, got #{sbuf.bytesize}")
       end
-      @sbuf.readonly
+      @sbuf.noaccess
     end
 
     # Generate a new random KDF key.
     #
     # Make sure to save kdf.to_slice before kdf goes out of scope.
     def initialize
-      @sbuf = SecureBuffer.random KEY_SIZE
+      @sbuf = SecureBuffer.random(KEY_SIZE).noaccess
     end
 
     # Derive a consistent subkey based on `context` and `subkey_id`.
@@ -62,9 +66,12 @@ module Sodium
       end
 
       subkey = SecureBuffer.new subkey_size
-      if (ret = LibSodium.crypto_kdf_derive_from_key(subkey, subkey.bytesize, subkey_id, context, self.to_slice)) != 0
-        raise Sodium::Error.new("crypto_kdf_derive_from_key returned #{ret} (subkey size is probably out of range)")
+      @sbuf.readonly do
+        if (ret = LibSodium.crypto_kdf_derive_from_key(subkey, subkey.bytesize, subkey_id, context, self.to_slice)) != 0
+          raise Sodium::Error.new("crypto_kdf_derive_from_key returned #{ret} (subkey size is probably out of range)")
+        end
       end
+
       subkey
     end
 
diff --git a/src/sodium/lib_sodium.cr b/src/sodium/lib_sodium.cr
index ad27abb..91d2c0d 100644
--- a/src/sodium/lib_sodium.cr
+++ b/src/sodium/lib_sodium.cr
@@ -327,7 +327,11 @@ module Sodium
     true
   end
 
-  def self.memzero(bytes : Bytes)
+  def self.memzero(bytes : Bytes) : Nil
     LibSodium.sodium_memzero bytes, bytes.bytesize
   end
+
+  def self.memzero(str : String) : Nil
+    memzero str.to_slice
+  end
 end
diff --git a/src/sodium/secure_buffer.cr b/src/sodium/secure_buffer.cr
index 34abb8c..2a6b90d 100644
--- a/src/sodium/secure_buffer.cr
+++ b/src/sodium/secure_buffer.cr
@@ -13,10 +13,10 @@ module Sodium
     end
 
     enum State
-      Readwrite
-      Readonly
-      Noaccess
       Wiped
+      Noaccess
+      Readonly
+      Readwrite
     end
 
     @state = State::Readwrite
@@ -154,11 +154,18 @@ module Sodium
     end
 
     private def with_state(new_state : State)
-      old_state = @state
-      set_state new_state
-      yield
-    ensure
-      set_state old_state if old_state
+      # Only change when new_state needs more access than @state.
+      if new_state >= @state
+        yield
+      else
+        begin
+          old_state = @state
+          set_state new_state
+          yield
+        ensure
+          set_state old_state if old_state
+        end
+      end
     end
   end
 end