Account created on 31 January 2012, almost 13 years ago
#

Merge Requests

More

Recent comments

🇺🇸United States cmlara

@seeduardo:

I suggest opening a new issue as this one was closed as fixed years ago.

🇺🇸United States cmlara

Wouldn't setting the default value of _PHPUNIT_EXTRA to --no-configuration be equivalent to what we had before the change?

I’m not sure how —no-configuration and -c work togehter so that would have to need to be tested.

However even if it does word that would depend upon no jobs having a customized _PHPUNIT_EXTRA (I do for code coverage) or again back to requiring maintainers to make job changes.

🇺🇸United States cmlara
however are we, or should we be, hardening to that point

Is it hardening or fixing a bug would be a more fundamental question.

The move function is called move not move_and_chmod_public_read().
As I alluded to before, i work in the filesystem service as a contrib streamWrapper maintainer (s3fs) and this never even registered in my mind quick reading the code to implement in a decorator.

I can’t imagine the average developer has thought how this might put their deployment at risk and did not expect move() to function as a normal move. For context I did see the Windows workaround at 0600 and always thought “ok private no big deal maybe breaks some stuff but should fail closed”

I suspect the DST and core team are underestimating the culpability of core in this.

I’m out with the flu so not in a spot to go digging through the CVE’s, CWE’s and CAPEC’s to find possible citations.

The key part is this is two fold:

  1. We silently will perform a copy() when a rename fails (providing no signal to the caller they need to take actions for a change of ownership)
  2. We silently change the permissions without the request of the caller.

Considering this method is suppose to be a drop in (more friendly OS agnostic replacement for php’s built-in move() it’s has already taken on higher responsibility. By hiding errors or has taken on the additional responsibility of being the secure error handler.

I ran across a PHP package the other day that went as far as to use flysystem IIRC to do local file management, at one point it seemed like overkill, however when you think of scenarios like this it makes a bit more logical sense that you “trust a library instead of trying to implant it all yourself” (not suggesting we convert to flysystem, just pointing out file management responsibilities can be large).

There a few assumptions in the following example however here is an example of what concerns me

User1:

$ touch private.key
$ chown www-user:user-group private.key
$ chmod 460 private.key
User1 securely puts contents into private.key
$ chmod 440 private.key
$ mv private.key /var/www/not_doc_root/private.key

User2 (not a memebr of user-group)

$ cat var /www/not_doc_root/private.key 
> File not readable.

Some time in the future $fileSystem->move(‘/var/www/not_doc_root/private.key’, ‘/var/www/not_doc_root/private-old.key’) (inside a hook_update as part of center magament, as part of a user taking an action in IMCE, whatever).

User2 (not a member of user-group)

$ cat var /www/not_doc_root/private-old.key 
> It is now time to go have some fun :)

The above example shows local however simialr risks occur through remote frontends too (think of a server running multiple HTTP server daemons some process isolation scheme such as multiple docker containers with shared storage).

Couple with full path disclosure vulnerabilities in core and you have even more fun (yes that is talking chained exploits, however it not like those have not happened in the past in 🐛 Maintenance pages leak sensitive environment information. Needs review and that we are sure they will never happen again in the future).

Going back to the IS:

(1) a file in the public files directory that deliberately has the above somewhat-unusual permissions, and (2) a contrib module installed that allows an attacker to affect the input to file_unmanaged_move(). I don't know of such a module off the top of my head,

This would not be limited to just public:// when you consider the local vector, though yes public makes it a LOT easier.

As for contrib module, IMCE would be a high-risk candidate to me unless it has already been proven that it can’t do so.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

There is already an open issue for the 4.x and newer branches at 📌 Increase max path length Active that can be used for further consideration of this issue for supported releases.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-3.x and newer branches.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-3.x and newer branches.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

Latest Drupal 10/11 releases no longer store CSS/JS in the public:// streamWrapper and instead use a new assets:// wrapper.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-3.x and newer branches.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-3.x and newer branches.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

Latest Drupal 10/11 releases no longer store CSS/JS in the public:// streamWrapper and instead use a new assets:// wrapper.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

Latest Drupal 10/11 releases no longer store CSS/JS in the public:// streamWrapper and instead use a new assets:// wrapper.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

The 8.x-3.x branch and newer already support this feature.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

The 8.x-3.x branch and newer already support this feature.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

the 8.x-3.x and newer releases require composer managed installs and do not experience the issues described here.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

Latest Drupal 10/11 releases no longer store CSS/JS in the public:// streamWrapper and instead use a new assets:// wrapper.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

Latest Drupal 10/11 releases no longer store CSS/JS in the public:// streamWrapper and instead use a new assets:// wrapper.

Addtionaly we solved this issue in the 8.x-3.x branch prior to core moving away from using the public://wrapper.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

Latest Drupal 10/11 releases no longer store CSS/JS in the public:// streamWrapper and instead use a new assets:// wrapper.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

Latest Drupal 10/11 releases no longer store CSS/JS in the public:// streamWrapper and instead use a new assets:// wrapper.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

