Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add disclaimer about limited slow retrieval attack protection #928

Merged

Conversation

lukpueh
Copy link
Member

@lukpueh lukpueh commented Oct 3, 2019

Fixes issue #:
None. Related to #781.

Description of the changes being introduced by the pull request:
Since #781 we only provide limited protection against slow retrieval attacks. So far this has only been discussed in above issue and hinted at by a disabled test and a code comment in that test.

This change adds a corresponding disclaimer to a more prominent place, i.e. the list of attacks in SECURITY.md.

Please verify and check that the pull request fulfills the following
requirements
:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@lukpueh
Copy link
Member Author

lukpueh commented Oct 3, 2019

We may also want to exclude (or add a comment to) the section about slow retrieval attacks in the tutorial-like ATTACKS.md document.

docs/SECURITY.md Outdated Show resolved Hide resolved
@trishankatdatadog
Copy link
Member

Thanks, @lukpueh! Just want to make it clearer that this limitation is not due to our code...

docs/SECURITY.md Outdated
@@ -20,7 +20,8 @@ snapshot metadata, and thus new updates could never be downloaded.

* **Endless data attacks**. An attacker responds to a file download request with an endless stream of data, causing harm to clients (e.g. a disk partition filling up or memory exhaustion).

* **Slow retrieval attacks**. An attacker responds to clients with a very slow stream of data that essentially results in the client never continuing the update process.
* **~~Slow retrieval attacks~~**. An attacker responds to clients with a very slow stream of data that essentially results in the client never continuing the update process.\
**_NOTE: The TUF reference implementation currently provides only limited protection against slow retrieval attacks (see [tuf#781](https://github.com/theupdateframework/tuf/pull/781))._**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be trying to fix this upstream in request? Is that sensible? If so, should we link to that discussion instead?

The issue we refer to here has a lot going on, I'd prefer we had something clearer to point to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I guess we should at least file an issue that points to slow loris attacks.

Since theupdateframework#781 we
only provide limited protection against slow retrieval attacks.
So far this has only been discussed in above issue and hinted at
by a disabled test and a code comment in that test.

This change adds a corresponding disclaimer to a more prominent
place, i.e. the list of attacks in SECURITY.md.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Co-Authored-By: Trishank K Kuppusamy <33133073+trishankatdatadog@users.noreply.github.com>
@lukpueh lukpueh force-pushed the add-slow-retrieval-disclaimer branch from fec30ca to 42a4cee Compare October 10, 2019 14:44
@lukpueh
Copy link
Member Author

lukpueh commented Oct 10, 2019

I created an issue #932 (downstream for the time being) that summarizes key points from #781 and updated the disclaimer in this PR. Please review/approve/merge.

I think this is the last thing we wanted to get in before bumping tuf to 0.12.0 (#921).

Copy link
Contributor

@mnm678 mnm678 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@JustinCappos
Copy link
Member

Is the failing test a race condition / Heisenbug? I would merge otherwise

@lukpueh
Copy link
Member Author

lukpueh commented Oct 11, 2019

Is the failing test a race condition / Heisenbug? I would merge otherwise

It's a timeout issue, which seems to happen fairly often for tests on appveyor/windows (less often on travis/linux too). In any case, the fail here is impossibly related to the change in this PR. I say it's safe to merge.

@lukpueh lukpueh merged commit 232a4d6 into theupdateframework:develop Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants