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

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P1
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

(Blocks 1 bug)

Trunk
mozilla61
Unspecified
Windows
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: inj+)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
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.
Attachment #8967179 - Flags: review?(davidp99)
(Assignee)

Comment 2

a year ago
Rebased atop latest revision of bug 1432653
Attachment #8967179 - Attachment is obsolete: true
Attachment #8967179 - Flags: review?(davidp99)
Attachment #8968634 - Flags: review?(davidp99)
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?
Attachment #8968634 - Flags: review?(davidp99) → review+
(Assignee)

Comment 6

a year ago
(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.
Flags: needinfo?(davidp99)
Makes sense to me.  nit - can you add a comment to kInlineStorage to that effect?  Beyond that, its all good.
Flags: needinfo?(davidp99)
(Assignee)

Comment 9

a year ago
Will do.
(Assignee)

Comment 10

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

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eea27e04c541
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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?
Flags: needinfo?(aklotz)
Flags: in-testsuite+
(Assignee)

Comment 13

a year ago
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.
Flags: needinfo?(aklotz)
Thanks for the info, Aaron, that helps a lot. NI Cornel so Release QA knows about the new test for 61+.
Flags: needinfo?(cornel.ionce)
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?
Flags: needinfo?(cornel.ionce) → needinfo?(aklotz)
(Assignee)

Comment 16

11 months ago
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.
Flags: needinfo?(aklotz)
You need to log in before you can comment on or make changes to this bug.