Fix #4882, use different secrets for different signing purposes (#4883)

Fix #4882, use different secrets for different signing purposes

This adds signing 'scopes', so that if you get something signed for one scope, you can't use it for another scope. E.g., you can't get a download URL and use that for an authentication key.

This keeps the 'legacy' scope, which is the current single key. This can be used now to make sure everything works when people upgrade, but removed later as people have used to the new specific scopes. But nothing new will be signed with the legacy scope once this is deployed.

This also updates some functions to use async/await.

Additionally, do not allow the download filename created for one image to be used for another image

Before this change you could take the `?download=...&sig=...` from one image and put it on another image URL, causing that other image to be downloaded with the other filename.

server/db-patches/patch-25-26.sql

@@ -0,0 +1,5 @@
+ALTER TABLE signing_keys ADD COLUMN scope TEXT;
+-- We don't want 'legacy' to actually be the default, we just want to set our existing
+-- keys to this scope, and force new keys to have an explicit scope set:
+UPDATE signing_keys SET scope = 'legacy';
+ALTER TABLE signing_keys ALTER COLUMN scope SET NOT NULL;

server/db-patches/patch-26-25.sql

@@ -0,0 +1 @@
+ALTER TABLE signing_keys DROP COLUMN scope;

server/src/dbschema.js

@@ -7,7 +7,9 @@ const mozlog = require("./logging").mozlog("dbschema");
 
 // When updating the database, please also run ./bin/dumpschema --record
 // This updates schema.sql with the latest full database schema
-const MAX_DB_LEVEL = exports.MAX_DB_LEVEL = 25;
+const MAX_DB_LEVEL = exports.MAX_DB_LEVEL = 26;
+// This is a list of all the Keygrip scopes we allow (and we make sure these exist):
+const KEYGRIP_SCOPES = ["auth", "proxy-url", "download-url", "ga-user-nonce"];
 
 exports.forceDbVersion = function(version) {
   mozlog.info("forcing-db-version", {db: db.constr, version});
@@ -93,57 +95,82 @@ exports.createTables = function() {
   });
 };
 
-let keys;
-let textKeys;
+let keysByScope;
+let textKeysByScope;
 
