Fix TUF rollback protection

.github/workflows/ci.yml

@@ -135,7 +135,7 @@ jobs:
         run: touch requirements.txt
 
       - name: Run the TUF conformance tests
-        uses: theupdateframework/tuf-conformance@94bcfb2b21c3dec514cbc0ba2afd225f2c5eb6d7
+        uses: theupdateframework/tuf-conformance@67f38b6718a25642fb489419260d0eddcef95a7a
         with:
           entrypoint: ${{ github.workspace }}/bin/tuf-conformance-entrypoint
           artifact-name: "test repositories ${{ matrix.ruby }} ${{ matrix.os }}"

lib/sigstore/tuf/error.rb

@@ -18,16 +18,21 @@
 
 module Sigstore::TUF
   class Error < ::Sigstore::Error
-    class LengthOrHashMismatch < Error; end
-    class ExpiredMetadata < Error; end
-    class EqualVersionNumber < Error; end
-    class BadVersionNumber < Error; end
+    # An error with a repository's state, such as a missing file.
+    class RepositoryError < Error; end
+
+    class LengthOrHashMismatch < RepositoryError; end
+    class ExpiredMetadata < RepositoryError; end
+    class BadVersionNumber < RepositoryError; end
+    class EqualVersionNumber < BadVersionNumber; end
+    class TooFewSignatures < RepositoryError; end
+
     class BadUpdateOrder < Error; end
-    class TooFewSignatures < Error; end
-    class MetaVersionLower < Error; end
-    class MetaVersionHigher < Error; end
     class InvalidData < Error; end
 
+    # An error occurred while attempting to download a file.
+    class DownloadError < Error; end
+
     class Fetch < Error; end
     class RemoteConnection < Fetch; end
 

lib/sigstore/tuf/file.rb

@@ -29,7 +29,7 @@ def verify_hashes(data, expected_hashed)
           actual_hash = Digest(algorithm.upcase).hexdigest(data)
           unless actual_hash == expected_hash
             raise Error::LengthOrHashMismatch,
-                  "Observed hash #{actual_hash} does not match expected hash #{expected_hash} (algorithm: #{algorithm})"
+                  "observed hash #{actual_hash} does not match expected hash #{expected_hash}"
           end
         end
       end
@@ -43,15 +43,17 @@ def verify_length(data, expected_length)
       end
 
       def validate_hashes(hashes)
+        raise ArgumentError, "hashes must be non-empty" if hashes.empty?
+
         hashes.each do |algorithm, hash|
-          raise "hashes items must be strings" unless algorithm.is_a?(String) && hash.is_a?(String)
+          raise TypeError, "hashes items must be strings" unless algorithm.is_a?(String) && hash.is_a?(String)
         end
       end
 
       def validate_length(length)
         return unless length.negative?
 
-        raise "length must be a non-negative integer, got #{length.inspect}"
+        raise ArgumentError, "length must be a non-negative integer, got #{length.inspect}"
       end
     end
   end

lib/sigstore/tuf/trusted_metadata_set.rb

@@ -18,6 +18,8 @@
 require_relative "root"
 require_relative "../internal/json"
 
+require "json"
+
 module Sigstore::TUF
   class TrustedMetadataSet
     include Sigstore::Loggable
@@ -95,9 +97,16 @@ def snapshot=(data, trusted: false)
 
       new_snapshot, = load_data(Snapshot, data, root)
 
-      if include?(Snapshot::TYPE) && (new_snapshot.version < snapshot.version)
-        raise Error::BadVersionNumber,
-              "snapshot version decreased"
+      # If an existing trusted snapshot is updated, check for rollback attack
+      if include?(Snapshot::TYPE)
+        snapshot.meta.each do |filename, file_info|
+          new_file_info = new_snapshot.meta[filename]
+          raise Error::RepositoryError, "new snapshot is missing info for #{filename}" unless new_file_info
+
+          if new_file_info.version < file_info.version
+            raise Error::BadVersionNumber, "expected #{filename} v#{new_file_info.version}, got v#{file_info.version}"
+          end
+        end
       end
 
       @trusted_set["snapshot"] = new_snapshot
@@ -118,25 +127,20 @@ def update_delegated_targets(data, role, parent_role)
 
       check_final_snapshot
 
-      delegator = @trusted_set.fetch(parent_role)
+      delegator = @trusted_set[parent_role]
       logger.debug { "Updating #{role} delegated by #{parent_role.inspect} to #{delegator.class}" }
       raise Error::BadUpdateOrder, "cannot load targets before delegator" unless delegator
 
-      logger.debug { "Updating #{role} delegated by #{parent_role}" }
-
-      meta = snapshot.meta.fetch("#{role}.json")
-      raise "No metadata for role: #{role}" unless meta
+      meta = snapshot.meta["#{role}.json"]
+      raise Error::RepositoryError, "no metadata for role #{role} in snapshot" unless meta
 
       meta.verify_length_and_hashes(data)
 
       new_delegate, = load_data(Targets, data, delegator, role)
       version = new_delegate.version
-      if (comp = version <=> meta.version).nonzero?
-        cls = comp.positive? ? Error::MetaVersionLower : Error::MetaVersionHigher
-        raise cls, "delegated targets version (#{version}) does not match meta version (#{meta.version})"
-      end
+      raise Error::BadVersionNumber, "expected #{role} v#{meta.version}, got v#{version}" if version != meta.version
 
-      raise Error::ExpiredMetadata, "expired delegated targets" if new_delegate.expired?(@reference_time)
+      raise Error::ExpiredMetadata, "new #{role} is expired" if new_delegate.expired?(@reference_time)
 
       @trusted_set[role] = new_delegate
       logger.debug { "Updated #{role} v#{version}" }
@@ -166,6 +170,8 @@ def load_data(type, data, delegator, role_name = nil)
       canonical_signed = Sigstore::Internal::JSON.canonical_generate(signed)
       delegator&.verify_delegate(role_name || type::TYPE, canonical_signed, signatures)
       [metadata, canonical_signed, signatures]
+    rescue JSON::ParserError => e
+      raise Error::InvalidData, "Failed to parse #{type}: #{e.message}"
     end
 
     def check_final_timestamp
@@ -179,13 +185,9 @@ def check_final_snapshot
       raise Error::ExpiredMetadata, "final snapshot.json is expired" if snapshot.expired?(@reference_time)
 
       snapshot_meta = timestamp.snapshot_meta
-      return unless snapshot.version != snapshot_meta.version
-
-      version = snapshot.version
-      return unless (comp = version <=> snapshot_meta.version).nonzero?
+      return if snapshot.version == snapshot_meta.version
 
-      cls = comp.positive? ? Error::MetaVersionLower : Error::MetaVersionHigher
-      raise cls, "snapshot version (#{version}) does not match meta version (#{snapshot_meta.version})"
+      raise Error::BadVersionNumber, "expected snapshot version #{snapshot_meta.version}, got #{snapshot.version}"
     end
   end
 end

lib/sigstore/tuf/updater.rb

@@ -39,12 +39,8 @@ def initialize(metadata_dir:, metadata_base_url:, target_base_url:, target_dir:,
         raise ArgumentError, "Unsupported envelope type: #{@config[:envelope_type].inspect}"
       end
 
-      begin
-        data = load_local_metadata("root")
-        @trusted_set = TrustedMetadataSet.new(data, "metadata", reference_time: Time.now)
-      rescue ::JSON::ParserError => e
-        raise "Invalid JSON in #{File.join(@dir, "root.json")}: #{e.class} #{e}"
-      end
+      data = load_local_metadata("root")
+      @trusted_set = TrustedMetadataSet.new(data, "metadata", reference_time: Time.now)
     end
 
     def refresh
@@ -115,21 +111,23 @@ def load_root
       upper_bound = lower_bound - 1 + @config.max_root_rotations
 
       lower_bound.upto(upper_bound) do |version|
-        data = download_metadata("root", version)
-        @trusted_set.root = data
-        persist_metadata("root", data)
+        data = download_metadata(Root::TYPE, version)
       rescue Error::UnsuccessfulResponse => e
+        logger.debug { "Failed to download root metadata v#{version}: #{e.class} #{e.message}" }
         break if %w[403 404].include?(e.response.code)
 
         raise
+      else
+        @trusted_set.root = data
+        persist_metadata(Root::TYPE, data)
       end
     end
 
     def load_timestamp
       begin
         data = load_local_metadata(Timestamp::TYPE)
         @trusted_set.timestamp = data
-      rescue Errno::ENOENT, Error::ExpiredMetadata, Error::TooFewSignatures => e
+      rescue Errno::ENOENT, Error::RepositoryError => e
         logger.debug "Local timestamp not valid as final: #{e.class} #{e.message}"
       end
 
@@ -147,9 +145,9 @@ def load_timestamp
 
     def load_snapshot
       data = load_local_metadata(Snapshot::TYPE)
-      @trusted_set.snapshot = data
+      @trusted_set.send(:snapshot=, data, trusted: true)
       logger.debug "Loaded snapshot from local metadata"
-    rescue Errno::ENOENT, Error::TooFewSignatures, Error::MetaVersionHigher => e
+    rescue Errno::ENOENT, Error::RepositoryError => e
       logger.debug "Local snapshot not valid as final: #{e.class} #{e.message}"
 
       snapshot_meta = @trusted_set.timestamp.snapshot_meta
@@ -161,19 +159,22 @@ def load_snapshot
     end
 
     def load_targets(role, parent_role)
-      return @trusted_set[role] if @trusted_set.include?(role)
+      if @trusted_set.include?(role)
+        logger.debug { "Returning cached targets for #{role}" }
+        return @trusted_set[role]
+      end
 
       begin
         data = load_local_metadata(role)
         @trusted_set.update_delegated_targets(data, role, parent_role).tap do
           logger.debug { "Loaded targets for #{role} from local metadata" }
         end
-      rescue Errno::ENOENT, Error::MetaVersionHigher, Error::MetaVersionLower, Error::TooFewSignatures => e
+      rescue Errno::ENOENT, Error::RepositoryError => e
         logger.debug { "No local targets for #{role}, fetching: #{e.class} #{e.message}" }
 
         snapshot = @trusted_set.snapshot
-        metainfo = snapshot.meta.fetch("#{role}.json")
-        raise "No metadata for role: #{role}" unless metainfo
+        metainfo = snapshot.meta["#{role}.json"]
+        raise Error::RepositoryError, "role #{role} was delegated but is not part of snapshot" unless metainfo
 
         version = metainfo.version if @trusted_set.root.consistent_snapshot
         data = download_metadata(role, version)

test/sigstore/tuf/trusted_metadata_set_test.rb

@@ -42,7 +42,9 @@ class Sigstore::TUF::TrustedMetadataSetTest < Test::Unit::TestCase
           "snapshot.json" => {
             "version" => 137,
             "length" => 104,
-            "hashes" => {}
+            "hashes" => {
+              "sha256" => "5c4853e87c01a1621f410ad55f80bedb6c5b7e55c1f6e59d739769c0b54cf558"
+            }
           }
         }
       },