Skip to content

Commit

Permalink
[#101] NB Change default encryption from AES-CBC to AES-GCM
Browse files Browse the repository at this point in the history 
Why?

  - AES-GCM is faster and can be more secure, Ref. https://goo.gl/Dsc9mL, etc.
  - AES-GCM is an authenticated[1] encryption mechanism, providing
    automatic integrity checks. This is relevant to [#101].

What's the issue with #101?

  - We    compress then encrypt    on freeze ; Reverse would make compression useless
  - So we decrypt  then decompress on thaw

Attempting CBC decryption with the wrong password will often but not
*always* throw. Meaning it's possible for decompression could be
attempted with a junk ba. And this can cause some decompressors to
fail in a destructive way, including large allocations (DDoS) or even
taking down the JVM in extreme cases.

Possible solutions?

  - We could add our own HMAC, etc.
  - And/or we could use something like AES-GCM which offers built-in
    integrity and will throw an AEADBadTagException on failure.

There may indeed be reasons [2,3,4] to consider adding a custom HMAC -
and that's still on the cards for later.

But in the meantime, the overall balance of pros/cons seems to lean
in the direction of choosing AES-GCM as a reasonable default.

Note that the change in this commit is done in a backward-compatible
way using Nippy's versioned header: new payloads will be written using
AES-GCM by default. But old payloads already written using AES-CBC will
continue to be read using that scheme.

References
  [1] https://en.wikipedia.org/wiki/Authenticated_encryption
  [2] https://www.daemonology.net/blog/2009-06-24-encrypt-then-mac.html
  [3] https://blog.cryptographyengineering.com/2011/12/04/matt-green-smackdown-watch-are-aead/
  [4] HMAC vs AEAD integrity,           https://crypto.stackexchange.com/q/24379
  [5] AES-GCM vs HMAC-SHA256 integrity, https://crypto.stackexchange.com/q/30627
  • Loading branch information
@ptaoussanis
ptaoussanis committed Jan 6, 2019
1 parent ae8baa6 commit 23276ac
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 31 deletions.
61 changes: 39 additions & 22 deletions src/taoensso/nippy.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,25 +58,38 @@
(def ^:private ^:const head-version "Current Nippy header format version" 1)
(def ^:private ^:const head-meta
"Final byte of 4-byte Nippy header stores version-dependent metadata"

;; Currently
;; - 5 compressors, #{nil :snappy :lz4 :lzma2 :else}
;; - 4 encryptors, #{nil :aes128-cbc-sha512 :aes128-gcm-sha512 :else}

{(byte 0) {:version 1 :compressor-id nil :encryptor-id nil}
(byte 2) {:version 1 :compressor-id nil :encryptor-id :aes128-cbc-sha512}
(byte 14) {:version 1 :compressor-id nil :encryptor-id :aes128-gcm-sha512}
(byte 4) {:version 1 :compressor-id nil :encryptor-id :else}
(byte 5) {:version 1 :compressor-id :else :encryptor-id nil}
(byte 6) {:version 1 :compressor-id :else :encryptor-id :else}
;;
(byte 2) {:version 1 :compressor-id nil :encryptor-id :aes128-sha512}
;;

(byte 1) {:version 1 :compressor-id :snappy :encryptor-id nil}
(byte 3) {:version 1 :compressor-id :snappy :encryptor-id :aes128-sha512}
(byte 3) {:version 1 :compressor-id :snappy :encryptor-id :aes128-cbc-sha512}
(byte 15) {:version 1 :compressor-id :snappy :encryptor-id :aes128-gcm-sha512}
(byte 7) {:version 1 :compressor-id :snappy :encryptor-id :else}
;;

;;; :lz4 used for both lz4 and lz4hc compressor (the two are compatible)
(byte 8) {:version 1 :compressor-id :lz4 :encryptor-id nil}
(byte 9) {:version 1 :compressor-id :lz4 :encryptor-id :aes128-sha512}
(byte 9) {:version 1 :compressor-id :lz4 :encryptor-id :aes128-cbc-sha512}
(byte 16) {:version 1 :compressor-id :lz4 :encryptor-id :aes128-gcm-sha512}
(byte 10) {:version 1 :compressor-id :lz4 :encryptor-id :else}
;;

(byte 11) {:version 1 :compressor-id :lzma2 :encryptor-id nil}
(byte 12) {:version 1 :compressor-id :lzma2 :encryptor-id :aes128-sha512}
(byte 13) {:version 1 :compressor-id :lzma2 :encryptor-id :else}})
(byte 12) {:version 1 :compressor-id :lzma2 :encryptor-id :aes128-cbc-sha512}
(byte 17) {:version 1 :compressor-id :lzma2 :encryptor-id :aes128-gcm-sha512}
(byte 13) {:version 1 :compressor-id :lzma2 :encryptor-id :else}

(byte 5) {:version 1 :compressor-id :else :encryptor-id nil}
(byte 18) {:version 1 :compressor-id :else :encryptor-id :aes128-cbc-sha512}
(byte 19) {:version 1 :compressor-id :else :encryptor-id :aes128-gcm-sha512}
(byte 6) {:version 1 :compressor-id :else :encryptor-id :else}})

