Message-ID: <e136aba47e21e60735eac045f40add43e41ea8cb.camel@redhat.com>
Date:   Thu, 01 Apr 2021 13:53:22 -0400
From:   Simo Sorce <simo@...hat.com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     Ard Biesheuvel <ardb@...nel.org>, Dexuan Cui <decui@...rosoft.com>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        "crecklin@...hat.com" <crecklin@...hat.com>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: Fix hibernation in FIPS mode?

On Thu, 2021-04-01 at 18:31 +0200, Rafael J. Wysocki wrote:
> On Thu, Apr 1, 2021 at 6:22 PM Simo Sorce <simo@...hat.com> wrote:
> > On Thu, 2021-04-01 at 18:02 +0200, Rafael J. Wysocki wrote:
> > > On Thu, Apr 1, 2021 at 3:54 PM Ard Biesheuvel <ardb@...nel.org> wrote:
> > > > On Thu, 1 Apr 2021 at 15:38, Rafael J. Wysocki <rafael@...nel.org> wrote:
> > > > > On Thu, Apr 1, 2021 at 10:47 AM Ard Biesheuvel <ardb@...nel.org> wrote:
> > > > > > On Tue, 30 Mar 2021 at 21:56, Simo Sorce <simo@...hat.com> wrote:
> > > > > > > On Tue, 2021-03-30 at 21:45 +0200, Ard Biesheuvel wrote:
> > > > > > > > On Tue, 30 Mar 2021 at 20:05, Simo Sorce <simo@...hat.com> 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@...rosoft.com> 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@vger.kernel.org/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)
> > > 
> > > That depends on how you count.
> > > 
> > > Surely, there are modifications caught by MD5 that will not be caught by CRC32.
> > 
> > This is a technically correct statement, but does it matter in this
> > context? (Hint, probably not)
> > 
> > > > 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.
> > > 
> > > This clearly is a matter of opinion.
> > 
> > Sorry, but this is not a matter of opinion.
> > The only reason to use a cryptographic hash is that you want to protect
> > from active tampering, rather than from accidental changes. And if you
> > need to protect from active tampering then you cannot use a known
> > broken hash, there is no point.
> > 
> > OTOH if you do not care for active tampering but only to catch
> > transmission/storage errors then all you care for is error checking. In
> > that case a cryptographic hash is overkill because it entails a lot
> > more computation than is needed.
> 
> But the amount of data in question is not huge in this case.
> 
> > > I'm not religious about it though.  If there is a general consensus
> > > that CRC32 is sufficient for error detection in hibernation files,
> > > then it can be used.  So is there such a consensus and if so, can you
> > > give me a pointer to some research that it is based on?
> > 
> > CRC32 is an industry standard to check for accidental modifications of
> > a bit stream. The chances of missing an accidental change are 1 in 4
> > billion.
> 
> This is not about accidental change which basically is my point.
> 
> The BIOSes in question change the memory map over hibernation/resume,
> because they think that the memory layout is now different, so this is
> about detecting a sort of intentional change.  Definitely not random,
> though.

Ok, not random, but also not intentional, it is just "accidental".

> But as stated elsewhere, it is just about failing more gracefully at
> least in some cases, so let's just go ahead with using CRC32 here
> (worst case, it will not fail more gracefully in super-corner cases).

Sounds good.

Simo.

-- 
Simo Sorce
RHEL Crypto Team
Red Hat, Inc

Powered by blists - more mailing lists