pfSense

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

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

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!

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

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

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

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