New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

--remote-header-name doesn't work when a scheme is missing from a URL #760

Closed
nodakai opened this Issue Apr 11, 2016 · 12 comments

Comments

Projects
None yet
3 participants
@nodakai

nodakai commented Apr 11, 2016

I did this

curl -v -OJL win.rustup.rs

I expected the following

I expected to successfully download rustup-setup.exe according to

Content-Disposition: attachement; filename=rustup-setup.exe

but I got the following error:

{ [3724 bytes data]
Warning: Remote filename has no length!
* Failed writing body (0 != 3724)
  0 9026k    0  3724    0     0   7548      0  0:20:24 --:--:--  0:20:24  7538
* Closing connection 0
curl: (23) Failed writing body (0 != 3724)

curl/libcurl version

$ curl -V
curl 7.48.0-DEV (x86_64-unknown-linux-gnu) libcurl/7.48.0-DEV OpenSSL/1.0.2c zlib/1.2.3 libidn/1.18
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
Features: IDN IPv6 Largefile NTLM NTLM_WB SSL libz TLS-SRP UnixSockets

operating system

$ more /etc/redhat-release
CentOS release 6.6 (Final)

@nodakai nodakai changed the title from remote-header-name to --remote-header-name doesn't work when a scheme is missing from a URL Apr 11, 2016

@bagder

This comment has been minimized.

Member

bagder commented Apr 11, 2016

I don't think it is related to the missing scheme, I think it is the redirect that causes it. Hm, but why does the scheme-using one work then... ?

@jay

This comment has been minimized.

Member

jay commented Apr 11, 2016

We don't honor the content disposition filename unless the scheme is explicit and http or https. How about we relax it a little:

diff --git a/src/tool_operate.c b/src/tool_operate.c
index c8bf12b..94a9658 100644
--- a/src/tool_operate.c
+++ b/src/tool_operate.c
@@ -1290,7 +1290,10 @@ static CURLcode operate_do(struct GlobalConfig *global,
         if(config->content_disposition
            && (urlnode->flags & GETOUT_USEREMOTE)
            && (checkprefix("http://", this_url) ||
-               checkprefix("https://", this_url)))
+               checkprefix("https://", this_url) ||
+               (!strstr(this_url, "://") &&
+                (!config->proto_default ||
+                 checkprefix("http", config->proto_default)))))
           hdrcbdata.honor_cd_filename = TRUE;
         else
           hdrcbdata.honor_cd_filename = FALSE;
@bagder

This comment has been minimized.

Member

bagder commented Apr 11, 2016

I completely forgot that piece of logic.

I'm thinking perhaps we can skip the protocol check completely. There are (or at least these once existed) for example users who'd get FTP:// URLs over a HTTP proxy that can also get HTTP headers like this. To me, it seems quite harmless to just remove that check and have all protocols get this ability.

Thoughts?

@jay

This comment has been minimized.

Member

jay commented Apr 11, 2016

born in a884ffe. If we remove the scheme check could there be a vulnerability if some other protocol sends a content disposition header with the filename?

@bagder

This comment has been minimized.

Member

bagder commented Apr 12, 2016

I don't see how any other protocol would, and we only check headers if asked for it with the specific command line option so it seems like a very small risk that would ever happen.

@bagder bagder added the cmdline tool label Apr 18, 2016

@bagder

This comment has been minimized.

Member

bagder commented Apr 25, 2016

I say please land that patch and close this issue!

@bagder bagder closed this in 0dc4d8e Apr 29, 2016

@bagder

This comment has been minimized.

Member

bagder commented Apr 29, 2016

I landed your patch with a test that verifies it. Thanks!

@jay

This comment has been minimized.

Member

jay commented Apr 30, 2016

For the record the way you landed this does not open up a risk afaict since the scheme check is still there, and I'm fine with that!

@bagder

This comment has been minimized.

Member

bagder commented Apr 30, 2016

It was your patch, I hope you like it! =)

It is still a small risk AFAICS because of libcurl's protocol "guessing" logic based on the host name prefix. If you specify "curl ftp.funet.fi", it will speak FTP...

@jay

This comment has been minimized.

Member

jay commented Apr 30, 2016

Oops I forgot about that, I guess I'll eat my words. How about this:

diff --git a/src/tool_cb_hdr.c b/src/tool_cb_hdr.c
index 5be02aa..f7d8355 100644
--- a/src/tool_cb_hdr.c
+++ b/src/tool_cb_hdr.c
@@ -48,6 +48,7 @@ size_t tool_header_cb(void *ptr, size_t size, size_t nmemb, vo
   const char *str = ptr;
   const size_t cb = size * nmemb;
   const char *end = (char*)ptr + cb;
+  char *url = NULL;

   /*
    * Once that libcurl has called back tool_header_cb() the returned value
@@ -88,7 +89,9 @@ size_t tool_header_cb(void *ptr, size_t size, size_t nmemb, vo
    */

   if(hdrcbdata->honor_cd_filename &&
-     (cb > 20) && checkprefix("Content-disposition:", str)) {
+     (cb > 20) && checkprefix("Content-disposition:", str) &&
+     !curl_easy_getinfo(outs->config->easy, CURLINFO_EFFECTIVE_URL, &url) &&
+     url && (checkprefix("http://", url) || checkprefix("https://", url))) {
     const char *p = str + 20;

     /* look for the 'filename=' parameter
diff --git a/src/tool_operate.c b/src/tool_operate.c
index ab29c00..5f49efb 100644
--- a/src/tool_operate.c
+++ b/src/tool_operate.c
@@ -1295,12 +1295,7 @@ static CURLcode operate_do(struct GlobalConfig *global,
           my_setopt_flags(curl, CURLOPT_REDIR_PROTOCOLS, config->proto_redir);

         if(config->content_disposition
-           && (urlnode->flags & GETOUT_USEREMOTE)
-           && (checkprefix("http://", this_url) ||
-               checkprefix("https://", this_url) ||
-               (!strstr(this_url, "://") &&
-                (!config->proto_default ||
-                 checkprefix("http", config->proto_default)))))
+           && (urlnode->flags & GETOUT_USEREMOTE))
           hdrcbdata.honor_cd_filename = TRUE;
         else
           hdrcbdata.honor_cd_filename = FALSE;
@bagder

This comment has been minimized.

Member

bagder commented May 1, 2016

Oh, clever. Should work just fine indeed!

jay added a commit that referenced this issue May 1, 2016

tool_cb_hdr: Fix --remote-header-name with schemeless URL
- Move the existing scheme check from tool_operate.

In the case of --remote-header-name we want to parse Content-disposition
for a filename, but only if the scheme is http or https. A recent
adjustment 0dc4d8e was made to account for schemeless URLs however it's
not 100% accurate. To remedy that I've moved the scheme check to the
header callback, since at that point the library has already determined
the scheme.

Bug: #760
Reported-by: Kai Noda
@jay

This comment has been minimized.

Member

jay commented May 1, 2016

Thanks, landed in b9728bc.

@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.