Densaugeo/uploadserver

leftovers in case of token mismatch

abbbe opened this issue · 8 comments

abbbe commented

I have noticed that sometimes uploadserver leaves temporary files even if token does not match.

To reproduce:

abb@mc:~/ae0ohYee$ python3 -m pip install -U uploadserver
Requirement already up-to-date: uploadserver in /home/abb/.local/lib/python3.8/site-packages (4.0.0)

abb@mc:~/ae0ohYee$ ls -la
total 8
drwxr-xr-x  2 abb abb 4096 Feb 10 12:31 .
drwxr-xr-x 10 abb abb 4096 Feb 10 12:30 ..

abb@mc:~/ae0ohYee$ python3 -m uploadserver -t ae0ohYee
File upload available at /upload
Serving HTTP on 0.0.0.0 port 8000 (http://0.0.0.0:8000/) ...

In 2nd terminal:

abb@mc:~/ae0ohYee$ md5sum /tmp/uploadserver-test.txt
edb68e00f7d67e7872f7787220804482  /tmp/uploadserver-test.txt

abb@mc:~/ae0ohYee$ curl -X POST http://127.0.0.1:8000/upload -F 'files=@/tmp/uploadserver-test.txt' -F 'token=abrakadabra'
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN"
        "http://www.w3.org/TR/html4/strict.dtd">
<html>
    <head>
        <meta http-equiv="Content-Type" content="text/html;charset=utf-8">
        <title>Error response</title>
    </head>
    <body>
        <h1>Error response</h1>
        <p>Error code: 403</p>
        <p>Message: Token is enabled on this server, and your token is missing or wrong.</p>
        <p>Error code explanation: HTTPStatus.FORBIDDEN - Request forbidden -- authorization will not help.</p>
    </body>
</html>

Back to the 1st terminal:

127.0.0.1 - - [10/Feb/2023 12:31:35] Upload of "uploadserver-test.txt" rejected (bad token)
127.0.0.1 - - [10/Feb/2023 12:31:35] code 403, message Token is enabled on this server, and your token is missing or wrong
127.0.0.1 - - [10/Feb/2023 12:31:35] "POST /upload HTTP/1.1" 403 -
^C
Keyboard interrupt received, exiting.

abb@mc:~/ae0ohYee$ ls -la
total 12
drwxr-xr-x  2 abb abb 4096 Feb 10 12:31 .
drwxr-xr-x 10 abb abb 4096 Feb 10 12:30 ..
-rw-------  1 abb abb 1320 Feb 10 12:31 tmphhxeecw5

abb@mc:~/ae0ohYee$ md5sum tmphhxeecw5
edb68e00f7d67e7872f7787220804482  tmphhxeecw5

This behaviour does not appear on very short files, so I have uploaded the test file to sprunge to let you reproduce it.

abb@mc:~/ae0ohYee$ cat /tmp/uploadserver-test.txt | curl -F 'sprunge=<-' http://sprunge.us
http://sprunge.us/vymoVG

Thanks for the bug report, I've reproduced it.

The underlying issue is that transmission order is not guaranteed for form fields, so the server has to receive the whole request before the token can be validated.

This has caused issues that I had to work around before, so I'm going to try putting the token into a header instead, and see if the underlying framework will let it reject bad requests before transferring the file.

abbbe commented

Maybe you should remove the temporary file somewhere around here https://github.com/Densaugeo/uploadserver/blob/master/uploadserver/__init__.py#L175 ?

That'd be a quick fix, but I suspect there would still be ways for a malicious client to make the server store large amounts of junk data. Still, would be a backup option if using a header doesn't do what I hope.

abbbe commented

Right, I guess this happens here

form = PersistentFieldStorage(fp=handler.rfile, headers=handler.headers, environ={'REQUEST_METHOD': 'POST'})
.

I wonder why invent your own authentication scheme, you could just reuse basic HTTP authentication method, like here https://gist.github.com/dragermrb/108158f5a284b5fba806.

If you do that, it would be cool to (perhaps optionally) cover normal download operations ;).

I'm not very knowledgeable about authentication schemes, and went with the current one because someone sent it in a pull request and it seemed pretty simple.

The most obvious issue I see with HTTP basic is it typically involves usernames and passwords, and I'd rather not get into maintaining any sort of account system. What's the simplest way you know to implement it?

It looks like there's a standard of passing tokens in a header like this: "Authorization: Bearer TOKEN", which as far as I can tell is basically the same as HTTP basic but with a token in place of an encoded username/password pair.

abbbe commented

Ok, here we go :)

abbbe commented

Bearer authorisation is indeed an option, but it is typically used for machine-to-machine communications, to carry stuff like JWT. Basic authentication is better suited for human being and is supported by browsers.

abbbe commented

Normally the issue will not occur if basic auth is used, so closing the issue )