CVE-2021-23431: Cross-Site Request Forgery (CSRF) (CWE-352)
value: The package joplin before 2.3.2 are vulnerable to Cross-site Request Forgery (CSRF) due to missing CSRF checks in various forms.
CWE description: The web application does not, or can not, sufficiently verify whether a well-formed, valid, consistent request was intentionally provided by the user who submitted the request.
CVSS v3 base score: 8.8
Published: 2021-08-24T08:15Z
Repository: https://github.com/laurent22/joplin
Language: TypeScript
Files changed: packages/server/src/app.ts, packages/server/src/routes/index/upgrade.ts, packages/server/src/routes/index/users.ts, packages/server/src/utils/csrf.ts, packages/server/src/utils/requestUtils.ts, packages/server/src/utils/routeUtils.ts, packages/server/src/views/index/upgrade.mustache, packages/server/src/views/index/user.mustache
Commit date: 2021-07-24 17:45:30 +0100
Commit message: Server: Added form tokens to prevent CSRF attacks

Diff: diff --git a/packages/server/src/app.ts b/packages/server/src/app.ts
index c68f9c3fa..f1f2c8e21 100644
--- a/packages/server/src/app.ts
+++ b/packages/server/src/app.ts
@@ -133,11 +133,23 @@ async function main() {
 	app.use(async (ctx: AppContext, next: KoaNext) => {
 		try {
 			await next();
 		} catch (error) {
 			ctx.status = error.httpCode || 500;
-			ctx.body = JSON.stringify({ error: error.message });
+
+			// Since this is a low level error, rendering a view might fail too,
+			// so catch this and default to rendering JSON.
+			try {
+				ctx.body = await ctx.joplin.services.mustache.renderView({
+					name: 'error',
+					title: 'Error',
+					path: 'index/error',
+					content: { error },
+				});
+			} catch (anotherError) {
+				ctx.body = { error: anotherError.message };
+			}
 		}
 	});
 
 	// Creates the request-specific "joplin" context property.
 	app.use(async (ctx: AppContext, next: KoaNext) => {
diff --git a/packages/server/src/routes/index/upgrade.ts b/packages/server/src/routes/index/upgrade.ts
index 349b57e30..b373f4ac0 100644
--- a/packages/server/src/routes/index/upgrade.ts
+++ b/packages/server/src/routes/index/upgrade.ts
@@ -8,10 +8,11 @@ import defaultView from '../../utils/defaultView';
 import { stripeConfig, updateSubscriptionType } from '../../utils/stripe';
 import { bodyFields } from '../../utils/requestUtils';
 import { NotificationKey } from '../../models/NotificationModel';
 import { AccountType } from '../../models/UserModel';
 import { ErrorBadRequest } from '../../utils/errors';
+import { createCsrfTag } from '../../utils/csrf';
 
 interface FormFields {
 	upgrade_button: string;
 }
 
@@ -19,11 +20,11 @@ const router: Router = new Router(RouteType.Web);
 
 function upgradeUrl() {
 	return `${config().baseUrl}/upgrade`;
 }
 
-router.get('upgrade', async (_path: SubPath, _ctx: AppContext) => {
+router.get('upgrade', async (_path: SubPath, ctx: AppContext) => {
 	interface PlanRow {
 		basicLabel: string;
 		proLabel: string;
 	}
 
@@ -49,10 +50,11 @@ router.get('upgrade', async (_path: SubPath, _ctx: AppContext) => {
 	view.content = {
 		planRows,
 		basicPrice: plans.basic.price,
 		proPrice: plans.pro.price,
 		postUrl: upgradeUrl(),
+		csrfTag: await createCsrfTag(ctx),
 	};
 	view.cssFiles = ['index/upgrade'];
 	return view;
 });
 
diff --git a/packages/server/src/routes/index/users.ts b/packages/server/src/routes/index/users.ts
index 731ac8679..4d0a830ea 100644
--- a/packages/server/src/routes/index/users.ts
+++ b/packages/server/src/routes/index/users.ts
@@ -15,10 +15,11 @@ import uuidgen from '../../utils/uuidgen';
 import { formatMaxItemSize, formatMaxTotalSize, formatTotalSize, formatTotalSizePercent, yesOrNo } from '../../utils/strings';
 import { getCanShareFolder, totalSizeClass } from '../../models/utils/user';
 import { yesNoDefaultOptions } from '../../utils/views/select';
 import { confirmUrl } from '../../utils/urlUtils';
 import { cancelSubscription, updateSubscriptionType } from '../../utils/stripe';
+import { createCsrfTag } from '../../utils/csrf';
 
 export interface CheckRepeatPasswordInput {
 	password: string;
 	password2: string;
 }
@@ -144,10 +145,11 @@ router.get('users/:id', async (path: SubPath, ctx: AppContext, user: User = null
 	view.content.isNew = isNew;
 	view.content.buttonTitle = isNew ? 'Create user' : 'Update profile';
 	view.content.error = error;
 	view.content.postUrl = postUrl;
 	view.content.showDisableButton = !isNew && !!owner.is_admin && owner.id !== user.id && user.enabled;
+	view.content.csrfTag = await createCsrfTag(ctx);
 
 	if (subscription) {
 		view.content.subscription = subscription;
 		view.content.showCancelSubscription = !isNew;
 		view.content.showUpdateSubscriptionBasic = !isNew && !!owner.is_admin && user.account_type !== AccountType.Basic;
diff --git a/packages/server/src/utils/csrf.ts b/packages/server/src/utils/csrf.ts
new file mode 100644
index 000000000..26be358b3
--- /dev/null
+++ b/packages/server/src/utils/csrf.ts
@@ -0,0 +1,37 @@
+import { ErrorForbidden } from './errors';
+import { escapeHtml } from './htmlUtils';
+import { bodyFields, isApiRequest } from './requestUtils';
+import { AppContext } from './types';
+
+interface BodyWithCsrfToken {
+	_csrf: string;
+}
+
+export async function csrfCheck(ctx: AppContext, isPublicRoute: boolean) {
+	if (isApiRequest(ctx)) return;
+	if (isPublicRoute) return;
+	if (!['POST', 'PUT'].includes(ctx.method)) return;
+	if (ctx.path === '/logout') return;
+
+	const userId = ctx.joplin.owner ? ctx.joplin.owner.id : '';
+	if (!userId) return;
+
+	const fields = await bodyFields<BodyWithCsrfToken>(ctx.req);
+	if (!fields._csrf) throw new ErrorForbidden('CSRF token is missing');
+
+	if (!(await ctx.joplin.models.token().isValid(userId, fields._csrf))) {
+		throw new ErrorForbidden(`Invalid CSRF token: ${fields._csrf}`);
+	}
+
+	await ctx.joplin.models.token().deleteByValue(userId, fields._csrf);
+}
+
+export async function createCsrfToken(ctx: AppContext) {
+	if (!ctx.joplin.owner) throw new Error('Cannot create CSRF token without a user');
+	return ctx.joplin.models.token().generate(ctx.joplin.owner.id);
+}
+
+export async function createCsrfTag(ctx: AppContext) {
+	const token = await createCsrfToken(ctx);
+	return `<input type="hidden" name="_csrf" value="${escapeHtml(token)}"/>`;
+}
diff --git a/packages/server/src/utils/requestUtils.ts b/packages/server/src/utils/requestUtils.ts
index 5c5e86aa7..99ffbfea4 100644
--- a/packages/server/src/utils/requestUtils.ts
+++ b/packages/server/src/utils/requestUtils.ts
@@ -20,21 +20,29 @@ export async function formParse(req: any): Promise<FormParseResult> {
 		if (req.files) output.files = req.files;
 		output.fields = req.body || {};
 		return output;
 	}
 
+	if (req.__parsed) return req.__parsed;
+
 	// Note that for Formidable to work, the content-type must be set in the
 	// headers
 	return new Promise((resolve: Function, reject: Function) => {
 		const form = formidable({ multiples: true });
 		form.parse(req, (error: any, fields: any, files: any) => {
 			if (error) {
 				reject(error);
 				return;
 			}
 
-			resolve({ fields, files });
+			// Formidable seems to be doing some black magic and once a request
+			// has been parsed it cannot be parsed again. Doing so will do
+			// nothing, the code will just end there, or maybe wait
+			// indefinitely. So we cache the result on success and return it if
+			// some code somewhere tries again to parse the form.
+			req.__parsed = { fields, files };
+			resolve(req.__parsed);
 		});
 	});
 }
 
 export async function bodyFields<T>(req: any/* , filter:string[] = null*/): Promise<T> {
diff --git a/packages/server/src/utils/routeUtils.ts b/packages/server/src/utils/routeUtils.ts
index 1170c099f..3539ddb8b 100644
--- a/packages/server/src/utils/routeUtils.ts
+++ b/packages/server/src/utils/routeUtils.ts
@@ -2,10 +2,11 @@ import { baseUrl } from '../config';
 import { Item, ItemAddressingType, Uuid } from '../db';
 import { ErrorBadRequest, ErrorForbidden, ErrorNotFound } from './errors';
 import Router from './Router';
 import { AppContext, HttpMethod, RouteType } from './types';
 import { URL } from 'url';
+import { csrfCheck } from './csrf';
 
 const { ltrimSlashes, rtrimSlashes } = require('@joplin/lib/path-utils');
 
 function dirname(path: string): string {
 	if (!path) throw new Error('Path is empty');
@@ -186,14 +187,18 @@ export async function execRequest(routes: Routers, ctx: AppContext) {
 	if (!match) throw new ErrorNotFound();
 
 	const endPoint = match.route.findEndPoint(ctx.request.method as HttpMethod, match.subPath.schema);
 	if (ctx.URL && !isValidOrigin(ctx.URL.origin, baseUrl(endPoint.type), endPoint.type)) throw new ErrorNotFound(`Invalid origin: ${ctx.URL.origin}`, 'invalidOrigin');
 
+	const isPublicRoute = match.route.isPublic(match.subPath.schema);
+
 	// This is a generic catch-all for all private end points - if we
 	// couldn't get a valid session, we exit now. Individual end points
 	// might have additional permission checks depending on the action.
-	if (!match.route.isPublic(match.subPath.schema) && !ctx.joplin.owner) throw new ErrorForbidden();
+	if (!isPublicRoute && !ctx.joplin.owner) throw new ErrorForbidden();
+
+	await csrfCheck(ctx, isPublicRoute);
 
 	return endPoint.handler(match.subPath, ctx);
 }
 
 // In a path such as "/api/files/SOME_ID/content" we want to find:
diff --git a/packages/server/src/views/index/upgrade.mustache b/packages/server/src/views/index/upgrade.mustache
index 0f59759c7..37b75e7fd 100644
--- a/packages/server/src/views/index/upgrade.mustache
+++ b/packages/server/src/views/index/upgrade.mustache
@@ -1,9 +1,10 @@
 <h1 class="title">Upgrade your account</h1>
-<p class="subtitle">Upgrading to a Pro account to get the following benefits.</p>
+<p class="subtitle">Upgrade to a Pro account to get the following benefits.</p>
 
 <form id="upgrade_form" action="{{{postUrl}}}" method="POST">
+	{{{csrfTag}}}
 	<table class="table is-hoverable user-props-table">
 		<tbody>
 			<tr>
 				<th class="basic-plan">Basic - {{basicPrice}}</th>
 				<th>Pro - {{proPrice}}</th>
diff --git a/packages/server/src/views/index/user.mustache b/packages/server/src/views/index/user.mustache
index 3149ff3f5..cc3ac2f2b 100644
--- a/packages/server/src/views/index/user.mustache
+++ b/packages/server/src/views/index/user.mustache
@@ -2,10 +2,11 @@
 
 <form id="user_form" action="{{{postUrl}}}" method="POST">
 
 	<div class="block">
 		{{> errorBanner}}
+		{{{csrfTag}}}
 		<input type="hidden" name="id" value="{{user.id}}"/>
 		<input type="hidden" name="is_new" value="{{isNew}}"/>
 		<div class="field">
 			<label class="label">Full name</label>
 			<div class="control">
@@ -92,15 +93,15 @@
 				<input type="submit" name="restore_button" class="button is-danger" value="Restore" />
 			{{/showRestoreButton}}
 		</div>
 	</div>
 
-	<h1 class="title">Your subscription</h1>
+	{{#subscription}}
+		<h1 class="title">Your subscription</h1>
 
-	<div class="block">
-		{{#global.owner.is_admin}}
-			{{#subscription}}
+		<div class="block">
+			{{#global.owner.is_admin}}
 				<div class="control block">
 					<p class="block">Stripe Subscription ID: <a href="https://dashboard.stripe.com/subscriptions/{{subscription.stripe_subscription_id}}">{{subscription.stripe_subscription_id}}</a></p>
 					{{#showUpdateSubscriptionBasic}}
 						<input type="submit" name="update_subscription_basic_button" class="button is-warning" value="Downgrade to Basic" />
 					{{/showUpdateSubscriptionBasic}}
@@ -109,27 +110,25 @@
 					{{/showUpdateSubscriptionPro}}
 					{{#showCancelSubscription}}
 						<input type="submit" name="cancel_subscription_button" class="button is-danger" value="Cancel subscription" />
 					{{/showCancelSubscription}}
 				</div>
-			{{/subscription}}
-		{{/global.owner.is_admin}}
+			{{/global.owner.is_admin}}
 
-		{{^global.owner.is_admin}}
-			{{#subscription}}
+			{{^global.owner.is_admin}}
 				<div class="control block">
 					{{#showUpdateSubscriptionPro}}
 						<a href="{{{global.baseUrl}}}/upgrade" class="button is-warning block">Upgrade to Pro</a>
 					{{/showUpdateSubscriptionPro}}
 					{{#showCancelSubscription}}
 						<p id="user_cancel_subscription_link" class="block"><a href="#">Cancel subscription</a></p>
 						<input type="submit" id="user_cancel_subscription_button" name="user_cancel_subscription_button" class="button is-danger" value="Cancel subscription" />
 					{{/showCancelSubscription}}
 				</div>
-			{{/subscription}}
-		{{/global.owner.is_admin}}
-	</div>
+			{{/global.owner.is_admin}}
+		</div>
+	{{/subscription}}
 
 </form>
 
 <script>
 	$(() => {

Vulnerable code: 			ctx.body = JSON.stringify({ error: error.message });
router.get('upgrade', async (_path: SubPath, _ctx: AppContext) => {
			resolve({ fields, files });
	if (!match.route.isPublic(match.subPath.schema) && !ctx.joplin.owner) throw new ErrorForbidden();
<p class="subtitle">Upgrading to a Pro account to get the following benefits.</p>
	<h1 class="title">Your subscription</h1>
	<div class="block">
		{{#global.owner.is_admin}}
			{{#subscription}}
			{{/subscription}}
		{{/global.owner.is_admin}}
		{{^global.owner.is_admin}}
			{{#subscription}}
			{{/subscription}}
		{{/global.owner.is_admin}}
	</div>
Fixed code: 
			// Since this is a low level error, rendering a view might fail too,
			// so catch this and default to rendering JSON.
			try {
				ctx.body = await ctx.joplin.services.mustache.renderView({
					name: 'error',
					title: 'Error',
					path: 'index/error',
					content: { error },
				});
			} catch (anotherError) {
				ctx.body = { error: anotherError.message };
			}
import { createCsrfTag } from '../../utils/csrf';
router.get('upgrade', async (_path: SubPath, ctx: AppContext) => {
		csrfTag: await createCsrfTag(ctx),
import { createCsrfTag } from '../../utils/csrf';
	view.content.csrfTag = await createCsrfTag(ctx);
import { ErrorForbidden } from './errors';
import { escapeHtml } from './htmlUtils';
import { bodyFields, isApiRequest } from './requestUtils';
import { AppContext } from './types';

interface BodyWithCsrfToken {
	_csrf: string;
}

export async function csrfCheck(ctx: AppContext, isPublicRoute: boolean) {
	if (isApiRequest(ctx)) return;
	if (isPublicRoute) return;
	if (!['POST', 'PUT'].includes(ctx.method)) return;
	if (ctx.path === '/logout') return;

	const userId = ctx.joplin.owner ? ctx.joplin.owner.id : '';
	if (!userId) return;

	const fields = await bodyFields<BodyWithCsrfToken>(ctx.req);
	if (!fields._csrf) throw new ErrorForbidden('CSRF token is missing');

	if (!(await ctx.joplin.models.token().isValid(userId, fields._csrf))) {
		throw new ErrorForbidden(`Invalid CSRF token: ${fields._csrf}`);
	}

	await ctx.joplin.models.token().deleteByValue(userId, fields._csrf);
}

export async function createCsrfToken(ctx: AppContext) {
	if (!ctx.joplin.owner) throw new Error('Cannot create CSRF token without a user');
	return ctx.joplin.models.token().generate(ctx.joplin.owner.id);
}

export async function createCsrfTag(ctx: AppContext) {
	const token = await createCsrfToken(ctx);
	return `<input type="hidden" name="_csrf" value="${escapeHtml(token)}"/>`;
}
	if (req.__parsed) return req.__parsed;

			// Formidable seems to be doing some black magic and once a request
			// has been parsed it cannot be parsed again. Doing so will do
			// nothing, the code will just end there, or maybe wait
			// indefinitely. So we cache the result on success and return it if
			// some code somewhere tries again to parse the form.
			req.__parsed = { fields, files };
			resolve(req.__parsed);
import { csrfCheck } from './csrf';
	const isPublicRoute = match.route.isPublic(match.subPath.schema);

	if (!isPublicRoute && !ctx.joplin.owner) throw new ErrorForbidden();

	await csrfCheck(ctx, isPublicRoute);
<p class="subtitle">Upgrade to a Pro account to get the following benefits.</p>
	{{{csrfTag}}}
		{{{csrfTag}}}
	{{#subscription}}
		<h1 class="title">Your subscription</h1>
		<div class="block">
			{{#global.owner.is_admin}}
			{{/global.owner.is_admin}}
			{{^global.owner.is_admin}}
			{{/global.owner.is_admin}}
		</div>
	{{/subscription}}