From a9820aef4e01fc0b0d66a4926d4f1e0b3bb9f614 Mon Sep 17 00:00:00 2001 From: heartsucker Date: Mon, 10 Sep 2018 14:52:10 +0200 Subject: [PATCH 1/2] check ownership of reply before deletion This patch fixes a bug where an authenticated source A could delete the replies send to source B if they could guess the filename. Since we don't throttle source interactions, and since the wordlists are public, an attack could attempt to enumerate them. (cherry picked from commit fc9db8485810aa7d46e3c82677c6df3807031c89) --- securedrop/source_app/main.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/securedrop/source_app/main.py b/securedrop/source_app/main.py index cd52fe42f8..e3dd66b4c5 100644 --- a/securedrop/source_app/main.py +++ b/securedrop/source_app/main.py @@ -212,8 +212,9 @@ def delete(): history. """ - query = Reply.query.filter( - Reply.filename == request.form['reply_filename']) + query = Reply.query.filter_by( + filename=request.form['reply_filename'], + source_id=g.source.id) reply = get_one_or_else(query, current_app.logger, abort) reply.deleted_by_source = True db.session.add(reply) From 985784c6fb313c0b3376dd33f431c4cdd0914c56 Mon Sep 17 00:00:00 2001 From: heartsucker Date: Mon, 10 Sep 2018 15:22:38 +0200 Subject: [PATCH 2/2] regresssion test for source reply deletion (cherry picked from commit 4f713f05230358a6aa8a55abcbff90699c6fea9a) --- securedrop/tests/test_source.py | 46 ++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/securedrop/tests/test_source.py b/securedrop/tests/test_source.py index 7bae4c710f..dd8d9527dd 100644 --- a/securedrop/tests/test_source.py +++ b/securedrop/tests/test_source.py @@ -5,7 +5,7 @@ import subprocess from cStringIO import StringIO -from flask import session, escape, current_app, url_for +from flask import session, escape, current_app, url_for, g from mock import patch, ANY import crypto_util @@ -647,3 +647,47 @@ def test_csrf_error_page(config, source_app): resp = app.post(url_for('main.create'), follow_redirects=True) text = resp.data.decode('utf-8') assert 'Your session timed out due to inactivity' in text + + +def test_source_can_only_delete_own_replies(source_app): + '''This test checks for a bug an authenticated source A could delete + replies send to source B by "guessing" the filename. + ''' + source0, codename0 = utils.db_helper.init_source() + source1, codename1 = utils.db_helper.init_source() + journalist, _ = utils.db_helper.init_journalist() + replies = utils.db_helper.reply(journalist, source0, 1) + filename = replies[0].filename + confirmation_msg = 'Reply deleted' + + with source_app.test_client() as app: + resp = app.post(url_for('main.login'), + data={'codename': codename1}, + follow_redirects=True) + assert resp.status_code == 200 + assert g.source.id == source1.id + + resp = app.post(url_for('main.delete'), + data={'reply_filename': filename}, + follow_redirects=True) + assert resp.status_code == 404 + assert confirmation_msg not in resp.data.decode('utf-8') + + reply = Reply.query.filter_by(filename=filename).one() + assert not reply.deleted_by_source + + with source_app.test_client() as app: + resp = app.post(url_for('main.login'), + data={'codename': codename0}, + follow_redirects=True) + assert resp.status_code == 200 + assert g.source.id == source0.id + + resp = app.post(url_for('main.delete'), + data={'reply_filename': filename}, + follow_redirects=True) + assert resp.status_code == 200 + assert confirmation_msg in resp.data.decode('utf-8') + + reply = Reply.query.filter_by(filename=filename).one() + assert reply.deleted_by_source