Skip to content

Add disclaimer about limited slow retrieval attack protection#928

Merged
lukpueh merged 1 commit intotheupdateframework:developfrom
lukpueh:add-slow-retrieval-disclaimer
Oct 11, 2019
Merged

Add disclaimer about limited slow retrieval attack protection#928
lukpueh merged 1 commit intotheupdateframework:developfrom
lukpueh:add-slow-retrieval-disclaimer

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 lukpueh force-pushed the add-slow-retrieval-disclaimer branch from 75e5351 to 5203754 Compare October 3, 2019 11:44
@lukpueh lukpueh mentioned this pull request Oct 3, 2019
@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.

@trishankatdatadog
Copy link
Contributor

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

docs/SECURITY.md Outdated

* **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
Contributor

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