Repository: CTFd/CTFd
Pull request #2379 (2023-07-31 17:39:20)

fix: Accept multipart/form-data with token auth
## Problem

Uploading files by web UI invokes `post_files_list` API with "multipart/form-data" content-type.
But that content-type is not accepted when users are authenticated with token. As a result, scripting such operations is hard.

## Changes

* In validation of token authentication, accept "multipart/form-data" as well as "application/json"
* Use `flask.Request.mimetype` instead of `flask.Request.content_type` in that validation because "multipart/form-data" usually has "boundary" parameter which does not affect to the validation

ColdHeat: This feels like it introduces something incorrect and even possibly a security issue if it's applied to every endpoint. 

What is wrong with the existing code in ctfcli? 
https://github.com/CTFd/ctfcli/blob/7b4a09af8414eb1f5f6da9a8422fb53b5e9cbc15/ctfcli/utils/challenge.py#L278
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/CTFd/CTFd/pull/2379?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CTFd) Report
> Merging [#2379](https://app.codecov.io/gh/CTFd/CTFd/pull/2379?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CTFd) (27991af) into [master](https://app.codecov.io/gh/CTFd/CTFd/commit/b89cb3cb98cde8e6c82e0c01d3d2bf5bfcf4ee38?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CTFd) (b89cb3c) will **decrease** coverage by `2.77%`.
> Report is 56 commits behind head on master.
> The diff coverage is `75.70%`.

> :exclamation: Current head 27991af differs from pull request most recent head 646c691. Consider uploading reports for the commit 646c691 to get more accurate results

```diff
@@            Coverage Diff             @@
##           master    #2379      +/-   ##
==========================================
- Coverage   87.56%   84.79%   -2.77%     
==========================================
  Files         146      147       +1     
  Lines        8899     9043     +144     
==========================================
- Hits         7792     7668     -124     
- Misses       1107     1375     +268     
```


| [Files Changed](https://app.codecov.io/gh/CTFd/CTFd/pull/2379?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CTFd) | Coverage Δ | |
|---|---|---|
| [CTFd/admin/\_\_init\_\_.py](https://app.codecov.io/gh/CTFd/CTFd/pull/2379?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CTFd#diff-Q1RGZC9hZG1pbi9fX2luaXRfXy5weQ==) | `74.84% <0.00%> (ø)` | |
| [CTFd/api/v1/statistics/challenges.py](https://app.codecov.io/gh/CTFd/CTFd/pull/2379?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CTFd#diff-Q1RGZC9hcGkvdjEvc3RhdGlzdGljcy9jaGFsbGVuZ2VzLnB5) | `35.84% <ø> (ø)` | |
| [CTFd/api/v1/topics.py](https://app.codecov.io/gh/CTFd/CTFd/pull/2379?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CTFd#diff-Q1RGZC9hcGkvdjEvdG9waWNzLnB5) | `88.29% <ø> (ø)` | |
| [CTFd/config.py](https://app.codecov.io/gh/CTFd/CTFd/pull/2379?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CTFd#diff-Q1RGZC9jb25maWcucHk=) | `77.14% <0.00%> (-1.12%)` | :arrow_down: |
| [CTFd/errors.py](https://app.codecov.io/gh/CTFd/CTFd/pull/2379?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CTFd#diff-Q1RGZC9lcnJvcnMucHk=) | `90.00% <ø> (ø)` | |
| [CTFd/forms/setup.py](https://app.codecov.io/gh/CTFd/CTFd/pull/2379?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CTFd#diff-Q1RGZC9mb3Jtcy9zZXR1cC5weQ==) | `100.00% <ø> (ø)` | |
| [CTFd/schemas/challenges.py](https://app.codecov.io/gh/CTFd/CTFd/pull/2379?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CTFd#diff-Q1RGZC9zY2hlbWFzL2NoYWxsZW5nZXMucHk=) | `95.83% <ø> (ø)` | |
| [CTFd/schemas/pages.py](https://app.codecov.io/gh/CTFd/CTFd/pull/2379?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CTFd#diff-Q1RGZC9zY2hlbWFzL3BhZ2VzLnB5) | `83.33% <ø> (ø)` | |
| [CTFd/schemas/tokens.py](https://app.codecov.io/gh/CTFd/CTFd/pull/2379?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CTFd#diff-Q1RGZC9zY2hlbWFzL3Rva2Vucy5weQ==) | `100.00% <ø> (ø)` | |
| [CTFd/utils/challenges/\_\_init\_\_.py](https://app.codecov.io/gh/CTFd/CTFd/pull/2379?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CTFd#diff-Q1RGZC91dGlscy9jaGFsbGVuZ2VzL19faW5pdF9fLnB5) | `100.00% <ø> (ø)` | |
| ... and [25 more](https://app.codecov.io/gh/CTFd/CTFd/pull/2379?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CTFd) | |

... and [7 files with indirect coverage changes](https://app.codecov.io/gh/CTFd/CTFd/pull/2379/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CTFd)

:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CTFd)

ntoofu: Thank you for your review.

> What is wrong with the existing code in ctfcli?

I tried ctfcli and it succeeded to upload files, but by capturing requests I found cookies are sent to API endpoints. I think ctfcli is authenticated not by tokens but by session key in cookies, when it POST to `/api/v1/files` .

Is that an intended? IMHO, most API is authenticated only by tokens and session cookie is not required.
ntoofu: BTW, [this code](https://github.com/CTFd/CTFd/blob/6d74c1c4267b4dc395949a65331c369e0737d16f/CTFd/utils/initialization/__init__.py#L293) might be problematic.

In my experiment with curl, accessing to API endpoint with session cookie has failed because it lacked CSRF-Token. But ctfcli seems to be authenticated without CSRF-Token. I think requests with session cookie and API token can bypass CSRF token validation, regardless of the API token's validity.
ColdHeat: ctfcli never receives a cookie in its configuration. You only specify an API key. Can you provide some evidence that ctfcli is communicating using a cookie?

I don't believe via CSRF you can set an arbitrary header directly. To pull the attack off you would need to leak an API key which no attacker would do since the API key grants all the access they need anyway. 

With XSS you can set the Authorization header but at that point you could just leverage the existing session cookie. Leaking the API key would just give the victim more access. 

I'm not really understanding your attack vectors so you would need to provide a POC.


ntoofu: > Can you provide some evidence that ctfcli is communicating using a cookie?

Here is the packet capture. I executed `ctf challenge install` command while running tcpdump.

[attachment.zip](https://github.com/CTFd/CTFd/files/12232124/attachment.zip)

> I'm not really understanding your attack vectors so you would need to provide a POC.

I just suggested the possibility and I can't come up with attack vectors. I will report a PoC if I find it.
ColdHeat: Thank you for the packet capture, it's very useful! I put it up at https://apackets.com/pcaps?pcap=5b6b7989976c6f7a627317f3eafcd619.pcap&view=charts. 

I see that the session is generated which is the source of the issue. I see what you are saying. I will need to investigate this issue and see what the implication is of the change. 
ColdHeat: Thanks for providing that. However I dont know how a CSRF exploit would set the Authorization header? 
ntoofu: Currently, two things prevent an exploit using the bypass of CSRF token validation.

* `SameSite` attribute on session cookie is set to `Lax`
* CTFd does not support CORS

If an attacker provide javascript calling `fetch()` or `XMLHttpRequest()` via malicious site, potentially any request can be crafted and sent from victims. But a request containing Authorization header is not "Simple Request", so most browsers send preflight request and CTFd does not send CORS related headers in response. Additionally, if the preflight check passed in any way, session cookie with SameSite=Lax is not sent and the request fails to be authenticated.

So I think it is not exploitable immediately, but if [the code](https://github.com/CTFd/CTFd/blob/6d74c1c4267b4dc395949a65331c369e0737d16f/CTFd/utils/initialization/__init__.py#L293) is intended to check users are authenticated by API tokens, that intention is not implemented properly.
ColdHeat: @MilyMilo FYI the changes being discussed here will affect ctfcli 
MilyMilo: I've been working on ctfcli recently and I can confirm this. CTFCLI's API Session object is not configured to reject cookies, and CTFd will issue sessions even to requests originally authenticated with the Token - making ctfcli save them, which led to this going unnoticed as everything was working. Making a `multipart/form-data` request to `/api/v1/files` is in fact rejected without the session.

I think the best temporary solution to this would be to check the path inside the tokens() function and only allow `multipart/form-data` on the `/api/v1/files` route like this:

```python3
@app.before_request
def tokens():
    token = request.headers.get("Authorization")
    is_multipart_files_upload = request.path == "/api/v1/files"
    if token and (request.mimetype == "application/json" or
                  request.mimetype == "multipart/form-data" and is_multipart_files_upload):

    # (...)
```

@ColdHeat 
ColdHeat: I think it's better to:

* Check for request.path == "/api/v1/files". Just need to make sure that this doesn't break on subdirectory deployments.
* Only do this check if we have the authorization token and not before we know if it's a token request since this behavior only matters for token requests.

I think with that change we can accept this. 
ntoofu: @ColdHeat Thanks for the comment.

Is there any concern in the code suggested at https://github.com/CTFd/CTFd/pull/2379#issuecomment-1685934823 ?
I think `request.endpoint` is better in the aspect of sub-directory deployments, because the value of `request.endpoint` is invariable even when CTFd is running in a sub-directory.
ColdHeat: I think it's a toss up. I am okay with `request.endpoint`.

The API path is very unlikely to change if ever. I am concerned that one day the API naming convention will change but we can let that slide for the sake of this PR. 
ntoofu: @ColdHeat 
I have updated the code. Since `request.endpoint` is consistent even in a case of sub-directory deployment, I adopted it. And I avoided to use the endpoint name (`files_files_list`) which is more likely to change, by looking up `flask.url_map` with rule string such as `/api/v1/files`.
ColdHeat: @ntoofu thank you for that. I will take over and sheperd this into the codebase. 

Patch:
diff --git a/CTFd/utils/initialization/__init__.py b/CTFd/utils/initialization/__init__.py
index 5f38bf41..14d29383 100644
--- a/CTFd/utils/initialization/__init__.py
+++ b/CTFd/utils/initialization/__init__.py
@@ -282,7 +282,15 @@ def init_request_processors(app):
     @app.before_request
     def tokens():
         token = request.headers.get("Authorization")
-        if token and request.content_type == "application/json":
+        if token and (
+            request.mimetype == "application/json"
+            # Specially allow multipart/form-data for file uploads
+            or (
+                request.endpoint == "api.files_files_list"
+                and request.method == "POST"
+                and request.mimetype == "multipart/form-data"
+            )
+        ):
             try:
                 token_type, token = token.split(" ", 1)
                 user = lookup_user_token(token)


Test patch:
diff --git a/tests/api/test_tokens.py b/tests/api/test_tokens.py
index f5a0907a..92ed8bdc 100644
--- a/tests/api/test_tokens.py
+++ b/tests/api/test_tokens.py
@@ -1,7 +1,9 @@
 import datetime
+import os
+from io import BytesIO
 
 from CTFd.exceptions import UserNotFoundException, UserTokenExpiredException
-from CTFd.models import Tokens
+from CTFd.models import Files, Tokens, Users
 from CTFd.utils.security.auth import generate_user_token, lookup_user_token
 from tests.helpers import create_ctfd, destroy_ctfd, gen_token, gen_user
 
@@ -76,3 +78,28 @@ def test_user_token_access():
             assert resp["data"]["email"] == "user1@examplectf.com"
             assert resp["data"]["name"] == "user1"
     destroy_ctfd(app)
+
+
+def test_token_api_file_upload():
+    """Test that tokens can upload files with multipart/form-data content type"""
+    app = create_ctfd()
+    with app.app_context():
+        admin = Users.query.filter_by(id=1).first()
+        token = generate_user_token(admin, expiration=None)
+        with app.test_client() as client:
+            headers = {"Authorization": "token " + token.value}
+            r = client.post(
+                "/api/v1/files",
+                headers=headers,
+                content_type="multipart/form-data",
+                data={
+                    "file": (BytesIO(b"test file content"), "test.txt"),
+                },
+            )
+            assert r.status_code == 200
+            f = Files.query.filter_by(id=1).first()
+            filepath = os.path.join(app.config["UPLOAD_FOLDER"] + "/" + f.location)
+            with open(filepath) as f:
+                assert f.read() == "test file content"
+            os.remove(filepath)
+    destroy_ctfd(app)