* 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
* Re: [PATCH] upload-pack: report "not our ref" to client
From: Junio C Hamano @ 2017-02-23 20:14 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <20170223184303.26530-1-jonathantanmy@google.com>
Thanks.
^ permalink raw reply
* git bugs
From: Sean Hunt @ 2017-02-23 20:27 UTC (permalink / raw)
To: git
There are a few bugs I git I noticed when using mingw, mingw64,
cygwin, and cygwin64. These bugs are the following:
if I do git ``rebase -i --root`` and tell it to edit every commit to
gpg sign all my commits it bugs out and merges all of the commits into
1 commit instead of only appending the ``-S`` to each and every commit
and keeping all of the commits. It is as if I told it to squash the
commits but yet I did not. There is also another bug where if I clone
a repo on Windows and not on github desktop and that I placed commits
to the repo on github web and then when I rebase to squash the commits
to 1 commit (some repos are doing it as a requirement for 1 commit
PR's) that all of my commits on the remote (fork in this case) that is
linked to an open pull request are discarded and then the pull request
is somehow and oddly closed. It is super annoying.
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Junio C Hamano @ 2017-02-23 20:37 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin, David Turner, git@vger.kernel.org,
sandals@crustytoothpaste.net, Eric Sunshine
In-Reply-To: <20170223194237.eckkpiqv7inuz7un@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> 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
In your [1] you wanted to mention that Digest would have the same
property as Basic, or something like that?
> 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.
I do not have a strong opinion, either, but it sounds like that even
the "disable emptyAuth hack if the server is Basic only" variant
would be much better than setting emptyAuth on by default. At least
the user whose issue was reported in Dscho's message would be fixed
by such a variant, I would think (i.e. talking to a server with no
Negotiate and emptyAuth set to true results in no attempt to give
the user a chance to tell who s/he is --- your 2/2 will turn
emptyAuth off in that case).
^ permalink raw reply
* Re: SHA1 collisions found
From: Øyvind A. Holm @ 2017-02-23 20:47 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Jeff King, Joey Hess, Git Mailing List
In-Reply-To: <CA+55aFx=0EVfSG2iEKKa78g3hFN_yZ+L_FRm4R749nNAmTGO9w@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1378 bytes --]
On 2017-02-23 11:09:32, Linus Torvalds wrote:
> 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?
The problem with this is that there are many repos with errors out
there, for example coreutils.git and nasm.git, which complains about
"missingSpaceBeforeDate: invalid author/committer line - missing space
before date".
There are also lots of repositories bitten by the Github bug from back
in 2011 where they zero-padded the file modes, git clone aborts with
"zeroPaddedFilemode: contains zero-padded file modes".
Paranoid as I am, I'm using fetch.fsckObjects and receive.fsckObjects
set to "true", but that means I'm not able to clone repositories with
these kind of errors, have to use the alias
fclone = clone -c "fetch.fsckObjects=false"
So enabling them by default will create problems among users. Of course,
one solution would be to turn these kind of errors into warnings so the
clone isn't aborted.
Reagards,
Øyvind
+-| Øyvind A. Holm <sunny@sunbase.org> - N 60.37604° E 5.33339° |-+
| OpenPGP: 0xFB0CBEE894A506E5 - http://www.sunbase.org/pubkey.asc |
| Fingerprint: A006 05D6 E676 B319 55E2 E77E FB0C BEE8 94A5 06E5 |
+------------| c7e47a18-fa06-11e6-ad93-db5caa6d21d3 |-------------+
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Jeff King @ 2017-02-23 20:48 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, David Turner, git@vger.kernel.org,
sandals@crustytoothpaste.net, Eric Sunshine
In-Reply-To: <xmqqlgsw7iey.fsf@gitster.mtv.corp.google.com>
On Thu, Feb 23, 2017 at 12:37:25PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > 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
>
> In your [1] you wanted to mention that Digest would have the same
> property as Basic, or something like that?
Oops, yeah. What I was going to say is that we may want a list of auth
types where we _do_ want the hack on, rather than ones where we know it
does not work. People are more likely to notice when the list is wrong,
then.
> > 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.
>
> I do not have a strong opinion, either, but it sounds like that even
> the "disable emptyAuth hack if the server is Basic only" variant
> would be much better than setting emptyAuth on by default. At least
> the user whose issue was reported in Dscho's message would be fixed
> by such a variant, I would think (i.e. talking to a server with no
> Negotiate and emptyAuth set to true results in no attempt to give
> the user a chance to tell who s/he is --- your 2/2 will turn
> emptyAuth off in that case).
Yes, I agree that the "auto" behavior is better than defaulting to
"true". I am speaking from the perspective of git.git, which is
currently defaulting to "false". It is not clear to me if "auto" is
better than "false" because of the security implications.
For Git for Windows, it seems like the auto behavior would be a strict
improvement over the "true" default they've been shipping.
-Peff
^ permalink raw reply
* Re: SHA1 collisions found
From: Jakub Narębski @ 2017-02-23 20:49 UTC (permalink / raw)
To: David Lang, Junio C Hamano; +Cc: Joey Hess, Git Mailing List
In-Reply-To: <nycvar.QRO.7.75.62.1702230907340.6590@qynat-yncgbc>
W dniu 23.02.2017 o 18:12, David Lang pisze:
> On Thu, 23 Feb 2017, 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/
>>
>
> it doesn't help that the Google page on this explicitly says that
> this shows that it's possible to create two different git repos that
> have the same hash but different contents.
>
> https://shattered.it/
>
> How is GIT affected? GIT strongly relies on SHA-1 for the
> identification and integrity checking of all file objects and
> commits. It is essentially possible to create two GIT repositories
> with the same head commit hash and different contents, say a benign
> source code and a backdoored one. An attacker could potentially
> selectively serve either repository to targeted users. This will
> require attackers to compute their own collision.
The attack on SHA-1 presented there is "identical-prefix" collision,
which is less powerful than "chosen-prefix" collision. It is the
latter that is required to defeat SHA-1 used in object identity.
Objects in Git _must_ begin with given prefix; the use of zlib
compression adds to the difficulty. 'Forged' Git object would
simply not validate...
https://arstechnica.com/security/2017/02/at-deaths-door-for-years-widely-used-sha1-function-is-now-dead/
^ permalink raw reply
* Re: [PATCH] git svn branch fails with authenticaton failures
From: Eric Wong @ 2017-02-23 20:54 UTC (permalink / raw)
To: Hiroshi Shirosaki; +Cc: git
In-Reply-To: <1487731055-29153-1-git-send-email-h.shirosaki@gmail.com>
Hiroshi Shirosaki <h.shirosaki@gmail.com> wrote:
> I have the following authentication failure while svn rebase and
> svn dcommit works fine without authentication failures.
>
> $ git svn branch v7_3
> Copying https://xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx at r27519
> to https://xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/v7_3...
> Can't create session: Unable to connect to a repository at URL
> 'https://xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx': No more
> credentials or we tried too many times.
> Authentication failed at
> C:\Program Files\Git\mingw64/libexec/git-core\git-svn line 1200.
>
> I can workaround the issue to add auth configuration to
> SVN::Client->new().
Missing sign-off (see Documentation/SubmittingPatches).
Not my rule, but it's unfortunately required for this project.
Also, the Subject: should be in the imperative mood,
Perhaps something like:
Subject: [PATCH] git svn: fix authentication with 'branch'
I am less picky about the message body.
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1175,10 +1175,10 @@ sub cmd_branch {
> ::_req_svn();
> require SVN::Client;
>
> + my ($config, $baton, $callbacks) = Git::SVN::Ra::prepare_config_once();
Since we're not using it, here, you can avoid setting a variable
for $callbacks more explicitly:
my ($config, $baton, undef) = Git::SVN::Ra::prepare_config_once();
> my $ctx = SVN::Client->new(
> - config => SVN::Core::config_get_config(
> - $Git::SVN::Ra::config_dir
> - ),
> + auth => $baton,
> + config => $config,
> log_msg => sub {
> ${ $_[0] } = defined $_message
> ? $_message
> --
Anyways, this looks like a good change. I will accept a v2
with your sign-off and changes noted above. Thank you.
^ permalink raw reply
* Re: SHA1 collisions found
From: Joey Hess @ 2017-02-23 20:46 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
In-Reply-To: <20170223184637.xr74k42vc6y2pmse@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 975 bytes --]
Jeff King wrote:
> 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.
The current attack is an identical-prefix attack, not chosen-prefix, so
not quite to that point yet.
The MD5 chosen-prefix attack was 2^15 harder than the known-prefix attack,
but who knows if the numbers will be comprable for SHA1.
> 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.
git log and git show don't show such cruft headers either.
BTW, the SHA attack only added ~128 bytes to the pdfs, not really a
huge amount of garbage.
--
see shy jo
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: SHA1 collisions found
From: Jeff King @ 2017-02-23 20:57 UTC (permalink / raw)
To: Jakub Narębski
Cc: David Lang, Junio C Hamano, Joey Hess, Git Mailing List
In-Reply-To: <e57958d4-7c51-3f5e-6ff5-f863920fd883@gmail.com>
On Thu, Feb 23, 2017 at 09:49:09PM +0100, Jakub Narębski wrote:
> > How is GIT affected? GIT strongly relies on SHA-1 for the
> > identification and integrity checking of all file objects and
> > commits. It is essentially possible to create two GIT repositories
> > with the same head commit hash and different contents, say a benign
> > source code and a backdoored one. An attacker could potentially
> > selectively serve either repository to targeted users. This will
> > require attackers to compute their own collision.
>
> The attack on SHA-1 presented there is "identical-prefix" collision,
> which is less powerful than "chosen-prefix" collision. It is the
> latter that is required to defeat SHA-1 used in object identity.
> Objects in Git _must_ begin with given prefix;
I don't think this helps. The chosen-prefix lets you append hash data to
an existing file. Here we just have identical prefixes in the two
colliding halves. In the real-world example, they used a PDF header. But
it could have been a PDF header with "blob 1234" prepended to it (note
also that Git's use of the size doesn't help; the attack files are the
same length).
> the use of zlib
> compression adds to the difficulty. 'Forged' Git object would
> simply not validate...
No, zlib doesn't help. The sha1 is computed on the uncompressed data.
-Peff
^ permalink raw reply
* Re: [PATCH 4/4] ident: do not ignore empty config name/email
From: Junio C Hamano @ 2017-02-23 20:58 UTC (permalink / raw)
To: Jeff King; +Cc: bs.x.ttp, git
In-Reply-To: <20170223081708.ge34zjkmpsolocqx@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This one is perhaps questionable. Maybe somebody is relying on setting a
> per-repo user.name to override a ~/.gitconfig value and enforce
> auto-detection?
Thanks for splitting this step out. 1/4 and 2/4 are obvious
improvements, and 3/4 is a very sensible fix. Compared to those
three, this one does smell questionable, because I do not quite see
any other reasonable fallback other than the auto-detection if the
user gives an empty ident on purpose.
Erroring out to say "don't do that" is probably not too bad, but
perhaps we are being run by a script that is doing a best-effort
conversion from $ANOTHER_SCM using a list of known authors that is
incomplete, ending up feeding empty ident and allowing us to fall
back to attribute them to the user who runs the script. I do not
see a point in breaking that user and having her or him update the
script to stuff in a truly bogus "Unknown <unknown>" name.
>
> ident.c | 4 ++--
> t/t7518-ident-corner-cases.sh | 11 +++++++++++
> 2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/ident.c b/ident.c
> index ead09ff7f..c0364fe3a 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -153,7 +153,7 @@ static void copy_email(const struct passwd *pw, struct strbuf *email,
>
> const char *ident_default_name(void)
> {
> - if (!git_default_name.len) {
> + if (!(ident_config_given & IDENT_NAME_GIVEN) && !git_default_name.len) {
> copy_gecos(xgetpwuid_self(&default_name_is_bogus), &git_default_name);
> strbuf_trim(&git_default_name);
> }
> @@ -162,7 +162,7 @@ const char *ident_default_name(void)
>
> const char *ident_default_email(void)
> {
> - if (!git_default_email.len) {
> + if (!(ident_config_given & IDENT_MAIL_GIVEN) && !git_default_email.len) {
> const char *email = getenv("EMAIL");
>
> if (email && email[0]) {
> diff --git a/t/t7518-ident-corner-cases.sh b/t/t7518-ident-corner-cases.sh
> index 3d2560c3c..ef570ac62 100755
> --- a/t/t7518-ident-corner-cases.sh
> +++ b/t/t7518-ident-corner-cases.sh
> @@ -22,4 +22,15 @@ test_expect_success 'commit rejects all-crud name' '
> git commit --allow-empty -m foo
> '
>
> +# We must test the actual error message here, as an unwanted
> +# auto-detection could fail for other reasons.
> +test_expect_success 'empty configured name does not auto-detect' '
> + (
> + sane_unset GIT_AUTHOR_NAME &&
> + test_must_fail \
> + git -c user.name= commit --allow-empty -m foo 2>err &&
> + test_i18ngrep "empty ident name" err
> + )
> +'
> +
> test_done
^ 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