Skip to content

Fix 736: Rebuild roledb in refresh() even if root unchanged #737

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.

Sign up for GitHub

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 10 commits into from
Jun 12, 2018
Merged

Fix 736: Rebuild roledb in refresh() even if root unchanged #737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e18d3db
Fix 736: Rebuild roledb in refresh() even if root unchanged
awwad May 30, 2018
47dbdba
Discuss the special case where metadata is marked obselete and deleted
vladimir-v-diaz Jun 8, 2018
18a5aa4
Make slight revision to comment about always refreshing top-level met…
vladimir-v-diaz Jun 11, 2018
43efa42
Document new refresh() change in its docstring
vladimir-v-diaz Jun 11, 2018
e92680f
Fix test_multiple_repositories_integration and edit whitespace in upd…
vladimir-v-diaz Jun 11, 2018
9b6c91a
Refresh delegated metadata to fix test_mix_and_match_attack.py failure
vladimir-v-diaz Jun 11, 2018
f493161
Wrap the refresh() line that sets consistent snapshot
vladimir-v-diaz Jun 11, 2018
621ec3a
Add test condition for the persistent freeze attack
vladimir-v-diaz Jun 11, 2018
6373c26
Merge branch 'develop' into fix_736_freeze
vladimir-v-diaz Jun 11, 2018
e9cd01e
DOC: Clarify comment on freeze attack test
awwad Jun 11, 2018
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
98 changes: 98 additions & 0 deletions tests/test_indefinite_freeze_attack.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,5 +401,103 @@ def test_with_tuf(self):
' Freeze attack successful.')




# Test 3 Begin:
#
# Serve the client expired Snapshot. The client should reject the given,
# expired Snapshot and the locally trusted one, which should now be out of
# date.
# After the attack, attempt to re-issue a valid Snapshot to verify that
# the client is still able to update. A bug previously caused snapshot
# expiration or replay to result in an indefinite freeze; see
# github.com/theupdateframework/tuf/issues/736
repository = repo_tool.load_repository(self.repository_directory)

ts_key_file = os.path.join(self.keystore_directory, 'timestamp_key')
snapshot_key_file = os.path.join(self.keystore_directory, 'snapshot_key')
timestamp_private = repo_tool.import_ed25519_privatekey_from_file(
ts_key_file, 'password')
snapshot_private = repo_tool.import_ed25519_privatekey_from_file(
snapshot_key_file, 'password')

repository.timestamp.load_signing_key(timestamp_private)
repository.snapshot.load_signing_key(snapshot_private)

# Set ts to expire in 1 month.
ts_expiry_time = time.time() + 2630000

# Set snapshot to expire in 1 second.
snapshot_expiry_time = time.time() + 1

ts_datetime_object = tuf.formats.unix_timestamp_to_datetime(
int(ts_expiry_time))
snapshot_datetime_object = tuf.formats.unix_timestamp_to_datetime(
int(snapshot_expiry_time))
repository.timestamp.expiration = ts_datetime_object
repository.snapshot.expiration = snapshot_datetime_object
repository.writeall()

# Move the staged metadata to the "live" metadata.
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'))

# Wait just long enough for the Snapshot metadata (which is now on the
# repository) to expire.
time.sleep(max(0, snapshot_expiry_time - time.time() + 1))


try:
# We expect the following refresh() to raise a NoWorkingMirrorError.
self.repository_updater.refresh()

except tuf.exceptions.NoWorkingMirrorError as e:
# NoWorkingMirrorError indicates that we did not find valid, unexpired
# metadata at any mirror. That exception class preserves the errors from
# each mirror. We now assert that for each mirror, the particular error
# detected was that metadata was expired (the Snapshot we manually
# expired).
for mirror_url, mirror_error in six.iteritems(e.mirror_errors):
self.assertTrue(isinstance(mirror_error, tuf.exceptions.ExpiredMetadataError))
self.assertTrue(mirror_url.endswith('snapshot.json'))

else:
self.fail('TUF failed to detect expired, stale Snapshot metadata.'
' Freeze attack successful.')