The 4.x branches are undergoing a redesign, with the intent to move file migration to an external module that will be streamWrapper agnostic. This feature will be provided either by Bulk File Management or another contrib module.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

The requests in this issue have already been implemented in the 8.x-3.x and newer releases.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

The features requested in this issue have already been implemented in the the 8.x-3.x and newer releases.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

The feature requested in this issue has already been implemented into the 4.x dev branch for inclusion in a future release.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

The 4.x branches are undergoing a redesign, with the indent to move file migration to an external module that will be streamWrapper agnostic. This feature will be provided either by Bulk File Management or another contrib module.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

The concerns raised in this issue do not appear to exist in Drupal 8+.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

Documentation in the D8+ branches has already been improved to include more details on required permissions.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

There is already an open issue for the D8+ branches at #3249500: Allow partial cache updates that can be used for further consideration of this issue for supported releases..

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

The functions in this issue do not appear to exist in Drupal 8+ as such I am going to close this support request as outdated.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 will reach end of life on January 5th.

The 7.x releases of S3FS does not have any additional planned releases.

The functions in this issue do not appear to exist in Drupal 8+ as such I am going to close this support request as outdated.

🇺🇸United States cmlara

In preparation for D7 EOL in a couple days moving this to 2.x branch.

While using SSL should be 'common knowledge' for anyone deploying MFA I could see room for it to be included somewhere in the documentation. Possibly under the Install Hardening section? (Maybe rename to "Deployment Hardening" to be more agnostic to setup itself?). Could also be a good option to create a "Security Considerations" as I could see us desiring to document the DB storage and multi-environment related concerns.

🇺🇸United States cmlara

setRedirect() is overridden if the form is 're-building'. We do trigger from rebuild in multiple places. This is the most likely cause and should be investigated with a debugger to validate.

🇺🇸United States cmlara

Reading this issue it looks like @avpaderno may not have yet sent a message to the maintainers(no record is indicated in the issue).

This may end up needing another 14 day waiting period unless @avpaderno did send a message before and just did not put it in the issue queue.

Sending back to active until there is record of contact by queue maintainers as none of us have the necessary access to properly RTBC this.

I've seen issues get burried in this queue in the past.. @dieterholvoet (and only @dieterholvoet) may wish to try pinging @avpaderno on slack in #drupalorg.

I would suggest 📌 Automate the majority of the ownership transfer process (retain human approval) Active could help solve some of the issues of this queue regarding contacting maintainers, recording records of contact etc.

🇺🇸United States cmlara

think this proves that the permissions are set.

Good point. That should indeed imply that the permission is set as long as you are on 8.x-1.9 (if your on 8.x1.x-dev we recently changed operations so the access controller performs all the checks 🐛 Admin cannot disable TFA for a user Active ).

I'm a bit of a loss here. If you say you see accessSelfOrAdmin returns that access is permitted yet core is refusing the access I wonder if there is some issue upstream or in side-stream.

I haven't explicitly attempted to reproduce this this today, however I'm in that page often as test users for dev work and never seen a problem.

What version of core? Any other modules installed (especially those that apply core new Access Policy API as this changes how core processes permissions)?

🇺🇸United States cmlara

@sayan_k_dutta 'Redirect users on login to TFA Setup Page' impacts the Login logic this issue apepars to be about the Token Provisioning (TFASetupForm) which is indepdnet of the login logic.

🇺🇸United States cmlara

This sounds like the user does not actually have the 'setup own tfa' permission.
Permissions in routing.yml are an AND between _custom_access and _permission

🇺🇸United States cmlara

From the MR

UserAuthenticationInterface::lookupAccount() does not exist in decorators not implementing UserAuthenticationInterface.

Core's user login form assumes that if the User Auth service implements UserAuthenticationInterface it is safe to call lookupAccount.
This works fine when you have only the two sceanrios:

  • Core Login Form -> Core UserAuth service (implements UserAuthenticationInterface)
  • Core Login Form -> Decorator1(UserAuthenticationInterface) -> Core UserAuth(UserAuthenticationInterface)
  • Core Login Form -> Decorator1(UserAuthenticationInterface) -> Decorator2(UserAuthenticationInterface) -> Core UserAuth(UserAuthenticationInterface)

This logic however fails when you have

  • Core Login Form -> Decorator1(UserAuthenticationInterface) -> Decorator2(UserAuthInterface) -> Core UserAuth(UserAuthenticationInterface)
  • Core Login Form -> Decorator1(UserAuthenticationInterface) -> Decorator2(UserAuthInterface) -> Decorator3(UserAuthenticationInterface) -> Core UserAuth(UserAuthenticationInterface)

The outcomes in this scenario are that core calls Decorator1 and:

  • Decorator 1 calls non-existent method on Decorator2 (throws an exception) or
  • Decorator 1 attempts to resolve the data itself using Cores logic ( this is a breach of decorator design, and would mask that Decorator3 is being ignored)

I will note the first scenario is essentialy what you describe in #6, where mail_login was re-written to (incorrectly) assume every decorator will already implement UserAuthenticationInterface prior to D12, possibly because core made the same assumption.

