Security review of secure signing components for package manager

I think the IS is roughly in descending order of priority for security review, aside from two bits:

• 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.

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 #3351895: Add Drush command to allow running cron updates via console and by a separate user, for defense-in-depth for that 8 days ago.

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 has a couple of security checks but we have moved most of it down into Package Manager.
AutoUpdates checks the versions of installed and 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)

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.

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?

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

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 #3438611: Batch set processing triggerable via CSRF 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?).

As far as I know the list of issues hasn't been made public - can it be? I don't have access to the report or a list of the issues myself, so only someone who does can do that step. Bumping this to critical.

FYI, a summary of the report was published here: https://ostif.org/php-tuf-audit-complete/, which includes a link to the report itself.

Thanks for the link. Next step I think would be to link to issues/commits for the items highlighted in the report.

I did a quick check.

https://github.com/php-tuf/php-tuf/commit/7c2c58c5275618307d6b143ae7b2f0...

Fixes "M1: [PHP-TUF] Path Traversal in Delegated Role Metadata" which is on page 5 of the report.

https://github.com/php-tuf/php-tuf/commit/47b2a3a6e85fc4668a3755d769669f... looks like it's related "L2: [PHP-TUF] Canonical JSON Encoding Differential" (page 9) but I also think that's not the actual commit that fixed it?

Those are the only two issues in php-tuf, and the report doesn't mention composer stager, so if that's it, then from the point of view of what's going into core or becoming a dependency of core, this is done, but we might want the DA to confirm everything on the rugged/infra side.

For Rugged, we documented 3 tickets that came out of the OSTIF audit: https://gitlab.com/rugged/rugged/-/issues/?label_name%5B%5D=OSTIF-2023. These were previously confidential, but we have now made them all public.

• #156: This ticket just documents some broken links in the docs. So this is not a security issue at all.
• #157: This ticket listed some secrets in the source code:
• A CI job token that was cruft from when the project itself was non-public. This has been removed.
• A github token to download php-tuf and the composer integration plugin. This is required for CI; ie. won't fix.
• A hard-coded password for one of the services running in the dev env. This does not affect production deployments. We are considering disabling this by default.
• #158: This ticket describes access to keys in the dev/test environments. This does not affect prod environments. This will likely end up "won't fix".

All of these ticket are scheduled for final review (and remediation, if required) in the coming weeks.

@quietone noticed that the tickets mentioned in https://gitlab.com/rugged/rugged/-/issues/?label_name%5B%5D=OSTIF-2023 are still open, I don't think we can close this issue until those issues are actually closed - whether because they're fixed, or because they're closed as 'won't fix'.

And further, if they are to be fixed, is there an estimated timeline for that?

An issue summary update would help too. Thanks.

Setting to NW for the updates for the last two comments.

Updated the remaining tasks

I can report that the issues in https://gitlab.com/rugged/rugged/-/work_items?label_name%5B%5D=OSTIF-2023 have now been resolved.

@star-szr, thanks can you strikeout that item in the remaining tasks?

@quietone thanks!