Project

General

Profile

Actions

Regression #12897

closed

Attempting to decrypt an encrypted backup with the wrong password makes the GUI timeout

Added by Jim Pingle over 2 years ago. Updated over 2 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Backup / Restore
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
Plus Target Version:
22.05
Release Notes:
Force Exclusion
Affected Version:
Affected Architecture:

Description

Following the changes in #12556 attempting to decrypt an encrypted backup with the wrong password makes the GUI timeout. This happens from diag_backup.php as well as through AutoConfigBackup.

Previously the GUI would display an error stating "Could not decrypt config.xml" in a timely manner.

This may be due to the heavier encryption in use, but even so it seems to take much longer to fail than it should.

Might need to add a timeout of some sort in the process to prevent it from overrunning the GUI timeout. This could be similar to what we do for the NTP bootstrap (See #12769 and source:src/etc/rc.bootup#L344 )


Files

crypt.patch (1.92 KB) crypt.patch Iterations count proper Phil Wardt, 03/05/2022 05:42 AM

Related issues

Related to Todo #12556: Comply with current iteration standards when encrypting and decrypting configuration filesResolvedJim Pingle

Actions
Actions #1

Updated by Jim Pingle over 2 years ago

  • Related to Todo #12556: Comply with current iteration standards when encrypting and decrypting configuration files added
Actions #2

Updated by Phil Wardt over 2 years ago

Jim Pingle wrote:

Following the changes in #12556 attempting to decrypt an encrypted backup with the wrong password makes the GUI timeout. This happens from diag_backup.php as well as through AutoConfigBackup.

Previously the GUI would display an error stating "Could not decrypt config.xml" in a timely manner.

This may be due to the heavier encryption in use, but even so it seems to take much longer to fail than it should.

Might need to add a timeout of some sort in the process to prevent it from overrunning the GUI timeout. This could be similar to what we do for the NTP bootstrap (See #12769 and source:src/etc/rc.bootup#L344 )

I missed this bug on quick review of your commit
It is not a process decrypt taking too long, but an infinite decryption loop hogging the firewall CPU
The patch creates an infinite CPU loop when a decrypt fails for whatever reason. You can reproduce it with a custom backup using some custom iterations like 5000
Here's the fix:
https://github.com/pfsense/pfsense/pull/4559

I did not test, but it should fix the infinite loop on failed decrypt
Best regards

Actions #3

Updated by Jim Pingle over 2 years ago

  • Status changed from New to Feedback

Yep, I see it now, too. Good catch, thanks! I merged your PR, it will be in the next snapshot.

Actions #4

Updated by Phil Wardt over 2 years ago

Jim Pingle wrote in #note-3:

Yep, I see it now, too. Good catch, thanks! I merged your PR, it will be in the next snapshot.

please test it before merging, even if it looks proper to me

Actions #5

Updated by Jim Pingle over 2 years ago

Phil Wardt wrote in #note-4:

please test it before merging, even if it looks proper to me

I did, and it worked as expected. It failed in a timely manner with the correct error like it should. Thanks again!

Actions #6

Updated by Phil Wardt over 2 years ago

Jim Pingle wrote in #note-5:

Phil Wardt wrote in #note-4:

please test it before merging, even if it looks proper to me

I did, and it worked as expected. It failed in a timely manner with the correct error like it should. Thanks again!

Hi,
Can you look at this pull request please:
https://github.com/pfsense/pfsense/pull/4559/commits/4272ff66da4d4812a455d51c2dbe66a5ef28c291

I tested it in a custom patch and it seems fine
Basically, when I restored config with wrong pass or custom iterations, the temp files "php-encrypt*" filled the 256 GB of disk space
I had to delete them manually in shell and it took some time as they were so many and needed a find . command because rm args were exceeded !

Also, I noticed that the fall back to legacy could occur also during encryption, which could be a security issue. The risk is limited I agree because it means /tmp is compromised and a user tempers with the out temp file. However, if for any I/O reason the write fails temporarily, it could drop to 10k or even legacy no iterations backup. This should never occur in new versions

Actions #7

Updated by Phil Wardt over 2 years ago

And a full patch attached that I properly tested
It should be applied in place of https://redmine.pfsense.org/issues/12556

Actions #8

Updated by Phil Wardt over 2 years ago

Jim Pingle wrote in #note-5:

I did, and it worked as expected. It failed in a timely manner with the correct error like it should. Thanks again!

I pushed a mergeable fix based on current branch + fixed comments and made it more clear on why the temp files can remain in current release (during decryption if CPU is overloaded for any reason)
Also, it prevent any theorical generation of a poorly encrypted config without the user being aware of it
https://github.com/pfsense/pfsense/pull/4560

Best regards

Actions #9

Updated by Phil Wardt over 2 years ago

the clean of temp files lines are also maybe excessive. This can only occur if at the end, the GUI times out
Maybe I am too agressive here in prevention since I faced the disk full saturation issue with previous patch...

Feel free to keep it, remove these lines or implement it differently
Avoiding unwanted creation of low encryption backup files should be implemented though I think

Actions #10

Updated by Jim Pingle over 2 years ago

  • % Done changed from 0 to 100

I took a slightly different approach since I wasn't a fan of the repetition of the cleanup code.

I also added a PHP shell script which makes testing a lot easier.

$ pfSsh.php playback cryptconfig encrypt /conf/config.xml /root/enctest/test.xml
$ pfSsh.php playback cryptconfig decrypt /root/enctest/test.xml /root/enctest/out.xml

It prompts for the password in the terminal.

See https://github.com/pfsense/pfsense/commit/2404ca68b18b89b03083a3e6f127080a272a6eb3

Actions #11

Updated by Phil Wardt over 2 years ago

Jim Pingle wrote in #note-10:

I took a slightly different approach since I wasn't a fan of the repetition of the cleanup code.

I also added a PHP shell script which makes testing a lot easier.

[...]

It prompts for the password in the terminal.

See https://github.com/pfsense/pfsense/commit/2404ca68b18b89b03083a3e6f127080a272a6eb3

Looks the same result to me, thank you for the implementation and best regards

Actions #12

Updated by Christopher Cope over 2 years ago

  • Status changed from Feedback to Resolved

Tested and working correctly on

22.05-DEVELOPMENT (amd64)
built on Mon Mar 28 06:21:39 UTC 2022
FreeBSD 12.3-STABLE

marking resolved.

Actions

Also available in: Atom PDF