On a cursory glance I believe the only way we can use UserAuthenticationInterface is if we are on D12, OR we have a setting that manually enables the use of a new class when a site owner ensures 100% of their stack supports the UserAuthenticationInterface (in which case it is safe for exceptions to be thrown).

Drupal Core tried to do this in a manner to retain BC with minimal immediate contrib breakage and ended up creating a scenario that the probability of site breakage increased each day the commit remained in the repository by pushing design failures onto the contrib ecosystem. When we look at the entire issue as a whole we are essentially left with a non BC capable API addition that would have been easier to implement if it was mandatory.

🇺🇸United States cmlara

(1) will the fix be backported to 8.x branch?

Fundamentally in my opinion these are API breaking changes (combination of no 'well defined' API defined in 1.x, and being major operation workflow changes). I can't see a way to cleanly backport these to 1.x

(2) is there a list of known-to-have-issues modules, that are at this risk?

I do not have the resources to audit the entire contrib ecosystem. I believe all the modules know at the time of the issue are in the summary or linked issues, however there are likely more.

For 1.x: In general if the module edits the login form, alters the login form workflow, or allows authentication outside the login form it should be considered a risk that needs to be manually validated.

(3) should (new) production be yet moved to 2x (still -alpha, or -dev)?

My personal suggestion would be to use an external SSO provider with MFA if possible.

2.x is nowhere near stable or ready for production and I believe still has a few loose ends needed on know security issues in 1.x that need to be implemented differently in 2.x. I'm a bit surprised to see a higher level maintainer pushed an alpha3/alpha4 with some major know issues present.

If you can't use an external SSO, 1.x is likely better than nothing, however it comes with significant caveats that we keep finding holes in the implementation that just can not be properly fixed without major branch changes. We bandage up many of the known exploits, however the root fault is generally still there waiting for the next "oh we didn't expect that" incident.

🇺🇸United States cmlara

Until Drupal 12 is released contrib modules need to accept both interfaces and test which they have been provided. See 🐛 BC break in login auth changes from #3444978 Fixed comments #22 and #24.

Mail_login should indicate it is only compatible with Drupal12+ if it accepts only a single interface otherwise it needs to accept both. You likely will want to raise a bug report in mail_login to inform them of this issue.

Setting to a task as we will eventually need to do this. Moving to needs-work as D.O. has fully moved over to an MR only workflow, patches can not be tested only MR’s.

🇺🇸United States cmlara

a few customisations in the .gitlab-ci.yml and in particular is setting the global php version to $CORE_PHP_MAX.

Fair point. I did not realize we had yet another policy violation for gitlab_templates (PHP target is suppose to be the latest version of PHP supported by the latest version of core), I did not even think to go look at that in my .gitlab-ci.yml.

I can pull that out, although that only delays the inevitable here as eventually we will jump the version in templates.

Probably this kind of php version increase which causes new phpunit failures has not happened since we started the gitlab templates for contrib projects

This would not have happened (at least on the straight PHPUnit runs) before we committed 🐛 browser_output is empty when testing with Drupal 11 Active as phpunit defaults these values to false. It is the Core phpunit.xml that sets them to true. Additionally a significant amount of PHP 8.3 around no longer allowing coercion were detected by phpstan before the deprecation were even voted into PHP8.3 making them less likely to exist in code.

I can override this by placing a custom phpunit.xml in my project to avoid cores phpunit from impacting my configuration (far more ideal than allow failures) however my point is more that I should not have to, it should be those who want to opt into this that do so (as a baseline change from templates version 1.0.0 we are suppose to give deference to avoiding breaks in existing pipelines and make everyone else opt in to “breaking” changes).

🇺🇸United States cmlara

Regardless of if we feel they are worth having or not it is a template BC break.

Some of us setup our pipelines to not fail on deprecation, gitlab_template is not permitted to make changes that would cause test that would previously pass to fail without a new major branch (which has been repeatedly objected to doing so we have to keep the previous base for the foreseeable future).

Side note:
Some of us maintain compatibility to PHP 7.0 still (though I found a couple projects have accidentally breached this) not because we recommend using it, instead because it’s not our place to go arbitrarily changing the minimum system requirements inside a major branch and have zero justification to add a new major when BC has to date been extremely easy to maintain.

🇺🇸United States cmlara

Committed to dev on 2.x, postponing 1.x on 🌱 PHP 7.0 Support Active as we technically can't add this into the codebase without breaching declared minimum php support.

🇺🇸United States cmlara

Note:
This issue contains fixes that are detected by phpcs and automatically fixed by phpcbf.

This issue will not receive D.O. issue credit under Abuse of the Contribution Credit system .

🇺🇸United States cmlara

Was hit by a phcps fixable issue under the appearance of PHP 8.4 depredations.

Not enough for a strike at this time, however it does go to #10's note that we still see this vendor working 'spammy' issues.

🇺🇸United States cmlara

Visually looks good to me. Merged to to dev.

Attaching a static patch file to use with composer.

