Skip to content

Commit

Permalink
remove obsolete CSRF request token approach
Browse files Browse the repository at this point in the history 
AttestationServer prevents CSRF by strictly enforcing the presence of
the `Origin` header set to the site's domain. Firefox fixed a long time
bug with the `Origin` header a few years ago so we can rely on it being
available without falling back to `Referer`. It's dramatically simpler
than request tokens and doesn't require stateful sessions so it's easier
to correctly protect login/register without resorting to making sessions
for every user instead of only having login sessions.

Our strict approach to enforcing `Origin` requires entirely avoiding the
GET and HEAD methods for the web API since `Origin` isn't usually passed
for those methods. For example, we fetch an image from /api/account.png
as a blob and set it as the value for an img element in order to avoid
depending on a GET request where the `Origin` header wouldn't normally
be included. Most services wouldn't be able to rely entirely on this due
to serving dynamic HTML and using GET as a RESTful verb for retrieving
content. GET should still be protected from CSRF because it still often
results in side effects like updating access history and opens up side
channels for other web sites.

As a secondary layer of protection for the subset of the APIs requiring
a login session, our login session cookie has SameSite=Strict to
restrict it to same site requests. We also mark the cookie with Secure,
HttpOnly and Path=/. It's defined with a __Host- prefix as
__Host-session to enforce that Secure and Path=/ are present which
results in it being strictly same origin instead of the looser rules for
SameSite=Strict where it can cross subdomains. SameSite=Strict for the
login session cookie is not a full protection since it doesn't protect
login and registration like the strict `Origin` header enforcement. It
would be possible to make this into a full protection by adding another
kind of session separate from login with another SameSite=Strict cookie
but we currently don't want to do this for the same reasons we want to
avoid it with request tokens. Defense in depth is no use if it ends up
weakening your security by adding attack surface and we're concerned
about the implications for privacy and aiding denial of service too.

We also use the Fetch Metadata Request Headers feature when available to
enforce that `Sec-Fetch-Mode` and `Sec-Fetch-Site` are both set to the
`same-origin` value as a redundant check. These headers aren't available
in Safari so we can't enforce their presence yet. We also enforce that
the `Sec-Fetch-Dest` header is `none` to rule out requests coming from
an img element, etc. This is not particularly useful since we only use
POST for the API, but it might as well be enforced.

We have entirely static HTML, CSS and JavaScript with a strict Content
Security Policy enforcing Trusted Types with a `'none'` policy which
means custom sanitizers are completely disallowed. Since our HTML is
fully static with the enforcement by Trusted Types, we don't need to be
very concerned about content injection issues.

We also enforce that all of the content used in the page including
scripts and styles comes from 'self' without 'unsafe-inline' or
'unsafe-eval'. We have Subresource Integrity for both scripts and styles
with the aim of switching to permitting hashes rather than 'self' but
this is currently blocked by Firefox and Safari being missing the
standard external hash-source support for Content Security Policy and we
see no reliable way to deploy it only for Chromium.

Overall, we have extremely strong protections in place against CSRF and
content injection. Request tokens were adding a lot of complexity and we
already depending on `Origin` to fully protect the login API, since the
request tokens alone would prevent creating a login session but it would
be possible to update the last login time for a logged out user if we
didn't have the `Origin` check.
  • Loading branch information
@thestinger
thestinger committed Jun 28, 2022
1 parent 7c6ab45 commit 1757993
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 97 deletions.
115 changes: 55 additions & 60 deletions src/main/java/app/attestation/server/AttestationServer.java
View file
Expand Up @@ -139,8 +139,7 @@ private static void createAttestationTables(final SQLiteConnection conn) throws
"CREATE TABLE IF NOT EXISTS Sessions (\n" +
"sessionId INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT,\n" +
"userId INTEGER NOT NULL REFERENCES Accounts (userId) ON DELETE CASCADE,\n" +
"cookieToken BLOB NOT NULL,\n" +
"requestToken BLOB NOT NULL,\n" +
"token BLOB NOT NULL,\n" +
"expiryTime INTEGER NOT NULL\n" +
") STRICT");

