Skip to content

Commit

Permalink
Serializable: NB freeze default is now always to ALLOW ALL
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ptaoussanis committed Sep 10, 2020
1 parent db71943 commit db2c22e
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 18 deletions.
21 changes: 14 additions & 7 deletions src/taoensso/nippy.clj
Expand Up @@ -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:
Expand Down
25 changes: 14 additions & 11 deletions test/taoensso/nippy/tests/main.clj
Expand Up @@ -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")

Expand All @@ -280,16 +281,18 @@
(get-in
(nippy/thaw
(nippy/freeze (java.util.concurrent.Semaphore. 1)
{:serializable-whitelist "*"})
#_{:serializable-whitelist "*"})
{:serializable-whitelist #{}})
[:nippy/unthawable :cause]))

"Thaw will quarantine Serializable objects approved when freezing.")

(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")

Expand Down

0 comments on commit db2c22e

Please sign in to comment.