Todo #12556
closed
Comply with current iteration standards when encrypting and decrypting configuration files
Added by Phil Wardt almost 3 years ago. Updated over 2 years ago.
0%
Description
I pushed a commit since this should be really and easy enhancement:
https://github.com/pfsense/pfsense/pull/4545
OpenSSL currently defaults to 10'000 iterations, which is by far a low security standard that was acceptable before 2010.
Nowadays, even on entry level hardware, a count of 100'000 is the minimal.
The impact on import/export of config files is marginal for such an occasional task.
Storing the config file online with an iteration count of only 10'000 is really questionable.
Sure, even a 1'000'000 iterations count won't compensate for a weak password, but going with at least 100'000 is a minimum recommendation for a firewall
Files
| 1cfabed96df952c480c3bb17b93bdf53f0672326.patch (1.69 KB) 1cfabed96df952c480c3bb17b93bdf53f0672326.patch | Jim Pingle, 02/28/2022 02:19 PM |
Related issues
Updated by Jim Pingle almost 3 years ago
- Plus Target Version set to 22.01
For our own reference:
The man page doesn't state explicitly what the default number of iterations is, but it is set to 10k in the source:
https://github.com/pfsense/FreeBSD-src/blob/devel-12/crypto/openssl/apps/enc.c#L271
We'll need to test this on a few different varieties of hardware to ensure it doesn't slow down the process significantly. Also need to test that it can still decrypt backups made with the old iteration count, otherwise it will need a lot more logic to fall back to the old value if the current one fails, similar to the current fall back to "legacy" but it would be a new third tier.
Might not be a bad idea to make it a GUI option with a few choices (310k, 10k for old/slow hw, 750k, 1M, etc) but that could be a future/different feature request.
According to https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html 310k should be the minimum best practice value since it uses SHA256.
Updated by Phil Wardt almost 3 years ago
I added a note in github
Obviously, the current GUI will not be able to decode old backups
Updated by Viktor Gurov almost 3 years ago
Phil Wardt wrote in #note-2:
I added a note in github
Obviously, the current GUI will not be able to decode old backups
Unable to reproduce - I was able to restore 2.4.5-p1 backups on 2.5.2/2.6 appliance
Updated by Phil Wardt almost 3 years ago
Viktor Gurov wrote in #note-3:
Phil Wardt wrote in #note-2:
I added a note in github
Obviously, the current GUI will not be able to decode old backupsUnable to reproduce - I was able to restore 2.4.5-p1 backups on 2.5.2/2.6 appliance
I can confirm you are right. I was testing on a tarbal encrypted aes and piping to tar. Testing on a pfsense like code, openssl can restore a low iteration count encoded file if using a higher iterations count.I really was not aware of this and thought that iter count had to be the same as I always used a piped tar
So yes, my commit needs no further GUI changes. Old backups can be restored by new GUI, but old pfsense will not be able to restore new backups with teh higher iterations count
Hope this makes it possible for a quicker merge as no GUI changes are needed for backward compatibility with old backups
Updated by Phil Wardt almost 3 years ago
Viktor Gurov wrote in #note-3:
Phil Wardt wrote in #note-2:
I added a note in github
Obviously, the current GUI will not be able to decode old backupsUnable to reproduce - I was able to restore 2.4.5-p1 backups on 2.5.2/2.6 appliance
Well, I took time to test
If the iterations count is fixed to anything other than the current default 10000 iterations, the new GUI will not be able to restore old backups encoded with a different iterations count
The iterations count is not hard coded into the output file.
I found nothing in the sources or rfc, but Openssl doesn't check for success on each iteration. I know no option that enables it in current openssl
If we try to decrypt a config file with a different iterations count than the one set originally, it will fail:
bad decrypt
34371026944:error:06065064:digital envelope routines:EVP_DecryptFinal_ex:bad decrypt:/build/ce-crossbuild-252/sources/FreeBSD-src/crypto/openssl/crypto/evp/evp_enc.c:610:
I tested in the current freebsd host with a script to backup/decrypt. I also tested in different openssl environnements and I can confirm this
So, the only option is: if it fails with currently set iter count, it would try with current default
Updated by Phil Wardt almost 3 years ago
Viktor Gurov wrote in #note-3:
Phil Wardt wrote in #note-2:
I added a note in github
Obviously, the current GUI will not be able to decode old backupsUnable to reproduce - I was able to restore 2.4.5-p1 backups on 2.5.2/2.6 appliance
My submitted patch is proper in fact. I forgot that I properly set it this way:
$iter = ($legacy) ? "" : "-iter 500000";
So, if legacy is set, the iterations count is left to the current openssl default
Decrypting old backups works properly with this commit
Updated by Jim Pingle almost 3 years ago
- Assignee set to Jim Pingle
- Target version changed from 2.6.0 to CE-Next
- Plus Target Version changed from 22.01 to Plus-Next
Old backups can be restored what would fail is new backups made with different iteration counts. A backup made on a current snapshot would fail to restore once this patch is applied, for example. It needs to test three levels:
- Attempt to decrypt with current options and high iteration count
- Attempt to decrypt with current options and OpenSSL default iteration count
- Attempt to decrypt with legacy options
This is going to need more time than we have for the current release to make sure we do it properly.
Updated by Phil Wardt almost 3 years ago
Jim Pingle wrote in #note-7:
Old backups can be restored what would fail is new backups made with different iteration counts. A backup made on a current snapshot would fail to restore once this patch is applied, for example. It needs to test three levels:
- Attempt to decrypt with current options and high iteration count
- Attempt to decrypt with current options and OpenSSL default iteration count
- Attempt to decrypt with legacy optionsThis is going to need more time than we have for the current release to make sure we do it properly.
Yes, current non legacy backups will fail to restore because the iterations count must match and legacy would drop to md5 and other options
Just a note: once it is addressed, I'd set the default for 2.5.x version to 10'000 and not unset -iter option like I submitted. That way, if openssl ever changes the default, it doesn't break the current versions decode option
Feel free to close my github commit and start with a more complete GUI patch
Updated by Jim Pingle over 2 years ago
- Status changed from Pull Request Review to New
- Target version changed from CE-Next to 2.7.0
- Plus Target Version changed from Plus-Next to 22.05
Updated by Jim Pingle over 2 years ago
- Estimated time deleted (
0.01 h)
Based on the information in the link I posted previously, I tested iteration values of 310000 and 500000. At 310000 iterations even the SG-1000 could encrypt and decrypt a typical size configuration in under 5 seconds, which seems reasonable. At 500000 iterations it was about twice as long, but still between 8-11 seconds which doesn't seem out of line since this is typically a rare occurrence (when manually making or restoring a backup).
Based on that, a new default of 500k with a fallback test to check the current default of 10k if that fails seems to be a good place to start here.
Updated by Jim Pingle over 2 years ago
- Subject changed from Comply to current standards when encrypting config files for online upload to Comply with current iteration standards when encrypting and decrypting configuration files
- Status changed from In Progress to Pull Request Review
Updated by Phil Wardt over 2 years ago
Jim Pingle wrote in #note-11:
Based on the information in the link I posted previously, I tested iteration values of 310000 and 500000. At 310000 iterations even the SG-1000 could encrypt and decrypt a typical size configuration in under 5 seconds, which seems reasonable. At 500000 iterations it was about twice as long, but still between 8-11 seconds which doesn't seem out of line since this is typically a rare occurrence (when manually making or restoring a backup).
Based on that, a new default of 500k with a fallback test to check the current default of 10k if that fails seems to be a good place to start here.
I cannot see the source as the gitlab is not accessible from outside. In any case, I won't have time to compile, just review it online by source.
However, can you ensure that a non response time of 8-11 secs of the firewall is acceptable ? Some DOS attacks can occur when the firewall cannot respond. However, I assume that the SG-1000 is unlikely to exist in any corporate entity. Also, I think it is not acceptable to undermine security just to comply with such a low end CPU that will be probably not be sold any more in a short time...
500k should be the minimal nowadays
Updated by Jim Pingle over 2 years ago
- File 1cfabed96df952c480c3bb17b93bdf53f0672326.patch 1cfabed96df952c480c3bb17b93bdf53f0672326.patch added
I used the SG-1000 as a worst case as it's the slowest CPU I had on hand that might still be in general use. For that, 10s seems more than reasonable during the backup and restore actions. On more recent systems the time is ~1s or so.
Once the MR is merged it'll show up on Github publicly. In the meantime, I've attached a patch file with the change that could be applied locally with the system patches package.
With the patch applied, newly encrypted files use 500k iterations. When restoring it tries 500k iterations, then falls back to 10k, then falls back to the old legacy options. I tested it with configurations with this applied and with configs from 2.5.x/2.6.x as well as 2.4.x, all worked and the user doesn't have to know or care how it was made since it falls back gracefully.
Also if someone really wants to use higher values it's a one-line change in the source they could make manually. Adding a GUI option isn't viable since it would have to be set manually before restoring only to then be overridden by whatever is in the config being restored -- sort of a catch22/chicken-and-egg scenario going on there.
Updated by Jim Pingle over 2 years ago
- Status changed from Pull Request Review to Feedback
Changes merged. See dd9b24e95cf90bb5d1c61a693aea3b98b746d539 . Will be in snapshots tomorrow for testing.
Updated by Phil Wardt over 2 years ago
Jim Pingle wrote in #note-15:
Changes merged. See dd9b24e95cf90bb5d1c61a693aea3b98b746d539 . Will be in snapshots tomorrow for testing.
Thank you a lot,
The choice looks really good and transparent for users
I will patch using system patches and report back, but it really looks just perfect
Updated by Phil Wardt over 2 years ago
Jim Pingle wrote in #note-15:
Changes merged. See dd9b24e95cf90bb5d1c61a693aea3b98b746d539 . Will be in snapshots tomorrow for testing.
tested the new patch and it works as expected to backup and restore old/new format files
Updated by Jim Pingle over 2 years ago
Seems to OK here as well for backup/restore in the regular GUI page and ACB. A negative side effect seems to be that if you try to decrypt a backup with the wrong password it takes much longer to fail. Long enough that the GUI might time out, even on fast hardware. I'm not sure what we might be able to do there. I'm going to open a separate issue for that.
If you have the right password it can read older backups without issue.
Updated by Jim Pingle over 2 years ago
- Related to Regression #12897: Attempting to decrypt an encrypted backup with the wrong password makes the GUI timeout added
Updated by Jim Pingle over 2 years ago
- Status changed from Feedback to Resolved
Updated by Phil Wardt over 2 years ago
Jim Pingle wrote in #note-18:
Seems to OK here as well for backup/restore in the regular GUI page and ACB. A negative side effect seems to be that if you try to decrypt a backup with the wrong password it takes much longer to fail. Long enough that the GUI might time out, even on fast hardware. I'm not sure what we might be able to do there. I'm going to open a separate issue for that.
If you have the right password it can read older backups without issue.
the patch created a CPU loop
https://redmine.pfsense.org/issues/12897?next_issue_id=12896&prev_issue_id=12898