Maintainable path normalization library

[htuch]

Following up on the fix for CVE-2019-9900, where we snapshotted, minified and adapted parts of the Chromium URL library to support path normalization, we should figure out how to cleanly consume Chromium URL in Envoy or switch to another lighter weight path normalization library.

Action item for CVE-2019-9901

[giorgiozoppi]

Wanna help where should i look at?

[htuch]

@giorgiozoppi I think the first step here is to figure out if we want a tighter Chromium integration for the URL library or go another path. @danzh2010 @wu-bin do you have any thoughts on where our Chromium URL consumption is going?

[giorgiozoppi]

@htuch I will dig into the chromium_url meanwhile.

[wu-bin]

@htuch : Per @vasilvv, QUICHE only uses googleurl in some client utility class, not in the core code, so although we'd like to port googleurl into QUICHE, we can't really give a specific timeframe for that to happen.

[giorgiozoppi]

Options:

1. rewrite a tiny self container url normalization using RFC 3986
2. use googleurl.
   I would like to help in porting whatever urlnormalization library to quiche.

[danzh2010]

I just into a usage of url normalization in quiche core code actually: https://cs.chromium.org/chromium/src/net/third_party/quiche/src/quic/core/crypto/quic_crypto_server_config.cc?rcl=68d15a821c88b0e85c345d1393643a25322ce608&l=858
and
https://cs.chromium.org/chromium/src/net/third_party/quiche/src/quic/core/crypto/quic_crypto_client_config.cc?rcl=68d15a821c88b0e85c345d1393643a25322ce608&l=452

These are the only two places where we probably need googleurl in core code. But having a light-weight home-grown library isn't a bad idea either.

[htuch]

The danger here is that it's easy to make mistakes in a homegrown library with significant security implications. What I would like to see if we go down this path is:

1. Homegrown library does basically the same things as Chromium URL; we don't reinvent how normalization works, and a code review would be easy to see the correspondence between the two. It should however be a subset; we want to shrink to only the RFC required normalization.
2. We add to our tests for this library all the Chromium URL tests.
3. We write a fuzzer for this library, that not only tests for badness re: crashing but also asserts correctness properties.

I think if we have 1-3, I'd feel a little better. We still won't be picking up any security updates from Chromium URL, but maybe the smaller/simpler surface will reduce the need. CC @yanavlasov @asraa

[giorgiozoppi]

@htuch @yanavlasov @asraa. We could proceed with the same set of tests and add further tests , rippoff the things we need from chromium url, create a small test library and the game is over. If it is small it should be not difficult to mantain.

[yanavlasov]

After looking at chromuim URL I think this reasonable approach rather than trying to depend on googleurl.

[danzh2010]

Below are the functions quiche needs from GURL:
bool is_valid() const;
bool GURL::SchemeIsHTTPOrHTTPS() const;
bool has_username() const;
bool has_password() const;
bool has_path() const;
base::StringPiece path_piece() const;
bool has_query() const;
bool has_ref() const;
void InitCanonical(base::BasicStringPiece input_spec,
bool trim_path_end);

[htuch]

@danzh2010 do you have a plan-of-record for obtaining these? This is essentially what we pulled from Chromium URL:

envoy/source/common/chromium_url/url_canon_path.cc

Line 412 in 8cef5e3

bool CanonicalizePath(const char* spec, const Component& path, CanonOutput* output,

[jmarantz]

https://quiche.googlesource.com/googleurl/ and Envoy links that in as part of Quic, so presumably it can use the googleurl library from there.

[moderation]

Deprecation of http-parser is relevant here - #5155 (comment)

[htuch]

From #10488 (comment):

As to release policy, this is directory is a partial copy of https://cs.chromium.org/chromium/src/url/ and https://cs.chromium.org/chromium/src/base/. Chromium has its own versioned release. Right now this directory is an arbitrary snap of that. But in the future, we can definitely use the same cut as Chrome use, like M80. Their security bugs can be tracked at https://bugs.chromium.org/p/chromium/issues/list?q=Type%3DBug-Security, but not all of them are relevant to url parsing. A tracked change between versions can be found here: https://chromium.googlesource.com/chromium/src/+log/82.0.4085.12..83.0.4093.3?pretty=fuller&n=10000

I just push all the info here. But we should definitely develop a plan about how to update and monitor this repo. As QUICHE is not the only user of this repo (we only use it for toy client), I think other users will be more alert and watchful to any security bugs than us. I would be happy to discuss and work out a plan for that.

We should try and put in place a sustainable policy around repo management here before closing out this ticket.

[htuch]

#10599 is a reasonable chunk of the work to get us here.