From 7e875e021a34bc63ddee8d48b32c918d9561f7c0 Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Tue, 18 Sep 2018 08:35:58 +1000 Subject: [PATCH] Remove unnecessary loop. Remove the Close route/setting which can DoS another running upload. Fix detecting whether any uploads are still in progress before terminating the service after timer expires. Don't register 404s for uploads after expiry has finished (throw a 403 instead)" --- onionshare/settings.py | 3 +- onionshare/web.py | 210 +++++++++++------------- onionshare_gui/receive_mode/__init__.py | 2 +- onionshare_gui/settings_dialog.py | 13 -- share/locale/en.json | 1 - share/templates/403.html | 16 ++ share/templates/receive.html | 9 - test/test_onionshare_settings.py | 3 +- test/test_onionshare_web.py | 30 ---- 9 files changed, 115 insertions(+), 172 deletions(-) create mode 100644 share/templates/403.html diff --git a/onionshare/settings.py b/onionshare/settings.py index ef75be2f..6cc70954 100644 --- a/onionshare/settings.py +++ b/onionshare/settings.py @@ -72,8 +72,7 @@ class Settings(object): 'public_mode': False, 'slug': '', 'hidservauth_string': '', - 'downloads_dir': self.build_default_downloads_dir(), - 'receive_allow_receiver_shutdown': True + 'downloads_dir': self.build_default_downloads_dir() } self._settings = {} self.fill_in_defaults() diff --git a/onionshare/web.py b/onionshare/web.py index 9c3d52ba..c197caf1 100644 --- a/onionshare/web.py +++ b/onionshare/web.py @@ -119,6 +119,7 @@ class Web(object): self.done = False self.can_upload = True + self.uploads_in_progress = [] # If the client closes the OnionShare window while a download is in progress, # it should immediately stop serving the file. The client_cancel global is @@ -323,9 +324,7 @@ class Web(object): r = make_response(render_template( 'receive.html', - upload_action=upload_action, - close_action=close_action, - receive_allow_receiver_shutdown=self.common.settings.get('receive_allow_receiver_shutdown'))) + upload_action=upload_action)) return self.add_security_headers(r) @self.app.route("/") @@ -344,130 +343,102 @@ class Web(object): """ Upload files. """ - while self.can_upload: - # Make sure downloads_dir exists - valid = True - try: - self.common.validate_downloads_dir() - except DownloadsDirErrorCannotCreate: - self.add_request(Web.REQUEST_ERROR_DOWNLOADS_DIR_CANNOT_CREATE, request.path) - print(strings._('error_cannot_create_downloads_dir').format(self.common.settings.get('downloads_dir'))) - valid = False - except DownloadsDirErrorNotWritable: - self.add_request(Web.REQUEST_ERROR_DOWNLOADS_DIR_NOT_WRITABLE, request.path) - print(strings._('error_downloads_dir_not_writable').format(self.common.settings.get('downloads_dir'))) - valid = False - if not valid: - flash('Error uploading, please inform the OnionShare user', 'error') - if self.common.settings.get('public_mode'): - return redirect('/') - else: - return redirect('/{}'.format(slug_candidate)) - - files = request.files.getlist('file[]') - filenames = [] - print('') - for f in files: - if f.filename != '': - # Automatically rename the file, if a file of the same name already exists - filename = secure_filename(f.filename) - filenames.append(filename) - local_path = os.path.join(self.common.settings.get('downloads_dir'), filename) - if os.path.exists(local_path): - if '.' in filename: - # Add "-i", e.g. change "foo.txt" to "foo-2.txt" - parts = filename.split('.') - name = parts[:-1] - ext = parts[-1] - - i = 2 - valid = False - while not valid: - new_filename = '{}-{}.{}'.format('.'.join(name), i, ext) - local_path = os.path.join(self.common.settings.get('downloads_dir'), new_filename) - if os.path.exists(local_path): - i += 1 - else: - valid = True - else: - # If no extension, just add "-i", e.g. change "foo" to "foo-2" - i = 2 - valid = False - while not valid: - new_filename = '{}-{}'.format(filename, i) - local_path = os.path.join(self.common.settings.get('downloads_dir'), new_filename) - if os.path.exists(local_path): - i += 1 - else: - valid = True - - basename = os.path.basename(local_path) - if f.filename != basename: - # Tell the GUI that the file has changed names - self.add_request(Web.REQUEST_UPLOAD_FILE_RENAMED, request.path, { - 'id': request.upload_id, - 'old_filename': f.filename, - 'new_filename': basename - }) - - self.common.log('Web', 'receive_routes', '/upload, uploaded {}, saving to {}'.format(f.filename, local_path)) - print(strings._('receive_mode_received_file').format(local_path)) - f.save(local_path) - - # Note that flash strings are on English, and not translated, on purpose, - # to avoid leaking the locale of the OnionShare user - if len(filenames) == 0: - flash('No files uploaded', 'info') - else: - for filename in filenames: - flash('Sent {}'.format(filename), 'info') - - - # Register that uploads were sent (for shutdown timer) - self.done = True - + # Make sure downloads_dir exists + valid = True + try: + self.common.validate_downloads_dir() + except DownloadsDirErrorCannotCreate: + self.add_request(Web.REQUEST_ERROR_DOWNLOADS_DIR_CANNOT_CREATE, request.path) + print(strings._('error_cannot_create_downloads_dir').format(self.common.settings.get('downloads_dir'))) + valid = False + except DownloadsDirErrorNotWritable: + self.add_request(Web.REQUEST_ERROR_DOWNLOADS_DIR_NOT_WRITABLE, request.path) + print(strings._('error_downloads_dir_not_writable').format(self.common.settings.get('downloads_dir'))) + valid = False + if not valid: + flash('Error uploading, please inform the OnionShare user', 'error') if self.common.settings.get('public_mode'): return redirect('/') else: return redirect('/{}'.format(slug_candidate)) - @self.app.route("//upload", methods=['POST']) - def upload(slug_candidate): - self.check_slug_candidate(slug_candidate) - if self.can_upload: - return upload_logic(slug_candidate) + files = request.files.getlist('file[]') + filenames = [] + print('') + for f in files: + if f.filename != '': + # Automatically rename the file, if a file of the same name already exists + filename = secure_filename(f.filename) + filenames.append(filename) + local_path = os.path.join(self.common.settings.get('downloads_dir'), filename) + if os.path.exists(local_path): + if '.' in filename: + # Add "-i", e.g. change "foo.txt" to "foo-2.txt" + parts = filename.split('.') + name = parts[:-1] + ext = parts[-1] + + i = 2 + valid = False + while not valid: + new_filename = '{}-{}.{}'.format('.'.join(name), i, ext) + local_path = os.path.join(self.common.settings.get('downloads_dir'), new_filename) + if os.path.exists(local_path): + i += 1 + else: + valid = True + else: + # If no extension, just add "-i", e.g. change "foo" to "foo-2" + i = 2 + valid = False + while not valid: + new_filename = '{}-{}'.format(filename, i) + local_path = os.path.join(self.common.settings.get('downloads_dir'), new_filename) + if os.path.exists(local_path): + i += 1 + else: + valid = True + + basename = os.path.basename(local_path) + if f.filename != basename: + # Tell the GUI that the file has changed names + self.add_request(Web.REQUEST_UPLOAD_FILE_RENAMED, request.path, { + 'id': request.upload_id, + 'old_filename': f.filename, + 'new_filename': basename + }) + self.common.log('Web', 'receive_routes', '/upload, uploaded {}, saving to {}'.format(f.filename, local_path)) + print(strings._('receive_mode_received_file').format(local_path)) + f.save(local_path) + + # Note that flash strings are on English, and not translated, on purpose, + # to avoid leaking the locale of the OnionShare user + if len(filenames) == 0: + flash('No files uploaded', 'info') else: - return self.error404() + for filename in filenames: + flash('Sent {}'.format(filename), 'info') - @self.app.route("/upload", methods=['POST']) - def upload_public(): - if not self.common.settings.get('public_mode') or not self.can_upload: - return self.error404() - return upload_logic() - - - def close_logic(slug_candidate=''): - if self.common.settings.get('receive_allow_receiver_shutdown'): - self.force_shutdown() - r = make_response(render_template('closed.html')) - self.add_request(Web.REQUEST_CLOSE_SERVER, request.path) - return self.add_security_headers(r) + if self.common.settings.get('public_mode'): + return redirect('/') else: return redirect('/{}'.format(slug_candidate)) - @self.app.route("//close", methods=['POST']) - def close(slug_candidate): + @self.app.route("//upload", methods=['POST']) + def upload(slug_candidate): + if not self.can_upload: + return self.error403() self.check_slug_candidate(slug_candidate) - if self.can_upload: - return close_logic(slug_candidate) - else: - return self.error404() + return upload_logic(slug_candidate) - @self.app.route("/close", methods=['POST']) - def close_public(): - if not self.common.settings.get('public_mode') or not self.can_upload: + @self.app.route("/upload", methods=['POST']) + def upload_public(): + if not self.common.settings.get('public_mode'): return self.error404() - return close_logic() + if not self.can_upload: + return self.error403() + return upload_logic() + def common_routes(self): """ @@ -504,6 +475,12 @@ class Web(object): r = make_response(render_template('404.html'), 404) return self.add_security_headers(r) + def error403(self): + self.add_request(Web.REQUEST_OTHER, request.path) + + r = make_response(render_template('403.html'), 403) + return self.add_security_headers(r) + def add_security_headers(self, r): """ Add security headers to a request @@ -783,6 +760,8 @@ class ReceiveModeRequest(Request): datetime.now().strftime("%b %d, %I:%M%p"), strings._("receive_mode_upload_starting").format(self.web.common.human_readable_filesize(self.content_length)) )) + # append to self.uploads_in_progress + self.web.uploads_in_progress.append(self.upload_id) # Tell the GUI self.web.add_request(Web.REQUEST_STARTED, self.path, { @@ -816,6 +795,9 @@ class ReceiveModeRequest(Request): 'id': self.upload_id }) + # remove from self.uploads_in_progress + self.web.uploads_in_progress.remove(self.upload_id) + def file_write_func(self, filename, length): """ This function gets called when a specific file is written to. diff --git a/onionshare_gui/receive_mode/__init__.py b/onionshare_gui/receive_mode/__init__.py index a9f15749..8c360d19 100644 --- a/onionshare_gui/receive_mode/__init__.py +++ b/onionshare_gui/receive_mode/__init__.py @@ -99,7 +99,7 @@ class ReceiveMode(Mode): """ # TODO: wait until the final upload is done before stoppign the server? # If there were no attempts to upload files, or all uploads are done, we can stop - if self.web.upload_count == 0 or self.web.done: + if self.web.upload_count == 0 or not self.web.uploads_in_progress: self.server_status.stop_server() self.server_status_label.setText(strings._('close_on_timeout', True)) return True diff --git a/onionshare_gui/settings_dialog.py b/onionshare_gui/settings_dialog.py index c4e6b752..c23f8ad8 100644 --- a/onionshare_gui/settings_dialog.py +++ b/onionshare_gui/settings_dialog.py @@ -101,15 +101,9 @@ class SettingsDialog(QtWidgets.QDialog): downloads_layout.addWidget(self.downloads_dir_lineedit) downloads_layout.addWidget(downloads_button) - # Allow the receiver to shutdown the server - self.receive_allow_receiver_shutdown_checkbox = QtWidgets.QCheckBox() - self.receive_allow_receiver_shutdown_checkbox.setCheckState(QtCore.Qt.Checked) - self.receive_allow_receiver_shutdown_checkbox.setText(strings._("gui_settings_receive_allow_receiver_shutdown_checkbox", True)) - # Receiving options layout receiving_group_layout = QtWidgets.QVBoxLayout() receiving_group_layout.addLayout(downloads_layout) - receiving_group_layout.addWidget(self.receive_allow_receiver_shutdown_checkbox) receiving_group = QtWidgets.QGroupBox(strings._("gui_settings_receiving_label", True)) receiving_group.setLayout(receiving_group_layout) @@ -421,12 +415,6 @@ class SettingsDialog(QtWidgets.QDialog): downloads_dir = self.old_settings.get('downloads_dir') self.downloads_dir_lineedit.setText(downloads_dir) - receive_allow_receiver_shutdown = self.old_settings.get('receive_allow_receiver_shutdown') - if receive_allow_receiver_shutdown: - self.receive_allow_receiver_shutdown_checkbox.setCheckState(QtCore.Qt.Checked) - else: - self.receive_allow_receiver_shutdown_checkbox.setCheckState(QtCore.Qt.Unchecked) - public_mode = self.old_settings.get('public_mode') if public_mode: self.public_mode_checkbox.setCheckState(QtCore.Qt.Checked) @@ -802,7 +790,6 @@ class SettingsDialog(QtWidgets.QDialog): # Also unset the HidServAuth if we are removing our reusable private key settings.set('hidservauth_string', '') settings.set('downloads_dir', self.downloads_dir_lineedit.text()) - settings.set('receive_allow_receiver_shutdown', self.receive_allow_receiver_shutdown_checkbox.isChecked()) settings.set('public_mode', self.public_mode_checkbox.isChecked()) settings.set('use_stealth', self.stealth_checkbox.isChecked()) # Always unset the HidServAuth if Stealth mode is unset diff --git a/share/locale/en.json b/share/locale/en.json index beebac77..98addae9 100644 --- a/share/locale/en.json +++ b/share/locale/en.json @@ -168,7 +168,6 @@ "gui_settings_receiving_label": "Receiving options", "gui_settings_downloads_label": "Save files to", "gui_settings_downloads_button": "Browse", - "gui_settings_receive_allow_receiver_shutdown_checkbox": "Receive mode can be stopped by the sender", "gui_settings_public_mode_checkbox": "OnionShare is open to the public\n(don't prevent people from guessing the OnionShare address)", "systray_close_server_title": "OnionShare Server Closed", "systray_close_server_message": "A user closed the server", diff --git a/share/templates/403.html b/share/templates/403.html new file mode 100644 index 00000000..df81f3e7 --- /dev/null +++ b/share/templates/403.html @@ -0,0 +1,16 @@ + + + + OnionShare: 403 Forbidden + + + + +
+
+

