Re: Fix hibernation in FIPS mode?

[Ard Biesheuvel]

On Thu, 1 Apr 2021 at 15:38, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Thu, Apr 1, 2021 at 10:47 AM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
> >
> > On Tue, 30 Mar 2021 at 21:56, Simo Sorce <simo@xxxxxxxxxx> wrote:
> > >
> > > On Tue, 2021-03-30 at 21:45 +0200, Ard Biesheuvel wrote:
> > > > On Tue, 30 Mar 2021 at 20:05, Simo Sorce <simo@xxxxxxxxxx> wrote:
> > > > > On Tue, 2021-03-30 at 16:46 +0200, Rafael J. Wysocki wrote:
> > > > > > On Tue, Mar 30, 2021 at 12:14 AM Dexuan Cui <decui@xxxxxxxxxxxxx> wrote:
> > > > > > > Hi,
> > > > > > > MD5 was marked incompliant with FIPS in 2009:
> > > > > > > a3bef3a31a19 ("crypto: testmgr - Skip algs not flagged fips_allowed in fips mode")
> > > > > > > a1915d51e8e7 ("crypto: testmgr - Mark algs allowed in fips mode")
> > > > > > >
> > > > > > > But hibernation_e820_save() is still using MD5, and fails in FIPS mode
> > > > > > > due to the 2018 patch:
> > > > > > > 749fa17093ff ("PM / hibernate: Check the success of generating md5 digest before hibernation")
> > > > > > >
> > > > > > > As a result, hibernation doesn't work when FIPS is on.
> > > > > > >
> > > > > > > Do you think if hibernation_e820_save() should be changed to use a
> > > > > > > FIPS-compliant algorithm like SHA-1?
> > > > > >
> > > > > > I would say yes, it should.
> > > > > >
> > > > > > > PS, currently it looks like FIPS mode is broken in the mainline:
> > > > > > > https://www.mail-archive.com/linux-crypto@xxxxxxxxxxxxxxx/msg49414.html
> > > > >
> > > > > FYI, SHA-1 is not a good choice, it is only permitted in HMAC
> > > > > constructions and only for specified uses. If you need to change
> > > > > algorithm you should go straight to SHA-2 or SHA-3 based hashes.
> > > > >
> > > >
> > > > What is the reason for using a [broken] cryptographic hash here? if
> > > > this is just an integrity check, better use CRC32
>
> Not really.
>
> CRC32 is not really sufficient for integrity checking here AFAICS.  It
> might be made a fallback option if MD5 is not available, but making it
> the default would be somewhat over the top IMO.
>
> > > If the integrity check is used exclusively to verify there were no
> > > accidental changes and is not used as a security measure, by all means
> > > I agree that using crc32 is a better idea.
> > >
> >
> > Looking at 62a03defeabd58f74e07ca030d6c21e069d4d88e which introduced
> > this, it is only a best effort check which is simply omitted if md5
> > happens to be unavailable, so there is definitely no need for crypto
> > here.
>
> Yes, it is about integrity checking only.  No, CRC32 is not equivalent
> to MD5 in that respect AFAICS.
>

There are two possibilities:
- we care about an adversary attempting to forge a collision, in which
case you need a cryptographic hash which is not broken;
- we only care about integrity, in which case crypto is overkill, and
CRC32 is sufficient. (Note that the likelihood of an honest,
inadvertent modification not being caught by CRC32 is 1 in 4 billion)

MD5 does not meet either requirement, given that it is known to be
broken, and overkill for simple integrity checks. MD5 should be phased
out and removed, and moving this code onto the correct abstraction
would be a reasonable step towards that goal.