Problem/Motivation

This module has a potential CSRF vulnerability.
The module has a lot of routes and none of them seem to have CSRF protection. I have not audited them all but I think some of them could be vulnerable to CSRF (routes that execute an action without a confirmation).

At a glance, I think at least these routes should be checked for CSRF vulnerabilities:

  • straker_translate.setup_account_handshake
  • straker_translate.dashboard_endpoint
  • straker_translate.notify
  • straker_translate.batch
  • straker_translate.entity.lock_target
  • straker_translate.entity.unlock_target
  • straker_translate.entity.upload
  • straker_translate.entity.update
  • straker_translate.entity.download
  • straker_translate.config.upload
  • straker_translate.config.update
  • straker_translate.config.download
  • straker_translate.interface_translation.upload
  • straker_translate.interface_translation.update

Steps to reproduce

1. As a user that can insert HTML on the site, insert this tag:

<img src="http://example.com/admin/straker-translate/setup/account/handshake?access_token=newtoken">

2. If another user with the "administer straker_translate" permission displays this page, the new token is saved without any confirmation.

Proposed resolution

If any of these create/update/delete content or config without a form or a confirmation, they probably need the _csrf_token requirement.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork straker_translate-3566913

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

prudloff created an issue. See original summary.

prudloff’s picture

prudloff commented
Issue summary: View changes

jbhovik made their first commit to this issue’s fork.

jbhovik’s picture

jbhovik commented

@prudloff,

I've pushed some changes to the 3566913-routes-not-protected issue fork. Please let me know if the changes satisfy the requirements for this issue ticket.

prudloff’s picture

prudloff commented

I don't see any changes: https://git.drupalcode.org/issue/straker_translate-3566913/-/compare/1.0...

jbhovik’s picture

jbhovik commented

Sorry about that. The changes are present now.

prudloff’s picture

prudloff commented

_csrf_token requirement is usually the easiest to fix this.
However I don't know enough about this module to review if it works correctly for every route or if other routes would need to be protected.

jbhovik’s picture

jbhovik commented

Thanks for looking at the changes.

The Straker Translate uses the Straker Verify translation management system (https://verify.straker.ai) to translate Drupal content. I tried the reproduction step using <img src="http://example.com/admin/straker-translate/setup/account/handshake?access_token=newtoken"> and the token didn't change, so I think at least that case is fixed.

Is there anything else we can do to fix this ticket's issue? Should I add the "_csrf_token" requirement to every route?

prudloff’s picture

prudloff commented

_csrf_token should not be added on every route, only on routes that alter data or config without a confirmation.
You can read more about it here: https://www.drupal.org/docs/8/api/routing-system/access-checking-on-rout...

jbhovik’s picture

jbhovik commented

After taking a closer look at all our routes, I added the "_csrf_token" requirement to the "straker_translate.entity.confirm" route. I don't think any other routes need it.

What do you think? Do my changes satisfy the security requirements for this ticket?

cpagar’s picture

cpagar commented

Hi Pierre Rudloff,
Hope you are doing well.

Apologies if Join the community I am not a drupal developer but rather a project manager role for the drupal connector, and just want to follow up if what Joey has implemented is already sufficient to meet your security requirements?
Appreciate if we can have feedback as our team is already planning for a demo to use our new drupal connector and we really want to fix and pass this security warnings.

Thanks!

cpagar’s picture

cpagar commented

Hi @prudloff,
Hope all is well on your end. Apologies I am not part of the maintainer of this project as I work as a project manager rather a development and I Just want to follow up if the fixes applied by @jbhovik met your security requirements?
As we cannot do some demo to our possible client until we have clear all the warnings in https://www.drupal.org/project/straker_translate,

Appreciate if you can provide feedback if there are action items on here.

Thanks!

cmlara’s picture

cmlara
he/him
commented

Side question:

Did @prudloff attempt to contact any of the maintainers privately (Coordinated Disclosure) before publicly disclosing his concerns here?

prudloff’s picture

prudloff commented

@cpagar I am not a user of this module, I simply noticed this by having a look at the code.
The module has a lot of routes and I don't know enough about how it works to review this patch.

@cmlara I used the "Report a security vulnerability" link on the module page.
The module is not covered Drupal’s security advisory policy so it creates a public issue.

cmlara’s picture

cmlara
he/him
commented

@prudloff

In the future you may wish to consider contacting the maintainers directly. As a Drupal Security Team Member you can reasonably be expected to be aware that link is set by the Drupal Association without the full consent of the module owner.

Addtionaly as a Drupal Security Team member you reasonably could have been expected to know that the module owners were attempting to gain security coverage for the module in #3561589: [1.0.x] Straker Translate to consider granting them Coordinated Disclosure.

I had a similar conversation with another one of your colleagues who was on the Drupal Security Team some time back (years) where they were debating how to handle a module in a similar position. I am unable to find the thread at this time however they ended up following Coordinated Disclosure principals even though the D.A and DST would encourage otherwise.

Note; I am not the maintainer, they may find the public disclosure of a security flaw acceptable. This message is intended to point out how you as an individual can improve the ecosystem security.

Maintainers: My apologias for this side track in your issue.

cpagar’s picture

cpagar commented

Thanks @cmlara and @prudloff for the response here.

I believe this was the only platform or way we communicate to resolve this findings.
for the security advisory policy we have an open case here --> https://www.drupal.org/project/projectapplications/issues/3561589

@prudloff even after the fixes applied by @jbhovik you can still see that there is a lot of routes? And may I ask currently the status is in active who will move it to another status like for review as joey has applied the fixed also who will review and decide that this has been resolved?

prudloff’s picture

prudloff commented

The maintainers of the module have full control of this issue, they can decide to change the status, mark it as fixed, etc.
It is OK to ask for a review but you don't need my permission before committing a fix.
And anyone from the community can help review the patch.

jbhovik’s picture

jbhovik commented
Status: Active » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

prudloff’s picture

prudloff commented

It seems this was marked as fixed without any commit?

jbhovik’s picture

jbhovik commented
Status: Fixed » Reviewed & tested by the community
jbhovik’s picture

jbhovik commented

@prudloff,

My mistake. I got pulled into another development effort.

naresh_bavaskar opened merge request !1

  • a91bd78a committed on 1.0.x 
    Issue #3566913 by prudloff, jbhovik: Fixed routes not protected against...
jbhovik’s picture

jbhovik commented

@prudloff,

Since we ran into a merge conflict for the Resolve #3566913 "Routes not protected" merge request, we just committed a patch to the 1.0.x branch within the main repo. Is that okay?

prudloff’s picture

prudloff commented

Yes you are not required to use merge requests, committing directly to the branch is also OK (as long as the commit message contains the issue ID).

If you believe this is now fixed you can set the issue status to Fixed.

jbhovik’s picture

jbhovik commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.