Disallow :alg = :none and test

src/metabase/api/tiles.clj

@@ -92,7 +92,7 @@
         (.dispose graphics)))
     tile))
 
-(defn- tile->byte-array [^BufferedImage tile]
+(defn- tile->byte-array ^bytes [^BufferedImage tile]
   (let [output-stream (ByteArrayOutputStream.)]
     (try
       (when-not (ImageIO/write tile "png" output-stream) ; returns `true` if successful -- see JavaDoc

src/metabase/api/util.clj

@@ -27,10 +27,10 @@
   (stats/anonymous-usage-stats))
 
 (defendpoint GET "/random_token"
-  "Return a cryptographically secure random 32-byte token, encoded as a hexidecimal string.
+  "Return a cryptographically secure random 32-byte token, encoded as a base-64 string.
    Intended for use when creating a value for `embedding-secret-key`."
   []
-  {:token (crypto-random/hex 32)})
+  {:token (crypto-random/base64 32)})
 
 
 (define-routes)

src/metabase/pulse/render.clj

@@ -213,7 +213,7 @@
     (ImageIO/write (.getImage content-canvas) "png" os)))
 
 (defn- render-html-to-png
-  [html-body width]
+  ^bytes [html-body width]
   (let [html (html [:html [:body {:style (style {:margin           0
                                                  :padding          0
                                                  :background-color :white})}
@@ -402,7 +402,7 @@
       :else                                                        :table)))
 
 (defn render-pulse-card
-  "Render a single CARD for a `Pulse`. RESULT is the QP results."
+  "Render a single CARD for a `Pulse` to Hiccup HTML. RESULT is the QP results."
   [card {:keys [data error]}]
   [:a {:href   (card-href card)
        :target "_blank"
@@ -446,7 +446,7 @@
 
 
 (defn render-pulse-section
-  "Render a specific section of a Pulse, i.e. a single Card."
+  "Render a specific section of a Pulse, i.e. a single Card, to Hiccup HTML."
   [{:keys [card result]}]
   [:div {:style (style {:margin-top       :10px
                         :margin-bottom    :20px
@@ -459,6 +459,5 @@
 
 (defn render-pulse-card-to-png
   "Render a PULSE-CARD as a PNG. DATA is the `:data` from a QP result (I think...)"
-
-  [pulse-card result]
+  ^bytes [pulse-card result]
   (render-html-to-png (render-pulse-card pulse-card result) card-width))

src/metabase/util/embed.clj

@@ -1,7 +1,10 @@
 (ns metabase.util.embed
   "Utility functions for public links and embedding."
-  (:require (buddy.sign [jwt :as jwt]
+  (:require [clojure.string :as str]
+            [buddy.core.codecs :as codecs]
+            (buddy.sign [jwt :as jwt]
                         [util :as buddy-util])
+            [cheshire.core :as json]
             [ring.util.codec :as codec]
             [hiccup.core :refer [html]]
             [metabase.models.setting :as setting]
@@ -33,7 +36,7 @@
   (html [:meta {:name "generator", :content "Metabase"}]))
 
 (defn head
-  "Returns the <meta>/<link> tags for an embeddable public page"
+  "Returns the `<meta>`/`<link>` tags for an embeddable public page."
   ^String [^String url]
   (str embedly-meta (oembed-link url)))
 
@@ -56,13 +59,30 @@
                 "Invalid embedding-secret-key! Secret key must be a hexadecimal-encoded 256-bit key (i.e., a 64-character string)."))
             (setting/set-string! :embedding-secret-key new-value)))
 
+(defn- jwt-header
+  "Parse a JWT MESSAGE and return the header portion."
+  [^String message]
+  (let [[header] (str/split message #"\.")]
+    (json/parse-string (codecs/bytes->str (codec/base64-decode header)) keyword)))
+
+(defn- check-valid-alg
+  "Check that the JWT `alg` isn't `none`. `none` is valid per the standard, but for obvious reasons we want to make sure our keys are signed.
+   Unfortunately, I don't think there's an easy way to do this with the JWT library we use, so manually parse the token and check the alg."
+  [^String message]
+  (let [{:keys [alg]} (jwt-header message)]
+    (when-not alg
+      (throw (Exception. "JWT is missing `alg`.")))
+    (when (= alg "none")
+      (throw (Exception. "JWT `alg` cannot be `none`.")))))
+
 (defn unsign
   "Parse a \"signed\" (base-64 encoded) JWT and return a Clojure representation.
    Check that the signature is valid (i.e., check that it was signed with `embedding-secret-key`)
    and it's otherwise a valid JWT (e.g., not expired), or throw an Exception."
   [^String message]
   (when (seq message)
     (try
+      (check-valid-alg message)
       (jwt/unsign message
                   (or (embedding-secret-key)
                       (throw (ex-info "The embedding secret key has not been set." {:status-code 400})))

src/metabase/util/encryption.clj

@@ -11,13 +11,13 @@
 
 (defn secret-key->hash
   "Generate a 64-byte byte array hash of SECRET-KEY using 100,000 iterations of PBKDF2+SHA512."
-  [^String secret-key]
+  ^bytes [^String secret-key]
   (kdf/get-bytes (kdf/engine {:alg        :pbkdf2+sha512
                               :key        secret-key
                               :iterations 100000}) ; 100,000 iterations takes about ~160ms on my laptop
                  64))
 
-(defonce ^:private default-secret-key
+(defonce ^:private ^bytes default-secret-key
   (when-let [secret-key (env/env :mb-encryption-secret-key)]
     (when (seq secret-key)
       (assert (>= (count secret-key) 16)
@@ -34,18 +34,18 @@
   (^String [^String s]
    (encrypt default-secret-key s))
   (^String [^String secret-key, ^String s]
-   (let [iv (nonce/random-bytes 16)]
-     (codec/base64-encode (byte-array (concat iv
-                                              (crypto/encrypt (codecs/to-bytes s) secret-key iv {:algorithm :aes256-cbc-hmac-sha512})))))))
+   (let [initialization-vector (nonce/random-bytes 16)]
+     (codec/base64-encode (byte-array (concat initialization-vector
+                                              (crypto/encrypt (codecs/to-bytes s) secret-key initialization-vector {:algorithm :aes256-cbc-hmac-sha512})))))))
 
 (defn decrypt
   "Decrypt string S  using a SECRET-KEY (a 64-byte byte array), by default the hashed value of `MB_ENCRYPTION_SECRET_KEY`."
   (^String [^String s]
    (decrypt default-secret-key s))
   (^String [secret-key, ^String s]
-   (let [bytes        (codec/base64-decode s)
-         [iv message] (split-at 16 bytes)]
-     (codecs/bytes->str (crypto/decrypt (byte-array message) secret-key (byte-array iv) {:algorithm :aes256-cbc-hmac-sha512})))))
+   (let [bytes                           (codec/base64-decode s)
+         [initialization-vector message] (split-at 16 bytes)]
+     (codecs/bytes->str (crypto/decrypt (byte-array message) secret-key (byte-array initialization-vector) {:algorithm :aes256-cbc-hmac-sha512})))))
 
 
 (defn maybe-encrypt

test/metabase/util/embed_test.clj

@@ -0,0 +1,8 @@
+(ns metabase.util.embed-test
+  (:require [expectations :refer :all]
+            [metabase.util.embed :as embed]))
+
+;; check that we disallow tokens signed with alg = none
+(expect
+  clojure.lang.ExceptionInfo
+  (embed/unsign "eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJhZG1pbiI6dHJ1ZX0.hGR6ufA7hbxH4RVyh26Z8Lz96LkarlYh3nLe2fqAOe0")) ; token with alg = none