net/url: URL allows malformed query round trip #22907

artyom · 2017-11-28T20:32:07Z

What did you do?

package main

import (
	"fmt"
	"log"
	"net/url"
)

func main() {
	u, err := url.Parse("http://example.com/bad path/?bad query#bad fragment")
	if err != nil {
		log.Fatal(err)
	}
	fmt.Println(u.String())
}

https://play.golang.org/p/hdX1zpv3BN

What did you expect to see?

I expect either url.Parse return a non-nil error or URL.String method return fully escaped url representation — http://example.com/bad%20path/?bad%20query#bad%20fragment — with query being escaped the same way as path or fragment.

What did you see instead?

http://example.com/bad%20path/?bad query#bad%20fragment

For the reference, such url is rejected by net/http.Server: https://play.golang.org/p/2gujmbXZlu

Does this issue reproduce with the latest release (go1.9.2)?

Yes

System details

go version devel +9a13f8e11c Tue Nov 28 06:47:50 2017 +0000 darwin/amd64
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/artyom/Library/Caches/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/tmp/go:/Users/artyom/go"
GORACE=""
GOROOT="/Users/artyom/Repositories/go"
GOTMPDIR=""
GOTOOLDIR="/Users/artyom/Repositories/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/lb/3rk8rqs53czgb4v35w_342xc0000gn/T/go-build624293827=/tmp/go-build -gno-record-gcc-switches -fno-common"
GOROOT/bin/go version: go version devel +9a13f8e11c Tue Nov 28 06:47:50 2017 +0000 darwin/amd64
GOROOT/bin/go tool compile -V: compile version devel +9a13f8e11c Tue Nov 28 06:47:50 2017 +0000
uname -v: Darwin Kernel Version 17.2.0: Fri Sep 29 18:27:05 PDT 2017; root:xnu-4570.20.62~3/RELEASE_X86_64
ProductName:	Mac OS X
ProductVersion:	10.13.1
BuildVersion:	17B48
lldb --version: lldb-900.0.57
  Swift-4.0

https://tools.ietf.org/html/rfc3986#section-3.4 states that query component should be defined as (appendix A):

  query       = *( pchar / "/" / "?" )
  pchar         = unreserved / pct-encoded / sub-delims / ":" / "@"
  unreserved    = ALPHA / DIGIT / "-" / "." / "_" / "~"
  pct-encoded   = "%" HEXDIG HEXDIG
  sub-delims    = "!" / "$" / "&" / "'" / "(" / ")"
                  / "*" / "+" / "," / ";" / "="

There's no whitespace character in this list. whatwg agrees on that:

A URL-query string must be zero or more URL units.

[...]

The URL units are URL code points and percent-encoded bytes.

[...]

