From db2c22eed83a312d76469c22be3ef34eff5006b8 Mon Sep 17 00:00:00 2001
From: Peter Taoussanis <ptaoussanis@gmail.com>
Date: Thu, 10 Sep 2020 13:35:55 +0200
Subject: [PATCH] Serializable: NB freeze default is now always to ALLOW ALL

We have 2 options:

  A: Default to Serializable whitelist checks on both freeze and thaw
  B: Default to Serializable whitelist checks only on thaw

Before this commit, Nippy was taking option A.
As of  this commit, Nippy is  taking option B.

Both are equally safe re: the risk of Remote Code Execution in #130:

  - Freezing a        malicious payload  is  *not* a security risk
  - Thawing  a frozen malicious payload *is*       a security risk.

But option B has the benefit of not throwing exceptions by default
against a whitelist that has not [yet] been properly configured.

This is especially helpful for other libraries or applications that
may be using Nippy as an underlying dependency.

Behaviour under our two options against a whitelist that has not
[yet] been properly configured:

  A: Throw exception on freeze
  B: Freeze successfully, and thaw successully as
     {:nippy/unthawable {:class-name <> :content <quarantined-ba> :cause :quarantined}}

I think this is probably less of a nuissance, and so a better default.
---
 src/taoensso/nippy.clj             | 21 ++++++++++++++-------
 test/taoensso/nippy/tests/main.clj | 25 ++++++++++++++-----------
 2 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/src/taoensso/nippy.clj b/src/taoensso/nippy.clj
index dfaf3ac..b5b9509 100644
--- a/src/taoensso/nippy.clj
+++ b/src/taoensso/nippy.clj
@@ -1196,21 +1196,28 @@
     - :encryptor  nil
     - :no-header? true"
   [x]
-  (let [baos (ByteArrayOutputStream. 64)
-        dos  (DataOutputStream. baos)]
-    (with-cache (-freeze-with-meta! x dos))
-    (.toByteArray baos)))
+  (binding [*serializable-whitelist* "*"]
+    (let [baos (ByteArrayOutputStream. 64)
+          dos  (DataOutputStream. baos)]
+      (with-cache (-freeze-with-meta! x dos))
+      (.toByteArray baos))))
 
 (defn freeze
   "Serializes arg (any Clojure data type) to a byte array. To freeze custom
-  types, extend the Clojure reader or see `extend-freeze`."
+  types, extend the Clojure reader or see `extend-freeze`.
+
+  Note: `serializable-whitelist` is \"*\" by default for freezing (not thawing!).
+  If you want to use the value of `*serializable-whitelist*` instead, include
+  `:serializable-whitelist :default` in opts."
+
   ([x] (freeze x nil))
   ([x {:as   opts
        :keys [compressor encryptor password serializable-whitelist incl-metadata?]
        :or   {compressor :auto
-              encryptor  aes128-gcm-encryptor}}]
+              encryptor  aes128-gcm-encryptor
+              serializable-whitelist "*"}}]
 
-   (call-with-bindings opts
+   (call-with-bindings (assoc opts :serializable-whitelist serializable-whitelist)
      (fn []
 
        (let [;; Intentionally undocumented:
diff --git a/test/taoensso/nippy/tests/main.clj b/test/taoensso/nippy/tests/main.clj
index 8d396aa..bab06e9 100644
--- a/test/taoensso/nippy/tests/main.clj
+++ b/test/taoensso/nippy/tests/main.clj
@@ -252,26 +252,27 @@
 
   (is
     (thrown? Exception
-      (binding [nippy/*serializable-whitelist* #{}]
-        (nippy/freeze (java.util.concurrent.Semaphore. 1))))
+      (nippy/freeze (java.util.concurrent.Semaphore. 1)
+        {:serializable-whitelist #{}}))
 
     "Can't freeze Serializable object unless approved by whitelist")
 
   (is
     (:nippy/unthawable
 
-     (let [ba (binding [nippy/*serializable-whitelist* #{"java.util.concurrent.Semaphore"}]
-                (nippy/freeze (java.util.concurrent.Semaphore. 1)))]
+     (let [ba (nippy/freeze (java.util.concurrent.Semaphore. 1)
+                {:serializable-whitelist #{"java.util.concurrent.Semaphore"}})]
 
-       (binding [nippy/*serializable-whitelist* #{}]
-         (nippy/thaw ba))))
+       (nippy/thaw ba {:serializable-whitelist #{}})))
 
     "Can't thaw Serializable object unless approved by whitelist")
 
   (is
     (instance? java.util.concurrent.Semaphore
-      (binding [nippy/*serializable-whitelist* #{"java.util.concurrent.Semaphore"}]
-        (nippy/thaw (nippy/freeze (java.util.concurrent.Semaphore. 1)))))
+      (nippy/thaw
+        (nippy/freeze (java.util.concurrent.Semaphore. 1)
+          {:serializable-whitelist #{"java.util.concurrent.Semaphore"}})
+        {:serializable-whitelist #{"java.util.concurrent.Semaphore"}}))
 
     "Can freeze and thaw Serializable object if approved by whitelist")
 
@@ -280,7 +281,7 @@
       (get-in
         (nippy/thaw
           (nippy/freeze (java.util.concurrent.Semaphore. 1)
-            {:serializable-whitelist "*"})
+            #_{:serializable-whitelist "*"})
           {:serializable-whitelist #{}})
         [:nippy/unthawable :cause]))
 
@@ -288,8 +289,10 @@
 
   (is
     (instance? java.util.concurrent.Semaphore
-      (binding [nippy/*serializable-whitelist* #{"java.util.concurrent.*"}]
-        (nippy/thaw (nippy/freeze (java.util.concurrent.Semaphore. 1)))))
+      (nippy/thaw
+        (nippy/freeze (java.util.concurrent.Semaphore. 1)
+          {:serializable-whitelist #{"java.util.concurrent.*"}})
+        {:serializable-whitelist #{"java.util.concurrent.*"}}))
 
     "Strings in whitelist sets may contain \"*\" wildcards")