+

You are not allowed to perform that action at this time.

+
+
+ + diff --git a/share/templates/receive.html b/share/templates/receive.html index d8b02f73..e85b6ff9 100644 --- a/share/templates/receive.html +++ b/share/templates/receive.html @@ -34,14 +34,5 @@ - {% if receive_allow_receiver_shutdown %} - {% with messages = get_flashed_messages() %} - {% if messages %} -
- -
- {% endif %} - {% endwith %} - {% endif %} diff --git a/test/test_onionshare_settings.py b/test/test_onionshare_settings.py index 1521492d..fb9e1f69 100644 --- a/test/test_onionshare_settings.py +++ b/test/test_onionshare_settings.py @@ -63,8 +63,7 @@ class TestSettings: 'private_key': '', 'slug': '', 'hidservauth_string': '', - 'downloads_dir': os.path.expanduser('~/OnionShare'), - 'receive_allow_receiver_shutdown': True, + 'downloads_dir': os.path.expanduser('~/OnionShare') 'public_mode': False } diff --git a/test/test_onionshare_web.py b/test/test_onionshare_web.py index 2209a0fd..d523ea96 100644 --- a/test/test_onionshare_web.py +++ b/test/test_onionshare_web.py @@ -138,36 +138,6 @@ class TestWeb: res.get_data() assert res.status_code == 200 - def test_receive_mode_allow_receiver_shutdown_on(self, common_obj): - web = web_obj(common_obj, True) - - common_obj.settings.set('receive_allow_receiver_shutdown', True) - - assert web.running == True - - with web.app.test_client() as c: - # Load close page - res = c.post('/{}/close'.format(web.slug)) - res.get_data() - # Should return ok, and server should stop - assert res.status_code == 200 - assert web.running == False - - def test_receive_mode_allow_receiver_shutdown_off(self, common_obj): - web = web_obj(common_obj, True) - - common_obj.settings.set('receive_allow_receiver_shutdown', False) - - assert web.running == True - - with web.app.test_client() as c: - # Load close page - res = c.post('/{}/close'.format(web.slug)) - res.get_data() - # Should redirect to index, and server should still be running - assert res.status_code == 302 - assert web.running == True - def test_public_mode_on(self, common_obj): web = web_obj(common_obj, True) common_obj.settings.set('public_mode', True)