Handle long interned strings in MSI parsing #25079
Conversation
For #24720. Used https://github.com/ChaelChu/msi-props-reader/blob/master/src/msiPropsReader.ts as inspiration. Not sure why the shift is 17 bits rather than 16 here but confirmed that 17 works and 16 doesn't.
iansltx
force-pushed
the
24720-msi-upload-fix
branch
from
9f20f76 to
ba28c0c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25079 +/- ##
==========================================
+ Coverage 63.82% 63.83% +0.01%
==========================================
Files 1614 1614
Lines 153582 153596 +14
Branches 4040 4040
==========================================
+ Hits 98026 98051 +25
+ Misses 47752 47743 -9
+ Partials 7804 7802 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
pkg/file/msi.go
Outdated
| @@ -242,9 +242,25 @@ func decodeStrings(dataReader, poolReader io.Reader) ([]string, error) { | |||
| } | |||
| return nil, fmt.Errorf("failed to read pool entry: %w", err) | |||
| } | |||
| stringEntrySize := int(stringEntry.Size) | |||
|
|
|||
| // For string pool entries too long for the size to fit in a single 4-byte entry, we get an 8-byte entry instead, | |||
There was a problem hiding this comment.
| // For string pool entries too long for the size to fit in a single 4-byte entry, we get an 8-byte entry instead, | |
| // For string pool entries too long for the size to fit in a single 2-byte entry, we get an 4-byte entry instead, |
There was a problem hiding this comment.
I actually need to reword this, because the entry is 4 bytes but the size is two bytes of that. Will handle in a sec.
There was a problem hiding this comment.
Revised. Let me know if that's sufficiently clear.
|
|
||
| // For string pool entries too long for the size to fit in a single 4-byte entry, we get an 8-byte entry instead, | ||
| // with the first two bytes as zeroes, the next two are the two most-significant bytes of the size, shifted | ||
| // 17 (?!?) bits to the left, the following two are the less-significant bits of the size, and the last two are |
There was a problem hiding this comment.
Started with the openvpn-connect v3 MSI, read through the entire string data table (the format is just a bunch of concat'd strings so it's somewhat human-readable if you know what you're looking at), and noticed that the string that was breaking things was a big license blob. Then I just looked for what appeared to be the end of that license blob (fortunately the end was pretty obvious as the next interned string was for a field name or something similar), figured out the approximate length of the license blob from there, then made the assumption that the actual length was "bit shift of first entry refcount value plus second entry size value" based on the JS lib mentioned in the commit note. The length I was seeing was ~65k off of a 16-bit shift so I tried 17 and everything lined up.
Since refinary has the same issue, my guess is we'll get confirmation one way or the other from them trying more MSIs later.
There was a problem hiding this comment.
Yeah, just curious how you represent a length of e.g. 65536 + 1
There was a problem hiding this comment.
Was wondering that too.
Guessing that case will break this again, at which point we'll have enough data to explain this better. Or the refinery issue will explain what's going on here and I'll get a fix in for whatever they find.
For #24720. Used https://github.com/ChaelChu/msi-props-reader/blob/master/src/msiPropsReader.ts as inspiration. Not sure why the shift is 17 bits rather than 16 here but confirmed that 17 works and 16 doesn't. Tested against both existing GDrive MSIs for regression testing, plus the one mentioned in the ticket. # Checklist for submitter If some of the following don't apply, delete the relevant line. <!-- Note that API documentation changes are now addressed by the product design team. --> - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/Committing-Changes.md#changes-files) for more information. - [x] Manual QA for all new/changed functionality
For #24720. Used https://github.com/ChaelChu/msi-props-reader/blob/master/src/msiPropsReader.ts as inspiration. Not sure why the shift is 17 bits rather than 16 here but confirmed that 17 works and 16 doesn't.
Tested against both existing GDrive MSIs for regression testing, plus the one mentioned in the ticket.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.