(comment (count (sort (keys head-meta))))

(defmacro ^:private when-debug [& body] (when #_true false `(do ~@body)))

Expand Down Expand Up @@ -242,7 +255,10 @@

(enc/defalias encrypt encryption/encrypt)
(enc/defalias decrypt encryption/decrypt)
(enc/defalias aes128-encryptor encryption/aes128-encryptor)

(enc/defalias aes128-gcm-encryptor encryption/aes128-gcm-encryptor)
(enc/defalias aes128-cbc-encryptor encryption/aes128-cbc-encryptor)
(enc/defalias aes128-encryptor encryption/aes128-gcm-encryptor) ; Default

(enc/defalias freezable? utils/freezable?))

Expand Down Expand Up @@ -988,7 +1004,7 @@
([x] (freeze x nil))
([x {:keys [compressor encryptor password]
:or {compressor :auto
encryptor aes128-encryptor}
encryptor aes128-gcm-encryptor}
:as opts}]
(let [;; Intentionally undocumented:
no-header? (or (get opts :no-header?)
Expand Down Expand Up @@ -1322,18 +1338,19 @@
:lzma2 lzma2-compressor
:lz4 lz4-compressor
:no-header (throw (ex-info ":auto not supported on headerless data." {}))
:else (throw (ex-info ":auto not supported for non-standard compressors." {}))
(throw (ex-info (str "Unrecognized :auto compressor id: " compressor-id)
{:compressor-id compressor-id}))))
:else (throw (ex-info ":auto not supported for non-standard compressors." {}))
(do (throw (ex-info (str "Unrecognized :auto compressor id: " compressor-id)
{:compressor-id compressor-id})))))

(defn- get-auto-encryptor [encryptor-id]
(case encryptor-id
nil nil
:aes128-sha512 aes128-encryptor
:no-header (throw (ex-info ":auto not supported on headerless data." {}))
:else (throw (ex-info ":auto not supported for non-standard encryptors." {}))
(throw (ex-info (str "Unrecognized :auto encryptor id: " encryptor-id)
{:encryptor-id encryptor-id}))))
nil nil
:aes128-gcm-sha512 aes128-gcm-encryptor
:aes128-cbc-sha512 aes128-cbc-encryptor
:no-header (throw (ex-info ":auto not supported on headerless data." {}))
:else (throw (ex-info ":auto not supported for non-standard encryptors." {}))
(do (throw (ex-info (str "Unrecognized :auto encryptor id: " encryptor-id)
{:encryptor-id encryptor-id})))))

(def ^:private err-msg-unknown-thaw-failure
"Decryption/decompression failure, or data unfrozen/damaged.")
Expand Down
28 changes: 20 additions & 8 deletions src/taoensso/nippy/encryption.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@
[taoensso.encore :as enc]
[taoensso.nippy.crypto :as crypto]))

(def standard-header-ids "These'll support :auto thaw" #{:aes128-sha512})
(def standard-header-ids
"These'll support :auto thaw"
#{:aes128-cbc-sha512
:aes128-gcm-sha512})