Now that D.O. has moved to a 100% MR based workflow there should be no need to upload patches for fixes to issues.

It is recommend to store patches in your local install and not host them on D.O., especially for a security sensitive modules (including Vault). While files on D.O. are more write resistant than issue forks they are not guaranteed to be immutable.

🇺🇸United States cmlara

With D7 EOL approaching closing as a duplicate of Confirmation forms should not require passwords Needs review that is already open for the 2.x branch

🇺🇸United States cmlara

With D7 EOL approaching moving to 2.x for consideration.

🇺🇸United States cmlara

With D7 EOL approaching moving to 2.x for consideration.

We do now have built-in features to redirect when a user logs in without TFA.

🇺🇸United States cmlara

With D7 EOL approaching moving to 2.x for consideration.

I do have concerns about sending via email as they may retain a long-term copy and progress through multiple intermediaries.

I can agree only transmission over HTTPS would be ideal, though perhaps that may be better left to another module/site configuration, especially now that modern SSL practices indicate that HTTPS over the open web may always be the case for some environments.

🇺🇸United States cmlara

i've not had an issue with php-fpm service's `PrivateTmp=true` with any other service using php-fpm.
i don't know if this is, or isn't, intended behavior in Drupal.

I believe this is intended behavior, that any path configured for $settings['file_temp_path'] is expected to be present and accessible to Drupal at the time we are bootstrapped. In other words, I believe it is intentional that we will not call mkdir($settings['file_temp_path]).

Perhaps other systems 'tried to help you' by recursively creating these folders however they would have masked the underlying 'fault' that you are not directly using /tmp and instead going through a systemd provided abstraction layer.

Since we don't mkdir() the path, and since PHP-FPM gets a 'virtual' /tmp that does not already have this directory fopen() fails as "path does not exist" which is where the errors above come from. In theory the mkdir() suggestion from #12 could exist indefinitely as a solution for you.

🇺🇸United States cmlara

@pgndrupal Can you check if your deployment is using SystemD PrivateTmp=true?
https://stackoverflow.com/questions/53700985/why-cant-write-info-into-tm...

Note for anyone using #8 you MUST mkdir /tmp/drupal/mysite/ (do not do so in the PHP script as that will contaminate the test) first otherwise file_put_contents will always fail.

Another test for @pgndrupal would be in settings.php to put @mkdir('/tmp/drupal/mysite/');. If the issue goes away that also implies that PHP is running with a private /tmp space.

🇺🇸United States cmlara

Setting as postponed on upstream https://github.com/php-tuf/composer-integration/issues/127

As discovered in 📌 Manually test TUF-enabled Composer projects Active in even basic lab deployments the plug-in causes an excessive increase in memory consumption.

🇺🇸United States cmlara

Manual rebase (conflict was due to new comments in ResourceTestBase.php).

Reran jobs with random failures.

🇺🇸United States cmlara

Worth asking:
Should permissions just be retained?
Should owner be retained?

Thinking of other attack vectors (Quick visual review, not confirmed in the dev lab):
1)
File owned by www-data:some-other-group server runs as www-data:www-data permison 600 (only the web-server can read, no other users can) copied to same stream wrapper:
file_unmanaged_move() will (even with this patch) make the file world and group readable, this could allow another process (not the webserver) (through either the group read, or the public read) to read the file.

2)
File owned by www-data:some-other-group server runs as www-data:www-data
file_unmanaged_move() will either end up with the file being www-data:some-other-group OR www-data:-www-data (depends on if rename succeeds which will depend on if it crosses hard mount points or streamWrappers eg public:// to private://

Implied the above question is that this is not limited to just the public:// scheme or a path inside the webserver docroot, it impacts all files that Drupal touches as the webserver would not be the only access vector.

Systems admin hat on: Shell mv usually retains the users and permissions. I could very easily see devs not being aware of this and silently injecting some public exposure in random locations that allows sensitive information to leak.

Tangent: I saw the chmod() line in the filesystem service and didn't even think twice about how far ranging these impacts would be until this issue randomly popped up in my feed.

🇺🇸United States cmlara

@keithkhl any feedback from running patch in #10?

🇺🇸United States cmlara

Closing as outdated as a maintainer was added, and D7 was supported to to the revised EOL dates.

It is anticipated Drupal Infra will force mark the current D7 release unsupported sometime after January 5th with core 7 finally going EOL.

🇺🇸United States cmlara

8.x-3.7 tagged and released.

🇺🇸United States cmlara

Related message on Slack:
https://drupal.slack.com/archives/C1BB308HH/p1734593804687089?thread_ts=...

Main note is I haven’t had time to login to my dev machine this week (core schedule slipping threw my timeline off) to do one final manual test before publishing (we’re in that weird state where I still like to hand review some basics ops as our test suite can miss things, and core had a flurry of what seemed like significant issues going in after the RC was published).

Anticipating being able to look at this tonight.

Allow for the s3fs module to support all minor versions of drupal 10

See #3324737: Do not support core versions newer than current published beta or higher .

Some parts of what we have to do for s3fs are “off api” we can’t reliably say it will work with the next minor release before it reaches publication.

Simple feature such as decorating the file system service to prevent data loss are (WSOD) breakable as core defines Interfaces as API only for callers.

🇺🇸United States cmlara

Most of the other organization scopes I reviewed have rather brief and broad scope statements

We have a large number of Drupalisms, policies I’ve seen in no other CNA. Most tend to follow the CNA rules and be as open as possible, we have a number of policies that contradict the security industry norms.

An example is the security opt-in, compare that to GitHub and GitLab CNA. Neither require a maintainer to pass a test and opt in.

These policies likley always should have been in the scope and not the procedure document. It is confusing to reporters.

This is especially true if the DST claims to be overworked and understaffed as the dispute process will pull effort away just to enforce the inevitable “we refuse to publish even if our parent CNA consider this a vulnerability”

I'm hesitant to enshrine too much of the project's practices into the
scope.

These are not really practices, they are defined policies about what the DST wants/will consider issuing advisories for.

I guess a good system would be to revisit this periodically

Does not hurt to do, though any security policy change should be coupled with a scope review to avoid it ever getting out of sync.

🇺🇸United States cmlara

I keep forgetting to come here and comment.

At first I thought we were doing a full evaluation of the CNA scope, latter messaging made it more clear we are more just trying to make the scope match with what policy is with some small changes to scope.

Anything that’s in our rules regarding exclusions should probaly be added to the scope if we are going to keep them. This includes the question above about projects must be opted in being included in scope wording.

There is some discussion in 📌 Create CVEs for contributed projects in 2024 Active , if we are going to keep the 10,000 install limit, If we do it should likley be in the CNA scope too.

Additionally our excluded vulnerability types would be good to add.

All of the above allows the DST to ignore the issues and defer them to another CNA for issuance without having to go through the CVE Dispute procedure (and allows other CNA’s to quickly take up the issues as the policy is formally published with the CNA secretary).

🇺🇸United States cmlara

I wonder if it's worth adding something to that to spit out an appropriate commit message, like we have on d.o issues now?

I’ll note this might also be a reason to invest in client side tooling.

Yes can add the UI and it can be useful, however worth looking at your daily operations and analyzing it this is the sort of task that can be put into a local script to parse the issues, prompt the committer for credit/title/description, push the credit, Squash, merge, push to GDO, close the MR and the issue all in a single command saving committers time. Just throwing it out there as “long term” how to possibly be more efficient as more and more moves into Gitalab where a proper full API exists.

Also we need to decide what email adres to use.

The email challenge is probably the one negative of moving this out of GDO and trying to use a UI that can’t (shouldn’t) have access to the full email database.

If GDO does make the public address avaliable that’s a good solution. We might need Allow users to set GitLab Commit email Active (looks like it’s on the same page but a different field as the commit email). (This issue reminded me I needed to re-open that one.)

If we can’t use the public email field I would suggest:

For committers:
We should probably use whatever is in their commit.

Users can set their anonymous address before pushing to GDO.

Some of us want our real addresses for traceability and manageability (you can’t verify you “own” an anonymous D.O. address). GDO Infrastructure has seen similar requests with those importing commits from IIRC GitLab in the past where it’s not associated with their D.O. account.

For reviewers:
Probably a bit harder might just have to assume an anonymous address.

🇺🇸United States cmlara

Re-opening and slightly adjusting title of this issue since email management has moved directly to GDO in #3300281: Have git.drupalcode.org manage secondary emails, replacing multiple_email module without support for this feature.

Access to edit profile is needed to set the commit email.
https://docs.gitlab.com/ee/user/profile/#change-the-email-displayed-on-y...

I can’t find the issue at the moment, however in the past we have seen having anonymous emails create problems such as users who make commits in GitLab.com can not be associated with their D.O. account. The same problem would occur with D.O. addresses being imported into GitLab.com.

🇺🇸United States cmlara

I want to see support for tar/zip installs go away and move core to composer only. I do not use update module to update sites. With that in mind, given the speed in this issue is moved I feel a need to speak up for "the other side" of this and at least put something on the record for sites that may (for whatever reason I can not understand) be using these features.

I noted in #3285191-50: [meta] Only support Drupal core installs managed by composer I do not believe features such as this should be removed in minor releases. I suggested in April this should have been done as a deprecation in D10 for removing D11 however to my knowledge the release managers did not make it a release blocker and as such we missed the D11 window. That alone should be reason not to include this at this time.

Going a step further, even if this proceeds to be done in the 11.x branch, the release cycle is already at RC1 for 11.1.x, this is not the time for these sort of changes where their review time will be limited (unsure if core is releasing this week or if they pushed back since RC1 was delayed a week).

Ultimately it is a release manager decision, however from my pov, as much as I see no use for this feature set, this isn't bending things a bit, it is a significant code yank, one that we knew about and could have properly planned for rather than trying to shoe-horn in at the last minute.

🇺🇸United States cmlara

Updating IS to show first warning has been sent.

🇺🇸United States cmlara

Elephant in the room:
How to deal with the unsupported with known undisclosed vulnerabilities Drupalism?

Outside of Drupal we would publish the vulnerability details the same as we would for a program with a fix.

Would appear to me we have 3 choices:

  • Publish the vulnerability details at time of marking unsupported to allow publishing a CVE.
  • Hard code a time limit that unsupported modules must be disclosed to allow a CVE to be generated.
  • Refuse to publish CVE’s for these modules

Some of this has been lightly discussed in the past (not sure if I ever created an issue for this) however now we are changing the landscape again.

🇺🇸United States cmlara

It seems uncommon enough that more documentation is not warranted.

I would suggest that is exactly why this should have documentation. You do not want to be writing your "(Emergency) Response Procedures" in the middle of a situation, you want them documented before the situation so that you "pull the binder off the shelf" and start following what has already been written.

As long as we allow human error as a variable the risk of recurrence is there.

Quick slack search:
https://drupal.slack.com/archives/C5B7P7294/p1723053635495029?thread_ts=...
SA was published to D.O. but not announced to mailing list, not as significant as what started this thread, however it is closely related as it is part of the 'publishing' stage of Drupal Advisories and provides evidence the human error factor is still occurring.

I would have to dig deeper, however part of this has also been that many of us are now scrutinizing the security team's every action and flagging the issues publicly in the #security-team room when faults happen in the release cycle (Failing to publish a release, failing to publish the SA even though its been linked in the room, the SA being released to Composer when its not available on D.O.) etc. This does not mean the issue has disappeared nor is it a reliable solution.

🇺🇸United States cmlara

From a technical standpoint:
As long as the Drupal CNA accepts email reports (see: https://www.cve.org/PartnerInformation/ListofPartners/partner/drupal) the system will in theory need to support non D.O. users. If the email submission is shutdown it might need to trigger a review of all D.O. account policies (Users banned by the CWG for example may still need to be permitted to open Security Reports and explicit exemptions from a number of site wide terms of services may be needed for security reporters).

I believe it's extremely rare (maybe non existent) in the last 10 years for credit to be a link outside of Drupal.org

Flip side of that, how often does the Drupal Security team asked about how those working an issue want credit assigned?

We might have a case of 'process induced bias' here. I have worked a few security issues and never been asked how I want my credit attributed. Not saying I would change how I've been credited, however equally I'm not saying I would keep it either, since the choice has not been offered I have never considered it.

Tagging 📌 Create CVEs for contributed projects in 2024 Active as related since it is discussing using the content of the fields as authoritative data.

🇺🇸United States cmlara

The big question is what is our Time To Publish SLA going to be? Can we meet that SLA with accurate information without collecting this as part of the Draft SA process?

Remembering that until we publish a CVE 3rd party tools are not going to be able to detect the vulnerabilities, the shorter the better.

My personal goal: Within minutes after the Drupal SA is published the CVE record is also published to allow for 3rd party security tools to detect the vulnerabilities. This goal does not allow time for generating the data after Drupal SA publication. Is this goal reachable today, no, in the future, hopefully.

Yes when we get a combination of an inexperienced dev, and an inexperienced security reporter the DST will have to help fill in the values, however the flip side of that is when we get experienced reporters or experienced developers (such as the Drupal Core Team and the security focused contrib developers) they will be able to fill in this information. This also goes to another point I've made previously, CWE are both about classifying the vulnerability, and training developers to not make the same mistakes twice, It is in the DST and D.O. ecosystem best interest to help maintainers understand the root behavior faults so they can avoid it being repeated in their projects, Working with them as part of the process may mean a reduction in future vulnerabilities.

I went through https://www.cvedetails.com/vulnerability-search.php?f=1&publishdatestart... yesterday (at the time it was "new CVE's since yesterday" and had 110 entries, additional CVE's have been added since I started the list below)' yesterday, chose the first CVE I saw from each CNA and plotted them down the basics attempting to gather "When a CNA publishes an advisory how long after they publish is a a CVE published"

The following timelines are 'high level', no timezone conversion or comparison done. CVE Publication times are UTC, CNA publication times are undefined and may be in UTC or CNA Local Time.

CVE-2024-54679 - Mitre -No publication of advisory just a link to a commit 3 weeks prior.
CVE-2024-54140 - GitHub - Same Day
CVE-2024-54127 - Cert In - Same Day
CVE-2024-54014 - JP/Cert - Next day
CVE-2024-53703 - SonicWall - 2 days after publication
CVE-2024-52271 - VulSec Labs - Same Day
CVE-2024-51555 - Asea Brown Boveri Ltd. (ABB) - Same Day
CVE-2024-42195 - HCL Software - Same Day
CVE-2024-12247 - MatterMost - >30 days or Same day. Vendor/CNA publishes an internal ID number of a vulnerability with no details, 30 days later they post public details and apply for a CVE on same day.
CVE-2024-11942 - Drupal.org - Skipped as we don't want to reference ourselves.
CVE-2024-12130 - Rockwell Automation - Next day site lists two dates in publication, assuming worst case of the oldest date.
CVE-2024-11779 - Wordfence - Next Day
CVE-2024-11149 - CISA - No CNA publication, just links to previously announced (but no CVE) security fixes in Freebsd.
CVE-2024-10716 - PegaSystems Inc - Same Day
CVE-2024-6219 - Canonical - 3 Days
CVE-2022-41137 - CVE-2022-41137 - Next day
CVE-2018-9391 - Android - No CNA publication, appears to be a bulk load of vulnerabilities from 2018 and before, unknown why not published when already publicly available.

Same Day encompass where the CNA publication date shows the same calendar day as the CVE.
Next day encompass where the CNA publication day shows the day prior to CVE publication.
Next day could be 'within hours'. Example Wordpress published on the 4th (no timezone specified) and the CVE was issued 09:23:07 UTC on the 5th, this could have been a 'same day' and within hours for the organization, or it could have been at the far end of ~33 hours.

Most of these sites only list a day not an exact time on their publication limiting the precision level to 'days'.

17 CNA's
1 is Drupal.org
3 did not publish their own advisory.
6 (or 8 depending how you classify MatterMost and Rockwell Automation) published same day, often within hours.
4 (Or 3 depending upon Rockwell's dates) published "next day"
2 published in the 2-3 day range.
1(or 0 depending on MatterMost classification) published at >30 days)

Back to the root question: What will SLA on publication?

🇺🇸United States cmlara

Another user from the same org with similar behavior (but far fewer issues opened).

https://www.drupal.org/user/3740035/track

🇺🇸United States cmlara

Back to active as I was still evaluating if I'm happy with what I put in.

Especially considering my use of '@cSpell', it is not technically a PHPDOC tag, however PHPCS was complaining without it (quick glance: I may have been on an older version of drupal/coder, need to check and re-evaluate and possibly re-write).

🇺🇸United States cmlara

Personally: I would like to see all SA’s receive CVE’s regardless of the install count (I especially note that none of my sites phone home and I assume some larger site owners configure similar).

This allows 3rd party tooling to work with our data without D.O. or 3rd parties needing bespoke parsers. This becomes more important the less usages as word of mouth is less likely to inform a site owner of a need to update.

RenovateBot for example is currently unable to track Core and Contrib vulnerabilities due to our lack of CVE’s https://github.com/renovatebot/renovate/discussions/22918#discussioncomm...

This also applies to other parsers such as Gitlab Code Security, Amazon Inspector, Docker Hub’s scanner, etc.

Provide contrib security advisories in OpenSSF Vulnerability format for dependency tracking Active could also possibly be helped as we would be part of the general data pool and could be aggregated (automatically) by an existing deed source.

As a security engineer: I want to use my existing tools and have it catch the issues.

The 10,000 limit it is also relevant to
📌 Update CNA scope Needs review on what I would suggest we include in the CNA scope if we keep it.

🇺🇸United States cmlara

Is this being limited to ones that impact only 10,000 installs or are we opening it up to all contrib SA’s?

Do we have a plan to ping the origonal reporters and maintainers for their comment before publication? The CNA can technically publish without maintainer/reporter approval however I would think we would want to get their feedback for their own issues before publication.

🇺🇸United States cmlara

Committed to dev. Will allow to sit a few days before tagging the feature release ahead of Drupal Core moving out of RC status.

🇺🇸United States cmlara

Linking Slack thread in case the OP puts more details there without updating this issue:
https://drupal.slack.com/archives/C1BB308HH/p1733234367979989

🇺🇸United States cmlara

Most modules I see rely on the packager to add that

Most modules have never embraced the composer system fully. The facade was as added as a hack to allow Drupal sites to use composer without having to add a composer.json to each module Now that we have GitLab Ci comper.json is essentially mandatory and it is no longer a significant burden to maintain.

and unless something breaks it shouldn't really be added to the composer.json file

Documented https://www.drupal.org/docs/develop/using-composer/add-a-composerjson-fi... that Composer itself needs it when used to its full capability. It is necessary when using GIT composer repositories (dev branches) and also useful when using manual checkouts for merging with a parent composer.json for easier development.

having it in two places can lead to those values becoming inconsistent with each other.

I've seen that from a few modules where the maintainers didn't double check their merge requests, though I haven't seen it cause a major problem in the ecosystem.

If we don't want to maintain in two places it would be better to set the module.info file to >=10.1 instead and use the composer.json as the official support indicator. Though thank you this reminded me I needed to open 📌 Prefer composer.json drupal/core requierment over module.info file for supproted versions field Active for the D.O. UI problems that have stopped me from doing that on other modules.

🇺🇸United States cmlara

Needs work as the proposed patch implies there is a deeper root fault that should be identified and fixed.

🇺🇸United States cmlara

Apply the following patch to your site and run a copy operation with all files.

Check /tmp/s3fs_scan_dir_output.txt for a path of a file that already exists on disk but was not uploaded to your s3 bucket, then check /tmp/s3fs_upload_list.txt, where you should see two entries of the paths.

Advise your results.

diff --git a/src/Batch/S3fsFileMigrationBatch.php b/src/Batch/S3fsFileMigrationBatch.php
index 5b4080e..b9c1516 100644
--- a/src/Batch/S3fsFileMigrationBatch.php
+++ b/src/Batch/S3fsFileMigrationBatch.php
@@ -109,6 +109,7 @@ public function dirScan($dir) {
         }
         else {
           $output[] = $path;
+          file_put_contents('/tmp/s3fs_scan_dir_output.txt', $path . PHP_EOL, FILE_APPEND);
         }
       }
     }
