Message-ID: <2da3f6d36dccb86f19292015ea48e5d7a89e3171.camel@HansenPartnership.com>
Date: Fri, 01 Aug 2025 14:03:47 -0400
From: James Bottomley <James.Bottomley@...senPartnership.com>
To: Eric Biggers <ebiggers@...nel.org>
Cc: Peter Huewe <peterhuewe@....de>, Jarkko Sakkinen <jarkko@...nel.org>, 
	linux-integrity@...r.kernel.org, Jason Gunthorpe <jgg@...pe.ca>, 
	linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org, 
	stable@...r.kernel.org
Subject: Re: [PATCH 1/2] tpm: Compare HMAC values in constant time

On Fri, 2025-08-01 at 10:11 -0700, Eric Biggers wrote:
> On Fri, Aug 01, 2025 at 07:36:02AM -0400, James Bottomley wrote:
> > On Thu, 2025-07-31 at 20:02 -0700, Eric Biggers wrote:
> > > On Thu, Jul 31, 2025 at 10:28:49PM -0400, James Bottomley wrote:
> > > > On Thu, 2025-07-31 at 14:52 -0700, Eric Biggers wrote:
> > > > > To prevent timing attacks, HMAC value comparison needs to be
> > > > > constant time.  Replace the memcmp() with the correct
> > > > > function, crypto_memneq().
> > > > 
> > > > Um, OK, I'm all for more security but how could there possibly
> > > > be a timing attack in the hmac final comparison code?  All it's
> > > > doing is seeing if the HMAC the TPM returns matches the
> > > > calculated one.  Beyond this calculation, there's nothing
> > > > secret about the HMAC key.
> > > 
> > > I'm not sure I understand your question.  Timing attacks on MAC
> > > validation are a well-known issue that can allow a valid MAC to
> > > be guessed without knowing the key.  Whether it's practical in
> > > this particular case for some architecture+compiler+kconfig
> > > combination is another question, but there's no reason not to use
> > > the constant-time comparison function that solves this problem.
> > > 
> > > Is your claim that in this case the key is public, so the MAC
> > > really just serves as a checksum (and thus the wrong primitive is
> > > being used)?
> > 
> > The keys used for TPM HMAC calculations are all derived from a
> > shared secret and updating parameters making them one time ones
> > which are never reused, so there's no benefit to an attacker
> > working out after the fact what the key was.
> 
> MAC timing attacks forge MACs; they don't leak the key.

> It's true that such attacks don't work with one-time keys.  But here
> it's not necessarily a one-time key.  E.g., tpm2_get_random() sets a
> key, then authenticates multiple messages using that key.

The nonces come one from us and one from the TPM.  I think ours doesn't
change if the session is continued although it could, whereas the TPM
one does, so the HMAC key is different for every communication of a
continued session.

> I guses I'm struggling to understand the point of your comments.

Your commit message, still quoted above, begins "To prevent timing
attacks ..." but I still don't think there are any viable timing
attacks against this code.  However, that statement gives the idea that
it's fixing a crypto vulnerablility and thus is going to excite the AI
based CVE producers.

>   Even if in a follow-up message you're finally able to present a
> correct argument for why memcmp() is okay, it's clearly subtle enough
> that we should just use crypto_memneq() anyway, just like everywhere
> else in the kernel that validates MACs.  If you're worried about
> performance, you shouldn't be: it's a negligible difference that is
> far outweighed by all the optimizations I've been making to
> lib/crypto/.

So if you change the justification to something like "crypto people
would like to update hmac compares to be constant time everywhere to
avoid having to check individual places for correctness" I think I'd be
happy.

Regards,

James

Powered by blists - more mailing lists