# The client should have rejected the malicious Snapshot metadata, and
# distrusted the local snapshot file that is no longer valid.
self.assertTrue('snapshot' not in self.repository_updater.metadata['current'])
self.assertEqual(sorted(['root', 'targets', 'timestamp']),
sorted(self.repository_updater.metadata['current']))

# Verify that the client is able to recover from the malicious Snapshot.
# Re-sign a valid Snapshot file that the client should accept.
repository = repo_tool.load_repository(self.repository_directory)

repository.timestamp.load_signing_key(timestamp_private)
repository.snapshot.load_signing_key(snapshot_private)

# Set snapshot to expire in 1 month.
snapshot_expiry_time = time.time() + 2630000

snapshot_datetime_object = tuf.formats.unix_timestamp_to_datetime(
int(snapshot_expiry_time))
repository.snapshot.expiration = snapshot_datetime_object
repository.writeall()

# Move the staged metadata to the "live" metadata.
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'))

self.repository_updater.refresh()
self.assertTrue('snapshot' in self.repository_updater.metadata['current'])
self.assertEqual(sorted(['root', 'targets', 'timestamp', 'snapshot']),
sorted(self.repository_updater.metadata['current']))



Copy link
Contributor Author

@awwad awwad Jun 11, 2018

Choose a reason for hiding this comment

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

Test looks good.

if __name__ == '__main__':
unittest.main()
9 changes: 7 additions & 2 deletions tests/test_multiple_repositories_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,11 +209,16 @@ def test_update(self):
self.assertEqual(sorted(['role1', 'root', 'snapshot', 'targets', 'timestamp']),
sorted(tuf.roledb.get_rolenames('test_repository2')))

# Note: refresh() resets the known metadata and updates the latest
# top-level metadata.
self.repository_updater.refresh()

