Skip to content
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

urlapi: stricter CURLUPART_PORT parsing #3762

Closed
wants to merge 1 commit into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Apr 11, 2019

Only allow well formed decimal numbers in the input.

Document that the number MUST be between 1 and 65535.

Add tests to test 1560 to verify the above.

Ref: #3753

lib/urlapi.c Outdated Show resolved Hide resolved
Copy link
Member

@danielgustafsson danielgustafsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bagder
Copy link
Member Author

bagder commented Apr 11, 2019

Nice fix @jay. I think we should also add a test for that case. I can do that tomorrow.

@jay
Copy link
Member

jay commented Apr 11, 2019

I might not have gone far enough. The reporter in #3753 basically showed that CURLOPT_PORT 1234L set that port in his curlu but CURLOPT_PORT 0L didn't reset it to the default port. Of course neither one of those things should have had any effect on his curlu and you fixed that in #3761. Separately from that however is still the issue of how it is treated internally. The reporter repro boiled down is basically this:

curl_easy_setopt(curl, CURLOPT_CURLU, curlu);
curl_easy_setopt(curl, CURLOPT_PORT, 1234L);
curl_code = curl_easy_perform(curl);
// now reset port to go back to port 80 (not!)
curl_easy_setopt(curl, CURLOPT_PORT, 0L);
curl_code = curl_easy_perform(curl);

Internally parse_remote_port() will be called in the first case to update the internal CURLU (ie the CURLU that would be copied from the user's curlu in this example). Yet that does not happen in the second case since data->set.use_port is 0:

curl/lib/url.c

Lines 3009 to 3025 in 521bbbe

static CURLcode parse_remote_port(struct Curl_easy *data,
struct connectdata *conn)
{
if(data->set.use_port && data->state.allow_port) {
/* if set, we use this instead of the port possibly given in the URL */
char portbuf[16];
CURLUcode uc;
conn->remote_port = (unsigned short)data->set.use_port;
msnprintf(portbuf, sizeof(portbuf), "%d", conn->remote_port);
uc = curl_url_set(data->state.uh, CURLUPART_PORT, portbuf, 0);
if(uc)
return CURLE_OUT_OF_MEMORY;
}
return CURLE_OK;
}

My fix would allow the user to use curl_url_set(uh, CURLUPART_PORT, "", 0); to reset the port in their own CURLU (which is read-only for us) but doesn't address this with the internal CURLU. In other words the user still can't use CURLOPT_PORT 0L to reset the port in the URL AFAICS (granted I haven't actually tried this yet).

@bagder
Copy link
Member Author

bagder commented Apr 12, 2019

I might not have gone far enough.

I think you're overthinking it. This PR and change is only about how to parse the CURLUPART_PORT string when curl_url_set is called.

The #3753 issue was totally separate. It occurred primarily because curl modified the handle that it wasn't supposed to touch so even when setting CURLOPT_PORT to zero, the port number in the URL wasn't correctly reset. That part was fixed in #3761.

@bagder
Copy link
Member Author

bagder commented Apr 12, 2019

Oh sorry, this extra "" support is unnecessary. The man page actually already says:

Setting a part to a NULL pointer will effectively remove that part's contents from the CURLU handle.

So NULL, not "", has that effect. I'll add a test case for it.

@jay
Copy link
Member

jay commented Apr 12, 2019

Oops. I also missed that in the code and now it seems totally obvious. Does portnum need to be cleared:

diff --git a/lib/urlapi.c b/lib/urlapi.c
index 3da8ff8..0eb06d2 100644
--- a/lib/urlapi.c
+++ b/lib/urlapi.c
@@ -1145,6 +1145,7 @@ CURLUcode curl_url_set(CURLU *u, CURLUPart what,
       storep = &u->host;
       break;
     case CURLUPART_PORT:
+      u->portnum = 0;
       storep = &u->port;
       break;
     case CURLUPART_PATH:

@bagder
Copy link
Member Author

bagder commented Apr 12, 2019

yes!

Only allow well formed decimal numbers in the input.

Document that the number MUST be between 1 and 65535.

Add tests to test 1560 to verify the above.

Ref: #3753
@bagder bagder force-pushed the bagder/urlapi-set-port-zero branch from 7161253 to 89543f7 Compare April 12, 2019 11:30
@bagder bagder closed this in d715d2a Apr 13, 2019
@bagder bagder deleted the bagder/urlapi-set-port-zero branch April 13, 2019 09:18
@lock lock bot locked as resolved and limited conversation to collaborators Jul 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

4 participants