Summary

@nathand reported a security vulnerability to HackerOne that allowed a user to determine redacted or private information through search results in very specific cases. We applaud @nathand's thoroughness and effort for disclosing this security vulnerability to us. HackerOne conducted a root cause analysis of the vulnerability and determined that it affected a few publicly disclosed reports. No signs of exploitation were found.

Root cause analysis

To analyze the root cause, it is important to understand the timeline first of the security vulnerability that was reported.

The engineering team that implemented the search functionality designed the feature in a way that the backfill should get the search vectors in the exact same state as when the report would be submitted after the backfill. The backfill ran before Hacktivity Search was released to the public. One of the architectural decisions the team made to keep search vectors up to date, was by hooking into the after_commit hook in the Report model. This hook would then validate whether the vector had to be updated. Below is an excerpt of that code.

class Report < ActiveRecord::Base
  # ...
  after_commit do
    update_hacktivity_search_content_async
  end

  # ...

  def update_hacktivity_search_content_async
    return unless should_update_hacktivity_search_content?

    # schedule a job to update the vector
  end

  def should_update_hacktivity_search_content?
    new? ||
      previous_changes.keys.include?('title') ||
      previous_changes.keys.include?('vulnerability_information') ||
      previous_changes.keys.include?('weakness_id') ||
      previous_changes.keys.include?('structured_scope_id') ||
      previous_changes.keys.include?('team_id') ||
      previous_changes.keys.include?('public_view') ||
      previous_changes.keys.include?('disclosed_at')
  end

  # ...
end

As can be seen in the code, whenever one of the attributes mentioned in the should_update_hacktivity_search_content method would be changed, an asynchronous job would be scheduled to update the vector. This meant that the team didn't have to go through every code path and make sure they triggered the correct job to update the vector. At HackerOne, usage of ActiveRecord hooks is generally discouraged due to the complexity it adds to a request or transaction lifecycle.

The team looked into the behaviors of the previous_changes hash in combination with the after_commit hook. Small code snippets were used to track down the exact behavior. Some of those code examples are shared below.

Running the code below invokes an update of the vector, as expected:

report = Report.find 123
report.update title: 'new title'

Because the after_commit hook is used, it'll execute the code after the transaction has been completed OR immediately when the SQL query is not ran in a transaction block. The following code, when run on a Rails console, will call the hook twice:

report = Report.find 123
report.update title: 'new title'
report.update vulnerability_information: 'new information'

However, when the code is ran in a transaction block, the Postgres state and Ruby memory state start to diverge. This means that the following code will only trigger the hook once:

ActiveRecord::Base.transaction do
  report = Report.find 123
  report.update title: 'new title'
  report.update vulnerability_information: 'new information'
end

It is important to know that every request (except show and index actions) is automatically wrapped in a transaction. Subsequently, a number of service objects explicitly create transactions. The behavior described above does not change with nested transactions.

This all matters because a code path that updates a single Report object multiple times in a single transaction may cause the after_commit hook to miss columns that the engineers expected it to be stored in the previous_changes hash. This is especially confusing because in integration tests for some interactions, individual service object are tested, which are not necessarily wrapped in a transaction. This means the behavior in the test suite was not representative of the behavior of other environments.

When conducting the root cause analysis, HackerOne established a code pattern that could be used to identify code paths that update Report models multiple times during a request lifecycle. A number of code paths were identified, including the scenario where a team member requested disclosure, the reported agreed, and then the team member would update the title of the report. Below is an excerpt of that code path.

app/controllers/reports_controller.rb

class ReportsController < ApplicationController
  # ...

  def update
    report = Report.find_by(id: params[:id])

    # ...
        interact Interactors::Reports::UpdateTitle, report: report, title: params[:title]

    # ...
  end

  # ...
end

The controller dispatches the interaction to the UpdateTitle service object. Because it's called through a controller action, the interaction is automatically wrapped in a transaction.

app/backend/interactors/reports/update_title.rb

module Interactors
  module Reports
    class UpdateTitle < HackeroneInteractor
      # ...

      def execute
        old_title = report.title

        return unless report.update(title: title) # <-- FIRST UPDATE CALL

        interact ::Interactors::Activities::Create, # <-- LOGGING THE TITLE CHANGE
          # ...
      end

      # ...
    end
  end
end

The interaction above contains the first update call. If the title is updated, an activity will be created to track the change.

app/backend/interactors/activities/create.rb

module Interactors
  module Activities
    class Create < HackeroneInteractor
      # ...

      def execute
        activity = nil

        ActiveRecord::Base.transaction do # <-- NESTED TRANSACTION
          activity = interact Commands::Activities::Create,
            # ...

          if activity.persisted?
            if subject.is_a? Report
              # ...

              if !activity.internal? &&
                  current_user.is_a?(User) &&
                  current_user.member_of_team?(subject.team) &&
                  !activity.is_a?(::Activities::BugFiled)
                subject.stop_timer Report::TIMER_FIRST_PROGRAM_RESPONSE
                subject.save! # <-- SECOND UPDATE
        # ...
      end
    end
  end
end

Following the logic above, subject.save! is called when the program member updates the title. The TIMER_FIRST_PROGRAM_RESPONSE had already stopped for report #413412, but calling save! on a model without changes will still clear out the previous_changes hash. This means that when the transaction block is committed, previous_changes will be an empty hash -- resulting in the job to update the vector not to be scheduled. This proved the security vulnerability as reported by @nathand.

When going through other code paths, a functional error was discovered, too. After every created activity in the backend of HackerOne, a side effect is called that updates a cache on the Report model to track certain dates; for example, when the last public activity was posted and when the last internal activity was posted. Because this was called every time the Interactors::Activities::Create service object was invoked, the previous_changes hash would typically only contain those attributes and disregard any other attribute that was updated in the request. This resulted in a functional bug where the vector wouldn't be updated at all and default to the name of the program, handle of the program, and the username of the reporter.

Remediation

The engineering team wrote a regression spec to prove the security vulnerability and functional bug. It turned out that a number of other engineers outside of HackerOne have run into similar problems, letting them to develop a library that solves this problem. They have proposed to merge this change back into ActiveRecord and make it a first class citizen of the Rails / ActiveRecord family. HackerOne has leveraged this library to mitigate the reported security vulnerability.