Skip to content

Conversation

@DJAndries
Copy link
Collaborator

Resolves brave/brave-browser#40099

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@DJAndries DJAndries requested a review from a team as a code owner July 30, 2024 03:53
@github-actions
Copy link
Contributor

[puLL-Merge] - brave/brave-core@24908

Description

This PR enhances the security and functionality of the P3A (Privacy-Preserving Product Analytics) system in Brave, particularly focusing on the STAR (Secure Telemetry Analysis for Reporting) randomness server interaction. It introduces a new network service observer for handling SSL certificate verification, updates the attestation process, and refines the randomness server communication.

Possible Issues

  1. The removal of support for the old user data scheme in attestation documents may cause compatibility issues with older systems or deployments.
  2. The changes to the certificate verification process might potentially break existing connections if not properly tested across all scenarios.

Security Hotspots

  1. The introduction of StarURLLoaderNetworkServiceObserver for handling SSL certificate errors needs careful review to ensure it doesn't introduce vulnerabilities in the certificate validation process.
  2. The changes in AttestServer and VerifyRandomnessCert functions should be thoroughly tested to confirm they maintain or improve the security of the attestation process.
Changes

Changes

  1. components/p3a/BUILD.gn:

    • Added new files star_url_loader_network_service_observer.cc and star_url_loader_network_service_observer.h to the build.
  2. components/p3a/features.cc:

    • Updated the feature flag name for Constellation enclave attestation.
  3. components/p3a/nitro_utils/attestation.cc:

    • Removed old-style user data handling.
    • Updated RequestAndVerifyAttestationDocument to include a new attestation_network_service_observer parameter.
    • Modified VerifyUserDataKey to only support the new multihash format.
  4. components/p3a/nitro_utils/attestation.h:

    • Updated function signature to include attestation_network_service_observer.
  5. components/p3a/nitro_utils/attestation_unittest.cc:

    • Removed tests for old user data format.
  6. components/p3a/p3a_config.cc:

    • Added logic to use a different randomness host prefix based on whether STAR attestation is disabled.
  7. components/p3a/p3a_service.cc and p3a_service.h:

    • Removed DisableStarAttestationForTesting method.
  8. components/p3a/p3a_service_unittest.cc:

    • Updated test setup to disable STAR attestation.
  9. components/p3a/star_randomness_meta.cc and star_randomness_meta.h:

    • Introduced StarURLLoaderNetworkServiceObserver for handling network requests.
    • Updated attestation and randomness server communication to use the new observer.
    • Refactored certificate verification process.
  10. components/p3a/star_randomness_points.cc:

    • Updated to use the new network service observer for randomness requests.
  11. New files: star_url_loader_network_service_observer.cc and star_url_loader_network_service_observer.h:

    • Implemented a new class to handle URL loader network service observations, including SSL certificate error handling and other network-related callbacks.

These changes significantly enhance the security and flexibility of the P3A system's interaction with the STAR randomness server, particularly in terms of SSL certificate handling and attestation processes.

@DJAndries DJAndries force-pushed the p3a-star-attestation-v2 branch from 0fc89aa to c7b10f4 Compare July 30, 2024 21:15
@iefremov
Copy link
Contributor

@DJAndries have you filed a security review for this?

@DJAndries
Copy link
Collaborator Author

@DJAndries have you filed a security review for this?

not yet, i plan to raise an issue shortly


namespace p3a {

class StarURLLoaderNetworkServiceObserver
Copy link
Contributor

Choose a reason for hiding this comment

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

class-level comment

@iefremov
Copy link
Contributor

looks good, but can we write a test?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow self-signed certificates for P3A star-randsrv requests

5 participants