From: Junio C Hamano <hidden> Date: 2017-05-08 07:10:48
Ævar Arnfjörð Bjarmason [off-list ref] writes:
This won't be in my next PCRE submission, but I have a path locally to
simply import PCRE into git.git as compat/pcre2, so it can be compiled
with NO_PCRE=Y similar to how NO_REGEX=Y works.
This will hopefully address your concerns partially, i.e. when you do
want to try it out it'll be easier.
Eek, please don't.
Until pcre2 becomes _so_ stable that all reasonable distros give
choice to the end-users to install it easily in a packaged form,
such a "not a fork, but a copy" will be a moving target that I'd
rather not to have in compat/. IOW, our compat/$pkg should be a
last resort to help those on distros that are so hard to convince to
carry the version/variant of $pkg we would like to use.
From: brian m. carlson <hidden> Date: 2017-05-08 23:32:37
On Mon, May 08, 2017 at 04:10:41PM +0900, Junio C Hamano wrote:
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
This won't be in my next PCRE submission, but I have a path locally to
simply import PCRE into git.git as compat/pcre2, so it can be compiled
with NO_PCRE=Y similar to how NO_REGEX=Y works.
This will hopefully address your concerns partially, i.e. when you do
want to try it out it'll be easier.
Eek, please don't.
Until pcre2 becomes _so_ stable that all reasonable distros give
choice to the end-users to install it easily in a packaged form,
such a "not a fork, but a copy" will be a moving target that I'd
rather not to have in compat/. IOW, our compat/$pkg should be a
last resort to help those on distros that are so hard to convince to
carry the version/variant of $pkg we would like to use.
PCRE and PCRE2 also tend to have a lot of security updates, so I would
prefer if we didn't import them into the tree. It is far better for
users to use their distro's packages for PCRE, as it means they get
automatic security updates even if they're using an old Git.
We shouldn't consider shipping anything with a remotely frequent history
of security updates in our tree, since people very frequently run old or
ancient versions of Git.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
On Tue, May 9, 2017 at 1:32 AM, brian m. carlson
[off-list ref] wrote:
On Mon, May 08, 2017 at 04:10:41PM +0900, Junio C Hamano wrote:
quoted
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
This won't be in my next PCRE submission, but I have a path locally to
simply import PCRE into git.git as compat/pcre2, so it can be compiled
with NO_PCRE=Y similar to how NO_REGEX=Y works.
This will hopefully address your concerns partially, i.e. when you do
want to try it out it'll be easier.
Eek, please don't.
Until pcre2 becomes _so_ stable that all reasonable distros give
choice to the end-users to install it easily in a packaged form,
such a "not a fork, but a copy" will be a moving target that I'd
rather not to have in compat/. IOW, our compat/$pkg should be a
last resort to help those on distros that are so hard to convince to
carry the version/variant of $pkg we would like to use.
The reason I'm looking into this is because the WIP part of my PCRE
branch has changes which start to use PCRE as a general matching
engine in git, even. I.e.:
* git grep -F will be powered by it rather than kwset (which'll be git rm'd)
* Long standing limitations with \0s in regexes go away.
* grep -G and -E will use PCRE via a WIP POSIX -> PCRE pattern
translator (https://bugs.exim.org/show_bug.cgi?id=2106)
* Perhaps we can remove compat/regex/ entirely & use PCRE via its
POSIX emulation mode + pattern translator (we use regcomp/regexec a
lot for non-grep/log), I'm not sure yet.
I have messy but working code for this in a WIP branch, here's the
performance improvement against linux.git:
Test v2.13.0-rc2 HEAD
---------------------------------------------------------------------------------------
7820.1: basic grep how.to 0.31(1.20+0.46)
0.19(0.33+0.55) -38.7%
7820.2: extended grep how.to 0.31(1.19+0.46)
0.19(0.33+0.55) -38.7%
7820.3: perl grep how.to 0.30(1.12+0.46)
0.19(0.28+0.62) -36.7%
7820.5: basic grep ^how to 0.31(1.24+0.39)
0.19(0.32+0.56) -38.7%
7820.6: extended grep ^how to 0.30(1.18+0.44)
0.19(0.22+0.66) -36.7%
7820.7: perl grep ^how to 0.55(2.68+0.41)
0.19(0.32+0.56) -65.5%
7820.9: basic grep [how] to 0.47(2.17+0.44)
0.22(0.39+0.54) -53.2%
7820.10: extended grep [how] to 0.47(2.21+0.40)
0.22(0.39+0.55) -53.2%
7820.11: perl grep [how] to 0.53(2.64+0.39)
0.21(0.37+0.58) -60.4%
7820.13: basic grep \(e.t[^ ]*\|v.ry\) rare 0.63(3.16+0.48)
0.21(0.48+0.53) -66.7%
7820.14: extended grep (e.t[^ ]*|v.ry) rare 0.64(3.28+0.38)
0.21(0.45+0.57) -67.2%
7820.15: perl grep (e.t[^ ]*|v.ry) rare 1.00(5.77+0.37)
0.21(0.50+0.53) -79.0%
7820.17: basic grep m\(ú\|u\)ult.b\(æ\|y\)te 0.31(1.23+0.51)
0.19(0.30+0.58) -38.7%
7820.18: extended grep m(ú|u)ult.b(æ|y)te 0.32(1.26+0.47)
0.19(0.27+0.61) -40.6%
7820.19: perl grep m(ú|u)ult.b(æ|y)te 0.36(1.61+0.37)
0.19(0.30+0.57) -47.2%
7821.1: fixed grep int 0.52(1.71+0.64)
0.43(1.11+0.72) -17.3%
7821.2: basic grep int 0.54(1.62+0.70)
0.42(1.14+0.62) -22.2%
7821.3: extended grep int 0.53(1.67+0.64)
0.51(1.17+0.62) -3.8%
7821.4: perl grep int 0.53(1.71+0.59)
0.72(1.13+0.63) +35.8%
7821.6: fixed grep -i int 0.58(1.86+0.67)
0.47(1.32+0.62) -19.0%
7821.7: basic grep -i int 0.62(1.94+0.61)
0.57(1.25+0.72) -8.1%
7821.8: extended grep -i int 0.82(1.86+0.68)
0.50(1.41+0.56) -39.0%
7821.9: perl grep -i int 0.70(1.88+0.68)
0.56(1.25+0.70) -20.0%
7821.11: fixed grep æ 0.33(1.30+0.43)
0.19(0.22+0.64) -42.4%
7821.12: basic grep æ 0.33(1.35+0.38)
0.18(0.26+0.59) -45.5%
7821.13: extended grep æ 0.33(1.20+0.52)
0.18(0.32+0.53) -45.5%
7821.14: perl grep æ 0.33(1.31+0.40)
0.18(0.28+0.56) -45.5%
7821.16: fixed grep -i æ 0.25(0.87+0.50)
0.18(0.24+0.60) -28.0%
7821.17: basic grep -i æ 0.26(0.88+0.48)
0.18(0.24+0.60) -30.8%
7821.18: extended grep -i æ 0.26(0.92+0.44)
0.18(0.24+0.61) -30.8%
7821.19: perl grep -i æ 0.25(0.79+0.45)
0.19(0.32+0.56) -24.0%
In case that comes out misformatted it's also available at
https://github.com/avar/git/commit/ee5b2040e2c697e22a73c7b5f07f1b1e591f07e3
I.e. grepping is sped up by 50% and more in many cases, even for -G
and -E patterns which now get translated internally into PCRE
patterns.
PCRE and PCRE2 also tend to have a lot of security updates, so I would
prefer if we didn't import them into the tree. It is far better for
users to use their distro's packages for PCRE, as it means they get
automatic security updates even if they're using an old Git.
We shouldn't consider shipping anything with a remotely frequent history
of security updates in our tree, since people very frequently run old or
ancient versions of Git.
I'm aware of its security record[1], but I wonder what threat model
you have in mind here. I'm not aware of any parts of git (except maybe
gitweb?) where we take regexes from untrusted sources.
I.e. yes there have been DoS's & even some overflow bugs leading code
execution in PCRE, but in the context of powering git-grep & git-log
with PCRE this falls into the "stop hitting yourself" category.
1. https://www.cvedetails.com/vendor/3265/Pcre.html
From: brian m. carlson <hidden> Date: 2017-05-09 00:37:25
On Tue, May 09, 2017 at 02:00:18AM +0200, Ævar Arnfjörð Bjarmason wrote:
On Tue, May 9, 2017 at 1:32 AM, brian m. carlson
[off-list ref] wrote:
quoted
PCRE and PCRE2 also tend to have a lot of security updates, so I would
prefer if we didn't import them into the tree. It is far better for
users to use their distro's packages for PCRE, as it means they get
automatic security updates even if they're using an old Git.
We shouldn't consider shipping anything with a remotely frequent history
of security updates in our tree, since people very frequently run old or
ancient versions of Git.
I'm aware of its security record[1], but I wonder what threat model
you have in mind here. I'm not aware of any parts of git (except maybe
gitweb?) where we take regexes from untrusted sources.
I.e. yes there have been DoS's & even some overflow bugs leading code
execution in PCRE, but in the context of powering git-grep & git-log
with PCRE this falls into the "stop hitting yourself" category.
Just because you don't drive Git with untrusted regexes doesn't mean
other people don't. It's not a good idea to require a stronger security
model than we absolutely have to, since people can and will violate it.
Think how devastating Shellshock was even though technically nobody
should provide insecure environment variables to the shell.
And, yes, gitweb does in fact call git grep. That means that git grep
must in fact be secure against untrusted regexes, or you have a remote
code execution vulnerability.
Furthermore, at work we distribute Git with all releases of our product.
We normally only do non-security updates to the last couple of releases,
but we provide security updates to all supported versions. I'm not
comfortable shipping the entirety of PCRE or PCRE2 to customers without
providing security updates, so you're going to make my job (and my
coworkers') a lot harder by shipping it. Please don't.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
From: Johannes Schindelin <hidden> Date: 2017-05-09 10:41:09
Hi,
On Tue, 9 May 2017, brian m. carlson wrote:
On Tue, May 09, 2017 at 02:00:18AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
On Tue, May 9, 2017 at 1:32 AM, brian m. carlson
[off-list ref] wrote:
quoted
PCRE and PCRE2 also tend to have a lot of security updates, so I
would prefer if we didn't import them into the tree. It is far
better for users to use their distro's packages for PCRE, as it
means they get automatic security updates even if they're using an
old Git.
We shouldn't consider shipping anything with a remotely frequent
history of security updates in our tree, since people very
frequently run old or ancient versions of Git.
I'm aware of its security record[1], but I wonder what threat model
you have in mind here. I'm not aware of any parts of git (except maybe
gitweb?) where we take regexes from untrusted sources.
I.e. yes there have been DoS's & even some overflow bugs leading code
execution in PCRE, but in the context of powering git-grep & git-log
with PCRE this falls into the "stop hitting yourself" category.
Just because you don't drive Git with untrusted regexes doesn't mean
other people don't.
Or other applications.
It's not a good idea to require a stronger security model than we
absolutely have to, since people can and will violate it. Think how
devastating Shellshock was even though technically nobody should provide
insecure environment variables to the shell.
And, yes, gitweb does in fact call git grep. That means that git grep
must in fact be secure against untrusted regexes, or you have a remote
code execution vulnerability.
And not only grep is affected. Think HEAD^{/<regex>}. There are plenty of
sites where you are allowed to specify revs in a freer form than SHA-1s.
Having said that, I do like the prospect of a faster git grep.
Hopefully there will be a way to make use of PCRE that can be switched
off? Like, a compile-time replacement of the regex API backed by PCRE v2
*iff* PCRE v2 is used for building?
Ciao,
Dscho
On Tue, May 9, 2017 at 2:37 AM, brian m. carlson
[off-list ref] wrote:
On Tue, May 09, 2017 at 02:00:18AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
On Tue, May 9, 2017 at 1:32 AM, brian m. carlson
[off-list ref] wrote:
quoted
PCRE and PCRE2 also tend to have a lot of security updates, so I would
prefer if we didn't import them into the tree. It is far better for
users to use their distro's packages for PCRE, as it means they get
automatic security updates even if they're using an old Git.
We shouldn't consider shipping anything with a remotely frequent history
of security updates in our tree, since people very frequently run old or
ancient versions of Git.
I'm aware of its security record[1], but I wonder what threat model
you have in mind here. I'm not aware of any parts of git (except maybe
gitweb?) where we take regexes from untrusted sources.
I.e. yes there have been DoS's & even some overflow bugs leading code
execution in PCRE, but in the context of powering git-grep & git-log
with PCRE this falls into the "stop hitting yourself" category.
Just because you don't drive Git with untrusted regexes doesn't mean
other people don't. It's not a good idea to require a stronger security
model than we absolutely have to, since people can and will violate it.
Think how devastating Shellshock was even though technically nobody
should provide insecure environment variables to the shell.
Yes this is definitely something we should keep in mind. I see my
comment could be read as dismissing your concerns, I didn't mean that.
This is definitely something to be concerned about.
I was trying to solicit feedback on whether there were any other parts
of stock git that could take external input as regexes than gitweb,
I'm not aware of any.
I thought that gitweb wouldn't take regexes by default, but I see now
that the 'search' feature is on by default, and that allows you to
grep / pickaxe with regexes. Either -E or -F for grep, or
--pickaxe-regex (which implies -E) for log.
And, yes, gitweb does in fact call git grep. That means that git grep
must in fact be secure against untrusted regexes, or you have a remote
code execution vulnerability.
Indeed, but it's not as clear-cut as turning on on PCRE making you vulnerable.
* gitweb is vulnerable to CPU DoS now in its default configuration.
It's easy to provide an ERE that ends up slurping up 100% CPU for
several seconds on any non-trivial sized repo, do that in parallel &
you have a DoS vector.
* Obviously something that's 2x as fast or more (which my WIP code is)
makes this better.
* PCRE tends to be worse at pathological patterns, but this is because
it has really large limits by default and will try rather insanely
hard to match your pattern.
-DHEAP_LIMIT=20000000 \
-DMATCH_LIMIT=10000000 \
-DMATCH_LIMIT_DEPTH=10000000 \
-DMAX_NAME_COUNT=10000 \
-DMAX_NAME_SIZE=32 \
-DPARENS_NEST_LIMIT=250 \
This can be reduced dynamically via the API (and the patterns can't
change it, except to reduce it). For example on my system 2.11.0
(before any of my changes) on linux.git:
$ time git grep -E -- '-?-?-?-?-?-?-?-?-?-?-?-----------$' |wc -l
12434
real 0m0.444s
$ time git grep -P -- '-?-?-?-?-?-?-?-?-?-?-?-----------$' | wc -l
12434
real 1m20.849s
With my JIT changes:
$ time ~/g/git/git --exec-path=/home/avar/g/git grep -P --
'-?-?-?-?-?-?-?-?-?-?-?-----------$' | wc -l
12434
real 0m5.334s
However for user-supplied patterns we can just turn on really
conservative settings:
$ time ~/g/git/git --exec-path=/home/avar/g/git grep -P --
'(*LIMIT_RECURSION=1000)(*LIMIT_MATCH=1000)-?-?-?-?-?-?-?-?-?-?-?-----------$'
| wc -l
fatal: pcre2_jit_match failed with error code -47: match limit exceeded
0
real 0m0.021s
When I locally compile git with something like this:
- -DMATCH_LIMIT=10000000 \
- -DMATCH_LIMIT_DEPTH=10000000 \
- -DMATCH_LIMIT_RECURSION=10000000 \
- -DMAX_NAME_COUNT=10000 \
+ -DMATCH_LIMIT=1000 \
+ -DMATCH_LIMIT_DEPTH=1000 \
+ -DMATCH_LIMIT_RECURSION=1000 \
+ -DMAX_NAME_COUNT=100 \
-DMAX_NAME_SIZE=32 \
- -DPARENS_NEST_LIMIT=250 \
+ -DPARENS_NEST_LIMIT=10 \
All tests still pass, and from my own testing I can't find any
non-pathological patterns this would break. So we might actually
consider turning this down for git by default.
* Much of the rest of the patterns PCRE has CVEs for can similarly be
mitigated by simply turning various features off.
Furthermore, at work we distribute Git with all releases of our product.
We normally only do non-security updates to the last couple of releases,
but we provide security updates to all supported versions. I'm not
comfortable shipping the entirety of PCRE or PCRE2 to customers without
providing security updates, so you're going to make my job (and my
coworkers') a lot harder by shipping it. Please don't.
I'm not going to make your job harder. This WIP patch I have for
embedding PCRE2 as compat/pcre2 is for use by those who'd want to get
a faster grep, but for whatever reason don't have a handy PCRE v2
package already, or wouldn't mind supporting a git that comes bundled
with it.
My comments about "git rm"-ing kwset & compat/regex were meant in the
sense of "I have a branch where we don't use that anymore, and it's
faster", not that you couldn't compile a git without PCRE for the
forseeable future.
My WIP branch does remove much of the non-PCRE code, but that's just
because it was easier to hack up like that and not support a bunch of
if pcre/else branches, I'll amend that and keep PCRE optional when I
end up submitting those patches.
On Tue, May 9, 2017 at 12:40 PM, Johannes Schindelin
[off-list ref] wrote:
Hi,
On Tue, 9 May 2017, brian m. carlson wrote:
quoted
On Tue, May 09, 2017 at 02:00:18AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
On Tue, May 9, 2017 at 1:32 AM, brian m. carlson
[off-list ref] wrote:
quoted
PCRE and PCRE2 also tend to have a lot of security updates, so I
would prefer if we didn't import them into the tree. It is far
better for users to use their distro's packages for PCRE, as it
means they get automatic security updates even if they're using an
old Git.
We shouldn't consider shipping anything with a remotely frequent
history of security updates in our tree, since people very
frequently run old or ancient versions of Git.
I'm aware of its security record[1], but I wonder what threat model
you have in mind here. I'm not aware of any parts of git (except maybe
gitweb?) where we take regexes from untrusted sources.
I.e. yes there have been DoS's & even some overflow bugs leading code
execution in PCRE, but in the context of powering git-grep & git-log
with PCRE this falls into the "stop hitting yourself" category.
Just because you don't drive Git with untrusted regexes doesn't mean
other people don't.
Or other applications.
quoted
It's not a good idea to require a stronger security model than we
absolutely have to, since people can and will violate it. Think how
devastating Shellshock was even though technically nobody should provide
insecure environment variables to the shell.
And, yes, gitweb does in fact call git grep. That means that git grep
must in fact be secure against untrusted regexes, or you have a remote
code execution vulnerability.
And not only grep is affected. Think HEAD^{/<regex>}. There are plenty of
sites where you are allowed to specify revs in a freer form than SHA-1s.
That will still use reg(comp|exec) for the foreseeable future. We have
plenty of manual use of that all over the place:
$ git grep 'reg(comp|exec)\(' *.[ch] builtin/*.[ch]
And the ^{/rx} feature is powered by the one in sha1_name.c
Having said that, I do like the prospect of a faster git grep.
Hopefully there will be a way to make use of PCRE that can be switched
off? Like, a compile-time replacement of the regex API backed by PCRE v2
*iff* PCRE v2 is used for building?
Yup, see my just-sent
[off-list ref].
It'll be optional for now, as it's been for a while.
Aside from that I do think given these numbers it's worth considering
making PCRE a default dependency, and possibly getting rid of stuff
like kwset because a) it reduces the many codepaths we have now of
either doing fixed/basic/extended/pcre into one b) since the numbers
suggest pcre can support all of that faster that seems like a sensible
thing to do.
But anything like that will be a few patch series's down the road, for
now I'm just making it all optional.
On 9 May 2017 at 13:12, Ævar Arnfjörð Bjarmason [off-list ref] wrote:
On Tue, May 9, 2017 at 2:37 AM, brian m. carlson
[off-list ref] wrote:
quoted
On Tue, May 09, 2017 at 02:00:18AM +0200, Ævar Arnfjörð Bjarmason wrote:
* gitweb is vulnerable to CPU DoS now in its default configuration.
It's easy to provide an ERE that ends up slurping up 100% CPU for
several seconds on any non-trivial sized repo, do that in parallel &
you have a DoS vector.
Does one need an ERE? Can't one do that now to many parts of git just
with a glob?
Yves
On Tue, May 9, 2017 at 4:22 PM, demerphq [off-list ref] wrote:
On 9 May 2017 at 13:12, Ævar Arnfjörð Bjarmason [off-list ref] wrote:
quoted
On Tue, May 9, 2017 at 2:37 AM, brian m. carlson
[off-list ref] wrote:
quoted
On Tue, May 09, 2017 at 02:00:18AM +0200, Ævar Arnfjörð Bjarmason wrote:
* gitweb is vulnerable to CPU DoS now in its default configuration.
It's easy to provide an ERE that ends up slurping up 100% CPU for
several seconds on any non-trivial sized repo, do that in parallel &
you have a DoS vector.
Does one need an ERE? Can't one do that now to many parts of git just
with a glob?
in practice I don't think so because:
1) I'm now aware of any place where we expose globbing over the wire.
2) AFAICT for the issue detailed in [1] to trigger you also need a
pathological filename in the repo, e.g. I can't get git-ls-files to go
quadratic on either git.git or linux.git, whereas it's pretty easy to
come up with a really expensive regex since there's more content to
choose from when matching file content than filenames.
1. https://public-inbox.org/git/20170424211249.28553-1-avarab@gmail.com/