-
Notifications
You must be signed in to change notification settings - Fork 275
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
Fix #322 freeze attack by detecting expiry of stale metadata files #324
Fix #322 freeze attack by detecting expiry of stale metadata files #324
Conversation
…onflicts. Comment lines from the individual commits include: 1. Fix theupdateframework#322 by detecting expiry of stale files. initial attempt 2. temp commit of files from Soma 3. removing freeze_attack_stale_expiry and leaving the test added to indefinite freeze attack 4. fixing indefinite freeze attack test: now incorporates old reject-freshly-downloaded-but-expired-timestamp test as well as reject-stale-already-present-but-expired-snapshot test 5. small refinements to indefinite freeze attack test 6. Pulled the recursion out of the except block in refresh() to avoid unprintable nested exceptions. 7. Added comments to the last commit (retry_once) 8. Merge pull request #1 from awwad/detect_expiry_322_temp (removing cruft in another branch)
Tests all look good to me. Running full tox again on 2.7 and 3.4. LMK if you think this is OK, Vlad? |
March 9, 2016. | ||
Additional test added relating to issue: | ||
https://github.com/theupdateframework/tuf/issues/322 | ||
If a metadata file is not downloaded (no indication of a new version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this should instead say, "... a metadata file is not updated ..." or "... the expiration of the locally trusted metadata should be detected."
# root was checked for expiration at the beginning of each refresh() cycle, | ||
# and timestamp was always checked because it was always fetched.) Snapshot | ||
# and targets were never checked if the user does not have evidence that | ||
# they have changed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment block would be more helpful to have (or duplicated) in updater.py
. The <Purpose>
section of refresh() would be an ideal location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated that:
awwad@8adab68
I think that should be better. LMK if you think it needs more work.
@@ -171,12 +180,26 @@ def tearDown(self): | |||
|
|||
|
|||
def test_without_tuf(self): | |||
# Scenario: | |||
# Two tests are conducted here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, Test 2 is skipped. But I see why you'd want to acknowledge the two possible cases for stale metadata.
…had passed. (updater.refresh() can take a while.) - Put code back into working state after some failure mode debugging of a separate bug by Vlad and me. - Expanded some comments, added explanations, and made a few others more readable. - Removed an unnecessary path recalculation and an unnecessary sleep.
Sidenote: there is not enough information in the code about why "unsafely update root if necessary" is unsafe. |
I tried to explain here: https://github.com/awwad/tuf/blob/detect_expiry_322_clean/tuf/client/updater.py#L620-L624 Previously, "safely" fetching root.json required knowing its hash and file size (as specified in snapshot.json). Now, safely fetching root.json is knowing its exact version number. A root.json is unsafely fetched without knowing its version number (i.e., snapshot.json might have expired, thus the client is unsure of the currently trusted version). |
tuf.client.updater.refresh(): Removed the retry_once stuff, waiting for a fix to the python 3 string issue causing unprintable exceptions. test_indefinite_freeze_attack: Removed forced failure conditions that I'd left in for debugging the string issue. All should be in order again. As for this comment on cleaning up the test, I've tried to cut out pieces, but end up with behavior that surprises me. It may be that I'm failing to understand something. For example, within test_with_tuf, for test 2, the repository is reloaded and the signing key for timestamp is loaded again at current lines 356-362 even though the repository and its keys are already loaded earlier for test 1 at current lines 272-290. Logic suggests, I agree, that I can simply remove the second loading at L356-362 of repo and keys... but somehow, if I do remove the reloading of repo and keys, it leads to a different complaint at line 371's repository.write() call: that the root keys aren't loaded. Again, the root keys were already loaded earlier, and without the repo reload, there should be no need to load the root keys again. This doesn't make sense and suggests some corruption in the repository somewhere in the test setup. Incidentally, if I do load the root keys again, I then get:
Does this have something to do with consistent snapshots suddenly being toggled off or on? Why would that happen here? Shuffling pieces about has suggested oddly that the move of the repo files from staging to live and the refresh of client data affect the appearance of this possible bug. Since that doesn't make sense, and those things take time (multiple seconds in the case of the refresh and on the order of 200ms in the case of the write) I'm wondering if there's some sort of race condition... but I haven't been able to nail down the minimal set of requirements to yield this behavior yet. In summary: |
I've created a branch on my repo that reliably produces the unrelated error of the possible bug noted and will in the future address it in a separate issue & PR. Vlad, please let me know if there's anything left to do with this PR. |
I think the only remaining issue is the unprintable exception. See: |
It wasn't a need for There were two bugs in In particular:
|
Hokay. NOW I think this PR is done. Hehehe. |
I just recalled that import of That would explain the uninstantiated logging object. I verified that we didn't break the commit above. |
One reason why we might have missed the exceptions raised in init(): http://stackoverflow.com/questions/25374679/exception-in-python-exception-classs-str-missed |
Yeah, exceptions in str() are sneaky. |
WRT logging removal: |
The issue that Alex fixed only occurs if you import |
Correction: only occurs if you import |
Messed around with it and it seems OK to me on edge cases, too. Okay, so nothing else I'm aware of. I leave it to you. ^_^ |
I reviewed the changes and made edits here: dd8a7eb Please merge those changes with this pull request (if all my changes are good to go). |
This fixes Issue #322. Please see that issue!
It replaces PR #323, which got too messy.
Update:
This PR also:
This started as a manual squash commit of detect_expiry_322, to avoid merge conflicts.
Comment lines from the individual commits include: