408153 - Use IAttachmentExecute antivirus API when it is available

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect, P1)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

See bug 393305 for background info.

In a nutshell, at least the following vendors have ceased to implement the IOfficeAntiVirus interface:

Norton 2008 (AntiVirus and Internet Security)
McAfee VirusScan Plus 2008
Panda AntiVirus 2008

There are likely others; AVG and Windows Defender *DO* implement this API in their latest versions.

This bug was borne out of https://bugzilla.mozilla.org/show_bug.cgi?id=393305#c11 and https://bugzilla.mozilla.org/show_bug.cgi?id=393305#c13.

Comment 1

[Ryan Jones-Ward [:sciguyryan]]

I just thought I'd add that this may not require an API implementation at all. Some software allows you to enter a command line execution path which most anti-virus software supports, when the download finishes the command is then executed and the file is scanned by the anti-virus software.

This isn't really helpful, there may not be any feedback from the call and it would require knowledge of the anti-virus software in order to determine the information to be used but its better than nothing if all else fails.

Comment 2

[Shawn Wilsher :sdwilsh]

(In reply to comment #1)
We don't want to do that though because that's vendor specific, while using a windows API is just vendor agnostic.

Comment 3

[Ryan Jones-Ward [:sciguyryan]]

That is true and would require the user to be relatively advanced, in knowing how to change the settings look up the information and so on. I'm sure that some products do this only because they feel that there are no other sure alternatives, hopefully this will not be the case.

Comment 4

[Shawn Wilsher :sdwilsh]

Well, any virus scanner that's worth anything should be scanning every file on your system anyway.  This just ensures that it's getting done...

Comment 5

[Jim Mathies [:jimm]]

Attachment: eicar virus signature file

My guess is that these vendors are tapped into the system in other ways in which files firefox downloads are still being protected. I'd bet McAfee and Norton are latched into various places - at the firewall level, through hooks like a shell execute hook, etc.. Attached is a virus signature file that should trigger these protections when hit on.

Comment 6

[Jaime Mitchell (use [e-mail] for email)]

FWIW I currently get the following behaviour downloading the example signature file:

AV: F Secure Internet Security 2007
Browser: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2007121405 Minefield/3.0b3pre ID:2007121405

Firefox displays:
"C:\Users\Jaime\Desktop\eicar.com could not be saved, because you cannot change the contents of that folder.

Change the folder properties and try again, or try saving in a different location."

F-Secure displays "Virus Detected" and offers to Disinfect, Delete etc..

I have windows Defender installed.

So currently with Firefox and F-Secure I am protected however the message from the browser is slightly confusing.

With IE 7 I get the Virus Detected alert *before* I select if I want to Run/Save/Cancel the download. I am still able to select save however it needs elevated privileges and after elevation I was not able to save it. So IE 7 is a bit more confusing than Firefox.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

I don't think we can block on something when we don't know if there's any reasonable solution. I think we're doing the right thing by supporting IOfficeAntiVirus, and we definitely want to figure out what the '08 applications are doing, but let's treat that opportunistically; seems like, for now, they are still doing the scans.

Comment 8

[Masatoshi Kimura [:emk]]

What about using IAttachmentExecute when it's aviailable and enumerating IOfficeAntiVirus manually when IAttachmentExecute is unavailabe?
IAttachmentExecute will enumerate IOfficeAntiVirus on our behalf.
IE6SP2 and IE7 seem to use only IAttachmentExecute. They don't care about IOfficeAntiVirus because they support only WinXPSP2 or later. But we will have to continue to keep manual enumerating path unless we drop the Win2k support.

Comment 9

[Rob Arnold [:robarnold]]

I think supporting IAttachmentExecute if available is the right solution. It doesn't seem terribly difficult to implement and it maintains compatability with Win2k and XP pre sp2.

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: implementing IAttachmentExecute

Comment 12

[Justin Wood (:Callek)]

Comment on attachment 299617 [details] [diff] [review]
implementing IAttachmentExecute

>+  // At present all Mozilla products use GUID form
>+  UuidFromString((unsigned char *)id.get(), &mClientGUID);

Well, yes and no... all "Mozilla" products, yes.

But not all products based on Mozilla, (songbird for instance has [e-mail] as their id)

We have to think about more than just Mozilla products, as we can't predict other users as well.

Also is the dependancy on rpcrt4 really needed, I don't see it mentioned in the msdn docs I was looking at?

(I don't know enough about IAE to provide comment on the rest of your patch though)

Comment 13

[Shawn Wilsher :sdwilsh]

Jim should review this first since I'm not a windows guy.

Comment 14

[Masatoshi Kimura [:emk]]

(In reply to comment #12)
> We have to think about more than just Mozilla products, as we can't predict
> other users as well.
Hm, how can I get a product-by-product GUID?
Do I have to add a new field to application.ini?
> Also is the dependancy on rpcrt4 really needed, I don't see it mentioned in the
> msdn docs I was looking at?
UuidFromString() requires rpcrt4.
But it will not be required anymore because I found nsID::Parse().

Comment 15

[Jim Mathies [:jimm]]

The more I read about this interface the more I like the idea of supporting it. (Though I don't think it'll tap us into the 2008 products that dropped support for IOfficeAntiVirus - those I believe are handling scanning on their own. We'll know more about that hopfully in a bit re bug 412204.)  It does though give us better integration with the os's underlying security mechanisms for file downloads. For example our downloads would start supporting the security prompts you see on downloads from browsers like IE.

I'm going to run this a bit and see what the various side effects are. Will post back once I've had a chance to play with it a bit.

One note - in light of some of the feedback we've received (which is still under discussion) regarding virus scanning some users may not appreciate this kind of support.    

  

Comment 16

[Rob Arnold [:robarnold]]

Comment on attachment 299617 [details] [diff] [review]
implementing IAttachmentExecute

>+PRBool
>+nsDownloadScanner::IsAESAvailable()
>+{
>+  HRESULT hr;
>+  nsRefPtr<IAttachmentExecute> ae;
>+  hr = CoCreateInstance(CLSID_AttachmentServices, NULL, CLSCTX_INPROC,
>+                        IID_IAttachmentExecute, (void**)&ae);

This will leak the COM object. Following the example in DoScanOAV, use getter_addRefs(ae) instead of (void**)&ae.

> void
>-nsDownloadScanner::Scan::DoScan()
>+nsDownloadScanner::Scan::DoScanAES()
>+{
>+  HRESULT hr;
>+  nsRefPtr<IAttachmentExecute> ae;
>+  hr = CoCreateInstance(CLSID_AttachmentServices, NULL, CLSCTX_INPROC,
>+                        IID_IAttachmentExecute, (void**)&ae);

Same comment as above.

>     nsString mPath;
>     nsString mName;
>+    GUID mClientGUID;
>     nsString mOrigin;
>     // Also true if it is an ftp download
>     PRBool mIsHttpDownload;
>     PRBool mIsReadOnlyRequest;

I see mClientGUID defined in the Scan class, but you initialize it as if it were part of the nsDownloadScanner class in ListCLSID:

> +  // Get the app id for IAES Client ID
> +  nsCAutoString id;
> +  rv = appinfo->GetID(id);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  // Remove curly braces
> +  id.Cut(0, 1);
> +  id.Truncate(id.Length() - 1);
> +  // At present all Mozilla products use GUID form
> +  UuidFromString((unsigned char *)id.get(), &mClientGUID);

This initialization should go in the Scan constructor where all the other members of the Scan class are initialized.

Comment 17

[Shawn Wilsher :sdwilsh]

Comment on attachment 299617 [details] [diff] [review]
implementing IAttachmentExecute

r=sdwilsh for a very general review.  I don't know windows bits though, so this needs r from jimm or robarnold before it can land.

Comment 18

[Shawn Wilsher :sdwilsh]

Also - how soon do you think you could get a new patch?  We have a code freeze for beta 3 tomorrow, and I'd *really* like to get this in if it turns out to be nice.

Comment 19

[Jim Mathies [:jimm]]

Masatoshi - we're thinking we want to get this in before the freeze tonight, so we'll go ahead and start on the needed changes. If you're online, please post back and let us know if your actively working on this. 

Comment 20

[Jim Mathies [:jimm]]

>>+  // At present all Mozilla products use GUID form
>>+  UuidFromString((unsigned char *)id.get(), &mClientGUID);
>>
>Well, yes and no... all "Mozilla" products, yes.
>
>But not all products based on Mozilla, (songbird for instance has
>[e-mail] as their id)
>
>We have to think about more than just Mozilla products, as we can't predict
>other users as well.

The GUID settings apply to prompts. 

http://msdn2.microsoft.com/en-us/library/bb776301.aspx

I'd say define a generic guid here, which means applications would all share the same prompt settings. If a particular mozilla app wanted to customize that for themselves, they could override the default.

Comment 21

[Jim Mathies [:jimm]]

Attachment: IAttachExecute patch v.2

Comment 22

[Jim Mathies [:jimm]]

A couple notes on this - 

On the prompt GUID, I just hardcoded it here and commented it explaining it's purpose. We could pull that from someplace else although I'd rather spin that out to another bug than hold this up.

Save on IAE does not give "dirty" results. It either fails or it succeeds. The user is promped by their anti-virus software, so they know the file was dirty. Although with the way I've set this up our download manager will not display anything specific to this. We could change the final else on the result to AVSCAN_UGLY, however, there is the risk that if the scan fails for some other reason we would incorrectly report it as a virus.

As it stands though our virus warning UI will not show up with this patch. 

Comments welcome on both.

Comment 23

[Shawn Wilsher :sdwilsh]

Comment on attachment 300116 [details] [diff] [review]
IAttachExecute patch v.2

>+// IAttachementExecute supports user definable settings for certain
>+// security related prompts. This defines a general GUID for use in
>+// all projects. Individual projects can define an individual guid
>+// if they want to.
>+static const GUID GUID_MozillaVirusScannerPromptGeneric = 
>+{ 0xb50563d1, 0x16b6, 0x43c2, { 0xa6, 0x6a, 0xfa, 0xe6, 0xd2, 0x11, 0xf2, 0xea } };
How would they define this?  Wouldn't it be best to have something like this?

#ifndef MOZ_VIRUS_SCANNER_PROMPT_GUID
#define MOZ_VIRUS_SCANNER_PROMPT_GUID \
  { 0xb50563d1, 0x16b6, 0x43c2, { 0xa6, 0x6a, 0xfa, 0xe6, 0xd2, 0x11, 0xf2, 0xea } }
#endif
static const GUID GUID_MozillaVirusScannerPromptGeneric =
  MOZ_VIRUS_SCANNER_PROMPT_GUID;

this way they can actually specify their own :)

(In reply to comment #22)
> Save on IAE does not give "dirty" results. It either fails or it succeeds. The
> user is promped by their anti-virus software, so they know the file was dirty.
> Although with the way I've set this up our download manager will not display
> anything specific to this. We could change the final else on the result to
> AVSCAN_UGLY, however, there is the risk that if the scan fails for some other
> reason we would incorrectly report it as a virus.
Talking to UX folks on this RSN.

> As it stands though our virus warning UI will not show up with this patch. 
It returns Failed if it can't clean it too, right?

r=sdwilsh otherwise (rob has comments that he's adding to the bug too)

Comment 24

[Rob Arnold [:robarnold]]

This patch will bitrot my patch for bug 409815, so in the interest of making my life easier (and also for style reasons), could you eliminate the multiple return statements in DoScanAES? The error handling in but 409185 is now more complicated than just setting mStatus to AVSCAN_FAILED.

> As it stands though our virus warning UI will not show up with this patch.
I would hope that we can throw up some sort of UI for this if the scan fails, but I suppose we need some QA data to know how often this would happen.

Comment 25

[Shawn Wilsher :sdwilsh]

(In reply to comment #23)
> Talking to UX folks on this RSN.
let's set it to dirty

Comment 26

[Masatoshi Kimura [:emk]]

Sorry for the late response. I'll be offline again for a while. Feel free to modify the patch to land on Fx3 beta 3.
> *  * Use all available virus scanners instead of just the first one that is
> *    enumerated (or use some heuristic)
I thought this is fixed by bug 393305. Is it still valid?

Comment 27

[Rob Arnold [:robarnold]]

(In reply to comment #26)
> > *  * Use all available virus scanners instead of just the first one that is
> > *    enumerated (or use some heuristic)
> I thought this is fixed by bug 393305. Is it still valid?
> 

It's invalid and should be taken out

Comment 28

[Jim Mathies [:jimm]]

>I thought this is fixed by bug 393305. Is it still valid?

yep, sorry missed that when applying your patch manually.

>This patch will bitrot my patch for bug 409815, so in the interest of making my
>life easier (and also for style reasons), could you eliminate the multiple
>return statements in DoScanAES?

will do.

>How would they define this?  Wouldn't it be best to have something like this?

wfm, i'll add it.

>let's set it to dirty

done!

Comment 29

[Jim Mathies [:jimm]]

Attachment: IAttachExecute patch v.3

Comment 30

[Shawn Wilsher :sdwilsh]

Comment on attachment 300144 [details] [diff] [review]
IAttachExecute patch v.3

>+  if (SUCCEEDED(hr)) {
>+    ae->SetClientGuid(GUID_MozillaVirusScannerPromptGeneric);
>+    ae->SetLocalPath(mPath.BeginWriting());
>+    ae->SetSource(mOrigin.BeginWriting());
nit: do these return something?  if so, and you want to ignore them, please cast to void.

r=sdwilsh with that nit fixed.  Get a new patch up so we can get approval to land this :)

Comment 31

[Jim Mathies [:jimm]]

Attachment: IAttachExecute patch v.4

If any of those fail, Save would fail so I don't see any reason to check those results.

Comment 32

[Shawn Wilsher :sdwilsh]

Comment on attachment 300151 [details] [diff] [review]
IAttachExecute patch v.4

r=sdwilsh

Drivers - this change is a fairly local change that gives us a big platform integration win on XP SP2 and greater.  It only affects windows code.

Comment 33

[Rob Arnold [:robarnold]]

This looks good to me. It shouldn't cause any serious performance regressions (it may even be faster in a few cases). One possible downside, however, is that we may no longer be invoking Windows Defender on downloaded files. I don't know a good way to test this however since Defender does not raise any warning for the eicar.com test file.

Comment 34

[Damon Sicore (:damons)]

Comment on attachment 300151 [details] [diff] [review]
IAttachExecute patch v.4

a1.9+=damons

Comment 35

[Jim Mathies [:jimm]]

>One possible downside, however, is that
>we may no longer be invoking Windows Defender on downloaded files. I don't know
>a good way to test this however since Defender does not raise any warning for
>the eicar.com test file.

I can trigger that by pulling the file from a different location. I think once you hit "ignore" in WD's dialog it remembers you aren't worried about it. In general, WD is getting called, I've been testing with it today.

Comment 36

[Shawn Wilsher :sdwilsh]

Thanks for the work Masatoshi and Jim on getting this in for the beta!

Checking in toolkit/components/downloads/src/nsDownloadScanner.cpp;
new revision: 1.8; previous revision: 1.7
Checking in toolkit/components/downloads/src/nsDownloadScanner.h;
new revision: 1.3; previous revision: 1.2

Comment 37

[Shawn Wilsher :sdwilsh]

Windows tinderboxen no like

Checking in toolkit/components/downloads/src/nsDownloadScanner.cpp;
new revision: 1.9; previous revision: 1.8
Checking in toolkit/components/downloads/src/nsDownloadScanner.h;
new revision: 1.4; previous revision: 1.3

Comment 38

[Jim Mathies [:jimm]]

Attachment: IAttachExecute patch v.5

waiting on try server results to confirm, but I believe this should do it.

Comment 39

[Shawn Wilsher :sdwilsh]

Checking in toolkit/components/downloads/src/AttachmentServices.h;
initial revision: 1.1
Checking in toolkit/components/downloads/src/nsDownloadScanner.cpp;
new revision: 1.10; previous revision: 1.9
Checking in toolkit/components/downloads/src/nsDownloadScanner.h;
new revision: 1.5; previous revision: 1.4

Comment 40

[Shawn Wilsher :sdwilsh]

Boy it sure is annoying that try server worked and tinderbox didn't....

Removing toolkit/components/downloads/src/AttachmentServices.h;
new revision: delete; previous revision: 1.1
Checking in toolkit/components/downloads/src/nsDownloadScanner.cpp;
new revision: 1.11; previous revision: 1.10
Checking in toolkit/components/downloads/src/nsDownloadScanner.h;
new revision: 1.6; previous revision: 1.5

Comment 41

[Jim Mathies [:jimm]]

Just a recap for future reference - 

First burn was related to the IAttachmentExecute interface being defined in the 6.0 SDK but not on what is currently used in WINNT 5.2 fx-win32-tbox Dep Nightly. 

The second patch ifdef'd in the interface in cases where it wasn't defined. This ran through try server fine, but when it landed on fx-win32-tbox, the consts  broke the build due to multiple definitions. <- This I want to try and track down at some point, these shouldn't have been defined. 

The next patch will mangle the names of the two consts so they don't conflict. Unfortunately all this insures this patch isn't going to make the freeze. :/    

Comment 42

[Jim Mathies [:jimm]]

Related - 
https://bugzilla.mozilla.org/show_bug.cgi?id=414778

Comment 43

[Jim Mathies [:jimm]]

Attachment: IAttachExecute patch v.6

The same patch, I've just mangled the the names so there's no chance of a conflict. This just passed try server as well.

Comment 44

[uamjet602]

Any chance of also using the SetSource/SetReferrer methods to tell Explorer where the file came from? That way you (should) get the nice warnings when you open the files through explorer.

Comment 45

[Shawn Wilsher :sdwilsh]

We do have that information available to us (via nsDownload), so if we can, we probably should.

Comment 46

[Jim Mathies [:jimm]]

We already set the source. Referrer isn't needed - from msdn:

IAttachmentExecute::SetReferrer and IAttachmentExecute::SetSource have similar functionality. If both are set, the least-trusted zone of the two is used.

IAttachmentExecute::SetReferrer is used by container files to indicate indirect inheritance and avoid zone elevation. It can also be used with shortcut files to limit elevation based on parameters.

Calling IAttachmentExecute::SetReferrer is optional.

IAttachmentExecute::SetReferrer is only used to determine the security zone and its associated policies.

Comment 47

[Shawn Wilsher :sdwilsh]

well, if the referrer is less trusted than the source, wouldn't we want to set the referrer?

Comment 48

[Shawn Wilsher :sdwilsh]

re-requesting blocking for this since I think we *really* want it.

Comment 49

[Masatoshi Kimura [:emk]]

(In reply to comment #41)
You should not define IID and CLSID by yourself. They are already built into uuid.lib. Just use an external declaration.
And IAttachmentExecute is defined in shobjidl.h which is included from shlobj.h even in 5.2 SDK headers. I'm not sure why fx-win32-tbox burned.

(In reply to comment #47)
> well, if the referrer is less trusted than the source, wouldn't we want to set
> the referrer?
In short:
SetSource is only for archivers such as zip folder. That's none of DM's business.

Comment 50

[Masatoshi Kimura [:emk]]

Oops.
> SetSource is only for archivers
SetReferrer is only for archivers

Comment 51

[Jim Mathies [:jimm]]

>And IAttachmentExecute is defined in shobjidl.h which is included from shlobj.h
>even in 5.2 SDK headers. I'm not sure why fx-win32-tbox burned.

That was my original assumption as well, but we wanted it in. In the end we still didn't make it. I have a to-do item to try and find out why it wasn't defined.

Comment 52

[Jim Mathies [:jimm]]

Ok, the original patch - 

IAttachExecute patch v.4

just went through try server just fine.

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1201731026.1201733699.25664.gz

yet it broke WINNT 5.2 fx-win32-tbox Depend Nightly last night - 

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1201653660.1201654666.25187.gz

Spoke to build, they couldn't find the cause. I think we should just stick with
the final patch.

I'll do a final release / static build of this to make sure it's working there
too.

Comment 53

[Masatoshi Kimura [:emk]]

Attachment: IAttachExecute patch v.7

OK, I realized what's going on.
SDK6 headers #ifdef'ed IAttachmentExecute definitions, but MSVS8 headers did't.
TryServer includes MSVS8 headers first, but real tinderbox includes SDK6 headers first.
I could reproduce the problem by modifying include order.
I bet this will fix the tinderbox burning.

Comment 54

[Jim Mathies [:jimm]]

Hey Masatoshi -

>TryServer includes MSVS8 headers first, but real tinderbox includes SDK6
>headers first.

Can you fill me in on where/how you came to this conclusion? If so, we should fix the try server so that it matches tinderbox. 

Comment 55

[Jim Mathies [:jimm]]

> I could reproduce the problem by modifying include order.

Sorry, wasn't paying attention. I'll take a look.

Comment 56

[Jim Mathies [:jimm]]

You're right they ifdef'd a bunch of junk out.

Just spoke with build, try server had a malformed include string-

d:\sdks\v6.0\binD:\sdks\v6.0\include;D:\sdks\v6.0\include\atl;D:\msvs8\VC\ATLMFC\INCLUDE;D:\msvs8\VC\INCLUDE;D:\msvs8\VC\PlatformSDK\include

it's been fixed.

I'm still waiting on some feedback on including these com headers. Once I get the final word on that we can decide on the right patch.

Comment 57

[Shawn Wilsher :sdwilsh]

Jim - does your review mean that we are OK with the patch as is (without the extra headers)?

Comment 58

[Jim Mathies [:jimm]]

yeah we could go ahead and land it. if we get a ton of complaints we can add that header stuff once we get the ok on it. As things stand now this is the right way to do it.

Comment 59

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #23)
> (From update of attachment 300116 [details] [diff] [review])
> >+// IAttachementExecute supports user definable settings for certain
> >+// security related prompts. This defines a general GUID for use in
> >+// all projects. Individual projects can define an individual guid
> >+// if they want to.
> >+static const GUID GUID_MozillaVirusScannerPromptGeneric = 
> >+{ 0xb50563d1, 0x16b6, 0x43c2, { 0xa6, 0x6a, 0xfa, 0xe6, 0xd2, 0x11, 0xf2, 0xea } };
> How would they define this?  Wouldn't it be best to have something like this?
> 
> #ifndef MOZ_VIRUS_SCANNER_PROMPT_GUID
> #define MOZ_VIRUS_SCANNER_PROMPT_GUID \
>   { 0xb50563d1, 0x16b6, 0x43c2, { 0xa6, 0x6a, 0xfa, 0xe6, 0xd2, 0x11, 0xf2,
> 0xea } }
> #endif
> static const GUID GUID_MozillaVirusScannerPromptGeneric =
>   MOZ_VIRUS_SCANNER_PROMPT_GUID;
> 
> this way they can actually specify their own :)

maybe we could move the stringified guid to a pref (someday)

Comment 60

[Reed Loden [:reed]]

Checking in toolkit/components/downloads/src/nsDownloadScanner.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadScanner.cpp,v  <--  nsDownloadScanner.cpp
new revision: 1.12; previous revision: 1.11
done
Checking in toolkit/components/downloads/src/nsDownloadScanner.h;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadScanner.h,v  <--  nsDownloadScanner.h
new revision: 1.7; previous revision: 1.6
done

Comment 61

[aja+bugzilla]

This seems to have caused bug 416501

Comment 62

[Simon Bünzli]

Could this have caused bug 416683?

Comment 63

[Alex Vincent [:WeirdAl]]

Building with VC7.1, I repeatably cannot finish the compile.  IAttachmentExecute is undeclared...

Comment 64

[Jim Mathies [:jimm]]

Alex, are you working with the latest SDK? Our build environment was recently upgraded to the Vista SDK, so code is now landing that is not compatible with previous SDKs.

Also, can you paste in your compile errors?

Comment 65

[Alex Vincent [:WeirdAl]]

Attachment: compile errors, VC7.1, WinXP

Comment 66

[Jim Mathies [:jimm]]

an upgrade to the latest sdk should fix that. make sure the sdk include, bin, and lib folders are first in your exports.

Comment 67

[:Gavin Sharp [email: [e-mail]]]

I saw the same errors, and upgrading to the "Windows Server 2003 R2 Platform SDK" available at  http://www.microsoft.com/downloads/details.aspx?FamilyId=0BAF2B35-C656-4969-ACE8-E4C0C0716ADB&displaylang=en fixed them for me.

Comment 68

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED (functionality-wise), per my testing in https://bugzilla.mozilla.org/show_bug.cgi?id=416501#c11 with:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008022104 Minefield/3.0b4pre

-and-

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022104 Minefield/3.0b4pre

Comment 69

[Jim Mathies [:jimm]]

Just noticed something while working on bug 420230 - 

  nsCOMPtr<nsIURI> innerURI = NS_GetInnermostURI(uri);
  (void)innerURI->SchemeIs("http", &isHttp);
  (void)innerURI->SchemeIs("ftp", &isFtp);
  (void)innerURI->SchemeIs("https", &isHttps);

NS_GetInnermostURI can fail and return nsnull but I'm not sure if that would ever happen. Why do we use NS_GetInnermostURI here?

Comment 70

[Shawn Wilsher :sdwilsh]

(In reply to comment #69)
> NS_GetInnermostURI can fail and return nsnull but I'm not sure if that would
> ever happen. Why do we use NS_GetInnermostURI here?
We should have a null check there.  Edward added this, and there was a good reason, but I cannot recall why - check blame for the bug (we should probably add a comment there as to why as well).