Expand Down Expand Up @@ -282,7 +281,7 @@ public static void main(final String[] args) throws Exception {

final SQLiteStatement selectCreated = attestationConn.prepare("SELECT 1 FROM sqlite_master WHERE type='table' AND name='Configuration'");
if (!selectCreated.step()) {
attestationConn.exec("PRAGMA user_version = 8");
attestationConn.exec("PRAGMA user_version = 9");
}
selectCreated.dispose();

Expand Down Expand Up @@ -332,6 +331,31 @@ public static void main(final String[] args) throws Exception {
logger.info("Migrated to schema version: " + userVersion);
}

// remove obsolete requestToken column from Sessions table
if (userVersion < 9) {
attestationConn.exec("PRAGMA foreign_keys = OFF");
attestationConn.exec("BEGIN IMMEDIATE TRANSACTION");

attestationConn.exec("ALTER TABLE Sessions RENAME TO OldSessions");

createAttestationTables(attestationConn);

attestationConn.exec("INSERT INTO Sessions " +
"(sessionId, userId, token, expiryTime) " +
"SELECT " +
"sessionId, userId, cookieToken, expiryTime " +
"FROM OldSessions");

attestationConn.exec("DROP TABLE OldSessions");

createAttestationIndices(attestationConn);
attestationConn.exec("PRAGMA user_version = 9");
attestationConn.exec("COMMIT TRANSACTION");
userVersion = 9;
attestationConn.exec("PRAGMA foreign_keys = ON");
logger.info("Migrated to schema version: " + userVersion);
}

logger.info("Analyze database");
attestationConn.exec("ANALYZE");
logger.info("Vacuum database");
Expand Down Expand Up @@ -562,13 +586,11 @@ private static void changePassword(final long userId, final String currentPasswo

private static class Session {
final long sessionId;
final byte[] cookieToken;
final byte[] requestToken;
final byte[] token;

Session(final long sessionId, final byte[] cookieToken, final byte[] requestToken) {
Session(final long sessionId, final byte[] token) {
this.sessionId = sessionId;
this.cookieToken = cookieToken;
this.requestToken = requestToken;
this.token = token;
}
}

Expand Down Expand Up @@ -602,15 +624,13 @@ private static Session login(final String username, final String password)
delete.step();
delete.dispose();

final byte[] cookieToken = generateRandomToken();
final byte[] requestToken = generateRandomToken();
final byte[] token = generateRandomToken();

final SQLiteStatement insert = conn.prepare("INSERT INTO Sessions " +
"(userId, cookieToken, requestToken, expiryTime) VALUES (?, ?, ?, ?)");
"(userId, token, expiryTime) VALUES (?, ?, ?)");
insert.bind(1, userId);
insert.bind(2, cookieToken);
insert.bind(3, requestToken);
insert.bind(4, now + SESSION_LENGTH);
insert.bind(2, token);
insert.bind(3, now + SESSION_LENGTH);
insert.step();
insert.dispose();

Expand All @@ -623,7 +643,7 @@ private static Session login(final String username, final String password)

conn.exec("COMMIT TRANSACTION");

return new Session(conn.getLastInsertId(), cookieToken, requestToken);
return new Session(conn.getLastInsertId(), token);
} finally {
conn.dispose();
}
Expand Down Expand Up @@ -661,12 +681,10 @@ public void handlePost(final HttpExchange exchange) throws IOException, SQLiteEx
private static class ChangePasswordHandler extends PostHandler {
@Override
public void handlePost(final HttpExchange exchange) throws IOException, SQLiteException {
final String requestToken;
final String currentPassword;
final String newPassword;
try (final JsonReader reader = Json.createReader(exchange.getRequestBody())) {
final JsonObject object = reader.readObject();
requestToken = object.getString("requestToken");
currentPassword = object.getString("currentPassword");
newPassword = object.getString("newPassword");
} catch (final ClassCastException | JsonException | NullPointerException e) {
Expand All @@ -675,7 +693,7 @@ public void handlePost(final HttpExchange exchange) throws IOException, SQLiteEx
return;
}

final Account account = verifySession(exchange, false, requestToken.getBytes(StandardCharsets.UTF_8));
final Account account = verifySession(exchange, false);
if (account == null) {
return;
}
Expand Down Expand Up @@ -719,22 +737,18 @@ public void handlePost(final HttpExchange exchange) throws IOException, SQLiteEx
}

final Base64.Encoder encoder = Base64.getEncoder();
final byte[] requestToken = encoder.encode(session.requestToken);
exchange.getResponseHeaders().set("Set-Cookie",
String.format("__Host-session=%d|%s; HttpOnly; Secure; SameSite=Strict; Path=/; Max-Age=%d",
session.sessionId, new String(encoder.encode(session.cookieToken)),
session.sessionId, new String(encoder.encode(session.token)),
SESSION_LENGTH / 1000));
exchange.sendResponseHeaders(200, requestToken.length);
try (final OutputStream output = exchange.getResponseBody()) {
output.write(requestToken);
}
exchange.sendResponseHeaders(200, -1);
}
}

