Skip to content

Commit

Permalink
Fix multiple buffer overflows in CradLoader::load()
Browse files Browse the repository at this point in the history
This patches several memory issues while loading .rad files
in src/rad.cpp:
* Simplify the code reading the descrition and ensure not
  to write past the end of "desc".
* Add several checks for errors reading the file and fail
  loading in that case.
* Check instument number before using it as index to the
  "inst" array to avoid an out-of bounds write.
* Check order length before writing data to the array. Fixes
  a heap-based buffer overflow (issue #89).
* Check channel and row numbers before using them as indices
  for writing track data. Fixes another buffer overflow
  (issue #89).

This fixes CVE-2019-14733.

Fixes: #89
  • Loading branch information
miller-alex authored and Malvineous committed May 11, 2020
1 parent 30ddcfe commit cb71517
Showing 1 changed file with 30 additions and 12 deletions.
42 changes: 30 additions & 12 deletions src/rad.cpp
Expand Up @@ -35,7 +35,6 @@ bool CradLoader::load(const std::string &filename, const CFileProvider &fp)
binistream *f = fp.open(filename); if(!f) return false;
char id[16];
unsigned char buf,ch,c,b,inp;
char bufstr[2] = "\0";
unsigned int i,j;
unsigned short patofs[32];
const unsigned char convfx[16] = {255,1,2,3,255,5,255,255,255,255,20,255,17,0xd,255,19};
Expand All @@ -47,21 +46,26 @@ bool CradLoader::load(const std::string &filename, const CFileProvider &fp)

// load section
radflags = f->readInt(1);
i = 0;
if(radflags & 128) { // description
memset(desc,0,80*22);
while((buf = f->readInt(1)))
if(buf == 1)
strcat(desc,"\n");
while ((buf = f->readInt(1)) && !f->error()) {
if (i >= 80 * 22 - 1)
continue;
if (buf == 1)
desc[i++] = '\n';
else if (buf >= 2 && buf <= 0x1f)
while (buf-- && i < 80 * 22 - 1)
desc[i++] = ' ';
else
if(buf >= 2 && buf <= 0x1f)
for(i=0;i<buf;i++)
strcat(desc," ");
else {
*bufstr = buf;
strcat(desc,bufstr);
}
desc[i++] = buf;
}
}
desc[i] = 0;

while((buf = f->readInt(1))) { // instruments
if (buf > 250 || f->error()) {
fp.close(f); return false;
}
buf--;
inst[buf].data[2] = f->readInt(1); inst[buf].data[1] = f->readInt(1);
inst[buf].data[10] = f->readInt(1); inst[buf].data[9] = f->readInt(1);
Expand All @@ -70,9 +74,17 @@ bool CradLoader::load(const std::string &filename, const CFileProvider &fp)
inst[buf].data[0] = f->readInt(1);
inst[buf].data[8] = f->readInt(1); inst[buf].data[7] = f->readInt(1);
}

length = f->readInt(1);
if (length > 128) {
fp.close(f); return false;
}
for(i = 0; i < length; i++) order[i] = f->readInt(1); // orderlist
for(i = 0; i < 32; i++) patofs[i] = f->readInt(2); // pattern offset table
if (f->error()) {
fp.close(f); return false;
}

init_trackord(); // patterns
for(i=0;i<32;i++)
if(patofs[i]) {
Expand All @@ -81,6 +93,9 @@ bool CradLoader::load(const std::string &filename, const CFileProvider &fp)
buf = f->readInt(1); b = buf & 127;
do {
ch = f->readInt(1); c = ch & 127;
if (c >= 9 || b >= 64) {
fp.close(f); return false;
}
inp = f->readInt(1);
tracks[i*9+c][b].note = inp & 127;
tracks[i*9+c][b].inst = (inp & 128) >> 3;
Expand All @@ -94,6 +109,9 @@ bool CradLoader::load(const std::string &filename, const CFileProvider &fp)
}
} while(!(ch & 128));
} while(!(buf & 128));
if (f->error()) {
fp.close(f); return false;
}
} else
memset(trackord[i],0,9*2);
fp.close(f);
Expand Down

0 comments on commit cb71517

Please sign in to comment.