self.assertEqual(sorted(['role1', 'root', 'snapshot', 'targets', 'timestamp']),
self.assertEqual(sorted(['root', 'snapshot', 'targets', 'timestamp']),
sorted(tuf.roledb.get_rolenames('test_repository1')))
self.assertEqual(sorted(['role1', 'root', 'snapshot', 'targets', 'timestamp']),
Copy link
Contributor Author

@awwad awwad Jun 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'm confused by this change. How did this result from the PR?

Edit: ah, probably from the targets_of_role change...?

Copy link
Contributor

@vladimir-v-diaz vladimir-v-diaz Jun 11, 2018

Choose a reason for hiding this comment

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

refresh() rebuilds the roledb and only refreshes top-level metadata. See line 212.


# test_repository2 wasn't refreshed and should still know about delegated
# roles.
self.assertEqual(sorted(['root', 'role1', 'snapshot', 'targets', 'timestamp']),
sorted(tuf.roledb.get_rolenames('test_repository2')))

# 'role1.json' should be downloaded, because it provides info for the
Expand Down
38 changes: 24 additions & 14 deletions tuf/client/updater.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -873,8 +873,9 @@ def _rebuild_key_and_role_db(self):
Non-public method that rebuilds the key and role databases from the
currently trusted 'root' metadata object extracted from 'root.json'.
This private method is called when a new/updated 'root' metadata file is
loaded. This method will only store the role information of the
top-level roles (i.e., 'root', 'targets', 'snapshot', 'timestamp').
loaded or when updater.refresh() is called. This method will only store
the role information of the top-level roles (i.e., 'root', 'targets',
'snapshot', 'timestamp').

<Arguments>
None.
Expand Down Expand Up @@ -903,6 +904,7 @@ def _rebuild_key_and_role_db(self):
# reloading delegated roles is not required here.
tuf.keydb.create_keydb_from_root_metadata(self.metadata['current']['root'],
self.repository_name)

tuf.roledb.create_roledb_from_root_metadata(self.metadata['current']['root'],
self.repository_name)

Expand Down Expand Up @@ -1081,9 +1083,21 @@ def refresh(self, unsafely_update_root_if_necessary=True):
# do we blindly trust the downloaded root metadata here?
self._update_root_metadata(root_metadata)

# Ensure that the role and key information of the top-level roles is the
# latest. We do this whether or not Root needed to be updated, in order to
# ensure that, e.g., the entries in roledb for top-level roles are
# populated with expected keyid info so that roles can be validated. In
# certain circumstances, top-level metadata might be missing because it was
# marked obsolete and deleted after a failed attempt, and thus we should
# refresh them here as a protective measure. See Issue #736.
self._rebuild_key_and_role_db()
self.consistent_snapshot = \
self.metadata['current']['root']['consistent_snapshot']

# Use default but sane information for timestamp metadata, and do not
# require strict checks on its required length.
self._update_metadata('timestamp', DEFAULT_TIMESTAMP_UPPERLENGTH)

# TODO: After fetching snapshot.json, we should either verify the root
# fileinfo referenced there matches what was fetched earlier in
# _update_root_metadata() or make another attempt to download root.json.
Expand Down Expand Up @@ -1663,10 +1677,12 @@ def _update_metadata(self, metadata_role, upperbound_filelength, version=None):
"""
<Purpose>
Non-public method that downloads, verifies, and 'installs' the metadata
belonging to 'metadata_role'. Calling this method implies the metadata
has been updated by the repository and thus needs to be re-downloaded.
The current and previous metadata stores are updated if the newly
downloaded metadata is successfully downloaded and verified.
belonging to 'metadata_role'. Calling this method implies that the
'metadata_role' on the repository is newer than the client's, and thus
needs to be re-downloaded. The current and previous metadata stores are
updated if the newly downloaded metadata is successfully downloaded and
verified. This method also assumes that the store of top-level metadata
is the latest and exists.

<Arguments>
metadata_role:
Expand Down Expand Up @@ -1773,12 +1789,6 @@ def _update_metadata(self, metadata_role, upperbound_filelength, version=None):
self.metadata['current'][metadata_role] = updated_metadata_object
self._update_versioninfo(metadata_filename)

# Ensure the role and key information of the top-level roles is also updated
# according to the newly-installed Root metadata.
if metadata_role == 'root':
self._rebuild_key_and_role_db()
self.consistent_snapshot = updated_metadata_object['consistent_snapshot']




Expand Down Expand Up @@ -2584,11 +2594,11 @@ def targets_of_role(self, rolename='targets'):
# Raise 'securesystemslib.exceptions.FormatError' if there is a mismatch.
securesystemslib.formats.RELPATH_SCHEMA.check_match(rolename)

self._refresh_targets_metadata(refresh_all_delegated_roles=True)

Copy link
Contributor Author

@awwad awwad Jun 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Why is refresh_all_delegated_roles necessary? This could be quite a few roles.

Next point is a bit complex:
targets_of_role remains a strange function to me if called on anything other than the Targets role itself. If the indicated role is a delegated role, it just isn't clear what it means to obtain trustworthy target info for whatever files are listed by a trustworthy version of that role. (Whose word are we taking on the latter?)
Perhaps the best we can do is to: obtain some not-validated or partially-validated version of the delegated role (because delegation pathways depend on file paths and we haven't specified one), get the list of targets specified by that not-validated role file, and then properly obtain trustworthy target info for those file paths (whether or not they are ultimately provided by the same role?).

This more complex point plays into the correct way to run targets_of_role on a delegated role.

Copy link
Contributor

@vladimir-v-diaz vladimir-v-diaz Jun 11, 2018

Choose a reason for hiding this comment

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

My thinking is that all_targets() and targets_of_roles() should be for clients that do not know ahead of time which targets are available on the repo, or provided by a particular role.

Community repos will certainly use get_one_valid_targetinfo(), but we might want to provide all_targets() and targets_of_roles() for simpler use cases / adopters.

Copy link
Contributor

@vladimir-v-diaz vladimir-v-diaz Jun 11, 2018

Choose a reason for hiding this comment

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

After an offline discussion, @awwad and I came to the conclusion that it's better to not support all_targets() and targets_of_role(). We'll open a separate issue to remove these two functions.

if not tuf.roledb.role_exists(rolename, self.repository_name):
raise tuf.exceptions.UnknownRoleError(rolename)

self._refresh_targets_metadata(rolename)

return self._targets_of_role(rolename, skip_refresh=True)


Expand Down