Re: Java org.chromium.url.Origin

[Łukasz Anforowicz]

+net-dev to more broadly communicate the current plan for dealing with incorrect url->origin conversions (e.g. ending up using a wrong origin for "about:blank" and other cases after `render_frame_host.GetLastCommittedURL().GetOrigin()` or after `url::Origin::Create(render_frame_host.GetLastCommittedURL()`).  For more motivating gotchas please see https://chromium.googlesource.com/chromium/src/+/main/docs/security/origin-vs-url.md.  AFAIU the current plan is as follows:

Part1: Remove most calls to GURL::GetOrigin (https://crbug.com/512374):

• Step 1.1: Rename GURL::GetOrigin into DeprecatedGetOriginAsURL (started in https://crrev.com/c/3220292 - thanks mkwst@!)
• Step 1.2: Audit and replace DeprecatedGetOriginAsURL calls with either:
• A call to url::Origin::Resolve(gurl, base_origin)
• or a call to reintroduced GURL::UnsafeGetOrigin (maybe returning url::SchemeHostPort instead of GURL and/or url::Origin)

Part2: Remove most calls to url::Origin::Create (https://crbug.com/1259711):

• Step 1.1: Rename url::Origin::Create into A) url::Origin::CreateForTesting [if path contains "test" substring] or B) url::Origin::DeprecatedCreate
• Step 1.2: Audit and replace url::Origin::DeprecatedCreate calls with either:
• A call to url::Origin::Resolve(gurl, base_origin)
• or a call to reintroduced url::Origin::UnsafeCreate (or LossyCreate?  or SubtleCreate?)

Using url::Origin::Resolve can still lead to trouble (it doesn't take iframe.sandbox attribute into account + the caller can bring back url::Origin::Create behavior by conjuring and passing url::Origin() as the base origin), but hopefully having to explicitly think about which origin to pass will steer people toward using frame.GetLastCommittedOrigin().

Any feedback welcomed (including naming bike shedding - should it be url::Origin::CreateForTesting method vs url::Origin::ForTest method vs TEST_ORIGIN macro?;  should it be url::Origin::UnsafeCreate vs LossyCreate vs SubtleCreate vs PleaseCreateOriginBecauseIReallyKnowWhatIAmDoing).

On Wed, Oct 13, 2021 at 8:42 AM Avi Drissman <a...@google.com> wrote:

I put together a CL introducing a Java version of Origin::Create, but if we want to be more deliberate here I can withdraw it. Does someone here have more experience in Java to build the Java Origin API we want?

On Wed, Oct 13, 2021 at 10:05 AM Mike West <mk...@chromium.org> wrote:

On Wed, Oct 13, 2021 at 3:50 PM Łukasz Anforowicz <luk...@chromium.org> wrote:

On Wed, Oct 13, 2021 at 12:43 AM Mike West <mk...@chromium.org> wrote:

Yikes indeed.

I have no opinion about the Java questions here, but I'll put up a patch to deprecate the existing usage of `GetOrigin()`, and another to wrap `url::Origin::Create(url)` as `GURL::GetOrigin()`.

url::Origin::Create(url) is also prone to subtle mistakes - it ignores iframe.sandbox and mishandles about:blank and about:srcdoc (see also https://chromium.googlesource.com/chromium/src/+/main/docs/security/origin-vs-url.md#avoid-converting-urls-to-origins).  If we are effectively introducing a new method, then can we implement the new GURL::GetOrigin on top of url::Origin::Resolve and take 2 parameters (GURL=this, and a mandatory origin)?  In the (rare?) case when the caller really does only have a GURL, we can either 1) pass url::Origin() (an opaque origin) as the 2nd argument or 2) introduce another overload where the 2nd argument is an enum value that's spelled something like GURL::kUnsafeUrlToOriginConversion or GURL::kIKnowWhatIAmDoing.

I think I agree with all of this. I do think it's useful to be able to understand the origin of a given URL, but I also agree that that's often a footgun that isn't enabling the comparison you really want. It might be worthwhile to go through some of the soon-to-be-deprecated callsites to see what they're actually trying to do before adding a method on GURL...

The first patch is https://chromium-review.googlesource.com/c/chromium/src/+/3220292, but I should probably take it through the LSC process...

This LGTM.  Maybe it is worth pointing to //docs/security/origin-vs-url.md in the comment of the deprecated method?

I added you as a reviewer on the CL. Happy to make whatever changes to the comment you'd like to see. :)

-mike

[Matt Menke]

So how do you get an origin in the first place?  url::Origin::Resolve requires an origin to call, so you need some origin to start with.  Origins are often used when persisting data to disk and restoring it (admittedly, these should always be HTTP/HTTPs origins, though we shouldn't DCHECK that data loaded from disk is valid)

--
You received this message because you are subscribed to the Google Groups "net-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to net-dev+u...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/net-dev/CAA_NCUHWXzf4pB%3Dmho7Ka_%2BNi7y%3DeEEiNedW1Ss4VM%2Bypf3ZXg%40mail.gmail.com.

[Łukasz Anforowicz]

On Thu, Oct 14, 2021 at 9:48 AM Matt Menke <mme...@chromium.org> wrote:

So how do you get an origin in the first place?

Browser process:

RenderFrameHost::GetLastCommittedOrigin()

NavigationHandle::GetInitiatorOrigin

Network service process:

net::URLRequest::initiator()

network::ResourceRequest::initiator_origin

Renderer process:

blink::Frame::GetSecurityContext and blink::SecurityContext::GetSecurityOrigin

(it is probably desirable to come up with additional examples)

 

url::Origin::Resolve requires an origin to call, so you need some origin to start with.

Ack.  I hope that there are very few scenarios when there is no such origin to start with (I am addressing serialization below).

 

Origins are often used when persisting data to disk and restoring it (admittedly, these should always be HTTP/HTTPs origins, though we shouldn't DCHECK that data loaded from disk is valid)

Serializing an origin by round-tripping via url is lossy (e.g. it would lose origin's precursor information from url::Origin::GetTupleOrPrecursorTupleIfOpaque).  For serialization and deserialization we shouldn't use url::Origin::Create nor url::Origin::Resolve but rather use one of the url::Origin::Serialize* methods and the url::Origin::Deserialize method.

[Matt Menke]

We don't generally persist opaque origins, and have historically been explicitly discouraged from doing so.  url::Origin::Serialize is also lossy, since it drops precursor data.  I'm not seeing how that's different from persisting as a URL.  Am I missing something?

[Łukasz Anforowicz]

On Thu, Oct 14, 2021 at 10:41 AM Matt Menke <mme...@chromium.org> wrote:

We don't generally persist opaque origins, and have historically been explicitly discouraged from doing so.  url::Origin::Serialize is also lossy, since it drops precursor data.  I'm not seeing how that's different from persisting as a URL.  Am I missing something?

Ooops, my bad.  I knew that mojo serialization persists precursors and incorrectly assumed that it is implemented on top of url::Origin::Serialize.  And it seems that //url/mojom/origin_mojom_traits.cc doesn't currently expose any functions that could be reused for serialization/deserialization outside of mojo.  Providing such helper functions seems like something that would need to be done as part of auditing and replacing url::Origin::DeprecatedCreate.

[mark a. foltz]

Many unit tests create origins from strings via url::Origin::Create(GURL("")).

It sounds like you are adding some conditions in the future around origin creation.  What are those conditions?  Who will be responsible for fixing all of the tests that rely on pre-existing origin strings that don't mee the conditions?

Consider not setting conditions on url::Origin::CreateForTesting and declaring it in a test-only compilation unit so the migration for tests can be somewhat mechanical (not requiring updating test logic).

m.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/net-dev/CAA_NCUHZmAD20j1w4E_4K7TERLTnGzJLoJE_dvAEZ%2BYT%2BHN6sw%40mail.gmail.com.

[Daniel Cheng]

I actually think we should make it easier for unit tests to create origins—they should be able to skip the GURL() boilerplate if they just have a std::string, IMO.

Daniel

[Łukasz Anforowicz]

On Thu, Oct 14, 2021 at 12:07 PM Daniel Cheng <dch...@chromium.org> wrote:

I actually think we should make it easier for unit tests to create origins—they should be able to skip the GURL() boilerplate if they just have a std::string, IMO.

+1

Maybe we can have an overload of url::Origin::CreateForTesting that takes a base::StringPiece (in addition to the overload taking a `const GURL& `).  I think such a new overload would should be introduced in a separate, somewhat-orthogonal CL.

Daniel

On Thu, 14 Oct 2021 at 12:00, mark a. foltz <mfo...@chromium.org> wrote:

Many unit tests create origins from strings via url::Origin::Create(GURL("")).

It sounds like you are adding some conditions in the future around origin creation.  What are those conditions?

`url::Origin::Create(gurl)` *may* work okay if the following conditions hold:

• `gurl` is not an about:blank, nor about:srcdoc URL (nor about:blank?query=blah#ref
• The caller does not care about iframe sandboxing (via iframe.sandbox attribute, via CSP, etc.)
• The caller does not care about precursors (e.g. remembering that an opaque origin associated with a data: URL came from a http://foo.example.com origin).

I think `url::Origin::Create(gurl)` should be okay in most or even all tests. (Or `url::Origin::CreateForTesting(gurl)` after the proposed changes).

 

  Who will be responsible for fixing all of the tests that rely on pre-existing origin strings that don't mee the conditions?

If there are tests that incorrectly use `url::Origin::Create` then the incorrect usage already exists today.  The proposed changes (e.g. introducing `url::Origin::CreateForTesting`) do not introduce new incorrect behavior.  Therefore, I don't think the proposal needs to point out how to tackle such incorrect usage.  (I think we would use whatever process Chromium uses for dealing with test bugs elsewhere.)

Consider not setting conditions on url::Origin::CreateForTesting and declaring it in a test-only compilation unit so the migration for tests can be somewhat mechanical (not requiring updating test logic).

Right.  `url::Origin::CreateForTesting` will not enforce any conditions - the caller will need to "be careful". 

[Matt Menke]

I've been thinking about this, and it seems doable, though we should perhaps consider switching some consumers over to using SchemeHostPort in net/ wherever we're keying stuff by origin, since that's what we really mean by origin in net/, with the exception of the initiator.  We key a lot of things on origin+NetworkIsolationKey, and in a number of places we want to get the origin back out as a serialized string (e.g., H2 and H3 both do this as part of the protocol for messages I'm not familiar with).  Keying on the additional nonce only really introduces problems when we want to serialize the origin, but it's generally extraneous data, given that we need to key on NIKs to protect against cross-site leaks.  I guess creating methods to "really" serialize on origin would work, but I suspect that's not a direction we want to go in.

[Avi Drissman]

What is there to be done on https://crbug.com/1255713? It seems to be that we should do work on cleaning up our Origin story, but that seems to be a much bigger task than that bug.

[Avi Drissman]

Does anyone have ideas about what else to do about https://crbug.com/1255713? We should clean up Origins, but at this point the immediate problems pointed out in the bug are fixed.