Re: [PATCH 1/2] http: Fix handling of missing CURLPROTO_*

2017-08-20 Thread Jeff King
On Fri, Aug 11, 2017 at 05:30:34PM -0700, Junio C Hamano wrote:

> > -#if LIBCURL_VERSION_NUM >= 0x071304
> > +#ifdef CURLPROTO_HTTP
> > curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
> >  get_curl_allowed_protocols(0));
> > curl_easy_setopt(result, CURLOPT_PROTOCOLS,
> 
> This may make the code to _compile_, but is it sensible to let the
> code build and be used by the end users without the "these protocols
> are safe" filter, I wonder?  
> 
> Granted, ancient code was unsafe and people were happily using it,
> but now we know better, and more importantly, we have since added
> users of transport (e.g. blindly fetch submodules recursively) that
> may _rely_ on this layer of the code safely filtering unsafe
> protocols, so...

I don't think Tom's patch changes this in any meaningful way. The
"fallback to skipping the safety and showing a warning" dates back to
the original introduction of the feature.

But FWIW, that is exactly the kind of thing that led me to wanting to
implement a hard cutoff in the first place. The warning is small
consolation if Git allows an attack through anyway. Or worse, you don't
even see the warning because it's an automated process that is being
exploited.

There's a good chance if you have such an antique curl that it is also
riddled with other curl-specific bugs that have since been fixed. And
that would argue that we don't need to care that much anyway; people
running old curl have decided that it's not worth caring about the
security implications.

But in the case of RHEL, in theory they are patching security bugs in
curl but just not implementing new features. So if we have a
vulnerability introduced by using an old version of curl, we really are
making things worse. And that argues for having a hard cutoff.

But as Tom's series demonstrates, they are backporting _some_ features
(presumably ones needed by other programs like Git to fix security
bugs). Which argues for having #ifdefs that handle those backports,
which in theory gives us a secure Git on systems that do careful
backporting, and gives us an insecure-but-not-worse-than-it-already-was
Git on systems that don't do backporting.

I dunno. There were a lot of assumptions and mental gymnastics there.
I'm still tempted to target curl >= 7.19.4 just based on timing and
RHEL5's support life-cycle.

-Peff


Re: [PATCH 1/2] http: Fix handling of missing CURLPROTO_*

2017-08-12 Thread Tom G. Christensen

On 12/08/17 02:30, Junio C Hamano wrote:

This may make the code to _compile_, but is it sensible to let the
code build and be used by the end users without the "these protocols
are safe" filter, I wonder?



Git will display a warning at runtime if this is not available but 
perhaps this warning could be worded more strongly and/or make reference 
to CVE-2009-0037.


-tgc


Re: [PATCH 1/2] http: Fix handling of missing CURLPROTO_*

2017-08-11 Thread Junio C Hamano
"Tom G. Christensen"  writes:

> Commit aeae4db1 refactored the handling of the curl protocol restriction
> support into a function but failed to add a version check for older
> versions of curl that lack CURLPROTO_* support.
> This adds the missing check and at the same time converts it to a feature
> check instead of a version based check.
> This is done to ensure that vendor supported curl versions that have had
> CURLPROTO_* support backported are handled correctly.
>
> Signed-off-by: Tom G. Christensen 
> ---
>  http.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/http.c b/http.c
> index e00264cff..569909e8a 100644
> --- a/http.c
> +++ b/http.c
> @@ -685,6 +685,7 @@ void setup_curl_trace(CURL *handle)
>   curl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);
>  }
>  
> +#ifdef CURLPROTO_HTTP
>  static long get_curl_allowed_protocols(int from_user)
>  {
>   long allowed_protocols = 0;
> @@ -700,6 +701,7 @@ static long get_curl_allowed_protocols(int from_user)
>  
>   return allowed_protocols;
>  }
> +#endif
>  
>  static CURL *get_curl_handle(void)
>  {
> @@ -798,7 +800,7 @@ static CURL *get_curl_handle(void)
>  #elif LIBCURL_VERSION_NUM >= 0x071101
>   curl_easy_setopt(result, CURLOPT_POST301, 1);
>  #endif
> -#if LIBCURL_VERSION_NUM >= 0x071304
> +#ifdef CURLPROTO_HTTP
>   curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
>get_curl_allowed_protocols(0));
>   curl_easy_setopt(result, CURLOPT_PROTOCOLS,

This may make the code to _compile_, but is it sensible to let the
code build and be used by the end users without the "these protocols
are safe" filter, I wonder?  

Granted, ancient code was unsafe and people were happily using it,
but now we know better, and more importantly, we have since added
users of transport (e.g. blindly fetch submodules recursively) that
may _rely_ on this layer of the code safely filtering unsafe
protocols, so...