Skip to content

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

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

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 116 additions & 4 deletions tests/test_indefinite_freeze_attack.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@
than verifying text output), use pre-generated repository files, and
discontinue use of the old repository tools. -vladimir.v.diaz

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

available), its expiration must still be detected. This add'l test
complains if such does not occur, and accompanies code to detect it.
-sebastien.awwad

<Copyright>
See LICENSE for licensing information.

Expand Down Expand Up @@ -171,6 +179,7 @@ def tearDown(self):


def test_without_tuf(self):
# Test 1:
# Scenario:
# 'timestamp.json' specifies the latest version of the repository files.
# A client should only accept the same version of this file up to a certain
Expand Down Expand Up @@ -215,9 +224,108 @@ def test_without_tuf(self):
# Verify 'download_fileinfo' is equal to the current local file.
self.assertEqual(download_fileinfo, fileinfo)

# Test 2:
# See description of scenario in Test 2 in the test_with_tuf method.
# Without TUF, Test 2 is tantamount to Test 1, and so it is not tested
# again here.


def test_with_tuf(self):
# Two tests are conducted here.
# Test 1: If an expired timestamp is downloaded, is it recognized as such?
# Test 2: If we find that the timestamp acquired from a mirror indicates
# that there is no new snapshot file, and our current snapshot
# file is expired, is it recognized as such?
#
# Test 2 addresses this issue:
# https://github.com/theupdateframework/tuf/issues/322
#
# If time has passed and our snapshot (or any targets role) is expired, and
# the mirror whose timestamp we fetched doesn't indicate the existence of a
# new snapshot version, we still need to check that it's expired and notify
# the software update system / application / user. This test creates that
# scenario. The correct behavior is to raise an exception.
#
# Background: Expiration checks were previously conducted
# (ensure_not_expired) when the metadata file was downloaded. If no new
Copy link
Contributor

Choose a reason for hiding this comment

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

To make it easier to locate for readers unfamiliar with the code, consider specifying the module or full function name: updater._ensure_not_expired().

# metadata file was downloaded, no expiry check would occurs. (Exception:
# 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.


timestamp_path = os.path.join(self.repository_directory, 'metadata',
'timestamp.json')

# Modify the timestamp file on the remote repository. 'timestamp.json'
# must be properly updated and signed with 'repository_tool.py', otherwise
# the client will reject it as invalid metadata. The resulting
# 'timestamp.json' should be valid metadata, but expired (as intended).
repository = repo_tool.load_repository(self.repository_directory)

key_file = os.path.join(self.keystore_directory, 'timestamp_key')
timestamp_private = repo_tool.import_rsa_privatekey_from_file(key_file,
'password')

repository.timestamp.load_signing_key(timestamp_private)

# Load snapshot keys.
key_file = os.path.join(self.keystore_directory, 'snapshot_key')
snapshot_private = repo_tool.import_rsa_privatekey_from_file(key_file,
'password')
repository.snapshot.load_signing_key(snapshot_private)

# Load root keys.
key_file = os.path.join(self.keystore_directory, 'root_key')
root_private = repo_tool.import_rsa_privatekey_from_file(key_file,
'password')
repository.root.load_signing_key(root_private)

time.sleep(0.1)

# Expire snapshot in 7s
datetime_object = tuf.formats.unix_timestamp_to_datetime(int(time.time() +
7))
repository.snapshot.expiration = datetime_object

# Now write to the repository
repository.write()
# And move the staged metadata to the "live" metadata.
time.sleep(0.1)
shutil.rmtree(os.path.join(self.repository_directory, 'metadata'))
shutil.copytree(os.path.join(self.repository_directory, 'metadata.staged'),
os.path.join(self.repository_directory, 'metadata'))

# Refresh metadata on the client. For this refresh, all data is not expired.
time.sleep(0.1)
logger.info('Test: Refreshing #1 - Initial metadata refresh occurring.')
self.repository_updater.refresh()
logger.info("Test: Refreshed #1 - Initial metadata refresh occurred. Now "
"sleeping 6s.")

# Sleep for at least 7 seconds to ensure 'repository.snapshot.expiration'
# is reached.
time.sleep(7)
logger.info("Test: Refreshing #2 - Now trying to refresh again after "
" snapshot expiry.")
try:
self.repository_updater.refresh() # We expect this to fail!

except tuf.ExpiredMetadataError as e:
logger.info("Test: Refresh #2 - failed as expected. Stale-expired "
"snapshot case generated a tuf.ExpiredMetadataError exception"
". Test pass.")
#I think that I only expect tuf.ExpiredMetadata error here. A
# NoWorkingMirrorError indicates something else in this case - unavailable
# repo, for example.
else:
self.fail("TUF failed to detect expired stale snapshot metadata. Freeze.")
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be clearer to use the word "locally" here as well. The other case would be when the server serves the client stale metadata.

To be more precise: "Freeze attack was successful" instead of just "Freeze"





# Part 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

These labels are a bit confusing (what is Part 1? Is this test 1?). It's not immediately obvious where test 1 ends and test 2 begins, but I think Test 2 is started first (which is unexpected).

Copy link
Contributor

Choose a reason for hiding this comment

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

Test 1 / Part 1 can be cleaned up a bit (e.g., we don't have to reload the repository, reload the keys for the Timestamp role, etc.)


# The same scenario outlined in test_without_tuf() is followed here, except
# with a TUF client. The TUF client performs a refresh of top-level
# metadata, which also includes 'timestamp.json'.
Expand All @@ -237,8 +345,8 @@ def test_with_tuf(self):

repository.timestamp.load_signing_key(timestamp_private)

# expire in 1 second.
datetime_object = tuf.formats.unix_timestamp_to_datetime(int(time.time() + 1))
# expire in 4 seconds.
datetime_object = tuf.formats.unix_timestamp_to_datetime(int(time.time() + 4))
repository.timestamp.expiration = datetime_object
repository.write()

Expand All @@ -248,15 +356,19 @@ def test_with_tuf(self):
os.path.join(self.repository_directory, 'metadata'))

# Verify that the TUF client detects outdated metadata and refuses to
# continue the update process. Sleep for at least 2 seconds to ensure
# continue the update process. Sleep for at least 4 seconds to ensure
# 'repository.timestamp.expiration' is reached.
time.sleep(2)
time.sleep(4)
try:
self.repository_updater.refresh()

except tuf.NoWorkingMirrorError as e:
for mirror_url, mirror_error in six.iteritems(e.mirror_errors):
self.assertTrue(isinstance(mirror_error, tuf.ExpiredMetadataError))

else:
self.fail("TUF failed to detect expired stale timestamp metadata. Freeze "
"attack successful.")


if __name__ == '__main__':
Expand Down
62 changes: 55 additions & 7 deletions tuf/client/updater.py
Original file line number Diff line number Diff line change
Expand Up @@ -643,6 +643,13 @@ def refresh(self, unsafely_update_root_if_necessary=True):
logger.info('An expired Root metadata was loaded and must be updated.')
raise

# If an exception is raised during the metadata update attempts, we will
# attempt to update root metadata once by recursing with a special argument
# to avoid further recursion. We use this bool and pull the recursion out
# of the except block so as to avoid unprintable nested NoWorkingMirrorError
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 curious as to why we are getting an unprintable NoWorkingMirrorError exception. Is this a bug? I'm not sure if the retry_once boolean is a workaround for the unprintable exceptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using a second boolean (unsafely_update_root_if_necessary being the first one) to take care of retries, would it work to stick the following in the two places that retry_once is used?

self._update_metadata('root', DEFAULT_ROOT_UPPERLENGTH)
self.refresh(unsafely_update_root_if_necessary=False)

I find it confusing to use two boolean variables for the same thing, unless its an issue with printing nested exceptions. If it's the latter, I'd like to fix the source of the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to reproduce the issue again. Resolved it by moving that out of the except block, but perhaps there's a better way.

Was this:

======================================================================
ERROR: test_with_tuf (__main__.TestIndefiniteFreezeAttack)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/s/w/tuf/tuf/client/updater.py", line 668, in refresh
    referenced_metadata='timestamp')
  File "/Users/s/w/tuf/tuf/client/updater.py", line 1696, in _update_metadata_if_changed
    expected_versioninfo['version'], compression)
  File "/Users/s/w/tuf/tuf/client/updater.py", line 1490, in _update_metadata
    compression_algorithm)
  File "/Users/s/w/tuf/tuf/client/updater.py", line 1211, in _get_metadata_file
    raise tuf.NoWorkingMirrorError(file_mirror_errors)
tuf.NoWorkingMirrorError: <unprintable NoWorkingMirrorError object>

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "test_indefinite_freeze_attack.py", line 314, in test_with_tuf
    self.repository_updater.refresh()
  File "/Users/s/w/tuf/tuf/client/updater.py", line 696, in refresh
    self.refresh(unsafely_update_root_if_necessary=False)
  File "/Users/s/w/tuf/tuf/client/updater.py", line 668, in refresh
    referenced_metadata='timestamp')
  File "/Users/s/w/tuf/tuf/client/updater.py", line 1696, in _update_metadata_if_changed
    expected_versioninfo['version'], compression)
  File "/Users/s/w/tuf/tuf/client/updater.py", line 1490, in _update_metadata
    compression_algorithm)
  File "/Users/s/w/tuf/tuf/client/updater.py", line 1211, in _get_metadata_file
    raise tuf.NoWorkingMirrorError(file_mirror_errors)
tuf.NoWorkingMirrorError: <unprintable NoWorkingMirrorError object>

----------------------------------------------------------------------
Ran 2 tests in 16.465s

FAILED (errors=1)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please verify that _get_metadata_file() is correctly using logger.exception()?

See: https://github.com/theupdateframework/tuf/blob/develop/tuf/client/updater.py#L1340-L1343

logger.exception() should only be called from an exception handler. Python's documentation: https://docs.python.org/2/library/logging.html#logging.Logger.exception

Copy link
Contributor Author

Choose a reason for hiding this comment

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

# exceptions.
retry_once = False

# Use default but sane information for timestamp metadata, and do not
# require strict checks on its required length.
try:
Expand All @@ -652,19 +659,53 @@ def refresh(self, unsafely_update_root_if_necessary=True):
self._update_metadata_if_changed('root')
self._update_metadata_if_changed('targets')

# There are two distinct error scenarios that can rise from the
# _update_metadata_if_changed calls in the try block above:
#
# - tuf.NoWorkingMirrorError:
#
# If a change to a metadata file IS detected in an
# _update_metadata_if_changed call, but we are unable to download a
# valid (not expired, properly signed, valid) version of that metadata
# file, and unexpired version of that metadata file, a
# tuf.NoWorkingMirrorError rises to this point.
#
# - tuf.ExpiredMetadataError:
#
# If, on the other hand, a change to a metadata file IS NOT detected in
# a given _update_metadata_if_changed call, but we observe that the
# version of the metadata file we have on hand is now expired, a
# tuf.ExpiredMetadataError exception rises to this point.
#
except tuf.NoWorkingMirrorError as e:
if unsafely_update_root_if_necessary:
message = 'Valid top-level metadata cannot be downloaded. Unsafely '+\
'update the Root metadata.'
logger.info(message)

self._update_metadata('root', DEFAULT_ROOT_UPPERLENGTH)
self.refresh(unsafely_update_root_if_necessary=False)

logger.info('Valid top-level metadata cannot be downloaded. Unsafely '
'update the Root metadata.')
retry_once = True

else:
raise

except tuf.ExpiredMetadataError as e:
if unsafely_update_root_if_necessary:
logger.info('No changes were detected from the mirrors for a given role'
', and that metadata that is available on disk has been found to be '
'expired. Trying to update root in case of foul play.')
retry_once = True

# The caller explicitly requested not to unsafely fetch an expired Root.
else:
logger.info('No changes were detected from the mirrors for a given role'
', and that metadata that is available on disk has been found to be '
'expired. Your metadata is out of date.')
raise

# Update failed and we aren't already in a retry. Try once more after
# updating root.
if retry_once:
self._update_metadata('root', DEFAULT_ROOT_UPPERLENGTH)
self.refresh(unsafely_update_root_if_necessary=False)




Expand Down Expand Up @@ -1578,6 +1619,13 @@ def _update_metadata_if_changed(self, metadata_role,
expected_versioninfo):
logger.info(repr(uncompressed_metadata_filename) + ' up-to-date.')

# Since we have not downloaded a new version of this metadata, we
# should check to see if our version is stale and notify the user
# if so. This raises tuf.ExpiredMetadataError if the metadata we
# have is expired. Resolves issue #322.
self._ensure_not_expired(self.metadata['current'][metadata_role],
metadata_role)

return

logger.debug('Metadata ' + repr(uncompressed_metadata_filename) + ' has changed.')
Expand Down