-exports.getKeygrip = function() {
-  return keys;
+exports.getKeygrip = function(scope) {
+  if (!scope || !KEYGRIP_SCOPES.includes(scope)) {
+    throw new Error("You must give a valid scope");
+  }
+  return keysByScope[scope];
 };
 
-exports.getTextKeys = function() {
-  return textKeys;
+exports.getTextKeys = function(scope) {
+  if (!scope || !KEYGRIP_SCOPES.includes(scope)) {
+    throw new Error("You must give a valid scope");
+  }
+  return textKeysByScope[scope];
 };
 
 /** Loads the random signing key from the database, or generates a new key
     if none are found */
-function loadKeys() {
-  return db.select(
-    `SELECT key FROM signing_keys ORDER BY CREATED`,
+async function loadKeys() {
+  const rows = await db.select(
+    `SELECT key, scope FROM signing_keys ORDER BY CREATED`,
     []
-  ).then((rows) => {
-    if (rows.length) {
-      const textKeys = [];
-      for (let i = 0; i < rows.length; i++) {
-        textKeys.push(rows[i].key);
+  );
+  const textKeysByScope = {};
+  for (let i = 0; i < rows.length; i++) {
+    const scope = rows[i].scope;
+    if (scope === "legacy") {
+      continue;
+    }
+    let textKeys = textKeysByScope[scope];
+    if (!textKeys) {
+      textKeys = textKeysByScope[scope] = [];
+    }
+    textKeys.push(rows[i].key);
+  }
+  for (const scope of KEYGRIP_SCOPES) {
+    if (!textKeysByScope[scope]) {
+      const key = await makeKey();
+      const ok = await db.insert(
+        `INSERT INTO signing_keys (created, key, scope)
+         VALUES (NOW(), $1, $2)`,
+        [key, scope]
+      );
+      if (!ok) {
+        throw new Error("Could not insert key");
       }
-      return textKeys;
+      textKeysByScope[scope] = [key];
     }
-    return makeKey().then((key) => {
-      return db.insert(
-        `INSERT INTO signing_keys (created, key)
-         VALUES (NOW(), $1)`,
-        [key]
-      ).then((ok) => {
-        if (!ok) {
-          throw new Error("Could not insert key");
-        }
-        return [key];
-      });
-    });
-  });
+  }
+  for (let i = 0; i < rows.length; i++) {
+    const scope = rows[i].scope;
+    if (scope === "legacy") {
+      for (const otherScope in textKeysByScope) {
+        textKeysByScope[otherScope].push(rows[i].key);
+      }
+    }
+  }
+  return textKeysByScope;
 }
 
-exports.createKeygrip = function() {
-  return loadKeys().then((fetchedTextKeys) => {
-    textKeys = fetchedTextKeys;
-    keys = new Keygrip(textKeys);
-  }).catch((err) => {
+exports.createKeygrip = async function() {
+  try {
+    const fetchedTextKeysByScope = await loadKeys();
+    textKeysByScope = fetchedTextKeysByScope;
+    keysByScope = {};
+    for (const scope in textKeysByScope) {
+      keysByScope[scope] = new Keygrip(textKeysByScope[scope]);
+    }
+  } catch (err) {
     mozlog.warn("error-creating-signing-keys", {
       msg: "Could not create signing keys:",
       err,
     });
     throw err;
-  });
+  }
 };
 
 /** Returns a promise that generates a new largish ASCII random key */
@@ -169,7 +196,7 @@ function getCurrentDbPatchLevel() {
 exports.getCurrentDbPatchLevel = getCurrentDbPatchLevel;
 
 exports.connectionOK = function() {
-  if (!keys) {
+  if (!keysByScope) {
     return Promise.resolve(false);
   }
   return getCurrentDbPatchLevel().then(currentLevel => {

server/src/proxy-url.js

@@ -1,10 +1,11 @@
 const dbschema = require("./dbschema");
 const config = require("./config").getProperties();
 const { isValidClipImageUrl } = require("../shared/shot");
+const { URL } = require("url");
 
 exports.createProxyUrl = function(req, url, hash) {
   const base = `${req.protocol}://${config.contentOrigin}`;
-  const sig = dbschema.getKeygrip().sign(new Buffer(url, "utf8"));
+  const sig = dbschema.getKeygrip("proxy-url").sign(new Buffer(url, "utf8"));
   let proxy = `${base}/proxy?url=${encodeURIComponent(url)}&sig=${encodeURIComponent(sig)}`;
   if (hash) {
     proxy += "#" + hash;
@@ -13,7 +14,8 @@ exports.createProxyUrl = function(req, url, hash) {
 };
 
 exports.createDownloadUrl = function(url, filename) {
-  const sig = dbschema.getKeygrip().sign(new Buffer(filename, "utf8"));
+  const path = (new URL(url)).pathname;
+  const sig = dbschema.getKeygrip("download-url").sign(new Buffer(`${path} ${filename}`, "utf8"));
   if (!isValidClipImageUrl(url)) {
       return "";
   }

server/src/server.js

@@ -189,7 +189,7 @@ app.use(morgan("combined"));
 app.use(function(req, res, next) {
   const authHeader = req.headers["x-screenshots-auth"];
   let authInfo = {};
-  const cookies = new Cookies(req, res, {keys: dbschema.getKeygrip()});
+  const cookies = new Cookies(req, res, {keys: dbschema.getKeygrip("auth")});
   if (authHeader) {
     authInfo = decodeAuthHeader(authHeader);
   } else {
@@ -245,7 +245,7 @@ app.use(csrfProtection);
 function decodeAuthHeader(header) {
   /** Decode a string header in the format {deviceId}:{deviceIdSig};abtests={b64thing}:{sig} */
   // Since it's treated as opaque, we'll use a fragile regex
-  const keygrip = dbschema.getKeygrip();
+  const keygrip = dbschema.getKeygrip("auth");
   const match = /^([^:]{1,255}):([^;]{1,255});abTests=([^:]{1,1500}):(.{0,255})$/.exec(header);
   if (!match) {
     const exc = new Error("Invalid auth header");
@@ -406,10 +406,10 @@ app.post("/error", function(req, res) {
 
 function hashUserId(deviceId) {
   return new Promise((resolve, reject) => {
-    if (!dbschema.getTextKeys()) {
+    if (!dbschema.getTextKeys("ga-user-nonce")) {
       throw new Error("Server keys not initialized");
     }
-    const userKey = dbschema.getTextKeys()[0] + deviceId;
+    const userKey = dbschema.getTextKeys("ga-user-nonce")[0] + deviceId;
     genUuid.generate(genUuid.V_SHA1, genUuid.nil, userKey, function(err, userUuid) {
       if (err) {
         reject(err);
@@ -554,7 +554,7 @@ function sendAuthInfo(req, res, params) {
     throw new Error("Bad deviceId");
   }
   const encodedAbTests = b64EncodeJson(userAbTests);
-  const keygrip = dbschema.getKeygrip();
+  const keygrip = dbschema.getKeygrip("auth");
   const cookies = new Cookies(req, res, {keys: keygrip});
   cookies.set("user", deviceId, {signed: true, sameSite: "lax", maxAge: COOKIE_EXPIRE_TIME});
   if (accountId) {
@@ -784,7 +784,7 @@ app.post("/api/disconnect-device", function(req, res) {
     return;
   }
   disconnectDevice(req.deviceId).then((result) => {
-    const keygrip = dbschema.getKeygrip();
+    const keygrip = dbschema.getKeygrip("auth");
     const cookies = new Cookies(req, res, {keys: keygrip});
     if (result) {
       cookies.set("accountid");
@@ -949,7 +949,7 @@ app.get("/images/:imageid", function(req, res) {
       }
       res.header("Content-Type", contentType);
       if (download) {
-        if (dbschema.getKeygrip().verify(new Buffer(download, "utf8"), sig)) {
+        if (dbschema.getKeygrip("download-url").verify(new Buffer(`${req.path} ${download}`, "utf8"), sig)) {
           res.header("Content-Disposition", contentDisposition(download));
         }
       }

test/server/clientlib.py

@@ -81,13 +81,18 @@ def read_shot(self, url):
         title = None
         if title_match:
             title = title_match.group(1)
+        download_match = re.search(r'"([^"]+?download=[^"]+)"', page)
+        download_url = None
+        if download_match:
+            download_url = download_match.group(1).replace("&", "&")
         return {
             "page": page,
             "clip_url": clip_url,
             "clip_content": clip_content,
             "clip_content_type": clip_content_type,
             "csrf": csrf,
             "title": title,
+            "download_url": download_url,
         }
 
     def set_expiration(self, url, seconds):

test/server/pglib.py

@@ -27,7 +27,7 @@ def attach_device(device_id, account_id):
     token = str(uuid.uuid1())
     cur.execute("INSERT INTO accounts (id, token) VALUES (%s, %s)", (account_id, token))
     cur.execute("UPDATE devices SET accountid = %s WHERE id = %s", (account_id, device_id))
-    cur.execute("SELECT key FROM signing_keys ORDER BY created DESC LIMIT 1")
+    cur.execute("SELECT key FROM signing_keys WHERE scope = 'auth' ORDER BY created DESC LIMIT 1")
     key_row = cur.fetchone()
     account_id_hmac = __get_hmac("accountid=%s" % account_id, key_row[0])
     cur.close()

test/server/test_signatures.py

@@ -0,0 +1,51 @@
+from __future__ import print_function
+from clientlib import ScreenshotsClient
+import urllib
+
+
+def test_download_key():
+    user = ScreenshotsClient()
+    user.login()
+    shot_1_url = user.create_shot(docTitle="A_TEST_SITE_1")
+    shot_2_url = user.create_shot(docTitle="A_TEST_SITE_2")
+    shot_1_page = user.read_shot(shot_1_url)
+    shot_2_page = user.read_shot(shot_2_url)
+    shot_1_download_url = shot_1_page["download_url"]
+    shot_2_download_url = shot_2_page["download_url"]
+    resp = user.session.get(shot_1_download_url)
+    # This should normally work:
+    print("Normal download URL:", shot_1_download_url)
+    assert resp.headers["Content-Disposition"], "Should get a proper download response"
+    mixed_up = shot_1_download_url.split("?")[0] + "?" + shot_2_download_url.split("?")[1]
+    # Getting mixed_up should fail, since the signed download parameter comes from shot_2
+    # but we're fetching the image from shot_1
+    resp = user.session.get(mixed_up)
+    print("Mixed-up URL:", mixed_up)
+    print("Response:", resp)
+    print("Content-Disposition header:", resp.headers.get("Content-Disposition"))
+    assert not resp.headers.get("Content-Disposition"), "The signature shouldn't work"
+
+
+def test_scopes():
+    user = ScreenshotsClient()
+    user.login()
+    abtests = user.session.cookies["abtests"]
+    abtests_sig = user.session.cookies["abtests.sig"]
+    print(abtests, abtests_sig)
+    shot = user.create_shot(docTitle="A_TEST_SITE")
+    page = user.read_shot(shot)
+    download_url = page["download_url"]
+    resp = user.session.get(download_url)
+    assert resp.headers.get("Content-Disposition")
+    mixed_up = "%s?download=%s&sig=%s" % (
+        download_url.split("?")[0],
+        urllib.quote(abtests),
+        urllib.quote(abtests_sig),
+    )
+    resp = user.session.get(mixed_up)
+    assert not resp.headers.get("Content-Disposition")
+
+
+if __name__ == "__main__":
+    test_download_key()
+    test_scopes()