Project

General

Profile

Actions

Todo #12556

open

Comply to current standards when encrypting config files for online upload

Added by Phil Wardt about 2 months ago. Updated about 2 months ago.

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

0%

Estimated time:
0.01 h
Plus Target Version:
Plus-Next
Release Notes:
Default

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

Actions #1

Updated by Jim Pingle about 2 months 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.

Actions #2

Updated by Phil Wardt about 2 months ago

I added a note in github
Obviously, the current GUI will not be able to decode old backups

Actions #3

Updated by Viktor Gurov about 2 months 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

Actions #4

Updated by Phil Wardt about 2 months 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 backups

Unable 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

Actions #5

Updated by Phil Wardt about 2 months 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 backups

Unable 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

Actions #6

Updated by Phil Wardt about 2 months 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 backups

Unable 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

Actions #7

Updated by Jim Pingle about 2 months 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.

Actions #8

Updated by Phil Wardt about 2 months 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 options

This 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

Actions

Also available in: Atom PDF