| Message ID | 1397731433-15223-1-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/17/2014 04:43 AM, Fam Zheng wrote: > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > v2: PRIx32 -> SCNx32. (Kevin) > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > +++ b/block/vmdk.c > @@ -262,7 +262,7 @@ static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent) > p_name = strstr(desc, cid_str); > if (p_name != NULL) { > p_name += cid_str_size; > - sscanf(p_name, "%x", &cid); > + sscanf(p_name, "%" SCNx32, &cid); sscanf() has undefined behavior on integer overflow. This is not the only vulnerable site in the code base, but if you are ever reading from external input, and the ascii string being parsed does not fit in the variable requested by SCNx32, you risk silently parsing the wrong number. It is always safer to use the strtol family (or a sane wrapper thereof that gets errno handling correct) for parsing strings into integers. That said, I'm not going to reject this patch for using sscanf, so much as suggest that you look into a followup patch to avoid it.
On Thu, 04/17 06:00, Eric Blake wrote: > On 04/17/2014 04:43 AM, Fam Zheng wrote: > > Signed-off-by: Fam Zheng <famz@redhat.com> > > > > --- > > v2: PRIx32 -> SCNx32. (Kevin) > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > > > +++ b/block/vmdk.c > > @@ -262,7 +262,7 @@ static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent) > > p_name = strstr(desc, cid_str); > > if (p_name != NULL) { > > p_name += cid_str_size; > > - sscanf(p_name, "%x", &cid); > > + sscanf(p_name, "%" SCNx32, &cid); > > sscanf() has undefined behavior on integer overflow. This is not the > only vulnerable site in the code base, but if you are ever reading from > external input, and the ascii string being parsed does not fit in the > variable requested by SCNx32, you risk silently parsing the wrong > number. It is always safer to use the strtol family (or a sane wrapper > thereof that gets errno handling correct) for parsing strings into > integers. That said, I'm not going to reject this patch for using > sscanf, so much as suggest that you look into a followup patch to avoid it. > Good point, thanks for the explanation. The particular case of sscanf doesn't matter too much because it's only a time stamp, and the only possible impact of overflow is denial of using the image, where it is coincidentally appropriate. I'm putting sscanf replacing on my list and leaving it for future. Thanks, Fam
Am 17.04.2014 um 12:43 hat Fam Zheng geschrieben: > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > v2: PRIx32 -> SCNx32. (Kevin) > > Signed-off-by: Fam Zheng <famz@redhat.com> Thanks, applied to the block branch. Kevin
diff --git a/block/vmdk.c b/block/vmdk.c index 938a183..06a1f9f 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -262,7 +262,7 @@ static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent) p_name = strstr(desc, cid_str); if (p_name != NULL) { p_name += cid_str_size; - sscanf(p_name, "%x", &cid); + sscanf(p_name, "%" SCNx32, &cid); } return cid; @@ -290,7 +290,7 @@ static int vmdk_write_cid(BlockDriverState *bs, uint32_t cid) p_name = strstr(desc, "CID"); if (p_name != NULL) { p_name += sizeof("CID"); - snprintf(p_name, sizeof(desc) - (p_name - desc), "%x\n", cid); + snprintf(p_name, sizeof(desc) - (p_name - desc), "%" PRIx32 "\n", cid); pstrcat(desc, sizeof(desc), tmp_desc); } @@ -1708,8 +1708,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options, const char desc_template[] = "# Disk DescriptorFile\n" "version=1\n" - "CID=%x\n" - "parentCID=%x\n" + "CID=%" PRIx32 "\n" + "parentCID=%" PRIx32 "\n" "createType=\"%s\"\n" "%s" "\n" @@ -1851,7 +1851,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options, } /* generate descriptor file */ desc = g_strdup_printf(desc_template, - (unsigned int)time(NULL), + (uint32_t)time(NULL), parent_cid, fmt, parent_desc_line,