Problem/Motivation

This module has a (small) information disclosure vulnerability. This was initially reported as a security issue, but there it was deemed small enough to handle in public. For those that can reference the original, https://security.drupal.org/node/176404.

A security audit done for one of the sites my team maintains recommended to make sure cookies set by the site only apply to the site itself, and not to subdomains; both the Drupal session cookie and the TFA cookie set by this module's "trusted browser" plugin showed up as .www.example.com (note the leading dot, which means this also applies to subdomains of www.example.com).

For Drupal itself this is "easily" (actually, it took me some spelonking to find this out) accomplished by setting the cookie_domain container variable to empty string.

This module's trusted browser plugin sets the cookie domain to site's hostname explicitly, which at one point may have been a security measure. Modern browsers, however, treat a cookie that is set to the sites hostname explicitly such that it is also passed to subdomains (previously, a leading dot was needed for that behaviour, which is probably the root of this code). From https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie:

Domain= Optional
Defines the host to which the cookie will be sent.

If omitted, this attribute defaults to the host of the current document URL, not including subdomains.

Contrary to earlier specifications, leading dots in domain names (.example.com) are ignored.

Multiple host/domain values are not allowed, but if a domain is specified, then subdomains are always included.

Steps to reproduce

  • Configure the module, allowing the trusted browser login plugin
  • Log in with a user that has a role that needs TFA and has the "Setup TFA" permission
  • Set up TFA for the user
  • Make sure you click the "Trust this browser" checkbox
  • Inspect cookies. Notice how the domain for the TFA cookie has a leading dot (this may actually be down to your browser).

Proposed resolution

Set an empty cookie domain explicitly. The spirit of the code (ignoring the core cookie domain setting) seems to be to try and limit the cookie to just the current site's domain. Problem is that modern browsers changed how they interpret this and accomplish exactly what the code is trying to prevent (include subdomains).

We might consider turning it into a setting, although I really don't know how old a browser must be to have this very slight security benefit when getting an explicit cookie domain.

Also see https://www.drupal.org/project/drupal/issues/2940879

Remaining tasks

  • (Reach consencus about approach)
  • Create merge request
  • Merge

User interface changes

None.

API changes

None.

Data model changes

None.

Comments

eelkeblok created an issue. See original summary.

greggles’s picture

Adding some private discussion that explores potential solutions.

cmlara’s picture

Issue tags: +Security

Tagging TFA 8.x Security concerns that need to be addressed before a stable release.

jcnventura’s picture

Priority: Normal » Major
jcnventura’s picture

Made it a setting, reusing the same domain as core's session cookie domain when set to true. Which means it's valid in the same subdomains as Drupal's session cookie (or none if cookie_domain container variable is set to an empty string). If the setting is turned off, then it uses only the current domain.

  • 2350815 committed on 8.x-1.x
    Issue #3276595 by greggles, jcnventura, eelkeblok: Trusted browser...
jcnventura’s picture

Status: Active » Fixed

  • 2350815 committed on 2.x
    Issue #3276595 by greggles, jcnventura, eelkeblok: Trusted browser...

Status: Fixed » Closed (fixed)

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