(defprotocol IEncryptor
(header-id [encryptor])
Expand All @@ -15,7 +18,7 @@
(throw (ex-info
(str "Expected password form: "
"[<#{:salted :cached}> <password-string>].\n "
"See `default-aes128-encryptor` docstring for details!")
"See `aes128-encryptor` docstring for details!")
{:typed-password typed-password})))

(defn- destructure-typed-pwd [typed-password]
Expand All @@ -28,7 +31,7 @@

(comment (destructure-typed-pwd [:salted "foo"]))

(deftype AES128Encryptor [header-id salted-key-fn cached-key-fn]
(deftype AES128Encryptor [header-id cipher-kit salted-key-fn cached-key-fn]
IEncryptor
(header-id [_] header-id)
(encrypt [_ typed-pwd plain-ba]
Expand All @@ -42,7 +45,7 @@
(cached-key-fn nil pwd)))]

(crypto/encrypt
{:cipher-kit crypto/cipher-kit-aes-cbc
{:cipher-kit cipher-kit
:?salt-ba ?salt-ba
:key-ba key-ba
:plain-ba plain-ba})))
Expand All @@ -56,13 +59,13 @@
#(cached-key-fn % pwd))]

(crypto/decrypt
{:cipher-kit crypto/cipher-kit-aes-cbc
{:cipher-kit cipher-kit
:salt-size (if salt? 16 0)
:salt->key-fn salt->key-fn
:enc-ba enc-ba}))))

(def aes128-encryptor
"Default 128bit AES encryptor with many-round SHA-512 key-gen.
(def aes128-gcm-encryptor
"Default 128bit AES-GCM encryptor with many-round SHA-512 key-gen.
Password form [:salted \"my-password\"]
---------------------------------------
Expand Down Expand Up @@ -97,7 +100,16 @@
Faster than `aes128-salted`, and harder to attack any particular key - but
increased danger if a key is somehow compromised."

(AES128Encryptor. :aes128-sha512
(AES128Encryptor. :aes128-gcm-sha512
crypto/cipher-kit-aes-gcm
(do (fn [ salt-ba pwd] (crypto/take-ba 16 (crypto/sha512-key-ba salt-ba pwd (* Short/MAX_VALUE 5)))))
(enc/memoize_ (fn [_salt-ba pwd] (crypto/take-ba 16 (crypto/sha512-key-ba nil pwd (* Short/MAX_VALUE 64)))))))

(def aes128-cbc-encryptor
"Default 128bit AES-CBC encryptor with many-round SHA-512 key-gen.
See also `aes-128-cbc-encryptor`."
(AES128Encryptor. :aes128-cbc-sha512
crypto/cipher-kit-aes-cbc
(do (fn [ salt-ba pwd] (crypto/take-ba 16 (crypto/sha512-key-ba salt-ba pwd (* Short/MAX_VALUE 5)))))
(enc/memoize_ (fn [_salt-ba pwd] (crypto/take-ba 16 (crypto/sha512-key-ba nil pwd (* Short/MAX_VALUE 64)))))))

Expand Down
7 changes: 6 additions & 1 deletion test/taoensso/nippy/tests/main.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,12 @@
(thaw (org.xerial.snappy.Snappy/uncompress xerial-ba))
(thaw (org.xerial.snappy.Snappy/uncompress iq80-ba))
(thaw (org.iq80.snappy.Snappy/uncompress iq80-ba 0 (alength iq80-ba)))
(thaw (org.iq80.snappy.Snappy/uncompress xerial-ba 0 (alength xerial-ba)))))))
(thaw (org.iq80.snappy.Snappy/uncompress xerial-ba 0 (alength xerial-ba))))))

(is ; CBC auto-encryptor compatibility
(= "payload"
(thaw (freeze "payload" {:password [:salted "pwd"] :encryptor nippy/aes128-cbc-encryptor})
(do {:password [:salted "pwd"]})))))

;;;; Custom types & records

Expand Down

0 comments on commit 23276ac

Please sign in to comment.