{"thread":{"id":"46568","subject":"[PATCH 2/2] http: use a feature check to enable GSSAPI delegation control","startedAt":"2017-08-11T16:37:55Z","lastAt":"2017-08-23T15:41:24Z","messageCount":11,"participants":["Tom G. Christensen","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"326144","messageId":"b037d7116bb6eac965a5cba2357611b156c07b8d.1502462884.git.tgc@jupiterrise.com","threadId":"46568","inReplyTo":"cover.1502462884.git.tgc@jupiterrise.com","subject":"[PATCH 2/2] http: use a feature check to enable GSSAPI delegation control","fromName":"Tom G. Christensen","fromEmail":"tgc@jupiterrise.com","sentAt":"2017-08-11T16:37:34Z","receivedAt":"2017-08-11T16:37:55Z","isPatch":true,"sender":{"key":"tgc@jupiterrise.com","avatar":"https://avatars.githubusercontent.com/u/912180?v=4"},"body":"Turn the version check into a feature check to ensure this functionality\nis also enabled with vendor supported curl versions where the feature\nmay have been backported.\n\nSigned-off-by: Tom G. Christensen \n---\n http.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 569909e8a..a3ae58f13 100644\n--- a/http.c\n+++ b/http.c\n@@ -91,7 +91,7 @@ static struct {\n \t * here, too\n \t */\n };\n-#if LIBCURL_VERSION_NUM >= 0x071600\n+#ifdef CURLGSSAPI_DELEGATION_FLAG\n static const char *curl_deleg;\n static struct {\n \tconst char *name;\n@@ -356,7 +356,7 @@ static int http_options(const char *var, const char *value, void *cb)\n \t}\n \n \tif (!strcmp(\"http.delegation\", var)) {\n-#if LIBCURL_VERSION_NUM >= 0x071600\n+#ifdef CURLGSSAPI_DELEGATION_FLAG\n \t\treturn git_config_string(&curl_deleg, var, value);\n #else\n \t\twarning(_(\"Delegation control is not supported with cURL < 7.22.0\"));\n@@ -727,7 +727,7 @@ static CURL *get_curl_handle(void)\n \tcurl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);\n #endif\n \n-#if LIBCURL_VERSION_NUM >= 0x071600\n+#ifdef CURLGSSAPI_DELEGATION_FLAG\n \tif (curl_deleg) {\n \t\tint i;\n \t\tfor (i = 0; i < ARRAY_SIZE(curl_deleg_levels); i++) {\n-- \n2.14.1\n\n"},{"id":"326145","messageId":"cover.1502462884.git.tgc@jupiterrise.com","threadId":"46568","inReplyTo":"030356f8-0472-7400-c9f6-7492788dd2d0@jupiterrise.com","subject":"[PATCH 0/2] http: handle curl with vendor backports","fromName":"Tom G. Christensen","fromEmail":"tgc@jupiterrise.com","sentAt":"2017-08-11T16:37:32Z","receivedAt":"2017-08-11T16:37:56Z","isPatch":true,"sender":{"key":"tgc@jupiterrise.com","avatar":"https://avatars.githubusercontent.com/u/912180?v=4"},"body":"The curl packages provided by Red Hat for RHEL contain several\nbackports of features from later curl releases.\nThis causes problems with current version based checks in http.c.\n\nHere is an overview of the features that have been backported:\n7.10.6 (el3) Backports CURLPROTO_*\n7.12.1 (el4) Backports CURLPROTO_*\n7.15.5 (el5) Backports GSSAPI_DELEGATION_*\n Backports CURLPROTO_*\n7.19.7 (el6) Backports GSSAPI_DELEGATION_*\n Backports CURL_SSL_VERSION_TLSv1_{0,1,2}\n7.29.0 (el7) Backports CURL_SSL_VERSION_TLSv1_{0,1,2}\n\nThis patch series will update the current version based checks for\nprotocol restriction and GSSAPI delegation control support to ones\nbased on features to properly deal with the above listed backports.\nThe fine grained TLS version support does not seem to be\ndistinguishable via a preprocessor macro so I've left that alone.\n\nI have build tested these changes against upstream curl 7.12.0 (fails),\n7.12.1 and 7.15.5. I have also built and run the testsuite against the\nRed Hat provided curl versions listed above.\n\nTom G. Christensen (2):\n http: Fix handling of missing CURLPROTO_*\n http: use a feature check to enable GSSAPI delegation control\n\n http.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\n-- \n2.14.1\n\n"},{"id":"326146","messageId":"4d29d43d458f61c6dabca093f591ad8698ca2ceb.1502462884.git.tgc@jupiterrise.com","threadId":"46568","inReplyTo":"cover.1502462884.git.tgc@jupiterrise.com","subject":"[PATCH 1/2] http: Fix handling of missing CURLPROTO_*","fromName":"Tom G. Christensen","fromEmail":"tgc@jupiterrise.com","sentAt":"2017-08-11T16:37:33Z","receivedAt":"2017-08-11T16:37:58Z","isPatch":true,"sender":{"key":"tgc@jupiterrise.com","avatar":"https://avatars.githubusercontent.com/u/912180?v=4"},"body":"Commit aeae4db1 refactored the handling of the curl protocol restriction\nsupport into a function but failed to add a version check for older\nversions of curl that lack CURLPROTO_* support.\nThis adds the missing check and at the same time converts it to a feature\ncheck instead of a version based check.\nThis is done to ensure that vendor supported curl versions that have had\nCURLPROTO_* support backported are handled correctly.\n\nSigned-off-by: Tom G. Christensen \n---\n http.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/http.c b/http.c\nindex e00264cff..569909e8a 100644\n--- a/http.c\n+++ b/http.c\n@@ -685,6 +685,7 @@ void setup_curl_trace(CURL *handle)\n \tcurl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);\n }\n \n+#ifdef CURLPROTO_HTTP\n static long get_curl_allowed_protocols(int from_user)\n {\n \tlong allowed_protocols = 0;\n@@ -700,6 +701,7 @@ static long get_curl_allowed_protocols(int from_user)\n \n \treturn allowed_protocols;\n }\n+#endif\n \n static CURL *get_curl_handle(void)\n {\n@@ -798,7 +800,7 @@ static CURL *get_curl_handle(void)\n #elif LIBCURL_VERSION_NUM >= 0x071101\n \tcurl_easy_setopt(result, CURLOPT_POST301, 1);\n #endif\n-#if LIBCURL_VERSION_NUM >= 0x071304\n+#ifdef CURLPROTO_HTTP\n \tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,\n \t\t\t get_curl_allowed_protocols(0));\n \tcurl_easy_setopt(result, CURLOPT_PROTOCOLS,\n-- \n2.14.1\n\n"},{"id":"326174","messageId":"xmqq1sohzr85.fsf@gitster.mtv.corp.google.com","threadId":"46568","inReplyTo":"cover.1502462884.git.tgc@jupiterrise.com","subject":"Re: [PATCH 0/2] http: handle curl with vendor backports","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-11T22:15:06Z","receivedAt":"2017-08-11T22:15:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tom G. Christensen\" writes:\n\n> The curl packages provided by Red Hat for RHEL contain several\n> backports of features from later curl releases.\n> This causes problems with current version based checks in http.c.\n>\n> Here is an overview of the features that have been backported:\n> 7.10.6 (el3) Backports CURLPROTO_*\n> 7.12.1 (el4) Backports CURLPROTO_*\n> 7.15.5 (el5) Backports GSSAPI_DELEGATION_*\n> Backports CURLPROTO_*\n> 7.19.7 (el6) Backports GSSAPI_DELEGATION_*\n> Backports CURL_SSL_VERSION_TLSv1_{0,1,2}\n> 7.29.0 (el7) Backports CURL_SSL_VERSION_TLSv1_{0,1,2}\n>\n> This patch series will update the current version based checks for\n> protocol restriction and GSSAPI delegation control support to ones\n> based on features to properly deal with the above listed backports.\n> The fine grained TLS version support does not seem to be\n> distinguishable via a preprocessor macro so I've left that alone.\n\nThanks; these feature macros ought to be more dependable, and I\nthink this moves things in the right direction (regardless of which\nfeatures we might later pick as mandatory and cut off supports for\nolder versions).\n\n> I have build tested these changes against upstream curl 7.12.0 (fails),\n> 7.12.1 and 7.15.5. I have also built and run the testsuite against the\n> Red Hat provided curl versions listed above.\n\nHmph, what does \"(fails)\" mean here?\n\n>\n> Tom G. Christensen (2):\n> http: Fix handling of missing CURLPROTO_*\n> http: use a feature check to enable GSSAPI delegation control\n>\n> http.c | 10 ++++++----\n> 1 file changed, 6 insertions(+), 4 deletions(-)\n"},{"id":"326182","messageId":"xmqqo9rly6dx.fsf@gitster.mtv.corp.google.com","threadId":"46568","inReplyTo":"4d29d43d458f61c6dabca093f591ad8698ca2ceb.1502462884.git.tgc@jupiterrise.com","subject":"Re: [PATCH 1/2] http: Fix handling of missing CURLPROTO_*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-12T00:30:34Z","receivedAt":"2017-08-12T00:30:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tom G. Christensen\" writes:\n\n> Commit aeae4db1 refactored the handling of the curl protocol restriction\n> support into a function but failed to add a version check for older\n> versions of curl that lack CURLPROTO_* support.\n> This adds the missing check and at the same time converts it to a feature\n> check instead of a version based check.\n> This is done to ensure that vendor supported curl versions that have had\n> CURLPROTO_* support backported are handled correctly.\n>\n> Signed-off-by: Tom G. Christensen \n> ---\n> http.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/http.c b/http.c\n> index e00264cff..569909e8a 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -685,6 +685,7 @@ void setup_curl_trace(CURL *handle)\n> \tcurl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);\n> }\n> \n> +#ifdef CURLPROTO_HTTP\n> static long get_curl_allowed_protocols(int from_user)\n> {\n> \tlong allowed_protocols = 0;\n> @@ -700,6 +701,7 @@ static long get_curl_allowed_protocols(int from_user)\n> \n> \treturn allowed_protocols;\n> }\n> +#endif\n> \n> static CURL *get_curl_handle(void)\n> {\n> @@ -798,7 +800,7 @@ static CURL *get_curl_handle(void)\n> #elif LIBCURL_VERSION_NUM >= 0x071101\n> \tcurl_easy_setopt(result, CURLOPT_POST301, 1);\n> #endif\n> -#if LIBCURL_VERSION_NUM >= 0x071304\n> +#ifdef CURLPROTO_HTTP\n> \tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,\n> \t\t\t get_curl_allowed_protocols(0));\n> \tcurl_easy_setopt(result, CURLOPT_PROTOCOLS,\n\nThis may make the code to _compile_, but is it sensible to let the\ncode build and be used by the end users without the \"these protocols\nare safe\" filter, I wonder? \n\nGranted, ancient code was unsafe and people were happily using it,\nbut now we know better, and more importantly, we have since added\nusers of transport (e.g. blindly fetch submodules recursively) that\nmay _rely_ on this layer of the code safely filtering unsafe\nprotocols, so...\n\n"},{"id":"326190","messageId":"1b91d00f-eeae-30cf-0889-8fad05d849d3@jupiterrise.com","threadId":"46568","inReplyTo":"xmqq1sohzr85.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] http: handle curl with vendor backports","fromName":"Tom G. Christensen","fromEmail":"tgc@jupiterrise.com","sentAt":"2017-08-12T06:20:03Z","receivedAt":"2017-08-12T06:20:14Z","isPatch":true,"sender":{"key":"tgc@jupiterrise.com","avatar":"https://avatars.githubusercontent.com/u/912180?v=4"},"body":"On 12/08/17 00:15, Junio C Hamano wrote:\n> \"Tom G. Christensen\" writes:\n\n>> I have build tested these changes against upstream curl 7.12.0 (fails),\n>> 7.12.1 and 7.15.5. I have also built and run the testsuite against the\n>> Red Hat provided curl versions listed above.\n> \n> Hmph, what does \"(fails)\" mean here?\n> \n\nIt means building against 7.12.0 fails which is expected because it is \nmissing CURLINFO_SSL_DATA_{IN,OUT}. There are patches in the other \nthread that would add support for curl < 7.12.1 if necessary.\n\n-tgc\n"},{"id":"326207","messageId":"91420770-f02e-7685-9c0e-f840633f01d5@jupiterrise.com","threadId":"46568","inReplyTo":"xmqqo9rly6dx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] http: Fix handling of missing CURLPROTO_*","fromName":"Tom G. Christensen","fromEmail":"tgc@jupiterrise.com","sentAt":"2017-08-12T09:04:20Z","receivedAt":"2017-08-12T09:04:31Z","isPatch":true,"sender":{"key":"tgc@jupiterrise.com","avatar":"https://avatars.githubusercontent.com/u/912180?v=4"},"body":"On 12/08/17 02:30, Junio C Hamano wrote:\n> This may make the code to _compile_, but is it sensible to let the\n> code build and be used by the end users without the \"these protocols\n> are safe\" filter, I wonder?\n> \n\nGit will display a warning at runtime if this is not available but \nperhaps this warning could be worded more strongly and/or make reference \nto CVE-2009-0037.\n\n-tgc\n"},{"id":"326802","messageId":"20170820084725.ce5inn5jzkyor4zk@sigill.intra.peff.net","threadId":"46568","inReplyTo":"xmqq1sohzr85.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] http: handle curl with vendor backports","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-20T08:47:25Z","receivedAt":"2017-08-20T08:47:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 11, 2017 at 03:15:06PM -0700, Junio C Hamano wrote:\n\n> \"Tom G. Christensen\" writes:\n> \n> > The curl packages provided by Red Hat for RHEL contain several\n> > backports of features from later curl releases.\n> > This causes problems with current version based checks in http.c.\n> >\n> > Here is an overview of the features that have been backported:\n> > 7.10.6 (el3) Backports CURLPROTO_*\n> > 7.12.1 (el4) Backports CURLPROTO_*\n> > 7.15.5 (el5) Backports GSSAPI_DELEGATION_*\n> > Backports CURLPROTO_*\n> > 7.19.7 (el6) Backports GSSAPI_DELEGATION_*\n> > Backports CURL_SSL_VERSION_TLSv1_{0,1,2}\n> > 7.29.0 (el7) Backports CURL_SSL_VERSION_TLSv1_{0,1,2}\n> >\n> > This patch series will update the current version based checks for\n> > protocol restriction and GSSAPI delegation control support to ones\n> > based on features to properly deal with the above listed backports.\n> > The fine grained TLS version support does not seem to be\n> > distinguishable via a preprocessor macro so I've left that alone.\n> \n> Thanks; these feature macros ought to be more dependable, and I\n> think this moves things in the right direction (regardless of which\n> features we might later pick as mandatory and cut off supports for\n> older versions).\n\nYes, I agree that these are an improvement regardless. If we follow\nthrough on the cut-off to 7.19.4, then the CURLPROTO ones all go away.\nBut I don't mind rebasing any cut-off proposal on top of this work.\n\n-Peff\n"},{"id":"326803","messageId":"20170820085913.w6pxal3hlpwbio74@sigill.intra.peff.net","threadId":"46568","inReplyTo":"xmqqo9rly6dx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] http: Fix handling of missing CURLPROTO_*","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-20T08:59:14Z","receivedAt":"2017-08-20T08:59:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 11, 2017 at 05:30:34PM -0700, Junio C Hamano wrote:\n\n> > -#if LIBCURL_VERSION_NUM >= 0x071304\n> > +#ifdef CURLPROTO_HTTP\n> > \tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,\n> > \t\t\t get_curl_allowed_protocols(0));\n> > \tcurl_easy_setopt(result, CURLOPT_PROTOCOLS,\n> \n> This may make the code to _compile_, but is it sensible to let the\n> code build and be used by the end users without the \"these protocols\n> are safe\" filter, I wonder? \n> \n> Granted, ancient code was unsafe and people were happily using it,\n> but now we know better, and more importantly, we have since added\n> users of transport (e.g. blindly fetch submodules recursively) that\n> may _rely_ on this layer of the code safely filtering unsafe\n> protocols, so...\n\nI don't think Tom's patch changes this in any meaningful way. The\n\"fallback to skipping the safety and showing a warning\" dates back to\nthe original introduction of the feature.\n\nBut FWIW, that is exactly the kind of thing that led me to wanting to\nimplement a hard cutoff in the first place. The warning is small\nconsolation if Git allows an attack through anyway. Or worse, you don't\neven see the warning because it's an automated process that is being\nexploited.\n\nThere's a good chance if you have such an antique curl that it is also\nriddled with other curl-specific bugs that have since been fixed. And\nthat would argue that we don't need to care that much anyway; people\nrunning old curl have decided that it's not worth caring about the\nsecurity implications.\n\nBut in the case of RHEL, in theory they are patching security bugs in\ncurl but just not implementing new features. So if we have a\nvulnerability introduced by using an old version of curl, we really are\nmaking things worse. And that argues for having a hard cutoff.\n\nBut as Tom's series demonstrates, they are backporting _some_ features\n(presumably ones needed by other programs like Git to fix security\nbugs). Which argues for having #ifdefs that handle those backports,\nwhich in theory gives us a secure Git on systems that do careful\nbackporting, and gives us an insecure-but-not-worse-than-it-already-was\nGit on systems that don't do backporting.\n\nI dunno. There were a lot of assumptions and mental gymnastics there.\nI'm still tempted to target curl >= 7.19.4 just based on timing and\nRHEL5's support life-cycle.\n\n-Peff\n"},{"id":"326822","messageId":"xmqqziau6w63.fsf@gitster.mtv.corp.google.com","threadId":"46568","inReplyTo":"20170820084725.ce5inn5jzkyor4zk@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] http: handle curl with vendor backports","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-20T16:28:20Z","receivedAt":"2017-08-20T16:28:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Aug 11, 2017 at 03:15:06PM -0700, Junio C Hamano wrote:\n>\n>> \"Tom G. Christensen\" writes:\n>> \n>> > The curl packages provided by Red Hat for RHEL contain several\n>> > backports of features from later curl releases.\n>> > This causes problems with current version based checks in http.c.\n>> >\n>> > Here is an overview of the features that have been backported:\n>> > 7.10.6 (el3) Backports CURLPROTO_*\n>> > 7.12.1 (el4) Backports CURLPROTO_*\n>> > 7.15.5 (el5) Backports GSSAPI_DELEGATION_*\n>> > Backports CURLPROTO_*\n>> > 7.19.7 (el6) Backports GSSAPI_DELEGATION_*\n>> > Backports CURL_SSL_VERSION_TLSv1_{0,1,2}\n>> > 7.29.0 (el7) Backports CURL_SSL_VERSION_TLSv1_{0,1,2}\n>> >\n>> > This patch series will update the current version based checks for\n>> > protocol restriction and GSSAPI delegation control support to ones\n>> > based on features to properly deal with the above listed backports.\n>> > The fine grained TLS version support does not seem to be\n>> > distinguishable via a preprocessor macro so I've left that alone.\n>> \n>> Thanks; these feature macros ought to be more dependable, and I\n>> think this moves things in the right direction (regardless of which\n>> features we might later pick as mandatory and cut off supports for\n>> older versions).\n>\n> Yes, I agree that these are an improvement regardless. If we follow\n> through on the cut-off to 7.19.4, then the CURLPROTO ones all go away.\n> But I don't mind rebasing any cut-off proposal on top of this work.\n\nYeah I came to a similar conclusion and was about asking if you feel\nthe same way that your series should be made on top of Tom's fixes.\n\nThe aspect of that series I do like the most is to base our\ndecisions on features, not versions, and I also wonder if we can do\nsimilar in your \"abandon too old ones\" series, too.\n\nThanks.\n"},{"id":"327045","messageId":"20170823154114.zanr26hg2silquez@sigill.intra.peff.net","threadId":"46568","inReplyTo":"xmqqziau6w63.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] http: handle curl with vendor backports","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-23T15:41:14Z","receivedAt":"2017-08-23T15:41:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 20, 2017 at 09:28:20AM -0700, Junio C Hamano wrote:\n\n> > Yes, I agree that these are an improvement regardless. If we follow\n> > through on the cut-off to 7.19.4, then the CURLPROTO ones all go away.\n> > But I don't mind rebasing any cut-off proposal on top of this work.\n> \n> Yeah I came to a similar conclusion and was about asking if you feel\n> the same way that your series should be made on top of Tom's fixes.\n> \n> The aspect of that series I do like the most is to base our\n> decisions on features, not versions, and I also wonder if we can do\n> similar in your \"abandon too old ones\" series, too.\n\nYeah, I don't mind moving to feature flags where we can (though some\nfeatures do not have a useful flag; e.g., the only way to know whether\nwe must be strdup curl_easy_setopt() arguments is by checking the curl\nversion).\n\nOne annoying thing about \"feature\" flags instead of version flags is\nthat it takes a lot of legwork to figure out how old those features are\n(whereas with the versions I was able to look that up in the curl\nhistory pretty easily). Since people adding the feature flag generally\ndo that legwork, it's probably worth having a comment for each\nmentioning the general vintage (or maybe the commit message is an OK\nplace for that).\n\nI actually wonder if it is worth defining our own readable flags in a\nbig table at the beginning of the file, like:\n\n /*\n * introduced in curl 7.19.4, but backported by some distros like\n * RHEL. We can identify it by the presence of the PROTO flags.\n */\n #ifdef CURLPROTO_HTTP\n #define CURL_SUPPORTS_PROTOCOL_REDIRECTION\n #endif\n\nThat keeps the logic in one place (where it can be changed if we later\nfind that the define we picked for our feature isn't quite accurate).\nAnd then the #ifdefs sprinkled through the code itself become\nself-documenting.\n\n-Peff\n"}]}