* Re: SHA1 collisions found
From: David Lang @ 2017-02-23 17:52 UTC (permalink / raw)
To: Joey Hess; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <20170223173547.qljypk7sdqi37oha@kitenet.net>
On Thu, 23 Feb 2017, Joey Hess wrote:
> Junio C Hamano wrote:
>> On Thu, Feb 23, 2017 at 8:43 AM, Joey Hess <id@joeyh.name> wrote:
>>>
>>> Since we now have collisions in valid PDF files, collisions in valid git
>>> commit and tree objects are probably able to be constructed.
>>
>> That may be true, but
>> https://public-inbox.org/git/Pine.LNX.4.58.0504291221250.18901@ppc970.osdl.org/
>
> That's about someone replacing an valid object in Linus's repository
> with an invalid random blob they found that collides. This SHA1
> break doesn't allow generating such a blob anyway. Linus is right,
> that's an impractical attack.
>
> Attacks using this SHA1 break will look something more like:
>
> * I push a "bad" object to a repo on github I set up under a
> pseudonym.
> * I publish a "good" object in a commit and convince the maintainer to
> merge it.
> * I wait for the maintainer to push to github.
> * I wait for github to deduplicate and hope they'll replace the good
> object with the bad one I pre-uploaded, thus silently changing the
> content of the good commit the maintainer reviewed and pushed.
> * The bad object is pulled from github and deployed.
> * The maintainer still has the good object. They may not notice the bad
> object is out there for a long time.
>
> Of course, it doesn't need to involve Github, and doesn't need to
> rely on internal details of their deduplication[1];
> that only let me publish the bad object under a psydonym.
read that e-mail again, it covers the case where a central server gets a blob
replaced in it.
tricking a maintainerinto accepting a file that contains huge amounts of binary
data in it is going to be a non-trivial task, and even after you trick them into
accepting one bad file, you then need to replace the file they accepted with a
new one (breaking into github or assuming that github is putting both files into
the same repo, both of which are fairly unlikely)
David Lang
^ permalink raw reply
* Re: SHA1 collisions found
From: Joey Hess @ 2017-02-23 18:21 UTC (permalink / raw)
To: Linus Torvalds, Git Mailing List
In-Reply-To: <CA+55aFzFEpi1crykZ33r9f7BsvLt_kiB-CHXOkuCAX=fd4BU-w@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 591 bytes --]
Linus Torvalds wrote:
> What you describe pretty much already requires a pre-image attack,
> which the new attack is _not_.
>
> It's not clear that the "good" object can be anything sane.
Generate a regular commit object; use the entire commit object + NUL as the
chosen prefix, and use the identical-prefix collision attack to generate
the colliding good/bad objects.
(The size in git's object header is a minor complication. Set the size
field to something sufficiently large, and then pad out the colliding
objects to that size once they're generated.)
--
see shy jo
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: SHA1 collisions found
From: Linus Torvalds @ 2017-02-23 18:29 UTC (permalink / raw)
To: Joey Hess; +Cc: Git Mailing List
In-Reply-To: <20170223181018.ns4vyosgzmuoyiva@kitenet.net>
On Thu, Feb 23, 2017 at 10:10 AM, Joey Hess <id@joeyh.name> wrote:
>
> It would cost 6500 CPU years + 100 GPU years to generate valid colliding
> git objects using the methods of the paper's authors. That might be cost
> effective if it helped get a backdoor into eg, the kernel.
I still think it also needs to be interesting enough data, not just
random noise that is then trivial to find with automated tools.
Because for the kernel, it's not just that an attacker needs to do the
CPU time. Yes, first he needs the technical resources to just do just
the attack and create the situation you described.
But then he *also* needs to build up the social capital to get the end
result pulled into the tree (ie if he depends on the hidden spaces, he
needs somebody to actually do a git pull, not just apply a patch).
.. and if we then have a tool that then finds the problem trivially
(ie "git fsck"), he's not only wasted all those technical resources,
he's also burned his identity.
>> (b) we can probably easily add some extra sanity checks to the opaque
>> data we do have, to make it much harder to do the hiding of random
>> data that these attacks pretty much always depend on.
>
> For example, git fsck does warn about a commit message with opaque
> data hidden after a NUL. But, git show/merge/pull give no indication
> that something funky is going on when working with such commits.
I do agree that we might want to do some of the fsck checks
particularly at fetch time. That's when doing checks is both relevant
and cheap.
So we could do the opaque data checks, but we could/should probably
also add the attack pattern ("disturbance vectors") checks.
And the thing is, adding those checks is really cheap, and basically
makes the whole attack vector pointless against git.
Because unlike some "signing a pdf" attack, git doesn't fundamentally
depend on the SHA1 as some kind of absolute security. If we have the
minimal machinery in git to just notice the attack, the attack
essentially goes away. Attackers can waste infinite amounts of CPU
time, and if it's cheap for us to notice, it completely disarms all
that attack work.
Again, I'm not arguing that people shouldn't work on extending git to
a new (and bigger) hash. I think that's a no-brainer, and we do want
to have a path to eventually move towards SHA3-256 or whatever.
But I'm very definitely arguing that the current attack doesn't
actually sound like it really even _matters_, because it should be so
easy to mitigate against.
Linus
^ permalink raw reply
* Re: SHA1 collisions found
From: Junio C Hamano @ 2017-02-23 18:38 UTC (permalink / raw)
To: Joey Hess; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20170223181018.ns4vyosgzmuoyiva@kitenet.net>
Joey Hess <id@joeyh.name> writes:
> For example, git fsck does warn about a commit message with opaque
> data hidden after a NUL. But, git show/merge/pull give no indication
> that something funky is going on when working with such commits.
Would
$ git config transfer.fsckobjects true
help?
^ permalink raw reply
* Re: SHA1 collisions found
From: Linus Torvalds @ 2017-02-23 18:40 UTC (permalink / raw)
To: Joey Hess; +Cc: Git Mailing List
In-Reply-To: <20170223182147.hbsyxsmyijgkqu75@kitenet.net>
On Thu, Feb 23, 2017 at 10:21 AM, Joey Hess <id@joeyh.name> wrote:
> Linus Torvalds wrote:
>> What you describe pretty much already requires a pre-image attack,
>> which the new attack is _not_.
>>
>> It's not clear that the "good" object can be anything sane.
>
> Generate a regular commit object; use the entire commit object + NUL as the
> chosen prefix, and use the identical-prefix collision attack to generate
> the colliding good/bad objects.
So I agree with you that we need to make git check for the opaque
data. I think I was the one who brought that whole argument up.
But even then, what you describe doesn't work. What you describe just
replaces the opaque data - that git doesn't actually *use*, and that
nobody sees - with another piece of opaque data.
You also need to make the non-opaque data of the bad object be
something that actually encodes valid git data with interesting hashes
in it (for the parent/tree/whatever pointers).
So you don't have just that "chosen prefix". You actually need to also
fill in some very specific piece of data *in* the attack parts itself.
And you need to do this in the exact same size (because that's part of
the prefix), etc etc.
So I think it's challenging.
... and then we can discover it trivially.
Ok, so "git fsck" right now takes a couple of minutes for me and I
don't actually run it very often (I used to run it religiously back in
the days), but afaik kernel.org actually runs it nightly. So it's
pretty much "trivially discoverable" - imagine spending thousands of
CPU-hours and lots of social capital to get an attack in, and then the
next night the kernel.org fsck complains about the strange commit you
added?
Linus
^ permalink raw reply
* Re: SHA1 collisions found
From: Jeff King @ 2017-02-23 18:46 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Joey Hess, Git Mailing List
In-Reply-To: <CA+55aFxckeEW1ePcebrgG4iN4Lp62A2vU6tA=xnSDC_BnKQiCQ@mail.gmail.com>
On Thu, Feb 23, 2017 at 10:40:48AM -0800, Linus Torvalds wrote:
> > Generate a regular commit object; use the entire commit object + NUL as the
> > chosen prefix, and use the identical-prefix collision attack to generate
> > the colliding good/bad objects.
>
> So I agree with you that we need to make git check for the opaque
> data. I think I was the one who brought that whole argument up.
We do already.
> But even then, what you describe doesn't work. What you describe just
> replaces the opaque data - that git doesn't actually *use*, and that
> nobody sees - with another piece of opaque data.
>
> You also need to make the non-opaque data of the bad object be
> something that actually encodes valid git data with interesting hashes
> in it (for the parent/tree/whatever pointers).
>
> So you don't have just that "chosen prefix". You actually need to also
> fill in some very specific piece of data *in* the attack parts itself.
> And you need to do this in the exact same size (because that's part of
> the prefix), etc etc.
It's not an identical prefix, but I think collision attacks generally
are along the lines of selecting two prefixes followed by garbage, and
then mutating the garbage on both sides. That would "work" in this case
(modulo the fact that git would complain about the NUL).
I haven't read the paper yet to see if that is the case here, though.
A related case is if you could stick a "cruft ...." header at the end of
the commit headers, and mutate its value (avoiding newlines). fsck
doesn't complain about that.
-Peff
^ permalink raw reply
* [PATCH] upload-pack: report "not our ref" to client
From: Jonathan Tan @ 2017-02-23 18:43 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster
In-Reply-To: <xmqqo9xtajcu.fsf@gitster.mtv.corp.google.com>
Make upload-pack report "not our ref" errors to the client as an "ERR" line.
(If not, the client would be left waiting for a response when the server is
already dead.)
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Thanks, here is the independent patch.
upload-pack.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/upload-pack.c b/upload-pack.c
index 7597ba340..ffb028d62 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -822,9 +822,13 @@ static void receive_needs(void)
use_include_tag = 1;
o = parse_object(sha1_buf);
- if (!o)
+ if (!o) {
+ packet_write_fmt(1,
+ "ERR upload-pack: not our ref %s",
+ sha1_to_hex(sha1_buf));
die("git upload-pack: not our ref %s",
sha1_to_hex(sha1_buf));
+ }
if (!(o->flags & WANTED)) {
o->flags |= WANTED;
if (!((allow_unadvertised_object_request & ALLOW_ANY_SHA1) == ALLOW_ANY_SHA1
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* Re: [PATCH v2] send-email: only allow one address per body tag
From: Junio C Hamano @ 2017-02-23 18:53 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Johan Hovold, Jeff King, Kevin Daudt, Larry Finger, git
In-Reply-To: <vpqo9xxkqqo.fsf@anie.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Johan Hovold <johan@kernel.org> writes:
>
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -1563,7 +1563,7 @@ foreach my $t (@files) {
>> # Now parse the message body
>> while(<$fh>) {
>> $message .= $_;
>> - if (/^(Signed-off-by|Cc): (.*)$/i) {
>> + if (/^(Signed-off-by|Cc): ([^>]*>?)/i) {
>
> I think this is acceptable, but this doesn't work with trailers like
>
> Cc: "Some > Body" <Some.Body@example.com>
>
> A proper management of this kind of weird address should be doable by
> reusing the regexp parsing "..." in parse_mailbox:
>
> my $re_quote = qr/"(?:[^\"\\]|\\.)*"/;
>
> So the final regex would look like
>
> if (/^(Signed-off-by|Cc): (([^>]*|"(?:[^\"\\]|\\.)*")>?)/i) {
>
> I don't think that should block the patch inclusion, but it may be worth
> considering.
>
> Anyway, thanks for the patch!
Somehow this fell off the radar. So your reviewed-by: and then
we'll cook this in 'next' for a while?
Thanks.
^ permalink raw reply
* Re: SHA1 collisions found
From: Joey Hess @ 2017-02-23 18:31 UTC (permalink / raw)
To: Linus Torvalds, Git Mailing List
In-Reply-To: <20170223182147.hbsyxsmyijgkqu75@kitenet.net>
[-- Attachment #1: Type: text/plain, Size: 886 bytes --]
Joey Hess wrote:
> Linus Torvalds wrote:
> > What you describe pretty much already requires a pre-image attack,
> > which the new attack is _not_.
> >
> > It's not clear that the "good" object can be anything sane.
>
> Generate a regular commit object; use the entire commit object + NUL as the
> chosen prefix, and use the identical-prefix collision attack to generate
> the colliding good/bad objects.
>
> (The size in git's object header is a minor complication. Set the size
> field to something sufficiently large, and then pad out the colliding
> objects to that size once they're generated.)
Sorry! While that would work, it's a useless attack because the good and bad
commit objects still point to the same tree.
It would be interesting to have such colliding objects, to see what beaks,
but probably not worth $75k to generate them.
--
see shy jo
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Junio C Hamano @ 2017-02-23 19:06 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Jeff King, David Turner, git@vger.kernel.org,
sandals@crustytoothpaste.net, Eric Sunshine
In-Reply-To: <alpine.DEB.2.20.1702231806340.3767@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Wed, 22 Feb 2017, Jeff King wrote:
>> This patch drops the useless probe request:
> ...
>> but setting http.emptyauth adds back in the useless request. I think
>> that could be fixed by skipping the empty-auth thing when
>> http_auth_methods does not have CURLAUTH_NEGOTIATE in it (or perhaps
>> other methods need it to, so maybe skip it if _just_ BASIC is set).
>>
>> I suspect the patch above could probably be generalized as:
>>
>> /* cut out methods we know the server doesn't support */
>> http_auth_methods &= results.auth_avail;
>>
>> and let curl figure it out from there.
>
> Maybe this patch (or a variation thereof) would also be able to fix this
> problem with the patch:
>
> https://github.com/git-for-windows/git/issues/1034
>
> Short version: for certain servers (that do *not* advertise Negotiate),
> setting emptyauth to true will result in a failed fetch, without letting
> the user type in their credentials.
The issue described in that page looks rather serious.
I believe that a "variation" has become the first part of a
two-patch series that appear in the downthread from here. Perhaps
you can ask them to test it out (or even better if you have a setup
you can easily test against yourself)?
^ permalink raw reply
* Re: SHA1 collisions found
From: Jeff King @ 2017-02-23 18:42 UTC (permalink / raw)
To: Joey Hess; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20170223182147.hbsyxsmyijgkqu75@kitenet.net>
On Thu, Feb 23, 2017 at 02:21:47PM -0400, Joey Hess wrote:
> Linus Torvalds wrote:
> > What you describe pretty much already requires a pre-image attack,
> > which the new attack is _not_.
> >
> > It's not clear that the "good" object can be anything sane.
>
> Generate a regular commit object; use the entire commit object + NUL as the
> chosen prefix, and use the identical-prefix collision attack to generate
> the colliding good/bad objects.
FWIW, git-fsck complains about those (and transfer.fsck rejects them):
$ (git cat-file commit HEAD; printf '\0more stuff') |
git hash-object -w --stdin -t commit
ecb2e5165c184f9025cb4c49d8f75901f4830354
$ git fsck
warning in commit ecb2e5165c184f9025cb4c49d8f75901f4830354: nulInCommit: NUL byte in the commit object body
So as long as either your "good" or "evil" commit has binary junk in it,
you are likely to be noticed (not everybody turns on transfer.fsck, but
GitHub does).
-Peff
^ permalink raw reply
* Re: SHA1 collisions found
From: Linus Torvalds @ 2017-02-23 19:09 UTC (permalink / raw)
To: Jeff King; +Cc: Joey Hess, Git Mailing List
In-Reply-To: <20170223184637.xr74k42vc6y2pmse@sigill.intra.peff.net>
On Thu, Feb 23, 2017 at 10:46 AM, Jeff King <peff@peff.net> wrote:
>>
>> So I agree with you that we need to make git check for the opaque
>> data. I think I was the one who brought that whole argument up.
>
> We do already.
I'm aware of the fsck checks, but I have to admit I wasn't aware of
'transfer.fsckobjects'. I should turn that on myself.
Or maybe git should just turn it on by default? At least the
per-object fsck costs should be essentially free compared to the
network costs when you just apply them to the incoming objects.
I also do think that it would be good to check for the disturbance
vectors at receive time (and fsck). Not necessarily interesting during
normal operations.
And in particular, while the *kernel* doesn't generally have critical
opaque blobs, other projects do. Things like firmware images etc are
open to attack, and crazy people put ISO images in repositories etc.
So I don't think this discussion should focus exclusively on the git metadata.
It is likely much easier to replace a binary blob than it is to
replace a commit or tree (or a source file that has to go through a
compiler). And for many projects, that would be a bad thing.
> It's not an identical prefix, but I think collision attacks generally
> are along the lines of selecting two prefixes followed by garbage, and
> then mutating the garbage on both sides. That would "work" in this case
> (modulo the fact that git would complain about the NUL).
I think this particular attack depended on an actual identical prefix,
but I didn't go back to the paper and check.
But the attacks tend to very much depend on particular input bit
patterns that have very particular effects on the resulting
intermediate hash, and those bit patterns are specific to the hash and
known.
So a very powerful defense is to just look for those bit patterns in
the objects, and just warn about them. Those patterns don't tend to
exist in normal inputs anyway, but particularly if you just warn, it's
a heads-ups that "ok, something iffy is going on"
And as mentioned, a cheap "something iffy is going on" thing is
basically a death sentence to SCM attacks.
The whole _point_ of an SCM is that it isn't about a one-time event,
but about continuous history. That also fundamentally means that a
successful attack needs to work over time, and not be detectable.
In contrast, many other uses of hashes are "one-time" events. If you
use a hash to validate a single piece of data from a source that you
wouldn't otherwise trust, it's a one-time "all or nothing" trust
situation.
And the attack surface is very different for those "one-time" vs
"trust over time" cases. If you can get a bank to trust a session one
time, you can empty a bank account and live on a paradise island for
the rest of your life. It doesn't matter if it gets detected or not
after-the-fact.
But if you can fool a SCM one time, insert your code, and it gets
detected next week, you didn't actually do anything useful. You only
burned yourself.
See the difference? One-time vs having a continual interaction makes a
*fundamntal* difference in game theory.
Linus
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Junio C Hamano @ 2017-02-23 19:11 UTC (permalink / raw)
To: Jeff King
Cc: brian m. carlson, David Turner, git@vger.kernel.org,
Johannes Schindelin, Eric Sunshine
In-Reply-To: <20170222234246.wjp3567vesdusiaf@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Feb 22, 2017 at 11:34:19PM +0000, brian m. carlson wrote:
>
>> Browsers usually disable this feature by default, as it basically will
>> attempt to authenticate to any site that sends a 401. For Kerberos
>> against a malicious site, the user will either not have a valid ticket
>> for that domain, or the user's Kerberos server will refuse to provide a
>> ticket to pass to the server, so there's no security risk involved.
>>
>> I'm unclear how SPNEGO works with NTLM, so I can't speak for the
>> security of it. From what I understand of NTLM and from RFC 4559, it
>> consists of a shared secret. I'm unsure what security measures are in
>> place to not send that to an untrusted server.
>>
>> As far as Kerberos, this is a desirable feature to have enabled, with
>> little downside. I just don't know about the security of the NTLM part,
>> and I don't think we should take this patch unless we're sure we know
>> the consequences of it.
>
> Hmm. That would be a problem with my proposed patch 2 then, too, if only
> because it turns the feature on by default in more places.
>
> If it _is_ dangerous to turn on all the time, I'd think we should
> consider warning people in the http.emptyauth documentation.
I presume that we have finished discussing the security
ramification, and if I am not mistaken the conclusion was that it
could leak information if we turned on emptyAuth unconditionally
when talking to a wrong server, and that the documentation needs an
update to recommend those who use emptyAuth because they want to
talk to Negotiate servers to use the http.<site>.emptyAuth form,
limited to such servers, not a more generic http.emptyAuth, to avoid
information leakage?
If that is the case, let's take your 1/2 in the near-by thread
without 2/2 (auto-enable emptyAuth) for now, as Dscho seems to have
a case that may be helped by it.
^ permalink raw reply
* Re: SHA1 collisions found
From: Morten Welinder @ 2017-02-23 19:13 UTC (permalink / raw)
To: Joey Hess; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20170223183105.joxtpbut4wcqfbtu@kitenet.net>
The attack seems to generate two 64-bytes blocks, one quarter of which
is repeated data. (Table-1 in the paper.)
Assuming the result of that is evenly distributed and that bytes are
independent, we can estimate the chances that the result is NUL-free
as (255/256)^192 = 47% and the probability that the result is NUL and
newline free as (254/256)^192 = 22%. Clearly one should not rely of
NULs or newlines to save the day. On the other hand, the chances of
an ascii result is something like (95/256)^192 = 10^-83.
The actual collision in the paper has no newline, but it does have a NUL.
M.
On Thu, Feb 23, 2017 at 1:31 PM, Joey Hess <id@joeyh.name> wrote:
> Joey Hess wrote:
>> Linus Torvalds wrote:
>> > What you describe pretty much already requires a pre-image attack,
>> > which the new attack is _not_.
>> >
>> > It's not clear that the "good" object can be anything sane.
>>
>> Generate a regular commit object; use the entire commit object + NUL as the
>> chosen prefix, and use the identical-prefix collision attack to generate
>> the colliding good/bad objects.
>>
>> (The size in git's object header is a minor complication. Set the size
>> field to something sufficiently large, and then pad out the colliding
>> objects to that size once they're generated.)
>
> Sorry! While that would work, it's a useless attack because the good and bad
> commit objects still point to the same tree.
>
> It would be interesting to have such colliding objects, to see what beaks,
> but probably not worth $75k to generate them.
>
> --
> see shy jo
^ permalink raw reply
* usability bug: git-gui: keyboard shortcuts don't operate correctly on multi-file selections
From: peter fargas @ 2017-02-23 19:09 UTC (permalink / raw)
To: git
Ctrl+T/Ctrl+U add/remove only one file, not the whole selection - used
to work. Neither are access keys for menu underlined (Ease of access
center > underline keyboard shortcuts is on), so there is no way to
effectively work with keyb only.
git-gui verison 0.21 GITGUI
git version 2.11.1.windows.1
Tcl/Tk 8.6.6
64-bit installer
Windows 7 Professional
Originally posted at
https://github.com/git/git-scm.com/issues/939#issuecomment-276024341
peter.fargas @
informatik-handwerk.de
0176 / 458 67 358
^ permalink raw reply
* Re: SHA1 collisions found
From: David Lang @ 2017-02-23 19:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Joey Hess, Git Mailing List
In-Reply-To: <CAPc5daVZ79WWKSw76kxHgDra9a7fSR1AibZa_pvK9aUuuVawLQ@mail.gmail.com>
pointers to a little more info
https://shattered.it/static/
the two files are:
https://shattered.it/static/shattered-1.pdf
https://shattered.it/static/shattered-2.pdf
422435 shattered-2.pdf
422435 shattered-1.pdf
identical length and a lot smaller than I expected (~162K of the 413K file is
binary junk)
$ sha1sum shattered-*pdf
38762cf7f55934b34d179ae6a4c80cadccbb7f0a shattered-1.pdf
38762cf7f55934b34d179ae6a4c80cadccbb7f0a shattered-2.pdf
$ sum shattered-*pdf
62721 413 shattered-1.pdf
41606 413 shattered-2.pdf
$ md5sum shattered-*pdf
ee4aa52b139d925f8d8884402b0a750c shattered-1.pdf
5bd9d8cabc46041579a311230539b8d1 shattered-2.pdf
David Lang
^ permalink raw reply
* Fwd: Re: feature request: user email config per domain
From: Igor Djordjevic @ 2017-02-23 19:29 UTC (permalink / raw)
To: Tushar Kapila, git
In-Reply-To: <5e25e9d5-ca52-2b32-3701-4826599774ef@gmail.com>
Forwarding a message that ended on my end only, probably by accident.
-------- Forwarded Message --------
Subject: Re: feature request: user email config per domain
Date: Thu, 23 Feb 2017 13:32:56 +0530
From: Tushar Kapila <tgkprog@gmail.com>
To: Igor Djordjevic BugA <igor.d.djordjevic@gmail.com>
Hello All,
> I'd much rather see something based on the working tree path, like
Duy's conditional config includes. Then you write something in your
~/.gitconfig
> This would allow you to have two root directories, one for your work
projects and one for open source projects (for example).
I guess this can be extended for any number of root directories. Like,
when a consultant has multiple employer email ids.
This sounds great and it would enforce at commit time, which as
pointed out, is the correct time to do it. If for some reason it is
not adopted I at least hope that we have a simple global config which
specifies that user must set email for each repo and to ignore any
global config.
Am sure this can be done via hooks, but I would like something that is
really simple for newbies and companies to enforce with minimal
instruction.
* Will try what Igor and Grant Humphries suggests.
* Thank you all for your replies, and a big thank you for git - its a
great tool. I used to dream of something like it when I was stuck with
svn (pre 2010 I was introduced to git late.)
[1] http://stackoverflow.com/a/42354282
[2] http://stackoverflow.com/users/2167004
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Jeff King @ 2017-02-23 19:35 UTC (permalink / raw)
To: Junio C Hamano
Cc: brian m. carlson, David Turner, git@vger.kernel.org,
Johannes Schindelin, Eric Sunshine
In-Reply-To: <xmqq1suo90za.fsf@gitster.mtv.corp.google.com>
On Thu, Feb 23, 2017 at 11:11:05AM -0800, Junio C Hamano wrote:
> >> As far as Kerberos, this is a desirable feature to have enabled, with
> >> little downside. I just don't know about the security of the NTLM part,
> >> and I don't think we should take this patch unless we're sure we know
> >> the consequences of it.
> >
> > Hmm. That would be a problem with my proposed patch 2 then, too, if only
> > because it turns the feature on by default in more places.
> >
> > If it _is_ dangerous to turn on all the time, I'd think we should
> > consider warning people in the http.emptyauth documentation.
>
> I presume that we have finished discussing the security
> ramification, and if I am not mistaken the conclusion was that it
> could leak information if we turned on emptyAuth unconditionally
> when talking to a wrong server, and that the documentation needs an
> update to recommend those who use emptyAuth because they want to
> talk to Negotiate servers to use the http.<site>.emptyAuth form,
> limited to such servers, not a more generic http.emptyAuth, to avoid
> information leakage?
I don't know enough to evaluate the claims of emptyAuth being dangerous
or not (nor do I use it myself or admin a server whose users need it).
So I will let interested parties hash out whether it is a good idea or
not, and I'm happy to drop my 2/2 for now.
If we are to make it more widely available, I would prefer something
more like my 2/2 than always turning on http.emptyAuth, if only because
it reduces the cost to people not using the feature. I'm happy to work
more on the patch if we decide to go that route.
> If that is the case, let's take your 1/2 in the near-by thread
> without 2/2 (auto-enable emptyAuth) for now, as Dscho seems to have
> a case that may be helped by it.
Yes, I think 1/2 stands on its own. Whether it helps Dscho's case or
not, it eliminates an HTTP round-trip for Basic-only servers, which I
think is worth it.
-Peff
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Jeff King @ 2017-02-23 19:42 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, David Turner, git@vger.kernel.org,
sandals@crustytoothpaste.net, Eric Sunshine
In-Reply-To: <alpine.DEB.2.20.1702231806340.3767@virtualbox>
On Thu, Feb 23, 2017 at 06:08:49PM +0100, Johannes Schindelin wrote:
> > I suspect the patch above could probably be generalized as:
> >
> > /* cut out methods we know the server doesn't support */
> > http_auth_methods &= results.auth_avail;
> >
> > and let curl figure it out from there.
>
> Maybe this patch (or a variation thereof) would also be able to fix this
> problem with the patch:
>
> https://github.com/git-for-windows/git/issues/1034
>
> Short version: for certain servers (that do *not* advertise Negotiate),
> setting emptyauth to true will result in a failed fetch, without letting
> the user type in their credentials.
I suspect it isn't enough to help without 2/2. This will tell curl that
the server does not do Negotiate, so it will skip the probe request. But
Git will still feed curl the bogus empty credential.
That's what 2/2 tries to fix: only kick in the emptyAuth hack when there
is something besides Basic[1] to try. The way it is written adds an
extra "auto" mode to emptyAuth, as I wanted to leave "emptyauth=true" as
a workaround in case the "auto" behavior does not work. And then I
turned on "auto" by default, since that was what the discussion was
shooting for.
But if we are worried about turning on emptyAuth everywhere, the auto
behavior could be tied to emptyauth=true (and have something like
"emptyauth=always" to _really_ force it). I don't have an opinion there.
It sounds like emptyauth has been enabled by default on Windows for a
while. It's not clear to me if that's a security problem or not.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] http: add an "auto" mode for http.emptyauth
From: Jeff King @ 2017-02-23 19:44 UTC (permalink / raw)
To: David Turner
Cc: Junio C Hamano, git@vger.kernel.org, sandals@crustytoothpaste.net,
Johannes Schindelin, Eric Sunshine
In-Reply-To: <092a87cf9aa94d53aebf42facb75b985@exmbdft7.ad.twosigma.com>
On Thu, Feb 23, 2017 at 04:31:13PM +0000, David Turner wrote:
> > As somebody who is using non-Basic auth, can you apply these patches and
> > show us the output of:
> >
> > GIT_TRACE_CURL=1 \
> > git ls-remote https://your-server 2>&1 >/dev/null |
> > egrep '(Send|Recv) header: (GET|HTTP|Auth)'
> >
> > (without http.emptyauth turned on, obviously).
>
> The results appear to be identical with and without
> the patch. With http.emptyauth turned off,
> 16:27:28.208924 http.c:524 => Send header: GET /info/refs?service=git-upload-pack HTTP/1.1
> 16:27:28.212872 http.c:524 <= Recv header: HTTP/1.1 401 Authorization Required
> Username for 'http://git': [I just pressed enter]
> Password for 'http://git': [ditto]
> 16:27:29.928872 http.c:524 => Send header: GET /info/refs?service=git-upload-pack HTTP/1.1
> 16:27:29.929787 http.c:524 <= Recv header: HTTP/1.1 401 Authorization Required
Just to be sure: did you remove http.emptyauth config completely from
your config files, or did you turn it to "false"? Because the new
behavior only kicks in when it isn't configured at all (probably we
should respect "auto" as a user-provided name).
> (if someone else wants to replicate this, delete >/dev/null bit
> from Jeff's shell snippet)
Hrm, you shouldn't need to. The stderr redirection comes first, so it
should become the new stdout.
-Peff
^ permalink raw reply
* Re: SHA1 collisions found
From: Linus Torvalds @ 2017-02-23 19:47 UTC (permalink / raw)
To: Jeff King; +Cc: Joey Hess, Git Mailing List
In-Reply-To: <20170223193210.munuqcjltwbrdy22@sigill.intra.peff.net>
On Thu, Feb 23, 2017 at 11:32 AM, Jeff King <peff@peff.net> wrote:
>
> Yeah, they're not expensive. We've discussed enabling them by default.
> The sticking point is that there is old history with minor bugs which
> triggers some warnings (e.g., malformed committer names), and it would
> be annoying to start rejecting that unconditionally.
>
> So I think we would need a good review of what is a "warning" versus an
> "error", and to only reject on errors (right now the NUL thing is a
> warning, and it should probably upgraded).
I think even a warning (as opposed to failing the operation) is
already a big deal.
If people start saying "why do I get this odd warning", and start
looking into it, that's going to be a pretty strong defense against
bad behavior. SCM attacks depend on flying under the radar.
>> So a very powerful defense is to just look for those bit patterns in
>> the objects, and just warn about them. Those patterns don't tend to
>> exist in normal inputs anyway, but particularly if you just warn, it's
>> a heads-ups that "ok, something iffy is going on"
>
> Yes, that would be a wonderful hardening to put into Git if we know what
> those patterns look like. That part isn't clear to me.
There's actually already code for that, pointed to by the shattered project:
https://github.com/cr-marcstevens/sha1collisiondetection
the "meat" of that check is in lib/ubc_check.c.
Linus
^ permalink raw reply
* Re: SHA1 collisions found
From: Jeff King @ 2017-02-23 19:57 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Joey Hess, Git Mailing List
In-Reply-To: <CA+55aFxmr6ntWGbJDa8tOyxXDX3H-yd4TQthgV_Tn1u91yyT8w@mail.gmail.com>
On Thu, Feb 23, 2017 at 11:47:16AM -0800, Linus Torvalds wrote:
> On Thu, Feb 23, 2017 at 11:32 AM, Jeff King <peff@peff.net> wrote:
> >
> > Yeah, they're not expensive. We've discussed enabling them by default.
> > The sticking point is that there is old history with minor bugs which
> > triggers some warnings (e.g., malformed committer names), and it would
> > be annoying to start rejecting that unconditionally.
> >
> > So I think we would need a good review of what is a "warning" versus an
> > "error", and to only reject on errors (right now the NUL thing is a
> > warning, and it should probably upgraded).
>
> I think even a warning (as opposed to failing the operation) is
> already a big deal.
>
> If people start saying "why do I get this odd warning", and start
> looking into it, that's going to be a pretty strong defense against
> bad behavior. SCM attacks depend on flying under the radar.
Sorry, I conflated two things there. I agree a warning is better than
nothing. But right now transfer.fsck croaks even for warnings, and there
are some warnings that it is not worth croaking for. So before we turn
it on, we need to stop croaking on warnings (and possibly bump up some
warnings to errors).
I think it _is_ important to have dangerous things as errors, though.
Because it helps an unattended server (where nobody would see the
warning) avoid being a vector for spreading malicious objects to older
clients which do not do the fsck.
> There's actually already code for that, pointed to by the shattered project:
>
> https://github.com/cr-marcstevens/sha1collisiondetection
>
> the "meat" of that check is in lib/ubc_check.c.
Thanks, I hadn't seen that yet. That doesn't look like it should be hard
to integrate into Git.
-Peff
^ permalink raw reply
* Re: SHA1 collisions found
From: Jeff King @ 2017-02-23 19:32 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Joey Hess, Git Mailing List
In-Reply-To: <CA+55aFx=0EVfSG2iEKKa78g3hFN_yZ+L_FRm4R749nNAmTGO9w@mail.gmail.com>
On Thu, Feb 23, 2017 at 11:09:32AM -0800, Linus Torvalds wrote:
> On Thu, Feb 23, 2017 at 10:46 AM, Jeff King <peff@peff.net> wrote:
> >>
> >> So I agree with you that we need to make git check for the opaque
> >> data. I think I was the one who brought that whole argument up.
> >
> > We do already.
>
> I'm aware of the fsck checks, but I have to admit I wasn't aware of
> 'transfer.fsckobjects'. I should turn that on myself.
>
> Or maybe git should just turn it on by default? At least the
> per-object fsck costs should be essentially free compared to the
> network costs when you just apply them to the incoming objects.
Yeah, they're not expensive. We've discussed enabling them by default.
The sticking point is that there is old history with minor bugs which
triggers some warnings (e.g., malformed committer names), and it would
be annoying to start rejecting that unconditionally.
So I think we would need a good review of what is a "warning" versus an
"error", and to only reject on errors (right now the NUL thing is a
warning, and it should probably upgraded).
> And in particular, while the *kernel* doesn't generally have critical
> opaque blobs, other projects do. Things like firmware images etc are
> open to attack, and crazy people put ISO images in repositories etc.
>
> So I don't think this discussion should focus exclusively on the git metadata.
>
> It is likely much easier to replace a binary blob than it is to
> replace a commit or tree (or a source file that has to go through a
> compiler). And for many projects, that would be a bad thing.
Yes, I'd agree we need to consider both. And no matter what Git does in
its own data formats, blobs will always be a sequence of bytes. Hiding
collision-cruft in them isn't up to us, but rather the data format.
The nice thing about a blob collision, though, is that you can only
replace the opaque files, not, say, C source code. That doesn't make it
a non-issue, but it reduces the scope of an attack.
Replacing a commit or tree wholesale means the attacker has a lot more
flexibility. So to whatever degree we can make that harder (like
complaining of commits with NULs), the better.
> > It's not an identical prefix, but I think collision attacks generally
> > are along the lines of selecting two prefixes followed by garbage, and
> > then mutating the garbage on both sides. That would "work" in this case
> > (modulo the fact that git would complain about the NUL).
>
> I think this particular attack depended on an actual identical prefix,
> but I didn't go back to the paper and check.
The paper describes the content as:
SHA-1(P | M1 | M2 | S)
and they replace both "M1" and "M2", with a near-collision for the
first, and then the final collision for the second. What's not clear to
me is if part of M1 can be chosen, or if it's perturbed fully into
random garbage.
> But the attacks tend to very much depend on particular input bit
> patterns that have very particular effects on the resulting
> intermediate hash, and those bit patterns are specific to the hash and
> known.
>
> So a very powerful defense is to just look for those bit patterns in
> the objects, and just warn about them. Those patterns don't tend to
> exist in normal inputs anyway, but particularly if you just warn, it's
> a heads-ups that "ok, something iffy is going on"
Yes, that would be a wonderful hardening to put into Git if we know what
those patterns look like. That part isn't clear to me.
> The whole _point_ of an SCM is that it isn't about a one-time event,
> but about continuous history. That also fundamentally means that a
> successful attack needs to work over time, and not be detectable.
Yeah, I'd certainly agree with that. You spend loads of money to
generate a collision, there's a reasonably high chance of detection, and
then as soon as one person detects it, your investment is lost.
According to the paper, the current cost of the computation for a single
collision is ~$670K.
At least for now, an attacker is much better off using that money to
break into your house and install a keylogger.
-Peff
^ permalink raw reply
* Re: [PATCH v5 1/1] config: add conditional include
From: Junio C Hamano @ 2017-02-23 19:59 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Jeff King, sschuberth, Matthieu Moy
In-Reply-To: <20170223122346.12222-2-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>> There was some discussion after v4. I think the open issues are:
>>
>> - the commit message is rather terse (it should describe motivation,
>> and can refer to the docs for the "how")
> This allows some more flexibility in managing configuration across
> repositories.
That is not an ideal opening to describe motivation without people
knowing what "this" is ;-) Of course, the person who wrote the
sentence know it already after writing the patch and the subject,
but others don't.
Sometimes a set of repositories want to share configuration
settings among themselves that are distinct from other such
sets of repositories. A user may work on two projects, each
of which have multiple repositories, and use one user.email
for one project while using another for the other. Having
the settings in ~/.gitconfig, which would work for just one
set of repositories, would not well in such a situation.
Extend the include.path mechanism that lets a config file
include another config file, so that the inclusion can be
done only when some conditions hold. Then ~/.gitconfig can
say "include config-project-A only when working on project-A"
for each project A the user works on.
In this patch, the only supported grouping is based on
$GIT_DIR (in absolute path), so you would need to group
repositories by directory, or something like that to take
advantage of it.
We already have include.path for unconditional
includes. This patch goes with include-if.xxx.path to make
it clearer that a condition is required.
Similar to include.path, older git versions that don't
understand include-if will simply ignore them.
or something along that line?
> +Conditional includes
> +~~~~~~~~~~~~~~~~~~~~
> +
> +You can include one config file from another conditionally by setting
> +a special `include-if.<condition>.path` variable to the name of the
> +file to be included. The variable is treated the same way as
> +`include.path`.
Drop "special", as all configuration variables are "special" in their
own sense, it does not add any useful information.
I think we avoid '-' in Git-native variable and section names, so
"include-if" would become an odd-man-out.
The variable is obviously not treated the same way as include.path ;-)
When includeIf.<condition>.path variable is set in a
configuration file, the configuration file named by that
variable is included (in a way similar to how include.path
works) only if the <condition> holds true.
> +The condition starts with a keyword, followed by a colon and a
> +pattern. The interpretation of the pattern depends on the keyword.
"a pattern"? I think it is "some data whose format and meaning
depends on the keyword".
Hence...
> +Supported keywords are:
> +
> +`gitdir`::
> + The environment variable `GIT_DIR` must match the following
> + pattern for files to be included. The pattern can contain
The data that follows the keyword `gitdir:` is used as a
glob pattern. If the location of the .git directory (which
may be auto-discovered, or come from `GIT_DIR` environment
variable) match the pattern, the `<condition>` becomes true.
> + ...
> + * If the pattern ends with `/`, `**` will be automatically added. For
> + example, the pattern `foo/` becomes `foo/**`. In other words, it
> + matches "foo" and everything inside, recursively.
> +
> +`gitdir/i`::
> + This is the same as `gitdir` except that matching is done
> + case-insensitively (e.g. on case-insensitive file sytems)
> +
> +A few more notes on matching:
As future <keywords> may not be about path or matching at all, this
belongs to `gitdir`:: section (and it would be obvious that that
applies to `gitdir/i`:: because we say "this is the same as...").
> + * Symlinks in `$GIT_DIR` are not resolved before matching.
> +
> + * Note that "../" is not special and will match literally, which is
> + unlikely what you want.
> +static int prepare_include_condition_pattern(struct strbuf *pat)
> +{
> + struct strbuf path = STRBUF_INIT;
> + int prefix = 0;
> +
> + /* TODO: maybe support ~user/ too */
> + if (pat->buf[0] == '~' && is_dir_sep(pat->buf[1])) {
> + const char *home = getenv("HOME");
> +
> + if (!home)
> + return error(_("$HOME is not defined"));
Instead of half-duplicating it here yourself, can't we let
expand_user_path() do its thing?
> +static int include_condition_is_true(const char *cond, size_t cond_len)
> +{
> + /* no condition (i.e., "include.path") is always true */
> + if (!cond)
> + return 1;
> +
> + if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len))
> + return include_by_gitdir(cond, cond_len, 0);
> + else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len))
> + return include_by_gitdir(cond, cond_len, 1);
This may be OK for now, but it should be trivial to start from a
table with two entries, i.e.
struct include_cond {
const char *keyword;
int (*fn)(const char *, size_t);
};
and will show a better way to do things to those who follow your
footsteps.
^ permalink raw reply
* RE: [PATCH 2/2] http: add an "auto" mode for http.emptyauth
From: David Turner @ 2017-02-23 20:05 UTC (permalink / raw)
To: 'Jeff King'
Cc: Junio C Hamano, git@vger.kernel.org, sandals@crustytoothpaste.net,
Johannes Schindelin, Eric Sunshine
In-Reply-To: <20170223194418.eqi5ynhyhrcybiok@sigill.intra.peff.net>
> -----Original Message-----
> From: Jeff King [mailto:peff@peff.net]
> Sent: Thursday, February 23, 2017 2:44 PM
> To: David Turner <David.Turner@twosigma.com>
> Cc: Junio C Hamano <gitster@pobox.com>; git@vger.kernel.org;
> sandals@crustytoothpaste.net; Johannes Schindelin
> <johannes.schindelin@gmx.de>; Eric Sunshine <sunshine@sunshineco.com>
> Subject: Re: [PATCH 2/2] http: add an "auto" mode for http.emptyauth
>
> On Thu, Feb 23, 2017 at 04:31:13PM +0000, David Turner wrote:
>
> > > As somebody who is using non-Basic auth, can you apply these patches
> > > and show us the output of:
> > >
> > > GIT_TRACE_CURL=1 \
> > > git ls-remote https://your-server 2>&1 >/dev/null |
> > > egrep '(Send|Recv) header: (GET|HTTP|Auth)'
> > >
> > > (without http.emptyauth turned on, obviously).
> >
> > The results appear to be identical with and without the patch. With
> > http.emptyauth turned off,
> > 16:27:28.208924 http.c:524 => Send header: GET
> /info/refs?service=git-upload-pack HTTP/1.1
> > 16:27:28.212872 http.c:524 <= Recv header: HTTP/1.1 401
> Authorization Required
> > Username for 'http://git': [I just pressed enter] Password for
> > 'http://git': [ditto]
> > 16:27:29.928872 http.c:524 => Send header: GET
> /info/refs?service=git-upload-pack HTTP/1.1
> > 16:27:29.929787 http.c:524 <= Recv header: HTTP/1.1 401
> Authorization Required
>
> Just to be sure: did you remove http.emptyauth config completely from your
> config files, or did you turn it to "false"? Because the new behavior only kicks
> in when it isn't configured at all (probably we should respect "auto" as a user-
> provided name).
I turned it to false. With it completely removed, I get this, both times:
20:03:49.896797 http.c:524 => Send header: GET /info/refs?service=git-upload-pack HTTP/1.1
20:03:49.900776 http.c:524 <= Recv header: HTTP/1.1 401 Authorization Required
20:03:49.900929 http.c:524 => Send header: GET /info/refs?service=git-upload-pack HTTP/1.1
20:03:49.904754 http.c:524 <= Recv header: HTTP/1.1 401 Authorization Required
20:03:49.906649 http.c:524 => Send header: GET /info/refs?service=git-upload-pack HTTP/1.1
20:03:49.906654 http.c:524 => Send header: Authorization: Negotiate <redacted>
20:03:49.956753 http.c:524 <= Recv header: HTTP/1.1 200 OK - $gitservername
> > (if someone else wants to replicate this, delete >/dev/null bit from
> > Jeff's shell snippet)
>
> Hrm, you shouldn't need to. The stderr redirection comes first, so it should
> become the new stdout.
Weird. It didn't appear work earlier, but I must have screwed something up.
And I learned something about shell redirection.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox