git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "brian m. carlson" <sandals@crustytoothpaste.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>
Subject: Re: [PATCH] builtin/receive-pack: use constant-time comparison for HMAC value
Date: Fri, 10 Apr 2020 00:56:03 +0000	[thread overview]
Message-ID: <20200410005603.GC6639@camp.crustytoothpaste.net> (raw)
In-Reply-To: <xmqqr1wwkxqe.fsf@gitster.c.googlers.com>

[-- Attachment #1: Type: text/plain, Size: 3066 bytes --]

On 2020-04-10 at 00:09:29, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > When we're comparing a push cert nonce, we currently do so using strcmp.
> > Most implementations of strcmp short-circuit and exit as soon as they
> > know whether two values are equal.  This, however, is a problem when
> > we're comparing the output of HMAC, as it leaks information in the time
> > taken about how much of the two values match if they do indeed differ.
> >
> > In our case, the nonce is used to prevent replay attacks against our
> > server via the embedded timestamp and replay attacks using requests from
> > a different server via the HMAC.  Push certs, which contain the nonces,
> > are signed, so an attacker cannot tamper with the nonces without
> > breaking validation of the signature.  They can, of course, create their
> > own signatures with invalid nonces, but they can also create their own
> > signatures with valid nonces, so there's nothing to be gained.  Thus,
> > there is no security problem.
> >
> > Even though it doesn't appear that there are any negative consequences
> > from the current technique, for safety and to encourage good practices,
> > let's use a constant time comparison function for nonce verification.
> > POSIX does not provide one, but they are easy to write.
> 
> Devil's advocate mode on.
> 
> If the HMAC plus digital signature are the real security, even
> though writing this patch may be a nice mental exercise, is there a
> merit in deliberately adding more code and making the code
> immesurably slower by applying it?
> 
> You just established in the previous paragraph that "for safety" is
> a red herring.

Here's the thing: I'm pretty sure there's not a security issue, but I'm
not a cryptologist and can't speak for certain.  We didn't think TLS had
all of the issues it did, until we determined that it did.  If someone
who is a cryptologist comes up with an attack scenario we hadn't
considered, we're covered.

Also, if we ever use this HMAC implementation for something else in the
future, that use might be vulnerable.  So it makes sense to always use a
constant time comparison for HMAC so it looks wrong not to.  I think
it's a good idea to make people feel icky about doing things like
non-constant HMAC comparisons or using MD5 even in the rare case where
it doesn't matter because almost all of the time, it does.

If we were using a language like C++ or Rust that returned a typed value
for our results, we'd get this behavior for free, and we wouldn't know
or care about the extremely minor performance impact.  The cost of using
a constant time comparison instead of an optimized strcmp is going to be
a handful of cycles and be dwarfed by the fact that we're forking and
execing a process (and moreover, probably a shell) to verify the push
cert.

So I think this is valuable, although I understand if you don't want to
take it.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

  parent reply	other threads:[~2020-04-10  0:56 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-09 23:37 [PATCH] builtin/receive-pack: use constant-time comparison for HMAC value brian m. carlson
2020-04-10  0:09 ` Junio C Hamano
2020-04-10  0:21   ` Junio C Hamano
2020-04-10  0:56   ` brian m. carlson [this message]
2020-04-22 10:27 ` SZEDER Gábor
2020-04-22 15:57   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200410005603.GC6639@camp.crustytoothpaste.net \
    --to=sandals@crustytoothpaste.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).