1451511 - Add out-of-process memory access policies to DLL interceptor

Status: RESOLVED FIXED

(Core :: General, enhancement, P1)

Description

[(No longer employed by Mozilla) Aaron Klotz]

Once the interceptor refactoring is done, write a memory access policy that allows us to set up hooks from our process into a child process.

Write a unit test too.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Add cross-process function hooking to the DLL interceptor

This patch relies on the (current) implementation of ASLR on Windows:

In particular, though module base addresses are randomized, the same module always has the same load address across all processes until the system reboots.

ie, I can safely assume that ntdll.dll will be present at the same address in another process as it is in my own process.

Of course, this isn't set in stone: If I allocated a block of VM, followed by a DLL load that was intended for the same address, the loader would have to move the DLL.

This is actually a hard problem to solve definitively: If I create a child process in the suspended state, the child process's loader is not initialized yet, so I can't query anything to verify that we're safe.

I take solace in the fact that, for our purposes, we only plan to use cross-process interceptors while the child process is in a newly created and suspended state, so it should be pretty safe to manipulate ntdll.dll and the executable binary without worrying about other stuff.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Add cross-process function hooking to the DLL interceptor (r2)

Rebased atop latest revision of bug 1432653

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dd13a1371c4a9770c0586e5276d3aca6f733bf5

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84ff74612588f04c06c3ad50edacdeacc9c6998e

Comment 5

[David Parks [:handyman]]

Comment on attachment 8968634 [details] [diff] [review]
Add cross-process function hooking to the DLL interceptor (r2)

Review of attachment 8968634 [details] [diff] [review]:
-----------------------------------------------------------------

Keeping the pointer logic straight was difficult but I think its just a think that people will have to trudge through.  (For future reference, you use both Remote and Absolute to mean essentially the same thing and consolidating might help clarify... a _little_ -- but I'm not suggesting that for now.)  In the end, tho, I found almost nothing to gripe about.

::: mozglue/misc/interceptor/TargetFunction.h
@@ +365,5 @@
> +      return;
> +    }
> +
> +    size_t newSize = aDesiredLimit + 1;
> +    if (newSize < kInlineStorage) {

I'm missing the point on the kInlineStorage stuff.  Can you fill me in on why we want to read that much -- ie where did the values for kInlineStorage come from (16 in 32b, 32 in 64b)?

@@ +382,5 @@
> +    // We couldn't pull more bytes than needed (which may happen if those extra
> +    // bytes are not accessible). In this case, we try just to get the bare
> +    // minimum.
> +    newSize = aDesiredLimit + 1;
> +    MOZ_RELEASE_ASSERT(mLocalBytes.resize(newSize));

nit: ASSERTs that do work make me nervous.  Can this be split into two lines?

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to David Parks (dparks) [:handyman] from comment #5)
> Comment on attachment 8968634 [details] [diff] [review]
> Add cross-process function hooking to the DLL interceptor (r2)
> 
> Review of attachment 8968634 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm missing the point on the kInlineStorage stuff.  Can you fill me in on
> why we want to read that much -- ie where did the values for kInlineStorage
> come from (16 in 32b, 32 in 64b)?
> 

In an ideal world, we'd only read 5 bytes on 32-bit and 13 bytes on 64-bit, to match the minimum bytes that we need to see in in order to patch the target function. Since the actual opcodes will often require us to pull in extra bytes above that minimum, I wanted to set the inline storage to be larger in an effort to give us extra wiggle room in the Vector before we need to touch the heap.

16 and 32 were kind of arbitrary, but that's what I settled on as reasonably-sized values.

ni? to make sure that makes sense and you're okay with that.

> @@ +382,5 @@
> > +    // We couldn't pull more bytes than needed (which may happen if those extra
> > +    // bytes are not accessible). In this case, we try just to get the bare
> > +    // minimum.
> > +    newSize = aDesiredLimit + 1;
> > +    MOZ_RELEASE_ASSERT(mLocalBytes.resize(newSize));
> 
> nit: ASSERTs that do work make me nervous.  Can this be split into two lines?

Sure, done.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f705dd723a99f1c9b8f3c922075d9f3dde718e7

Comment 8

[David Parks [:handyman]]

Makes sense to me.  nit - can you add a comment to kInlineStorage to that effect?  Beyond that, its all good.

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

Will do.

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eea27e04c541c8f3f1221764b00a4f95de23ef78
Bug 1451511: Add cross-process function hooking to DLL interceptor; r=handyman

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/eea27e04c541

Comment 12

[Ryan VanderMeulen [:RyanVM]]

This has been known to be pretty regression-prone code in the past and we're in the middle of a soft code freeze for 61. Can this wait until after the version bump? Also, should SoftVision be adding TestDllInterceptorCrossProcess to their periodic DLL Interceptor testing?

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

This adds the code but does not activate it within Firefox.

It's in the tree but compiled and used nowhere but TestDllInterceptorCrossProcess itself, hence my decision to land during soft freeze.

But yes, I'd say that SV should add the new test to their repertoire.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Thanks for the info, Aaron, that helps a lot. NI Cornel so Release QA knows about the new test for 61+.

Comment 15

[Cornel Ionce [:noni] [Hubs QA]]

Thanks for the heads`up, Ryan.
For now, this is our Test Plan for DLL Interceptor: https://docs.google.com/document/d/1I5IMsG7ORzbGUWfZ9VRrHEiroq2VwGia5mHbZgQAXXc/edit#

Aaron, could you please give us some test instructions / guidance in what we should add/focus on for the TestDllInterceptorCrossProcess so we can update our testing plan?

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

TestDllInterceptorCrossProcess is a command line program that should be run for both 32-bit and 64-bit builds, on each supported tier-1 Windows plaform. It does not use any special command line flags or anything, just run it.

If you see one or more TEST-UNEXPECTED-FAIL messages in its output, then the test should be considered to have failed, the failures logged, and a bug reported.

Otherwise there will only be TEST-PASS messages.