Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Commit

Permalink
Fix #4882, use different secrets for different signing purposes (#4883)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ianb authored Sep 17, 2018
1 parent 50e6f02 commit 429a593
Show file tree
Hide file tree
Showing 8 changed files with 137 additions and 46 deletions.
5 changes: 5 additions & 0 deletions server/db-patches/patch-25-26.sql
Original file line number Diff line number Diff line change
@@ -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;
1 change: 1 addition & 0 deletions server/db-patches/patch-26-25.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE signing_keys DROP COLUMN scope;
99 changes: 63 additions & 36 deletions server/src/dbschema.js
Original file line number Diff line number Diff line change
Expand Up @@ -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});
Expand Down Expand Up @@ -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 */
Expand All @@ -169,7 +196,7 @@ function getCurrentDbPatchLevel() {
exports.getCurrentDbPatchLevel = getCurrentDbPatchLevel;

exports.connectionOK = function() {
if (!keys) {
if (!keysByScope) {
return Promise.resolve(false);
}
return getCurrentDbPatchLevel().then(currentLevel => {
Expand Down
6 changes: 4 additions & 2 deletions server/src/proxy-url.js
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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 "";
}
Expand Down
14 changes: 7 additions & 7 deletions server/src/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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));
}
}
Expand Down
5 changes: 5 additions & 0 deletions test/server/clientlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -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("&amp;", "&")
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):
Expand Down
2 changes: 1 addition & 1 deletion test/server/pglib.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
51 changes: 51 additions & 0 deletions test/server/test_signatures.py
Original file line number Diff line number Diff line change
@@ -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()

0 comments on commit 429a593

Please sign in to comment.