Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some sort of CSRF protection #72

Closed
Rich-Harris opened this issue Oct 27, 2020 · 16 comments · Fixed by #6510
Closed

Some sort of CSRF protection #72

Rich-Harris opened this issue Oct 27, 2020 · 16 comments · Fixed by #6510
Labels
breaking change feature request New feature or request p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc. security
Milestone

Comments

@Rich-Harris
Copy link
Member

(draw the rest of the owl)

@Rich-Harris Rich-Harris added this to the 1.0 milestone Oct 29, 2020
@halfnelson
Copy link
Contributor

Listening to your Svelte Summit (spring 2021) post talk discussion about the "enhance" form support and its integration with SvelteKit. Maybe that same integration is what can provide the CSRF support (considering since a non JS post to a form is where CSRF is most needed)

@nbgoodall
Copy link

@Rich-Harris I soon need to draw this owl for an app nearing production, and why duplicate work?

Do you have any thoughts on this or how you'd like to see it implemented? :)

@benmccann benmccann added feature request New feature or request security labels Jul 15, 2021
@nakleiderer
Copy link

nakleiderer commented Jul 24, 2021

I think, since svelte kit pairs nicely with FaaS, it would likely be best to avoid server side state. The Double Submit Cookie technique seems like a reasonable choice. https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#double-submit-cookie

I believe it could be implemented as a POC without modifying svelte kit code. There's a few strategies that could be considered, but I'd consider using the JWT standard to be the simplest.

Proposal using JWT:

  • Server generates anti-csrf token in the form of a JWT (hs256, maybe?) to generate anti-csrf token with expiration time, using server-side secret.
  • Token is given to client in either session, or cookie, or a custom hidden input field injected into each form.
    • Cookie or hidden form input would be best for no-javascript executions
  • Server uses middleware to validate token, rejecting with 403 if it is missing or invalid.
  • Server executes request and generates a new CSRF token.

@benmccann benmccann added the p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc. label Aug 4, 2021
@mlusetti
Copy link

I think, since svelte kit pairs nicely with FaaS, it would likely be best to avoid server side state. The Double Submit Cookie technique seems like a reasonable choice. https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#double-submit-cookie

I agree. A cookie with a SameSite: strict and HttpOnly (Secure too obviously) should do the trick

@Karlinator
Copy link
Contributor

@nbgoodall I'm also needing to draw this owl about now. Did you get anywhere with this?

My first idea was to set the double submit cookie and inject the form field/value though the handle function (now nicely chainable with sequence). The cookie worked, but the input value was of course overwritten by Svelte during hydration. I don't yet have an idea for injecting the token into the form (so it will also appear in the POST request).

I'm trying right now to do this purely as a handler middleware without changing any code in SvelteKit itself.

@Karlinator
Copy link
Contributor

So I've arrived at (I think) a working solution using only a handler.

It's essentially a signed double submit cookie with the added security of submitting it through a custom header (which means the same-origin policy comes to our defence as well).

The handler generates a JWT token with an expiry in a few minutes, then it adds a Set-Cookie header and injects a meta tag with the token in the document head.

On incoming requests, the handler reads the x-csrf-token custom header, compares it to the cookie, and verifies the signature. If everything is good it sets requests.locals.csrfVerified = true.

What I haven't decided is how to deal with getting new tokens. As it stands, tokens are only changed when the user reloads the page (and triggers SSR). The easy option is of course to just disable the router where necessary. That said, I don't think there's a way to disable the router for all navigation to a page, only from a page.

Alternately, you can just accept that the token is the same for however long the user remains on the page without refreshing.

@nbgoodall
Copy link

@Karlinator how are you adding the x-csrf-token header? In the fetch requests themselves?

My biggest motivation for this was making forms secure without JavaScript. It is a double submit cookie, storing an encrypted value in the cookie session and exposing a masked version in forms (similar to how Rails does it to protect against SSL breach attacks). I was already using an encrypted cookie to store arbitrary JSON, so the token lives in there.

