provide hidden variables to limit UI functionality (D7) [#1359384]

(this is based on some previous conversation, apologies if I skip some background information)

Backup and Migrate is a great tool in a lot of ways, but on a production environment it can currently be used for a lot of malicious things in a short amount of time: downloading a full db backup, uploading a new set of files/database code which might break the whole site, etc. It would be nice to provide ways limit that functionality though these settings, by nature, can't be in the UI and probably shouldn't be (they would be overly complex for 90% of the module's users).

Of course if someone has access to BAM they can probably also administer modules and other such things which would let them do malicious things in other ways - the idea here is *not* that disabling the BAM UI makes Drupal perfectly secure, it's just that it would require more work for someone to achieve actions which BAM makes trivial.

Comment	File	Size	Author
#14	backup_migrate-n1359384-14.patch	9.07 KB	BrankoC
#14	7.x-3.x: PHP 7.2 & MySQL 5.5, D7 17 pass PHP 7.3 & MySQL 5.7, D7 Patch Failed to Apply
#8	backup_migrate-n1359384-8-d7.patch	8.75 KB	DamienMcKenna
#8	7.x-3.x: PHP 7.1 & MySQL 5.5, D7 Patch Failed to Apply

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

couturier CreditAttribution: couturier as a volunteer commented 28 September 2017 at 15:36

Issue summary:	View changes
Status:	Active	» Postponed (maintainer needs more info)

I'm not sure that this is still an issue in the 7.x-3.x branch. The 7.x-2.x branch will be deprecated soon. See this issue: Verify 7.x-2.x -to- 7.x-3.x upgrade path, mark 7.x-2.x as unsupported.

Comment #2

greggles

he/him

English

Denver, Colorado, USA

CreditAttribution: greggles at CARD.com commented 28 September 2017 at 15:59

Version:	7.x-2.x-dev	» 7.x-3.x-dev
Status:	Postponed (maintainer needs more info)	» Active

The idea was to make a way to disable the UI in production, e.g. something in settings.php so the scenario works like this:

* admin installs BAM and configures it including a scheduled backup
* admin modifies their settings.php to disable the BAM admin UI
* if an attacker is able to login to the site now the attacker cannot modify the BAM settings to send the db backup to the attacker

I believe this is relevant for the 7.x-3.x and the 8.x branches, so moving it forward based on that belief.

Comment #3

couturier CreditAttribution: couturier as a volunteer commented 28 September 2017 at 16:06

Version:

7.x-3.x-dev

» 8.x-4.x-dev

If it is relevant to 8.x, then by all means let's move it there. All current development is taking place in the D8 port, and resources are very limited for this module right now.

Comment #4

Alex Andrascu CreditAttribution: Alex Andrascu at Intellix commented 28 September 2017 at 17:28

I agree with @greggles on this one and it might even worth backporting it. Patches welcome as always :)

Comment #5

Alex Andrascu CreditAttribution: Alex Andrascu at Intellix commented 28 September 2017 at 17:32

Issue tags:

+Security improvements

Comment #6

Alex Andrascu CreditAttribution: Alex Andrascu at Intellix commented 29 September 2017 at 18:36

Wondering if #2135827: The Download method should be related to the role permissions mitigates this in any way ? That's been recently fixed and will be pushed in the next release.

Comment #7

couturier CreditAttribution: couturier as a volunteer commented 29 September 2017 at 19:18

@Alex Andrascu This is a problem with both the 7.x-3.x and 8.x-4.x branch, that it appears @ronan has done some code changes in dev that weren't always seen in the issues, so that's why I've repeatedly asked reporters of the older issues to confirm that the problems still exist with your latest releases. This way we can find out what has been fixed and what hasn't. Also as you point out, some of the fixes that are marked as finished have already helped related issues.

Comment #8

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna at Mediacurrent commented 4 October 2017 at 15:50

Version:	8.x-4.x-dev	» 7.x-3.x-dev
Status:	Active	» Needs review

File	Size
backup_migrate-n1359384-8-d7.patch	8.75 KB
7.x-3.x: PHP 7.1 & MySQL 5.5, D7 Patch Failed to Apply

Maybe something like this? It's untested, but it gives you a general idea of one possible solution.

Comment #9

Alex Andrascu CreditAttribution: Alex Andrascu at Intellix commented 5 October 2017 at 17:11

This looks pretty solid to me. Can you please test the 7.x version and we'll look into the 8.x one ?

Comment #10

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna at Mediacurrent commented 5 October 2017 at 19:44

Status:	Needs review	» Needs work
Issue tags:		+Needs tests

We need a test to make sure it works as intended.

Comment #11

Alex Andrascu CreditAttribution: Alex Andrascu at Intellix commented 5 October 2017 at 20:08

We’re working on it. Should be in by early next week.

Later edit... just realised this is for 7.x. We are working on tests for 8.x ...

Comment #12

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna at Mediacurrent commented 5 October 2017 at 20:39

@Alex: No worries, I don't mean to put extra pressure on you, I just think that something with security implications definitely should have tests before it's committed. I'll see if I can throw something together soon (if someone doesn't beat me to it).

Comment #13

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna at Mediacurrent commented 19 November 2018 at 04:01

Title:

provide hidden variables to limit UI functionality

» provide hidden variables to limit UI functionality (D7)

Comment #14

BrankoC CreditAttribution: BrankoC as a volunteer commented 27 June 2019 at 11:50

File	Size
backup_migrate-n1359384-14.patch	9.07 KB
7.x-3.x: PHP 7.2 & MySQL 5.5, D7 17 pass PHP 7.3 & MySQL 5.7, D7 Patch Failed to Apply

A reroll of #8.

I haven't provided an interdiff, because I could not apply #8.

I added a bit to the documentation about clearing the menu cache.

I tried to create a test for this patch, but failed because 1) I don't know how to override variable overrides; and 2) I am not sure which settings file the testing module uses in the first place.

I had this so far:

  /**
   * Confirm the UI can be disabled in settigs.php.
   */
  public function testUIDisabled() {
    // Disable the UI and load the main B&M page.
    variable_set('backup_migrate_disable_ui', TRUE);
    cache_clear_all(NULL, 'cache_menu');
    $this->drupalGet(BACKUP_MIGRATE_MENU_PATH);
    $this->assertResponse(200); // Drupal will still return a valid admin page.
    $this->assertNoRaw('Backup and Migrate', 'Backup and Migrate UI absent.');

    // Enable the UI and load the main B&M page.
    variable_set('backup_migrate_disable_ui', FALSE);
    cache_clear_all(NULL, 'cache_menu');
    $this->drupalGet(BACKUP_MIGRATE_MENU_PATH);
    $this->assertResponse(200);
    $this->assertRaw('Backup and Migrate', 'Backup and Migrate UI present.');
  }