notebook: fix massive security vulnerability and get rid of all possible "internal server errors" when doing "Data --> Upload or attach file"

[williamstein]

Uploading or attaching an empty file or a file that doesn't exist, etc. can cause internal server errors instead of a proper error message.

Moreover, notice these lines in twist.py:

class Worksheet_do_upload_data
...
        dest = os.path.join(self.worksheet.data_directory(), name)
        if os.path.exists(dest):
            os.unlink(dest)

With a properly crafted URL I bet name could contain .. and hence one could make the notebook server delete any file it has access to! This is a critical security vulnerability.

See also #3849 for similar issues when uploading a worksheet.

Component: notebook

Author: William Stein

Reviewer: Mitesh Patel

Issue created by migration from https://trac.sagemath.org/ticket/7495

[williamstein]

comment:1

Yes, this is fully exploitable and allows one to delete any file on the server:
E.g., on my laptop I created a file tmp/xyz, then pasted in this url, and that file was deleted.

http://localhost:8000/home/admin/13/do_upload_data?urlField=%27%27&nameField=../../../../../../../../tmp/xyz

With a little more work, one could not only delete any file, but I think replace it by a file of ones choice. That's a pretty massive exploit.

So I'm upping this to a blocker and fixing it now.

[jasongrout]

comment:2

Can you do a quick grep through the source to see if any os.* functions are used in a like manner in the notebook?

[williamstein]

Attachment: sagenb-7495.patch.gz

[qed777]

comment:4

Creating a new file with name & raises an OSError.

[qed777]

comment:5

Replying to @qed777:

Creating a new file with name & raises an OSError.

Actually, clicking to delete this file raises the error:

        exceptions.OSError: [Errno 21] Is a directory: '/home/apps/.sage/sage_notebook.sagenb/home/admin/88/data/'

[qed777]

Attachment: sagenb-7495.2.patch.gz

Version 2. Minor simplifications. Apply only this patch to sagenb repo.

[qed777]

comment:6

I think the OSError above is a separate URI encoding problem.

[qed777]

Author: William Stein

[qed777]

comment:7

Anyway, my digression aside, my review is positive, as far as it goes.

[qed777]

Reviewer: Mitesh Patel

[qed777]

comment:8

Replying to @qed777:

I think the OSError above is a separate URI encoding problem.

This is now #7500.

[williamstein]

comment:9

"Anyway, my digression aside, my review is positive, as far as it goes. " so positive review.

[williamstein]

comment:10

merged into sage-4.3