On each request:

  1. If there's not a CSRF token in the encrypted cookie session, generate and store a new one.
  2. Mask the token using a one-time pad and add it to Svelte's $session.
  3. Add the token in a hidden input field to all non-GET forms (I've created a custom Form element). This works for me, but asking users to do something like this seems a bit much...
  4. On form submission, unmask the token and compare it to the encrypted one. If they don't match, throw an error.

The session is encrypted using AES as I needed to decrypt the contents, but HMAC would be slightly faster if all you need is to verify the token...

What I haven't decided is how to deal with getting new tokens. As it stands, tokens are only changed when the user reloads the page (and triggers SSR).

I'm not sure this is an issue; it's common for a token to last a whole session, and realistically how long is someone going to be on a page for?

@Karlinator
Copy link
Contributor

@nbgoodall

I'm adding it in the fetch requests, yes. We ended up not supporting non-JS anyway, so at that point there was no reason not to.

I tried to limit myself as far as I could to only doing stuff in the handle function, as I didn't really want to create custom components. That said, I did have to manually pass the token into the fetch calls so I only partly succeeded. That is the only thing that happens outside the handle hook though, which I am pretty happy with.

But I had to trade off non-JS function entirely so it's definitely not a universal solution. I think creating (or importing, if this were a package) a custom form component is probably reasonable given the alternatives right now? Otherwise we'd have to modify Svelte itself probably (and in the process "lie" about what the DOM will look like). I could definitely see a CSRFForm as a component with a slot (although that double F is killing me).

What I haven't decided is how to deal with getting new tokens. As it stands, tokens are only changed when the user reloads the page (and triggers SSR).

I'm not sure this is an issue; it's common for a token to last a whole session, and realistically how long is someone going to be on a page for?

You're probably right. Depending on who you ask (there are some long threads on this on StackOverflow) I'd probably be fine with just checking for the existence of the header and relying on the same-origin policy. It's generated anew every time it's sent out, after all, so it shouldn't really be a problem. I'm probably being a bit paranoid; I have more than enough layers of defence in depth already not to worry about the token living through some client-side routing.

Thanks for outlining your solution though!

@nakleiderer
Copy link

nakleiderer commented Sep 4, 2021

Here's more details on the solution I ended up using. It supports JS and non-JS forms and operated in a serverless environment, so seems to be perfectly aligned with svelte-kit's principles. I'm only using this token for CSRF protection and not to store any session info.

Generate & Hyrdate

To add CSRF to a non-js page, you can hydrate the tokens in the handle hook. You'll need to set your cookie value and inject values into the HTML, like so:

import * as cheerio from "cheerio";

export async function hydrateCsrfProtection(
  response: ServerResponse,
  csrfToken: string
): Promise<ServerResponse> {
  if (response.headers["content-type"] !== "text/html") {
    return response;
  }

  // Add csrf information to meta tags
  const $ = cheerio.load(response.body.toString());
  $("head").append(
    `<meta name="csrf-param" content="${clientConfig.csrfParam}" />`,
    `<meta name="csrf-token" content="${csrfToken}" />`
  );

  /// Add csrf information to forms in case javascript is disabled or not loaded
  $("body")
    .find("form")
    .each(function () {
      $(this).append(
        `<input type="hidden" name="csrf-token" content="${csrfToken}" />`
      );
    });

  return {
    ...response,
    body: $.html(),
  };
}

Enhance Forms

To solve the cookie going away with hydration, you can add a use directive to "enhance" the form. For example:

const csrfTokenMeta = (): HTMLMetaElement =>
  document.querySelector('meta[name="csrf-token"]');
const csrfParamMeta = (): HTMLMetaElement =>
  document.querySelector('meta[name="csrf-param"]');
export const csrfToken = (): string => csrfTokenMeta().content;
export const csrfParam = (): string => csrfParamMeta().content;

// this action (https://svelte.dev/tutorial/actions) allows us to
// progressively enhance a <form> that already works without JS

export function enhance(form: HTMLFormElement): void {
  if (browser) {
    const csrfParamInput = document.createElement("input");
    csrfParamInput.setAttribute("type", "hidden");
    csrfParamInput.setAttribute("name", csrfParam());
    csrfParamInput.setAttribute("value", csrfToken());
    form.append(csrfParamInput);
  }
}

Wrap fetch for client-side only posts

You can support client-side fetch like this:

export const clientFetch: typeof fetch = (
  input: RequestInfo,
  init?: RequestInit
) => {
  const initHeader =
    typeof input === "string" ? init?.headers : init.headers ?? input.headers;
  const headers = new Headers(initHeader);
  headers.set("Content-Type", "application/json");
  headers.set("x-csrftoken, csrfToken());

  if (typeof input === "string") {
    return fetch(input, { ...init, headers });
  }

  return fetch({ ...input, headers }, { ...init, headers });
};

Ensure the tokens in the header/form and cookie match

To get the token from the form or the header, you can use another handle hook modification

const extractCsrfToken = (request: ServerRequest) => {
  if (request.body === undefined) {
    return "";
  }

  return typeof request.body !== "string" && "get" in request.body
    ? request.body.get(clientConfig.csrfParam)
    : request.headers[clientConfig.csrfHeader];
};

You'll need to make sure the header/form value matches the cookie. To get the cookie:

  const cookies = cookie.parse(headers.cookie ?? "");
  const cookieCsrfToken = cookies.csrf_token;

Your matching algorithm will differ depending on how you implement your tokens. A good start would be a HS256 JWT for both tokens. Use the same sub value for both tokens with a different cyptographically-random jti. In addition, set an eat (15 minutes is probably a good start). Validate both JWTs to make sure they haven't been tampered with or expired. Then, make sure the sub value in each token matches.

@Prinzhorn
Copy link

Prinzhorn commented Sep 25, 2021

You don't need to add a CSRF token to a fetch request, when SOP/CORS already solve that (Origin header). An attacker cannot forge a request via fetch without CORS being configured, and then you'd additional need Access-Control-Allow-Credentials for it to work and a CORS misconfiguration that allows any (evil) host. The only reason a CSRF token for fetch is needed in some frameworks is because it already has a CSRF token for classic forms and then fetch requests need to be made compatible to that because the server doesn't know. If you do not use <form> and only fetch, then the Origin header solves CSRF. There is no need to use a token. Turns out all modern browsers also set Origin for <form> POSTs. That means if you only use fetch but an attacker uses <form> on a different origin then Origin header does not match. If it is missing (IE < 11 + some edge cases, Firefox for Android) you can safely discard the request, since you only use fetch, which always includes Origin for non-GET/HEAD. No false-positive in that case.

My suggestion for SvelteKit:

  1. Do not implement any form of CSRF token
  2. Strictly check Origin header on every request that is not GET or HEAD against a configured host (this is important, I recommend configuring a strict host and not rely on any X- headers etc.). If Origin is missing, fall back to Referer. If both are missing or do not match, reject the request.
  3. Make sure Method overrides #1046 is applied before the CSRF logic and only for POST, or else you can bypass the whole thing
  4. Serve cookies with SameSite=strict as additional measurement

To my knowledge there is no way to bypass this and this is the recommended approach moving forward. It is also recommended by OWASP. There is one downside: this can lead to false positives (rejecting valid requests) under very specific circumstances:

  1. If a user is using IE < 11, IE 11 in some cases, Firefox for Android or any other outdated browser that does not set Origin for <form>
  2. This user also has Referer disabled via some privacy settings
  3. This user also does not have JavaScript enabled or uses a browser that doesn't support fetch
  4. Then this user will not be able to submit your forms. I think this is acceptable.

Sources:

@Prinzhorn
Copy link

Prinzhorn commented Sep 26, 2021

I want to point out that OWASP (in the source I linked above) only recommends this in addition to the other approaches under the "Defense In Depth Techniques" section. Because of the following downsides (I didn't list all of them above):

This mitigation is working properly when origin or referrer headers are present in the requests. Though these headers are included majority of the time, there are few use cases where they are not included (most of them are for legitimate reasons to safeguard users privacy/to tune to browsers ecosystem). The following lists some use cases:

  • Internet Explorer 11 does not add the Origin header on a CORS request across sites of a trusted zone. The Referer header will remain the only indication of the UI origin. See the following references in Stack Overflow here and here.
  • In an instance following a 302 redirect cross-origin, Origin is not included in the redirected request because that may be considered sensitive information that should not be sent to the other origin.
  • There are some privacy contexts where Origin is set to "null" For example, see the following here.
  • Origin header is included for all cross origin requests but for same origin requests, in most browsers it is only included in POST/DELETE/PUT Note: Although it is not ideal, many developers use GET requests to do state changing operations.
  • Referer header is no exception. There are multiple use cases where referrer header is omitted as well (1, 2, 3, 4 and 5). Load balancers, proxies and embedded network devices are also well known to strip the referrer header due to privacy reasons in logging them.

However, some of these points don't really apply (e.g. if you are using GET to make mutating requests that's a bug in your app). It's up to the maintainers to decide if this is something that they want in the core. I personally think the benefits outweigh the downsides by a lot and I will take this approach, it's trivial to implement. I don't think a token based approach can be added to SvelteKit in a way that works for all use cases and adapters. There are too many edge cases since SvelteKit doesn't follow the classic request/response pattern to serve pages and swapping out tokens will become a pain (will end up in false positives as well because of edge cases and timed out tokens and whatnot, see https://github.com/xing/cross-application-csrf-prevention and read through the issues and confusion on https://github.com/j0lv3r4/next-csrf/issues).

I also want to point out that if your Svelte app uses a non-cookie approach to sessions (e.g. JWT in localStorage) CSRF does not exist. CSRF only exists because of cookies.

@benmccann benmccann added help wanted PRs welcomed. The implementation details are unlikely to cause debate and removed help wanted help wanted PRs welcomed. The implementation details are unlikely to cause debate labels Apr 4, 2022
@benmccann benmccann added the size:large significant feature with tricky design questions and multi-day implementation label Aug 1, 2022
@benmccann
Copy link
Member

benmccann commented Aug 3, 2022

It looks like Firefox for Android has likely added support for the Origin header, so the suggestion above seems pretty safe:

@benmccann
Copy link
Member

I notice that Play Framework uses a token and checks under the following conditions:

  • The request method is not GET, HEAD or OPTIONS.
  • The request has one or more Cookie or Authorization headers.
  • The CORS filter is not configured to trust the request’s origin.

And it has a warning about needing different settings when using NTLM or client certificate based authentication.

Rails and Laravel use tokens as well. Next.js apparently leaves it to the user 😝

Some additional thoughts about the configuration:

  • You might not want to bake the origin into the application at build time because you might deploy the site to different domains (e.g. a staging and production version of your site). I.e. maybe it should live in $env/dynamic/public? (should we rename our existing environment variables to fit the new naming scheme?)
  • Perhaps it should be on by default for good security? But that would be a breaking change as it would break sites that don't configure the origin. Maybe you'd also want a way to disable it? (E.g. if you're storing tokens in local storage instead of cookies.)

@benmccann benmccann removed the size:large significant feature with tricky design questions and multi-day implementation label Aug 9, 2022
@benmccann
Copy link
Member

It's possible we could just add this to the templates rather than the framework so that users have full control over the logic being used. E.g. something like this in handle:

if (request.headers.get('origin') !== url.origin) {
  return new Response('nice try, bad guy', { status: 403 });
}

@bato3
Copy link

bato3 commented Aug 13, 2022

I don't remember where I found this implementation, but I like it very much.

There are 2 values in the form: "token number" and the token itself. Each time, the user gets a new pair. It may have two tabs open and the submission of the forms will be correct.

I don't like the solution: <input type="hidden" name="csrf-token" content="${csrfToken}" /> because the first submission of the form will consume a token, and the next submission will report an error.

I imagine the use in SK as:

<Form csrf>

and:

/** @type {import('./$types').FormAction} */
export function createTodo({ request }) {
  if (!request.csrf.validate()) {
    return {
      errors: {
        csrf: 'Oh no!.. You failed'
      }
    };
  }
// another form validations
request.csrf.consume(); //Consider when to refresh the token: How the next message will behave, how errors occurred in the form / database.
  // ...
}

BTW: With the server, I can easily fake the header origin. :-P

@bato3 bato3 mentioned this issue Aug 13, 2022
@PatrickG
Copy link
Contributor

PatrickG commented Aug 13, 2022

BTW: With the server, I can easily fake the header origin. :-P

With a server, I can easily scrape the csrf-token from the html source and send it along.
The purpose of a csrf token is to protect your users which are visiting your site with a browser.

BTT
I think checking the origin header is enough.
Even Same-Site cookies should be enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change feature request New feature or request p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc. security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants