Project

General

Profile

Bug #17112

Persistence config files "insecure" backup get emptied in some situations

Added by intrigeri 6 months ago. Updated 2 months ago.

Status:
Resolved
Priority:
Elevated
Assignee:
-
Category:
Persistence
Target version:
Start date:
Due date:
% Done:

100%

Feature Branch:
bugfix/17112-dont-empty-persistence-config-backup
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Description

As we can see on #10976#note-73 and #10976#note-66, if the permissions/ownership of the root directory of the persistent filesystem are wrong, live-additional-software.conf.insecure_disabled ends up being empty, which is unfortunate as it's meant to be a backup that the user can later restore by hand.

I believe that's because every call to disable_and_create_empty_persistence_conf_file in live-persist will rename the current live-additional-software.conf to live-additional-software.conf.insecure_disabled. So if the user immediately fixes the problem, the first time it happens, then we're good. But if they reboot and unlock their persistence again, the (non-empty) backup gets overwritten by live-persist which replaces it with the new, empty config file it created during last boot.

Solution: ensure disable_and_create_empty_persistence_conf_file does not replace a non-empty .insecure_disabled backup file with an empty one.

Priority >> normal because it makes it harder to recover from the already painful #10976.

Note: I don't understand why, in Cody's report, the same problem did not happen for persistence.conf. There might be another problem on top of this one. But this should not block it from fixing the problem I've already understood here :)


Related issues

Related to Tails - Bug #10976: persistence.conf lost, recoverable by reconfiguring In Progress 02/12/2019
Related to Tails - Bug #17116: Improve lost persistence.conf instructions on known issues page Resolved
Blocks Tails - Feature #16209: Core work: Foundations Team Confirmed

Associated revisions

Revision ee6ff8d1 (diff)
Added by intrigeri 2 months ago

live-persist: don't backup empty configuration files (refs: #17112)

In some cases, the previous code would overwrite a non-empty backup file with an
empty one, making it harder to recover from the already painful #10976.

For example, if the permissions get wrong and we run
disable_and_create_empty_persistence_conf_file(), but the user does not
immediately fix the problem, then next time they unlock their persistence,
the (non-empty) backup gets overwritten by live-persist which replaces it with
the new, empty config file it created during last boot.

Revision 8614583e
Added by anonym 2 months ago

Merge remote-tracking branch 'origin/bugfix/17112-dont-empty-persistence-config-backup' into stable

Fix-committed: #17112

History

#1 Updated by intrigeri 6 months ago

  • Related to Bug #10976: persistence.conf lost, recoverable by reconfiguring added

#2 Updated by intrigeri 6 months ago

#3 Updated by cbrownstein 6 months ago

  • Related to Bug #17116: Improve lost persistence.conf instructions on known issues page added

#4 Updated by intrigeri 2 months ago

  • Status changed from Confirmed to In Progress

#5 Updated by intrigeri 2 months ago

  • Assignee set to intrigeri
  • Target version set to Tails_4.3
  • Feature Branch set to bugfix/17112-dont-empty-persistence-config-backup

#6 Updated by intrigeri 2 months ago

  • Status changed from In Progress to Needs Validation
  • Assignee deleted (intrigeri)

No regression spotted by Jenkins but that does not tell much: we have no test coverage for this code path.

I've tested this manually:

  1. Install an image built from this branch on a (virtual) USB stick
  2. Reboot, set up persistence, enable Additional Software
  3. Reboot, unlock persistence, install a package
  4. Reboot, unlock persistence, confirm that the additional package is installed
  5. Set unsafe permissions: chmod 777 /live/persistence/TailsData_unlocked
  6. Reboot, unlock persistence, confirm that:
    1. live-additional-software.conf was backup'ed and replaced by a new, empty file
    2. the persistent volume root directory still has unsafe access rights
    3. no persistent data directory was bind-mounted
  7. Reboot, unlock persistence, confirm that:
    1. the live-additional-software.conf and persistence.conf non-empty backup files are still there, and their mtime matches the previous reboot when they got created initially (while without the fix brought by this branch, they would have been replaced by empty files, which is no good)
    2. the persistent volume root directory still has unsafe access rights
    3. no persistent data directory was bind-mounted

#7 Updated by anonym 2 months ago

  • Assignee set to anonym

#8 Updated by anonym 2 months ago

intrigeri wrote:

No regression spotted by Jenkins

Confirmed, including a local run.

but that does not tell much: we have no test coverage for this code path.

Indeed. :)

I've tested this manually:

  1. Install an image built from this branch on a (virtual) USB stick
  2. Reboot, set up persistence, enable Additional Software
  3. Reboot, unlock persistence, install a package
  4. Reboot, unlock persistence, confirm that the additional package is installed
  5. Set unsafe permissions: chmod 777 /live/persistence/TailsData_unlocked
  6. Reboot, unlock persistence, confirm that:
    1. live-additional-software.conf was backup'ed and replaced by a new, empty file
    2. the persistent volume root directory still has unsafe access rights
    3. no persistent data directory was bind-mounted
  7. Reboot, unlock persistence, confirm that:
    1. the live-additional-software.conf and persistence.conf non-empty backup files are still there, and their mtime matches the previous reboot when they got created initially (while without the fix brought by this branch, they would have been replaced by empty files, which is no good)
    2. the persistent volume root directory still has unsafe access rights
    3. no persistent data directory was bind-mounted

I confirm both that this test makes sense, and that it works for me too.

And the code makes sense (TIL: test -s checks for existence of non-empty files). Merged!

#9 Updated by anonym 2 months ago

  • Status changed from Needs Validation to Resolved
  • Assignee deleted (anonym)
  • % Done changed from 0 to 100

Also available in: Atom PDF