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

Fix #322 freeze attack by detecting expiry of stale metadata files #324

Merged

Conversation

awwad
Copy link
Contributor

@awwad awwad commented Mar 10, 2016

This fixes Issue #322. Please see that issue!
It replaces PR #323, which got too messy.

Update:
This PR also:

  • 🐛🐛 Fixes two additional bugs issues identified below, both causing unprintable NoWorkingMirrorError exceptions - one in python 3 only and the other in either 2 or 3 with certain mirror name issues.
  • adds .DS_Store to .gitignore

This started as a manual squash commit of detect_expiry_322, to avoid merge conflicts.

Comment lines from the individual commits include:

  1. Fix Client: Freeze attack issue - expiry of metadata may not be detected #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 download.py updated #1 from awwad/detect_expiry_322_temp (removing cruft in another branch)

…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)
@awwad
Copy link
Contributor Author

awwad commented Mar 10, 2016

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
Copy link
Contributor

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.
Copy link
Contributor

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.

Copy link
Contributor Author

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.
Copy link
Contributor

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.

awwad added 7 commits March 14, 2016 16:30
…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.
@awwad
Copy link
Contributor Author

awwad commented Mar 16, 2016

Sidenote: there is not enough information in the code about why "unsafely update root if necessary" is unsafe.

@vladimir-v-diaz
Copy link
Contributor

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).

@awwad
Copy link
Contributor Author

awwad commented Mar 16, 2016

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:

  File "test_indefinite_freeze_attack.py", line 377, in test_with_tuf
    repository.write()
  File "/Users/s/w/tuf/tuf/repository_tool.py", line 284, in write
    consistent_snapshot)
  File "/Users/s/w/tuf/tuf/repository_lib.py", line 138, in _generate_and_write_metadata
    roleinfo['version'],
KeyError: u'version'

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:
While none of this influences the freeze attack scenario or this test (as leaving the code as is does not prompt the error condition), either I've spotted another bug or there's something wrong with the test setup (teardown? staging to live copies?). As far as I can tell, the freeze attack bugfix and test update (and so this PR) are still OK to merge
. Since I don't understand this incidental behavior, I want to have a way to reproduce it noted before I move on.

@awwad
Copy link
Contributor Author

awwad commented Mar 16, 2016

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.

@vladimir-v-diaz
Copy link
Contributor

I think the only remaining issue is the unprintable exception.
You can override __unicode__() (as we do with __str__()) so that exception messages are correctly printed in Python 3.

See:
https://github.com/awwad/tuf/blob/detect_expiry_322_clean/tuf/__init__.py#L332

@awwad
Copy link
Contributor Author

awwad commented Mar 16, 2016

It wasn't a need for __unicode__(), but we figured it out.

There were two bugs in tuf/__init__.py class NoWorkingMirrorError, both causing unprintable NoWorkingMirrorError.

In particular:

  • 🐛 c4ef697 fixed a bug that made all NoWorkingMirrorError exceptions unprintable in python 3. It was a python 3 compatibility issue (dictionary.iteritems()).
  • 🐛 7cd20fe fixed a bug that made NoWorkingMirrorError exceptions unprintable in any python version in certain mirror name edge cases. (Use of an uninstantiated logging object without logging imported.)

@awwad
Copy link
Contributor Author

awwad commented Mar 16, 2016

Hokay. NOW I think this PR is done. Hehehe.

@vladimir-v-diaz
Copy link
Contributor

I just recalled that import of tuf.log and logging were removed here:
1c51b8d

That would explain the uninstantiated logging object. I verified that we didn't break the commit above.

@vladimir-v-diaz
Copy link
Contributor

One reason why we might have missed the exceptions raised in init(): http://stackoverflow.com/questions/25374679/exception-in-python-exception-classs-str-missed

@awwad
Copy link
Contributor Author

awwad commented Mar 17, 2016

Yeah, exceptions in str() are sneaky.

@awwad
Copy link
Contributor Author

awwad commented Mar 17, 2016

WRT logging removal:
Right, I remember some traffic about this a while back. Should I remove the logging stuff again?

@vladimir-v-diaz
Copy link
Contributor

The issue that Alex fixed only occurs if you import log.py in init(). We now only import logging, so it should be fine. I tried to import 'tuf.conf', and the 'tuf.log' file is not created (as we wish).

@vladimir-v-diaz
Copy link
Contributor

Correction: only occurs if you import log.py

@awwad
Copy link
Contributor Author

awwad commented Mar 17, 2016

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. ^_^

@vladimir-v-diaz
Copy link
Contributor

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).

@vladimir-v-diaz vladimir-v-diaz merged commit 7cd20fe into theupdateframework:develop Mar 17, 2016
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.

Client: Freeze attack issue - expiry of metadata may not be detected
2 participants