Closed Bug 653361 Opened 9 years ago Closed 8 years ago

Dll Block list doesn't work when Avast! Anti-Virus is installed

Categories

(Toolkit :: General, defect)

x86
Windows Vista
defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 681238
mozilla9

People

(Reporter: m_kato, Assigned: m_kato)

References

Details

Attachments

(1 file)

- Env 
Windows 7
Avast! Anti-Virus
Nightly 2011-04-27 x86


I found this when I investigate bug 652747.

Avast! (snxhk.dll) installs API hoook into LdrLoadDll.  It modifies function entry to "JMP rel32" (e9 xx xx xx xx).  So injection by WindowsDllInterceptor::CreateTrampoline() is always failure, then dll blocklist doesn't work correctly.
Seeing if I can track a contact down on the Avast product/eng team.
Attached patch fixSplinter Review
Assignee: nobody → m_kato
Status: NEW → ASSIGNED
Attachment #531259 - Flags: review?(vladimir)
Is there someone else who can review this patch? Seems like something we ought to fix if it really does break one of our key user protection features.
Comment on attachment 531259 [details] [diff] [review]
fix


Looks fine to me, with a few fixes:

>+      } else if (origBytes[nBytes] == 0xe9) {

Add a comment saying what 0xE9 is

>+    if (nJmp32 >= 0) {
>+      // Function entry has JMP rel32.  We replace with correct target address.
>+      byteptr_t targetAddress =
>+        origBytes + nJmp32 + 5 + (*((LONG*)(origBytes+nJmp32+1)));
>+      *((intptr_t*)(tramp+nJmp32+1)) =
>+        (intptr_t)targetAddress - (intptr_t)(tramp+nBytes+5);
>+    } else {
>+      tramp[nBytes] = 0xE9; // jmp
>+      *((intptr_t*)(tramp+nBytes+1)) = (intptr_t)trampDest - (intptr_t)(tramp+nBytes+5); // target displacement
>+    }

All the math is gross, but can't really be helped.  Does this preserve the other hook?  That is, do we chain to the right thing (antivirus) after we do our own check?  If not we should at least add a comment here saying that we don't, and file a bug... disabling people's antivirus here for Firefox seems like a bad thing (regardless of how good the antivirus is or not :).
Attachment #531259 - Flags: review?(vladimir) → review+
Makoto-san: will you have time in the next week or two to address Vlad's comments?
(In reply to comment #5)
> Makoto-san: will you have time in the next week or two to address Vlad's
> comments?

Yes, I will handle this in next week.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #7)
> I'd love to get this in the tree.

I will land this tonight after returning to home.
(In reply to Vladimir Vukicevic (:vlad) from comment #4)
> Comment on attachment 531259 [details] [diff] [review]
> fix
> 
> 
> Looks fine to me, with a few fixes:
> 
> >+      } else if (origBytes[nBytes] == 0xe9) {
> 
> Add a comment saying what 0xE9 is
> 
> >+    if (nJmp32 >= 0) {
> >+      // Function entry has JMP rel32.  We replace with correct target address.
> >+      byteptr_t targetAddress =
> >+        origBytes + nJmp32 + 5 + (*((LONG*)(origBytes+nJmp32+1)));
> >+      *((intptr_t*)(tramp+nJmp32+1)) =
> >+        (intptr_t)targetAddress - (intptr_t)(tramp+nBytes+5);
> >+    } else {
> >+      tramp[nBytes] = 0xE9; // jmp
> >+      *((intptr_t*)(tramp+nBytes+1)) = (intptr_t)trampDest - (intptr_t)(tramp+nBytes+5); // target displacement
> >+    }
> 
> All the math is gross, but can't really be helped.  Does this preserve the
> other hook?  That is, do we chain to the right thing (antivirus) after we do
> our own check?  If not we should at least add a comment here saying that we
> don't, and file a bug... disabling people's antivirus here for Firefox seems
> like a bad thing (regardless of how good the antivirus is or not :).

This fix doesn't break anti-virus's hook.  This hook will call next hook chain.
http://hg.mozilla.org/mozilla-central/rev/be9c15f7dd33
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
Any chance this caused bug 680862? Our plugin-container uses the DLL trampoline to hook SetWindowLongA and today's nightly is infinitely recursing.
Depends on: 680862
(In reply to ben turner [:bent] from comment #12)
> Any chance this caused bug 680862?

A local backout has confirmed that this is the offending patch (see bug 680862 comment 19).
Backed out, http://hg.mozilla.org/mozilla-central/rev/33e4aa663bba
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I should block the injection that same injection is already installed ...
Depends on: 681924
Root cause of 680862 is bug 681924.  Injection for plugin-container is called several times.  We should only call it once.
Same fix is integrated to bug 681238.  mark as dup.
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 681238
You need to log in before you can comment on or make changes to this bug.