Security review of secure signing components for package manager

Created on 4 April 2023, about 1 year ago
Updated 6 April 2024, 3 months ago

Problem/Motivation

A security audit of the key components of the package signing infrastructure is a requirement before package manager can be committed as alpha.

The most essential non-Drupal components should be reviewed by an independent security vendor

  1. Rugged TUF server
  2. PHP TUF client library
    • An integration test between these two components.
  3. Drupal.org's infrastructure integrating Rugged
    • The collection of docker containers and their config.
    • Key handling.
    • Recommendation for key rotation schedule
  4. Composer integration plugin for PHP TUF
  5. The core satis mirror and signing process

Drupal-specific scope

  1. Composer stager (not TUF related, but relevant to the security surface area)
  2. Package manager - api for auto-updates and project browser
  3. Automatic updates (which calls composer stager, to call composer audit)
    • Decision tree about when and whether to update.
  4. Project browser may not have extra scope necessary to audit

Remaining tasks

  • - scope decided at DC Pittsburgh 2023
  • Engage a non-Drupal security audit consultant or firm with relevant experience
  • Engage a Drupal security audit consultant or firm with relevant experience
  • Make changes according to recommendations
๐ŸŒฑ Plan
Status

Needs work

Version

11.0 ๐Ÿ”ฅ

Component
Updateย  โ†’

Last updated 3 days ago

  • Maintained by
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States @tedbow
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States @dww
Created by

๐Ÿ‡บ๐Ÿ‡ธUnited States hestenet Portland, OR ๐Ÿ‡บ๐Ÿ‡ธ

Live updates comments and jobs are added and updated live.
  • Security

    It is used for security vulnerabilities which do not need a security advisory. For example, security issues in projects which do not have security advisory coverage, or forward-porting a change already disclosed in a security advisory. See Drupalโ€™s security advisory policy for details. Be careful publicly disclosing security vulnerabilities! Use the โ€œReport a security vulnerabilityโ€ link in the project pageโ€™s sidebar. See how to report a security issue for details.

  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Issue created by @hestenet
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States hestenet Portland, OR ๐Ÿ‡บ๐Ÿ‡ธ
  • ๐Ÿ‡จ๐Ÿ‡ฆCanada ergonlogic Montrรฉal, Quรฉbec ๐Ÿ‡จ๐Ÿ‡ฆ
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States xjm

    I think the IS is roughly in descending order of priority for security review. Package Manager is more important than composer-stager, but to understand the security features of Package Manager you also need to understand composer-stager. ๐Ÿคทโ€โ™€๏ธ Autoupdates is also more important to review/test than Project Browser since Package Manager will handle most of the security features of Project Browser, whereas Autoupdates has its own security/integrity stuff on top of the other layers.

    Ideally, all of the items would be in scope for security review since every stitch of it is brand-new code beyond anything the PHP community has currently, and the architecture as a whole designed to work together for defense-in-depth and safe responses to unsafe situations. I guess it depends on the resources available.

    That said, prior to including the Drupal-specific code in the review scope, we should make sure that they're familiar enough with Drupal's architecture to not give us reams of false positives from scanners or anything. We should also finish the work to allow autoupdates' cron job to be triggered by a separate user for defense-in-depth and correctly document that option, since it all eventually boils down to protecting executable code being written to the file system by the webhost user.

    Maybe there should be two separate RFPs, one for the TUF and Composer toolchain, and another for the Drupal modules?

    Pen testing, which would also be a separate RFP than the security review, would obviously also involve the whole end-to-end toolchain.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    We should also finish the work to allow autoupdatesโ€™ cron job to be triggered by a separate user for defense-in-depth

    FYI, @tedbow opened ๐Ÿ“Œ Add Drush command to allow running cron updates via console and by a separate user, for defense-in-depth Fixed for that 8 days ago.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States tedbow Ithaca, NY, USA

    re #5

    Package Manager will handle most of the security features of Project Browser, whereas Autoupdates has its own security/integrity stuff on top of the other layers.

    Autoupdates a couple of security checks but we have moved most of it down into Package Manager.
    AutoUpdates checks the versions of installed vs target to ensure it is allowed by the constraints of the module(VersionPolicyValidator) and it checks cron frequency if you are using Automated Cron.

    Otherwise as we worked on integrating Project Browser with Package Manager we realized that security checks we had in AutoUpdates actually belonged in Package Manager to avoid duplication and to make UI's built on top of Package Manage be secure by default(as much we can enforce at that level)

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    IOW per #7, reviewing Package Manager & Composer Stager will mean 98% of security aspects have been reviewed. #5 was based on the state of these modules of 6 months or longer ago.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States hestenet Portland, OR ๐Ÿ‡บ๐Ÿ‡ธ

    Thank you all for chiming in. I

    will see what the scope/budgetary requirements look like in conversation with OSTIF.org (which was universally recommended by TUF and OpenSSF communities as the right org to engage to find a sec audit vendor).

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States hestenet Portland, OR ๐Ÿ‡บ๐Ÿ‡ธ
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States hestenet Portland, OR ๐Ÿ‡บ๐Ÿ‡ธ
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States hestenet Portland, OR ๐Ÿ‡บ๐Ÿ‡ธ
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States hestenet Portland, OR ๐Ÿ‡บ๐Ÿ‡ธ
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States hestenet Portland, OR ๐Ÿ‡บ๐Ÿ‡ธ
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States hestenet Portland, OR ๐Ÿ‡บ๐Ÿ‡ธ
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States hestenet Portland, OR ๐Ÿ‡บ๐Ÿ‡ธ
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States hestenet Portland, OR ๐Ÿ‡บ๐Ÿ‡ธ
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States hestenet Portland, OR ๐Ÿ‡บ๐Ÿ‡ธ
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States hestenet Portland, OR ๐Ÿ‡บ๐Ÿ‡ธ
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States effulgentsia

    I think the issue summary here looks really good and the two lists are well thought out.

    Question about consistency between the issue title and summary and how that affects scope: the title implies that the scope of this issue is about the "secure signing components". That's the first list in the summary. The second list in the summary is the components that are either not related to or only minimally related to signature checking. Is the scope of the security review that's wanted for those components only about making sure they're properly integrated with signature checking? Or is the scope to security review their full behavior, not just the signature checking part? For example, reviewing if Composer Stager opens any vulnerabilities due to how it copies files, or if the Automatic Updates module is vulnerable to XSS or privilege escalation or ...?

    If the latter, then should we either retitle this issue, or split that second list into a separate issue?

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States hestenet Portland, OR ๐Ÿ‡บ๐Ÿ‡ธ

    @effulgentsia - I'm inclined to say we split this issue, and have one for the secure signing components (as this is currently titled) and a separate issue for the Drupal components. OR use this as a parent for 2 child issues.

    If there are no objections, I can look into making that change.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States effulgentsia

    +1 to splitting out a new issue for Composer Stager and the Drupal modules.

  • Status changed to RTBC 3 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States hestenet Portland, OR ๐Ÿ‡บ๐Ÿ‡ธ

    I am not sure if I can skip this one straight to closed - but we have had audits of both the client and server - and the reports have been shared with the initiative teams and private issues created where necessary to resolve issues discovered.

  • Status changed to Needs work 3 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    While private issues can't be linked here, I think we should link any issues that were determined to be fixed in public from this issue. I know that ๐Ÿ› Batch set processing triggerable via CSRF Active is one, but not sure about any others.

    Additionally, once any private issues are fixed and a suitable amount of time passed, I think it would be very good if the report was made public so it can be referred to (if that's possible?).

Production build 0.69.0 2024