@@ -147,6 +148,7 @@ public static function copyOperation(array $config, array $file_paths, $total, $
       $context['results']['errors'] = [];
     }
     foreach ($file_paths as $path) {
+      file_put_contents('/tmp/s3fs_upload_list.txt', "Processing: $path" . PHP_EOL, FILE_APPEND);
       $relative_path = substr_replace($path, '', 0, strlen($source_folder) + 1);
       $key_path = $target_folder . $relative_path;
       $uri = $scheme . '://' . $relative_path;
@@ -215,6 +217,7 @@ public static function copyOperation(array $config, array $file_paths, $total, $
       \Drupal::moduleHandler()->alter('s3fs_upload_params', $uploadParams);
 
       try {
+        file_put_contents('/tmp/s3fs_upload_list.txt', "Uploading: $path" . PHP_EOL, FILE_APPEND);
         $s3->putObject($uploadParams);
       }
       catch (\Exception $e) {
🇺🇸United States cmlara

@jcnventura Is there a reason you removed the core requirement in composer? This is required for composer to properly function in all use cases.

🇺🇸United States cmlara

Adding such a file to every project is cumbersome

I would suggest they are about equally as cumbersome.

cp /tmp/phpcs.xml.dist ./; git add phpcs.xml.dist; git commit -m ‘use ordered sniff’;
Vs
yq ‘.Variables += {NewVariable: NewValue}’ .gitlab-ci.yml; git add .gitlab-ci.yml; git commit -m ‘use ordered sniff’;

Note: the above are rough pseudo code intended to illustrate similar levels of work and that one has the complexity of editing lines in existing files vs adding a new file. The only edge case where this dramatically changes is where a maintainer has a single project they use as a base template to inherit all their other projects(I do this on some projects) , in which case it can be done with a single point change vs a multi point change.

I still belive the custom phpcs to be the better method for about equivalent effort and burden to a project long term, while making it easier for everyone who contributes (if I pickup a random project how does my local install of phpcs know which standard the maintainer prefers, it needs to be in the phpcs.xml.dist).

🇺🇸United States cmlara

we could offer an opt-in for projects that still want to use that sniff.

I would suggest just encourage the use of the phpcs config file.

  • It allows users writing code locally to use the same config as the CI runs.
  • It is one less option that needs to be tested when templates change
  • It is one less config option and file that needs to be maintained.

If gitlab_templates is going to add this it should also probaly add the case insensitive option too (as it is what has been enforced for years even though it was not an approved standard).

What is gitlab_templates plans for this option if core settles on a standard that does not match the committed option (in other words how involved does templates want to be in encouraging deviating from the “standard”) ?

What precedent does this set for gitlab_templates maintaining multiple options in the future? What about the next time a similar conflict occurs in coder? Is this project going to maintain to an exponential number of templates?

Is gitlab_templates prepared to start having to code custom rules if for some reason a conflict arises?

Leaving this as NR as those questions are out of my scope to answer and are intended to just get gitlab_templates to think about the impact of options rather than rushing head first as it often does while also pushing for thinking about local development and maintainer responsibilities at the same time.

🇺🇸United States cmlara

I don't think it is fair to always ping one person if there are other maintainers too, w

Generally fair, I will note klausi has made 100% of the releases since 8.x-3.2 in April 2019.

GitLab Activity report indicates klausi is the only individual to have pushed commits into the repo in the past 2 years
https://git.drupalcode.org/project/coder/activity

I believe we can call this (at least the D.O. side of the project) a 'single active maintainer' project.

🇺🇸United States cmlara

Discourse link explains a few things

Some of the issues you describe sound like NAT hairpinning problems, which you should avoid by using the internal IP’s or hostnames.
“Besides, I prefer not to use internal IP as the ‘S3 endpoint’” I suggest you sit down with your network engineer and have a conversation for a solution that fits your needs.

To my recollection officially path based buckets do not support CNAME’s,
It may work due to the simplistic nature in how we generate most URL’s however it is not API supported. I don’t guarantee it long term and strongly suggest you switch to DNS style buckets as the API will eventually drop support for path based buckets. IIRC our 4.x branch is dropping support for path based buckets and will only support domain names for the CNAME field. 4.x is by no means imminent for release, just a note for the long term future.

For now, I will compromise with the workaround,

Are you saying when using internal IP’s all files are correctly copied ?

🇺🇸United States cmlara

Anyone heard anything from @klausi? Should we ping on Slack?

Production build 0.71.5 2024