GNOME Bugzilla – Bug 638597
Handling of MakerNote can lead to an out of control process
Last modified: 2018-08-17 19:48:29 UTC
Using F-spot 0.8.0 I have a jpeg photo that, on import, sends the f-spot.exe process into massive memory usage (>700m vs usual 200m). Investigations show the problem is in taglib, importing the embedded exif data. More specifically, it is trying to parse the MakerNote. An exiv2 dump of this photo gives: Exif.Photo.MakerNote Undefined 72 1 4 3 1 1 1 255 255 0 3 37 54 30 47 147 44 1 0 0 255 0 0 255 32 0 0 40 0 0 0 12 53 68 67 80 48 51 56 51 51 46 74 80 71 4 56 0 0 0 1 222 224 0 1 0 0 0 1 181 96 0 1 198 132 0 1 0 0 0 1 173 120 The header (I guess '1 4 3 1 1 1') is not a recognised header, so in IFDReader.cs ParseMakernote it gets to the 'catch-all' attempt at the end, calling: new IFDReader (file, is_bigendian, ifd_structure, base_offset, offset, max_offset); From what I can summise, this initiates a reader loop trying to fish out some 260 entries (from the two bytes '1 4'), but I strongly suspect it is all garbage to the IFDReader. I guess that its mis-interpretation causes it to allocate a large chunk of memory (perhaps thinking it is going to read a massively long string or such like) and so on. Reading on Wikipedia that the MakerNote can be in a completely proprietary format, with no standard header, makes me wonder if it is ever safe to attempt to parse a MakerNote with an unrecongised header in this way. Or, at least, I guess we need to devise much more sanity checks to weed out hopeless cases. The photo was created by an old Kodak DC215 by the way.
Created attachment 177612 [details] [review] Don't parse implausible MakerNote data The attached patch fixes the problem. It puts into two sanity checks: 1) In the loop in Read() - if the next_offset is out of bounds stop looping 2) In ParseMakerNote() itself - Before dispatching the MakerNote data to a new IFDReader (for an assumed Canon device), check its entries would actually fit in the MakerNote data. These two checks have the desired affect for my pictures, preventing attempts to read a MakerNote structure that isn't intended to be valid IFD data.
Any thoughts on this?
Could you upload one of these files somewhere? I'll need to add them to the test suite to verify.
Created attachment 180319 [details] Sample Kodak DC215 file with bad MakerNote Here's a sample. As a photo from Karl Relton I am happy for you to include in testcase data and ship under whatever license fspot code is distributed under.
I've had a look at your file and it looks as if we fixed this one. I think it should be included in the f-spot 0.8.2 release. Fix is here: commit 8975dafa64d2f31219fe7f1177a3a08933cd82e4 Author: Tim Howard <timothy.howard@gmail.com> Date: Sun Dec 12 12:23:29 2010 -0500 Make sure the entry size is even possible before trying to create it https://bugzilla.gnome.org/show_bug.cgi?id=633972
No - that commit does not fix it. I had already tried with that latest TagLib code. I can't remember off the top of my head why that commit doesn't catch this one, but it does not. Attachment with comment #1 above is the way to go, as far as I can see.
Can you reproduce this with Taglib# master, using the ParsePhoto tool in the examples folder?
I'll give it a go - hopefully in the next few days.
Okay, I've given ParsePhoto a go and refreshed my memory. It is true that the fix for https://bugzilla.gnome.org/show_bug.cgi?id=633972 stops the parser from allocating lots of memory, because lots of impossible 'apparent' entries are now ignored. However, I would still advocate adding my suggested patch (or at least the 2nd part - putting in a test in ParseMakernote) because it makes the parser even more robust. Without it, the parser is still trying to parse some 260 entries in what it assumes is valid IFD data. Most of these get skipped as garbage (because the offsets fail the checks added in December), but some are processed as if proper data. I contend that we can know this is not valid IFD data at all, since the MakerNote data is simply not big enough to contain the number of entries it would have as an IFD structure. Since it is not IFD data, then why try and parse it as such? Why not simply skip it altogether, and avoid potential nasties or accumulating garbage data structures in the program? The patch does that test, and therefore only hands it on to ReadIFD if the MakerNote data passes that simple test.
I should also point out that applying my suggested patch flips the Writable? flag from false to true, and the Corrupt? flag from true to false, as seen by using ParsePhoto. In other words a photo formerly parsed as corrupt becomes good when using the patch. This makes sense, because in the test photo it is only the MakerNote that is leading to issues. And since MakerNote can be a blob or arbitrary data, it makes sense for us to ignore it unless we have good reason to think we can parse it. If we can't parse it, then let us not do so and risk seeing the whole image as corrupt.
Yeah, I noticed that and the lack of calling MarkAsCorrupt was one of my issues with your patch. The thing is: we don't want to write things we can't fully parse. Some MakerNotes have offsets in them which are relative to the file (as opposed to relative to the IFD). This means that if you write the block out unmodified, but move it in the file, it will still become corrupt. For this reason we refuse to write files which we don't fully understand. This makes sure that there's absolutely no chance of losing data.
Oh and apologies for the late reply, things are pretty hectic over here currently.
Ok - I understand. Would you like me to rework the patch to mark as corrupt if the MakerNote can't be parsed. Also, shall I keep the other sanity check that I put in the loop Read(). Does that need a mark as corrupt as well?
Created attachment 183530 [details] [review] New version - marks file as corrupt on unrecognisable Makernote A new version of the patch. On an implausible Makernote, it skips the Makernote marks the file as corrupt. Note the Makernote itself might be fine, but we mark as corrupt so that the metadata becomes non-writable so that we do not risk invalidating the Makernote by moving it. I also removed the other sanity check I had added in the first patch.
The patch that was used to fix https://bugzilla.gnome.org/show_bug.cgi?id=633972 (as per comment #5) has been reverted, which means the problem I am reporting here will come back to bite us. Applying the patch attached here (comment 14) would at least fix the problem reported here.
taglib-sharp has moved to Github a while ago. Furthermore, GNOME Bugzilla will be shut down and replaced by gitlab.gnome.org. If the problem reported in this Bugzilla ticket is still valid, please report it to https://github.com/mono/taglib-sharp/issues instead. Thank you! Closing this report as WONTFIX as part of Bugzilla Housekeeping.