** Description changed:
- Two more bugs from the same porting effort:
+ 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;
- }
+ 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;
- }
+ if(nextblock>=numblocks)
+ {
+ if(![self readNextFileHeader]) return 0;
+ }
- uint32_t offset=blocks[nextblock].offset; // may still be out of range
+ 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/XAD7ZipParser.m
+ +++ b/XAD7ZipParser.m
+ @@ -551,3 +551,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];
+ @@ -564,4 +566,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;
+ }
+ @@ -570,3 +575,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];
+ + }
+
+ @@ -605,2 +614,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++)
+ @@ -608,4 +619,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");
+ @@ -615,2 +628,5 @@ -(void)parseFolderForHandle:(CSHandle *)handle
dictionary:(NSMutableDictionary *
+ int numpackedstreams=totalinstreams-numbindpairs;
+ +
if(numpackedstreams<0||*packedstreamindex+numpackedstreams>(int)[packedstreams
count])
+ + [XADException raiseIllegalDataException];
+ +
+ if(numpackedstreams==1)
+ @@ -628,3 +644,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");
+ + }
+ }
+ @@ -655,3 +675,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
+ ```
** 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/XAD7ZipParser.m
- +++ b/XAD7ZipParser.m
- @@ -551,3 +551,5 @@ -(void)parseFolderForHandle:(CSHandle *)handle
dictionary:(NSMutableDictionary *
+ --- 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];
- @@ -564,4 +566,7 @@ -(void)parseFolderForHandle:(CSHandle *)handle
dictionary:(NSMutableDictionary *
+ @@ -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;
}
- @@ -570,3 +575,7 @@ -(void)parseFolderForHandle:(CSHandle *)handle
dictionary:(NSMutableDictionary *
+ @@ -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];
+ }
- @@ -605,2 +614,4 @@ -(void)parseFolderForHandle:(CSHandle *)handle
dictionary:(NSMutableDictionary *
+ @@ -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++)
- @@ -608,4 +619,6 @@ -(void)parseFolderForHandle:(CSHandle *)handle
dictionary:(NSMutableDictionary *
+ @@ -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");
- @@ -615,2 +628,5 @@ -(void)parseFolderForHandle:(CSHandle *)handle
dictionary:(NSMutableDictionary *
+ @@ -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)
- @@ -628,3 +644,7 @@ -(void)parseFolderForHandle:(CSHandle *)handle
dictionary:(NSMutableDictionary *
+ @@ -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");
+ }
}
- @@ -655,3 +675,5 @@ -(void)parseSubStreamsInfoForHandle:(CSHandle *)handle
folders:(NSArray *)folder
+ @@ -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
+
```
--
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