The URL code points are ASCII alphanumeric, U+0021 (!), U+0024 ($), U+0026 (&), U+0027 ('), U+0028 LEFT PARENTHESIS, U+0029 RIGHT PARENTHESIS, U+002A (*), U+002B (+), U+002C (,), U+002D (-), U+002E (.), U+002F (/), U+003A (:), U+003B (;), U+003D (=), U+003F (?), U+0040 (@), U+005F (_), U+007E (~), and code points in the range U+00A0 to U+10FFFD, inclusive, excluding surrogates and noncharacters.

The text was updated successfully, but these errors were encountered:

namusyaka · 2017-12-08T01:59:53Z

note:
This seems to be related to this line, and its behavior looks reasonable.

namusyaka · 2017-12-08T18:41:13Z

I have investigated the issue and confirmed that this comment is incorrect, or at least the current (*URL) String() implementation is wrong.
In fact, query components aren't escaped at all, not only in this example.
If the comment is positive, I think we should escape the query component appropriately.
/cc @tombergan

fraenkel · 2017-12-08T23:29:13Z

@namusyaka I think you are mistaken. The problem is with all forms of Parse which does no validation on the query or fragment pieces. The parse of the uri should have failed. Since it does not, the Fragment or RawQuery fields are incorrect and hence String() which is doing the right thing appears to provide a bad url.

All that has to happen is to apply validation of the query and fragment pieces in parse, and that should solve the issues. It might start breaking code that is sending around "bad" URLs.

fraenkel · 2017-12-09T04:06:56Z

Its not clear that this can be fixed given the behavior of shouldEscape() which states that most of the sub-delims should escape.

namusyaka · 2017-12-09T04:47:04Z

@fraenkel Yes, I knew it's more easy to understand the behavior.
However, if your opinion will reflect to the implementation, another cases such as http://example.com/foo bar, http://example.com/foo? bar and http://example.com/foo?bar# baz should be failed on parse, and then I'm worried that breaks backword-compatibility.

gopherbot · 2018-03-07T09:58:38Z

Change https://golang.org/cl/99135 mentions this issue: net/url: reject invalid query strings when parsing URLs.

bradfitz · 2018-07-11T23:13:52Z

Copying my comment from the CL:

I changed the commit message to accurately reflect what later versions of this CL did. It now says:

net/url: escape URL.RawQuery on Parse if it contains invalid characters

But while reviewing this, I'm worried it might change the behavior of:

https://golang.org/pkg/net/url/#URL.Query
which says:

"It silently discards malformed value pairs. To check errors use ParseQuery."

Before it silently discarded things, and with this CL it would start returning escaped versions instead. Do we care?

I think we probably should wait for Go 1.12 at this point. This needs a decision on what to do.

bradfitz · 2018-09-26T17:59:20Z

Reopening, as the fix is being reverted.

gopherbot · 2018-09-26T20:21:50Z

Change https://golang.org/cl/137716 mentions this issue: Revert "net/url: escape URL.RawQuery on Parse if it contains invalid characters"

…characters" This reverts commit CL 99135 (git rev 1040626). Reason for revert: breaks valid code; see #27302 Fixes #27302 Updates #22907 Change-Id: I82bb0c28ae1683140c71e7a2224c4ded3f4acea1 Reviewed-on: https://go-review.googlesource.com/137716 Reviewed-by: Ian Lance Taylor <iant@golang.org>

agnivade · 2018-12-09T18:24:01Z

@bradfitz - Any thoughts on this ? Or do we want to push to 1.13 ?

bradfitz · 2018-12-13T17:55:12Z

I'm kinda done with net/url.URL changes. They're always problematic compatibility-wise.

Definitely not for Go 1.12.

gopherbot · 2019-01-23T19:12:25Z

Change https://golang.org/cl/159157 mentions this issue: net/url, net/http: reject control characters in URLs

This is a more conservative version of the reverted CL 99135 (which was reverted in CL 137716) The net/url part rejects URLs with ASCII CTLs from being parsed and the net/http part rejects writing them if a bogus url.URL is constructed otherwise. Updates #27302 Updates #22907 Change-Id: I09a2212eb74c63db575223277aec363c55421ed8 Reviewed-on: https://go-review.googlesource.com/c/159157 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Filippo Valsorda <filippo@golang.org>

FiloSottile · 2019-01-24T18:28:55Z

@gopherbot please open backport issues.

This has security implications, and CL 159157 is safe enough to backport.

gopherbot · 2019-01-24T18:29:04Z

Backport issue(s) opened: #29922 (for 1.10), #29923 (for 1.11).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

gopherbot · 2019-01-24T22:55:17Z

Change https://golang.org/cl/159478 mentions this issue: [release-branch.go1.10] net/url, net/http: reject control characters in URLs

gopherbot · 2019-01-29T17:25:27Z

Change https://golang.org/cl/160178 mentions this issue: net/url, net/http: relax CTL-in-URL validation to only ASCII CTLs

CL 159157 was doing UTF-8 decoding of URLs. URLs aren't really UTF-8, even if sometimes they are in some contexts. Instead, only reject ASCII CTLs. Updates #27302 Updates #22907 Change-Id: Ibd64efa5d3a93263d175aadf1c9f87deb4670c62 Reviewed-on: https://go-review.googlesource.com/c/160178 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>

gopherbot · 2019-01-31T20:25:36Z

Change https://golang.org/cl/160678 mentions this issue: [release-branch.go1.10] net/http, net/url: reject control characters in URLs

gopherbot · 2019-02-01T15:28:15Z

Change https://golang.org/cl/160798 mentions this issue: [release-branch.go1.11] net/http, net/url: reject control characters in URLs

…in URLs Cherry pick of combined CL 159157 + CL 160178. Fixes #29923 Updates #27302 Updates #22907 Change-Id: I6de92c14284595a58321a4b4d53229285979b872 Reviewed-on: https://go-review.googlesource.com/c/160798 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>

…in URLs Cherry pick of combined CL 159157 + CL 160178. Fixes #29922 Updates #27302 Updates #22907 Change-Id: I6de92c14284595a58321a4b4d53229285979b872 Reviewed-on: https://go-review.googlesource.com/c/160678 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>

ianlancetaylor · 2019-02-15T23:01:41Z

Is there anything else to do for this issue?

gopherbot · 2019-02-15T23:43:39Z

Change https://golang.org/cl/162960 mentions this issue: doc/go1.12: document net/url.Parse now rejecting ASCII CTLs

gopherbot · 2019-02-15T23:54:43Z

Change https://golang.org/cl/162826 mentions this issue: [release-branch.go1.12] doc/go1.12: document net/url.Parse now rejecting ASCII CTLs

Updates #27302 Updates #22907 Change-Id: Iac6957f3517265dfb9c662efb7af31192e3bfd6c Reviewed-on: https://go-review.googlesource.com/c/162960 Reviewed-by: Ian Lance Taylor <iant@golang.org>

…ing ASCII CTLs Updates #27302 Updates #22907 Change-Id: Iac6957f3517265dfb9c662efb7af31192e3bfd6c Reviewed-on: https://go-review.googlesource.com/c/162960 Reviewed-by: Ian Lance Taylor <iant@golang.org> (cherry picked from commit ef454fd) Reviewed-on: https://go-review.googlesource.com/c/162826

CL 159157 was doing UTF-8 decoding of URLs. URLs aren't really UTF-8, even if sometimes they are in some contexts. Instead, only reject ASCII CTLs. Updates golang#27302 Updates golang#22907 Change-Id: Ibd64efa5d3a93263d175aadf1c9f87deb4670c62 Reviewed-on: https://go-review.googlesource.com/c/160178 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>

Updates golang#27302 Updates golang#22907 Change-Id: Iac6957f3517265dfb9c662efb7af31192e3bfd6c Reviewed-on: https://go-review.googlesource.com/c/162960 Reviewed-by: Ian Lance Taylor <iant@golang.org>

CL 159157 was doing UTF-8 decoding of URLs. URLs aren't really UTF-8, even if sometimes they are in some contexts. Instead, only reject ASCII CTLs. Updates golang#27302 Updates golang#22907 Change-Id: Ibd64efa5d3a93263d175aadf1c9f87deb4670c62 Reviewed-on: https://go-review.googlesource.com/c/160178 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>

Updates golang#27302 Updates golang#22907 Change-Id: Iac6957f3517265dfb9c662efb7af31192e3bfd6c Reviewed-on: https://go-review.googlesource.com/c/162960 Reviewed-by: Ian Lance Taylor <iant@golang.org>

bradfitz added this to the Go1.11 milestone Nov 28, 2017

bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 28, 2017

bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 11, 2018

bradfitz modified the milestones: Go1.11, Go1.12 Jul 11, 2018

gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 11, 2018

gopherbot closed this as completed in 1040626 Aug 21, 2018

ayanamist mentioned this issue Aug 28, 2018

net/url: regression after #22907 fixed #27302

Closed

bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 26, 2018

bradfitz reopened this Sep 26, 2018

gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 26, 2018

agnivade added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Dec 9, 2018

bradfitz modified the milestones: Go1.12, Go1.13 Dec 13, 2018

gibson042 mentioned this issue Dec 23, 2018

net/url: PathEscape treatment of reserved characters is inconsistent #27559

Open

This was referenced Jan 24, 2019

net/url: URL allows malformed query round trip [1.10 backport] #29922

Closed

net/url: URL allows malformed query round trip [1.11 backport] #29923

Closed

dmitshur mentioned this issue Jan 24, 2019

[release-branch.go1.10] net/url, net/http: reject control characters in URLs #29926

Closed

mattn mentioned this issue Mar 13, 2019

net/http: CRLF injection vulnerability #30794

Closed

bradfitz mentioned this issue Mar 18, 2019

net/url: Malformed query code breaks backwards compatibility #30903

Closed

alin04 mentioned this issue Apr 4, 2019

net/url: URL.String() does not escape Query #30922

Open

andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019

rsc modified the milestones: Go1.14, Backlog Oct 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net/url: URL allows malformed query round trip #22907

net/url: URL allows malformed query round trip #22907

artyom commented Nov 28, 2017

namusyaka commented Dec 8, 2017 •

edited

namusyaka commented Dec 8, 2017

fraenkel commented Dec 8, 2017

fraenkel commented Dec 9, 2017

namusyaka commented Dec 9, 2017

gopherbot commented Mar 7, 2018

bradfitz commented Jul 11, 2018

bradfitz commented Sep 26, 2018

gopherbot commented Sep 26, 2018

agnivade commented Dec 9, 2018

bradfitz commented Dec 13, 2018

gopherbot commented Jan 23, 2019

FiloSottile commented Jan 24, 2019

gopherbot commented Jan 24, 2019

gopherbot commented Jan 24, 2019

gopherbot commented Jan 29, 2019

gopherbot commented Jan 31, 2019

gopherbot commented Feb 1, 2019

ianlancetaylor commented Feb 15, 2019

gopherbot commented Feb 15, 2019

gopherbot commented Feb 15, 2019

net/url: URL allows malformed query round trip #22907

net/url: URL allows malformed query round trip #22907

Comments

artyom commented Nov 28, 2017

What did you do?

What did you expect to see?

What did you see instead?

Does this issue reproduce with the latest release (go1.9.2)?

System details

namusyaka commented Dec 8, 2017 • edited

namusyaka commented Dec 8, 2017

fraenkel commented Dec 8, 2017

fraenkel commented Dec 9, 2017

namusyaka commented Dec 9, 2017

gopherbot commented Mar 7, 2018

bradfitz commented Jul 11, 2018

bradfitz commented Sep 26, 2018

gopherbot commented Sep 26, 2018

agnivade commented Dec 9, 2018

bradfitz commented Dec 13, 2018

gopherbot commented Jan 23, 2019

FiloSottile commented Jan 24, 2019

gopherbot commented Jan 24, 2019

gopherbot commented Jan 24, 2019

gopherbot commented Jan 29, 2019

gopherbot commented Jan 31, 2019

gopherbot commented Feb 1, 2019

ianlancetaylor commented Feb 15, 2019

gopherbot commented Feb 15, 2019

gopherbot commented Feb 15, 2019

namusyaka commented Dec 8, 2017 •

edited