307429 - Incorrect size tag in update.xml does not cause automatic software update to fail integrity validation

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050902
Firefox/1.0+

A malformed update.xml file will not trigger a failure of integrity validation
with the automatic (or manual) software update. Currently the only by-design way
to cause a validation failure (as far as I know) is for the hash codes to differ
on the downloaded patch. We can either:

1. Remove the "size" value from the <patch> tags, or
2. Require that the "size" value matches the actual size of the downloaded file
and trigger a validation failure if it doesn't.

I think #2 is the smarter of the two approaches. Anyway, steps to reproduce:

1. Point the app.update.url.override pref to an update.xml file with a bad
"size" value in the <patch> tag.
2. Trigger an update and watch as Software Update downloads the update and
installs it.

Expected: Software Update should not install a file with a mismatched file size.

I'll attach some sample update.xml files with good and bad "size" values.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Correct "size" values

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Bad "size" value for the partial patch

Comment 3

[Darin Fisher]

-> me

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: size value integrity check

A simple patch that checks the file size value provided by aus against the
actual file size downloaded. It triggers a verification failure if the sizes
don't match. Does this check beore the hash verification to save some resources
if possible.

One problem I ran into with this patch: for some reason I couldn't simply check
'this._request.destination.fileSize' because it kept returning a nonsense
number no matter what size file I downloaded. The only way I could find around
it was to initialize another file object from 'this._request.destination'.
Anyone know why?

Comment 5

[Darin Fisher]

Comment on attachment 195359 [details] [diff] [review]
size value integrity check

>+    //bail out if the file size doesn't match the aus provided size
>+    var file = Components.classes["@mozilla.org/file/local;1"].
>+        createInstance(nsILocalFile);
>+    file.initWithFile(this._request.destination);
>+    if (file.fileSize != this._patch.size)
>+      return false;

Thanks for the patch.  It turns out that there is an easier
way to get at the fileSize property.  Just do the following
instead:

  var file = this._request.destination.QueryInterface(nsILocalFile);
  if (file.fileSize != this._patch.size)
    return false;

Also, please include a space at the start of a comment, so change
"//bail" to "// bail".	Otherwise, this patch seems like exactly
what we want.

Comment 6

[Darin Fisher]

oh, wait... fileSize is actually a property on nsIFile, so the QI should not
even be necessary.

Comment 7

[Josh Aas]

Attachment: size value integrity check v2.0

This patch seems to work on Mac OS X. However, Ben says it doesn't work on
Windows. The value of "this._request.destination.fileSize" is always 2^16-1 on
Windows. Maybe an Windows XPCOM/io bug? Can someone else verify that this is
broken on Windows?

We need to figure this out before we can take a patch, so not requesting review
on this patch.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

To clarify, patch v1 works on my Windows XP SP2 machine. patch v2 does not. 
Anyone interested in seeing how Linux does with this patch also?

Comment 9

[Josh Aas]

So the problem here is that Windows uses a stat cache, and the stat cache for
the lvalue in patch v2.0 is bad. It should be marked dirty, which isn't really
an option here. Mac OS X doesn't have a stat cache (see nsLocalFileOSX.cpp) and
that is why patch v2.0 works on the Mac.

Really, we should remove stat caches entirely, then take patch v2.0. Stat caches
are probably not a necessary performance improvement and they are fundamentally
flawed. Multiple nsIFile objects pointing to the same file are bound to get out
of sync. If we don't kill stat caches for this bug, others are just going to run
into similar problems. I will file a bug on killing them. I already have a patch
for all platforms.

Comment 10

[Josh Aas]

See bug 307815 for removing stat caches.

Comment 11

[Darin Fisher]

Comment on attachment 195371 [details] [diff] [review]
size value integrity check v2.0

r=darin assuming we fix the stat cache bug.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Patch v2 compiles and works as expected with the v2.1 patch of bug 307815.

XP sp2.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: size verification patch v3

Spoke with Josh and I guess we're going to wait on killing the stat cache for
the time being. This patch is a combo of v1 and v2, although the re-init has
been moved to the more appropriate onStopRequest function.

Comment 14

[Darin Fisher]

Comment on attachment 195813 [details] [diff] [review]
size verification patch v3

I appreciate why you are making this change, but I think I would prefer to see
any hack solution like this localized to the place where we need to call the
fileSize getter.

Comment 15

[Josh Aas]

Darin - what you're saying is contrary to what dougt and I talked about. I think
that if your code is going to advertise a file as being completed, and the file
that should be operated on, you should make sure its file object is in a good
state (you are the writer after all, and should clean up). There is only ever
going to be one instance where the file is done downloading. There could be any
number of calls that stat the file after the fact, and having them check every
time would be messy.

Comment 16

[Darin Fisher]

Josh,

So, you are arguing for modifying nsIncrementalDownload::GetDestination then,
right?  Perhaps we could make that function return a copy of the local file. 
That way, we avoid this entire problem.

Comment 17

[Darin Fisher]

Attachment: alternative patch

This is what I think we should do.  I haven't tested it fully yet, and I would
really appreciate some help testing it.

Comment 18

[Darin Fisher]

I tested this patch on Linux, and it seems to work properly there.

Comment 19

[Darin Fisher]

Attachment: v1 patch

This patch contains only the nsUpdateService.js.in changes necessary to fix
this bug.  I moved the other changes into separate bugs.

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Along with the patches from bug 308369 and bug 308370, this new patch works 
well on XP SP2.

Comment 21

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 195938 [details] [diff] [review]
v1 patch

r=[e-mail]

Comment 22

[Darin Fisher]

fixed-on-trunk

Comment 23

[Darin Fisher]

fixed1.8