** Attachment added: "Patch for 6"
https://bugs.launchpad.net/ubuntu/+source/unar/+bug/2168659/+attachment/6003085/+files/XAD7ZipParser.patch.txt
** Description changed:
Three more bugs from the same porting effort:
4. XADSkipHandle: broken insertion loop (heap overflow on out-of-order skips)
`XADSkipHandle.m:156-160`
```objc
for(int i=numregions-1;i>index;i++)
{
regions[i+1].actual=regions[i].actual;
regions[i+1].skip=regions[i].skip-end+start;
}
```
It is meant to shift regions up to make room for a new skip (the loop
must run *downwards*: `i--`). With `i++` it runs *upwards* instead:
whenever the new skip is not appended after all existing regions (`index
< numregions-1`), the loop keeps writing `regions[i+1]` past the
allocation — a heap buffer overflow — and never performs the intended
shift, corrupting the region table.
**Why it went unnoticed:** all current call sites (ALZip multi-volume,
split-file handling) append skips in increasing offset order, so `index`
always equals the last region and the loop body never executes. The bug
is latent, waiting for the first caller that adds an out-of-order skip.
** Potential fix:** `for(int i=numregions-1;i>index;i--)`.
5. NowCompress: reads uninitialized memory on degenerate headers
`XADNowCompressHandle.m:195-200`
```objc
if(nextblock>=numblocks)
{
if(![self readNextFileHeader]) return 0;
}
uint32_t offset=blocks[nextblock].offset; // may still be out of range
```
`readNextFileHeader` can succeed while leaving `numblocks == 0`
(degenerate header), after which `blocks[nextblock]` reads uninitialized
malloc memory.
**Potential fix:** `if(nextblock>=(int)blocks.size()) return 0;`.
6. XAD7ZipParser: truncation of values to signed 32-bit
64-bit integer values returned by `ReadNumber()` are cast directly to
signed `(int)`. Malformed archives can cause signed integer truncation
or negative lengths/counts.
** Potential fix:** Preserving uint64_t through all parsing and bounds-
- checking before narrowing conversions prevents overflow attacks. Diff is
- like this:
-
- ```
- --- a/XADMaster/XAD7ZipParser.m
- +++ b/XADMaster/XAD7ZipParser.m
- @@ -534,3 +534,5 @@ -(void)parseFolderForHandle:(CSHandle *)handle
dictionary:(NSMutableDictionary *
- {
- - int numcoders=(int)ReadNumber(handle);
- + uint64_t numcoders_val=ReadNumber(handle);
- + if(numcoders_val>1024) [XADException raiseIllegalDataException];
- + int numcoders=(int)numcoders_val;
- NSMutableArray *instreams=[NSMutableArray array];
- @@ -547,4 +549,7 @@ -(void)parseFolderForHandle:(CSHandle *)handle
dictionary:(NSMutableDictionary *
- if(flags&0x10)
- {
- - numinstreams=(int)ReadNumber(handle);
- - numoutstreams=(int)ReadNumber(handle);
- + uint64_t numin=ReadNumber(handle);
- + uint64_t numout=ReadNumber(handle);
- + if(numin>1024||numout>1024) [XADException
raiseIllegalDataException];
- + numinstreams=(int)numin;
- + numoutstreams=(int)numout;
- }
- @@ -553,3 +558,7 @@ -(void)parseFolderForHandle:(CSHandle *)handle
dictionary:(NSMutableDictionary *
- NSData *props=nil;
- - if(flags&0x20) props=[handle
readDataOfLength:(int)ReadNumber(handle)];
- + if(flags&0x20)
- + {
- + uint64_t propslen=ReadNumber(handle);
- + if(propslen>65536) [XADException
raiseIllegalDataException];
- + props=[handle readDataOfLength:(int)propslen];
- + }
-
- @@ -588,2 +597,4 @@ -(void)parseFolderForHandle:(CSHandle *)handle
dictionary:(NSMutableDictionary *
- + if(totaloutstreams==0) [XADException raiseIllegalDataException];
- +
- // Load binding pairs
- int numbindpairs=totaloutstreams-1;
- for(int i=0;i<numbindpairs;i++)
- @@ -591,4 +602,6 @@ -(void)parseFolderForHandle:(CSHandle *)handle
dictionary:(NSMutableDictionary *
- uint64_t inindex=ReadNumber(handle);
- uint64_t outindex=ReadNumber(handle);
- +
if(inindex>=(uint64_t)totalinstreams||outindex>=(uint64_t)totaloutstreams)
- + [XADException raiseIllegalDataException];
-
SetNumberEntryInArray(instreams,(int)inindex,outindex,@"SourceIndex");
-
SetNumberEntryInArray(outstreams,(int)outindex,inindex,@"DestinationIndex");
- @@ -598,2 +611,5 @@ -(void)parseFolderForHandle:(CSHandle *)handle
dictionary:(NSMutableDictionary *
- int numpackedstreams=totalinstreams-numbindpairs;
- +
if(numpackedstreams<0||*packedstreamindex+numpackedstreams>(int)[packedstreams
count])
- + [XADException raiseIllegalDataException];
- +
- if(numpackedstreams==1)
- @@ -611,3 +627,7 @@ -(void)parseFolderForHandle:(CSHandle *)handle
dictionary:(NSMutableDictionary *
- for(int i=0;i<numpackedstreams;i++)
- -
SetObjectEntryInArray(instreams,(int)ReadNumber(handle),[packedstreams
objectAtIndex:*packedstreamindex+i],@"PackedStream");
- + {
- + uint64_t sindex=ReadNumber(handle);
- + if(sindex>=(uint64_t)totalinstreams) [XADException
raiseIllegalDataException];
- +
SetObjectEntryInArray(instreams,(int)sindex,[packedstreams
objectAtIndex:*packedstreamindex+i],@"PackedStream");
- + }
- }
- @@ -638,3 +658,5 @@ -(void)parseSubStreamsInfoForHandle:(CSHandle *)handle
folders:(NSArray *)folder
- for(int i=0;i<numfolders;i++)
- {
- - int
numsubstreams=(int)ReadNumber(handle);
- + uint64_t numsub=ReadNumber(handle);
- + if(numsub>1000000) [XADException
raiseIllegalDataException];
- + int numsubstreams=(int)numsub;
- if(numsubstreams!=1) // Re-use default
substream when there is only one
-
- ```
+ checking before narrowing conversions prevents overflow attacks.
+ Proposed patch is lower.
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/2168659
Title:
Potential memory corruption
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/unar/+bug/2168659/+subscriptions
--
ubuntu-bugs mailing list
[email protected]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
