Sharing validation logic for blink and non-blink mojom types

[Matt Menke]

For FLEDGE auctions, we have an InterestGroup Mojo struct.  Since it uses strings, urls, and origins, Mojo makes separate blink and non-blink versions of the struct.  There's an (experimental) web-exposed API where a page passes us a Javascript version of the struct, we validate it (throwing exceptions if it's not valid), and then pass it to the browser process, which the re-validates it before using it, since we don't trust a renderer, and an invalid group could lead to a cross-site attack.

The validation logic should be largely identical in both processes, and I'd like to make sure it stays in sync.  The best way I see to do this is to either co-locate the blink and non-blink validation, or at least use one set of tests which test both sets of logic, using Mojo to convert between the two types.  Co-location doesn't seem to work, since content/ code can't include code from blink/renderer/modules, where the blink validation is needed, and blink/common code currently doesn't depend on internal blink types (SecurityOrigin, WTF::String, etc).

That leaves putting the code in different places, but sharing tests.  The only way I see to do this is to have tests and the blink validation code in blink/renderer/modules, and the non-blink validation code in blink/common, with a header in blink/public/common, which is included only by content/browser/.  Experimentally, this works, and is consistent with blink/common using Chrome types, including std::string.  However, I'm running into a single presubmit warning for the include of the non-blink mojom file in unit test file in blink/renderer/modules that points to this group:

"Files that include non-Blink variant mojoms found. You must include .mojom-blink.h, .mojom-forward.h or .mojom-shared.h"

The non-Blink mojom file is only used in a unit test file, not production code, so this seems fine to me.  Is it reasonable to exclude "test." files from the presubmit, or is there some other path forward here I should take, possibly changing how I make the two copies of the validation logic share tests?

Any feedback is much appreciated.

[Kentaro Hara]

Thanks Matt for reaching out!

"Files that include non-Blink variant mojoms found. You must include .mojom-blink.h, .mojom-forward.h or .mojom-shared.h"
The non-Blink mojom file is only used in a unit test file, not production code, so this seems fine to me.  Is it reasonable to exclude "test." files from the presubmit, or is there some other path forward here I should take, possibly changing how I make the two copies of the validation logic share tests?

It's fine to exclude the test from the presubmit. As written here, it's allowed to allow the blink vs. non-blink type exceptions at the boundary layer -- because otherwise you cannot share any code between blink and non-blink.

Another option is that you could put one validation code in blink/common/ (using non-blink types) and let Blink use it. Blink can convert SecurityOrigin, WTF::String etc to non-blink types and call the code in blink/common/. As long as the type conversion is done at the boundary layer (i.e., just before calling the code in blink/common/), it's fine. Would it avoid duplicating the validation code twice maybe? :)

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAEK7mvr3f19WQw6E8TJ_imO%2BVEGhSQ-uOZ_Tsp-hGRPPoGoSZQ%40mail.gmail.com.

--

Kentaro Hara, Tokyo

[Matt Menke]

Sorry, I accidentally hit reply in my first response, adding back the list

On Sun, Jun 27, 2021 at 10:11 PM Matt Menke <mme...@google.com> wrote:

On Sun, Jun 27, 2021 at 9:58 PM Kentaro Hara <har...@chromium.org> wrote:

Are you suggesting:
1) Add an inclusion exception for renderer/modules/ to include the forbidden non-blink mojom file (there are enough fields I assume you aren't suggesting they be passed in piecemeal).
2) Manually convert one Mojo from Vectors, KURLS, etc to std::vectors, GURLs, and the like, creating a non-blink Mojo struct (Or use Mojo to do so?  That seems simpler, and this shouldn't be perf sensitive).
3) Validate that non-blink Mojo struct.
4) Pass the original blink Mojo struct over Mojo, if it passes validation?

2), 3) and 4) -> correct.

Regarding 1), why do you need to include the forbidden non-blink mojom file in renderer/modules/? The original blink mojo struct is passed over mojo (i.e., renderer/modules/ uses the blink mojom), right?

The code to validate the struct takes the mojo struct.  I guess I could either make yet another struct to pass to it (the 4th for the same data:  two mojo structs, an IDL, and the new struct just for passing in to the validation method), or the validation API could take a mishmash of an origin (or two origins that we expect to be the same), 3 URLs with slightly different validation rules, and a vector of yet more URLs (which are passed between processes in yet another Mojo struct with blink and non-blink variants, and have slightly different validation rules) seems not great to me.

 

Validation is simple enough that the manual conversion seems not significantly simpler than just duplicating validation logic. If we used Mojo to convert instead, though, that seems simpler and quite robust, if a bit unusual.

If this is the case, duplicating the validation code sounds nicer to me :)

I'll go ahead and duplicate the validation code then, for now, at least.  Appreciate the suggestion, though!

 

On Mon, Jun 28, 2021 at 10:42 AM Matt Menke <mme...@google.com> wrote:

On Sun, Jun 27, 2021 at 9:28 PM Kentaro Hara <har...@chromium.org> wrote:

Thanks Matt for reaching out!

"Files that include non-Blink variant mojoms found. You must include .mojom-blink.h, .mojom-forward.h or .mojom-shared.h"
The non-Blink mojom file is only used in a unit test file, not production code, so this seems fine to me.  Is it reasonable to exclude "test." files from the presubmit, or is there some other path forward here I should take, possibly changing how I make the two copies of the validation logic share tests?

It's fine to exclude the test from the presubmit. As written here, it's allowed to allow the blink vs. non-blink type exceptions at the boundary layer -- because otherwise you cannot share any code between blink and non-blink.

Another option is that you could put one validation code in blink/common/ (using non-blink types) and let Blink use it. Blink can convert SecurityOrigin, WTF::String etc to non-blink types and call the code in blink/common/. As long as the type conversion is done at the boundary layer (i.e., just before calling the code in blink/common/), it's fine. Would it avoid duplicating the validation code twice maybe? :)

Thanks for the response!

Are you suggesting:

1) Add an inclusion exception for renderer/modules/ to include the forbidden non-blink mojom file (there are enough fields I assume you aren't suggesting they be passed in piecemeal).

2) Manually convert one Mojo from Vectors, KURLS, etc to std::vectors, GURLs, and the like, creating a non-blink Mojo struct (Or use Mojo to do so?  That seems simpler, and this shouldn't be perf sensitive).

3) Validate that non-blink Mojo struct.

4) Pass the original blink Mojo struct over Mojo, if it passes validation?

Or am I misunderstanding you?

Validation is simple enough that the manual conversion seems not significantly simpler than just duplicating validation logic. If we used Mojo to convert instead, though, that seems simpler and quite robust, if a bit unusual.

 

On Mon, Jun 28, 2021 at 2:12 AM 'Matt Menke' via platform-architecture-dev <platform-arc...@chromium.org> wrote:

For FLEDGE auctions, we have an InterestGroup Mojo struct.  Since it uses strings, urls, and origins, Mojo makes separate blink and non-blink versions of the struct.  There's an (experimental) web-exposed API where a page passes us a Javascript version of the struct, we validate it (throwing exceptions if it's not valid), and then pass it to the browser process, which the re-validates it before using it, since we don't trust a renderer, and an invalid group could lead to a cross-site attack.

The validation logic should be largely identical in both processes, and I'd like to make sure it stays in sync.  The best way I see to do this is to either co-locate the blink and non-blink validation, or at least use one set of tests which test both sets of logic, using Mojo to convert between the two types.  Co-location doesn't seem to work, since content/ code can't include code from blink/renderer/modules, where the blink validation is needed, and blink/common code currently doesn't depend on internal blink types (SecurityOrigin, WTF::String, etc).

That leaves putting the code in different places, but sharing tests.  The only way I see to do this is to have tests and the blink validation code in blink/renderer/modules, and the non-blink validation code in blink/common, with a header in blink/public/common, which is included only by content/browser/.  Experimentally, this works, and is consistent with blink/common using Chrome types, including std::string.  However, I'm running into a single presubmit warning for the include of the non-blink mojom file in unit test file in blink/renderer/modules that points to this group:

"Files that include non-Blink variant mojoms found. You must include .mojom-blink.h, .mojom-forward.h or .mojom-shared.h"

The non-Blink mojom file is only used in a unit test file, not production code, so this seems fine to me.  Is it reasonable to exclude "test." files from the presubmit, or is there some other path forward here I should take, possibly changing how I make the two copies of the validation logic share tests?

Any feedback is much appreciated.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAEK7mvr3f19WQw6E8TJ_imO%2BVEGhSQ-uOZ_Tsp-hGRPPoGoSZQ%40mail.gmail.com.

--

Kentaro Hara, Tokyo

--

Kentaro Hara, Tokyo

[Daniel Cheng]

I agree with haraken@ that adding an exception is fine.

I do have one question though: if the validation function is taking the Mojo struct directly, that sounds like we're not using typemapping and we're relying on callees to remember to call this validation function right? What sort of things is this validating, and how security-critical is it?

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAEK7mvr_HSMYcMgiHFfheKayW7GJpCXcyt5fzjFfSE%2BfkgvAyw%40mail.gmail.com.

[Matt Menke]

It contains URLs where various data related to running an auction (Scripts, JSON) can be downloaded from.  The origin of the page sending the data is also important, and validated in the same function, so we have to do validation of data the renderer can't safely provide, anyways.  With bad data, an attacker could theoretically either bid in auctions as another site, or provide bogus data to another site's bidding script.

We can't throw exceptions in Javascript on typemapping errors, which is one of the main reasons we need two validation calls in the first place, which typemapping doesn't really help us with.  We also don't want to kill a renderer if a site passes us bad data, but rather only if a compromised renderer manages to bypass those checks.

It's unclear to me the intended purpose of typemapping - the data is an origin, a couple URLs, a couple strings, etc, all stored in a database, and later consumed at a single level, so this is a rather simple struct, easily representable by Mojo object types (given GURL and origins have typemappings, of course).  We also need to check against the origin of the page the struct is passed in on, which is not something we can pass along in a Mojo struct, or handle in typemapping rules (since we don't trust the renderer to provide that information).

We already have an ID, a mojom file, and the two structs it compiles to.  Adding a 4th in-memory representation of the struct seems a bit excessive to me.