private static class LogoutHandler extends PostHandler {
@Override
public void handlePost(final HttpExchange exchange) throws IOException, SQLiteException {
final Account account = verifySession(exchange, true, null);
final Account account = verifySession(exchange, true);
if (account == null) {
return;
}
Expand All @@ -746,7 +760,7 @@ public void handlePost(final HttpExchange exchange) throws IOException, SQLiteEx
private static class LogoutEverywhereHandler extends PostHandler {
@Override
public void handlePost(final HttpExchange exchange) throws IOException, SQLiteException {
final Account account = verifySession(exchange, false, null);
final Account account = verifySession(exchange, false);
if (account == null) {
return;
}
Expand All @@ -769,7 +783,7 @@ public void handlePost(final HttpExchange exchange) throws IOException, SQLiteEx
private static class RotateHandler extends PostHandler {
@Override
public void handlePost(final HttpExchange exchange) throws IOException, SQLiteException {
final Account account = verifySession(exchange, false, null);
final Account account = verifySession(exchange, false);
if (account == null) {
return;
}
Expand Down Expand Up @@ -833,7 +847,7 @@ private static class Account {
}
}

private static Account verifySession(final HttpExchange exchange, final boolean end, byte[] requestTokenEncoded)
private static Account verifySession(final HttpExchange exchange, final boolean end)
throws IOException, SQLiteException {
final String cookie = getCookie(exchange, "__Host-session");
if (cookie == null) {
Expand All @@ -847,39 +861,25 @@ private static Account verifySession(final HttpExchange exchange, final boolean
return null;
}
final long sessionId = Long.parseLong(session[0]);
final byte[] cookieToken = Base64.getDecoder().decode(session[1]);

if (requestTokenEncoded == null) {
requestTokenEncoded = new byte[session[1].length()];
final DataInputStream input = new DataInputStream(exchange.getRequestBody());
try {
input.readFully(requestTokenEncoded);
} catch (final EOFException e) {
purgeSessionCookie(exchange);
exchange.sendResponseHeaders(403, -1);
return null;
}
}
final byte[] requestToken = Base64.getDecoder().decode(requestTokenEncoded);
final byte[] token = Base64.getDecoder().decode(session[1]);

final SQLiteConnection conn = new SQLiteConnection(AttestationProtocol.ATTESTATION_DATABASE);
try {
open(conn, !end);

final SQLiteStatement select = conn.prepare("SELECT cookieToken, requestToken, " +
"expiryTime, username, subscribeKey, Accounts.userId, verifyInterval, alertDelay " +
final SQLiteStatement select = conn.prepare("SELECT token, expiryTime, " +
"username, subscribeKey, Accounts.userId, verifyInterval, alertDelay " +
"FROM Sessions " +
"INNER JOIN Accounts on Accounts.userId = Sessions.userId " +
"WHERE sessionId = ?");
select.bind(1, sessionId);
if (!select.step() || !MessageDigest.isEqual(cookieToken, select.columnBlob(0)) ||
!MessageDigest.isEqual(requestToken, select.columnBlob(1))) {
if (!select.step() || !MessageDigest.isEqual(token, select.columnBlob(0))) {
purgeSessionCookie(exchange);
exchange.sendResponseHeaders(403, -1);
return null;
}

if (select.columnLong(2) < System.currentTimeMillis()) {
if (select.columnLong(1) < System.currentTimeMillis()) {
purgeSessionCookie(exchange);
exchange.sendResponseHeaders(403, -1);
return null;
Expand All @@ -893,8 +893,8 @@ private static Account verifySession(final HttpExchange exchange, final boolean
delete.dispose();
}

return new Account(select.columnLong(5), select.columnString(3), select.columnBlob(4),
select.columnInt(6), select.columnInt(7));
return new Account(select.columnLong(4), select.columnString(2), select.columnBlob(3),
select.columnInt(5), select.columnInt(6));
} finally {
conn.dispose();
}
Expand All @@ -903,7 +903,7 @@ private static Account verifySession(final HttpExchange exchange, final boolean
private static class AccountHandler extends PostHandler {
@Override
public void handlePost(final HttpExchange exchange) throws IOException, SQLiteException {
final Account account = verifySession(exchange, false, null);
final Account account = verifySession(exchange, false);
if (account == null) {
return;
}
Expand Down Expand Up @@ -951,7 +951,7 @@ private static void writeQrCode(final byte[] contents, final OutputStream output
private static class AccountQrHandler extends PostHandler {
@Override
public void handlePost(final HttpExchange exchange) throws IOException, SQLiteException {
final Account account = verifySession(exchange, false, null);
final Account account = verifySession(exchange, false);
if (account == null) {
return;
}
Expand All @@ -973,10 +973,8 @@ public void handlePost(final HttpExchange exchange) throws IOException, SQLiteEx
final int verifyInterval;
final int alertDelay;
final String email;
final String requestToken;
try (final JsonReader reader = Json.createReader(exchange.getRequestBody())) {
final JsonObject object = reader.readObject();
requestToken = object.getString("requestToken");
verifyInterval = object.getInt("verifyInterval");
alertDelay = object.getInt("alertDelay");
email = object.getString("email").trim();
Expand All @@ -986,7 +984,7 @@ public void handlePost(final HttpExchange exchange) throws IOException, SQLiteEx
return;
}

final Account account = verifySession(exchange, false, requestToken.getBytes(StandardCharsets.UTF_8));
final Account account = verifySession(exchange, false);
if (account == null) {
return;
}
Expand Down Expand Up @@ -1056,19 +1054,17 @@ public void handlePost(final HttpExchange exchange) throws IOException, SQLiteEx
private static class DeleteDeviceHandler extends PostHandler {
@Override
public void handlePost(final HttpExchange exchange) throws IOException, SQLiteException {
final String requestToken;
final String fingerprint;
try (final JsonReader reader = Json.createReader(exchange.getRequestBody())) {
final JsonObject object = reader.readObject();
requestToken = object.getString("requestToken");
fingerprint = object.getString("fingerprint");
} catch (final ClassCastException | JsonException | NullPointerException e) {
logger.log(Level.INFO, "invalid request", e);
exchange.sendResponseHeaders(400, -1);
return;
}

final Account account = verifySession(exchange, false, requestToken.getBytes(StandardCharsets.UTF_8));
final Account account = verifySession(exchange, false);
if (account == null) {
return;
}
Expand Down Expand Up @@ -1208,7 +1204,7 @@ private static void writeDevicesJson(final HttpExchange exchange, final long use
private static class DevicesHandler extends PostHandler {
@Override
public void handlePost(final HttpExchange exchange) throws IOException, SQLiteException {
final Account account = verifySession(exchange, false, null);
final Account account = verifySession(exchange, false);
if (account == null) {
return;
}
Expand All @@ -1224,8 +1220,7 @@ public void handlePost(final HttpExchange exchange) throws IOException, SQLiteEx
final JsonObject object = reader.readObject();
fingerprint = object.getString("fingerprint");
long offsetId = object.getJsonNumber("offsetId").longValue();
String requestToken = object.getString("requestToken");
final Account account = verifySession(exchange, false, requestToken.getBytes(StandardCharsets.UTF_8));
final Account account = verifySession(exchange, false);
if (account == null) {
return;
}
Expand Down

0 comments on commit 1757993

Please sign in to comment.