8:08 a.m.
On 02/04/2014 07:31 AM, Boris Fiuczynski wrote:
On 01/22/2014 08:30 PM, John Ferlan wrote:
> A new version of Coverity found a number of issues:
>
> get_pool_from_xml(): FORWARD_NULL
> parse_disk_pool(): RESOURCE_LEAK
> - If parse_disk_pool() returned name == NULL, then the strdup() of
> name into pool->id would dereference the NULL pointer leading to
> a segfault.
>
> Furthermore parse_disk_pool() had a few issues. First the 'type_str'
> could be NULL, so that needs to be checked. Second, 'type_str' was
> never free()'d (the get_attr_value returns a strdup()'d value).
>
> Realizing all that resulted in a few extra changes to not strdup()
> a value that we strdup()'d
>
> Eventually get_pool_from_xml() will return to get_disk_pool() which
> returns to diskpool_set_capacity() or _new_volume_template() which
> both error out when return value != 0 (although, I did change the
> latter to be more explicit and match the former).
>
> Finally, even though the parsing of the element will only ever have
> one "name" value - Coverity doesn't know that, so as a benign fix
be
> sure to not overwrite 'name' if "name" isn't already set.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> libxkutil/pool_parsing.c | 39 ++++++++++++++++++-----------------
> src/Virt_SettingsDefineCapabilities.c | 2 +-
> 2 files changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/libxkutil/pool_parsing.c b/libxkutil/pool_parsing.c
> index 922ff32..80bd4ca 100644
> --- a/libxkutil/pool_parsing.c
> +++ b/libxkutil/pool_parsing.c
> @@ -194,15 +194,17 @@ char *get_disk_pool_type(uint16_t type)
>
> }
>
> -static const char *parse_disk_pool(xmlNodeSet *nsv, struct disk_pool *pool)
> +static char *parse_disk_pool(xmlNodeSet *nsv, struct disk_pool *pool)
> {
> xmlNode **nodes = nsv->nodeTab;
> xmlNode *child;
> - const char *type_str = NULL;
> - const char *name = NULL;
> + char *type_str = NULL;
> + char *name = NULL;
> int type = 0;
>
> type_str = get_attr_value(nodes[0], "type");
> + if (type_str == NULL)
> + return NULL;
>
> if (STREQC(type_str, "dir"))
> type = DISK_POOL_DIR;
> @@ -220,12 +222,15 @@ static const char *parse_disk_pool(xmlNodeSet *nsv, struct
disk_pool *pool)
> type = DISK_POOL_SCSI;
> else
> type = DISK_POOL_UNKNOWN;
> + free(type_str);
>
> pool->pool_type = type;
> -
> +
> for (child = nodes[0]->children; child != NULL; child = child->next)
{
> - if (XSTREQ(child->name, "name")) {
> + if (XSTREQ(child->name, "name") && name ==
NULL) {
> name = get_node_content(child);
> + if (name == NULL)
> + return NULL;
> } else if (XSTREQ(child->name, "target"))
> parse_disk_target(child, pool);
> else if (XSTREQ(child->name, "source"))
> @@ -238,14 +243,18 @@ static const char *parse_disk_pool(xmlNodeSet *nsv, struct
disk_pool *pool)
> int get_pool_from_xml(const char *xml, struct virt_pool *pool, int type)
> {
> int len;
> - int ret = 0;
> + int ret = 1;
Isn't this a bug fix... beside a cleanup? Before the return value was
always 0! It also changes the method get_disk_pool in Virt_DevicePool.c
in its return value behavior in the same way.
True - a bug fix insomuch as either caller is concerned, but yet still
found by Coverity as a RESOURCE_LEAK initially and then me as I was
reading the code... I think the primary way to hit the bug would have
been through a *alloc()/strdup() failure though.
The usage of get_disk_pool in the method diskpool_set_capacity checks
for the return value and being !=0 it is considered an error
(Virt_DevicePool.c line 324). Doesn't that need to get changed in this
patch?
I don't think so. Prior to my change/cleanup if anything failed in
get_pool_from_xml(), then diskpool_set_capacity() could wrongly or even
partially set the properties for "Path", "Host",
"SourceDirectory", and
"OtherResourceType". Although perhaps only the type would be wrong,
since all others actually check for non NULL fields before strdup() and
CMSetProperty() calls. Thus perhaps "path" could be set, but host and
source directory not due to memory constraints, or some combination of
the 3. Probably not a good thing to be happening. Of course there are
those that contend that if you're that memory constrained, the code
isn't going much further anyway...
In any case, all my change does is acknowledge and return the failure
which the caller already handled properly.
FWIW: For context
Line 324 in diskpool_set_capacity():
if (get_disk_pool(pool, &pool_vals) != 0) {
CU_DEBUG("Error getting pool path for: %s", _pool->tag);
} else {
if (pool_vals->pool_info.disk.path != NULL) {
pool_str = strdup(pool_vals->pool_info.disk.path);
CMSetProperty(inst, "Path",
(CMPIValue *)pool_str, CMPI_chars);
}
...
Even so you mention above in your description the two chained
exploitation code pieces it seems that you fixed just one ... or did I
miss something?
The reason for the change in _new_volume_template() was to match the
return comparison from diskpool_set_capacity()... It's unnecessary
technically. One used to fail on "ret == 1" and the other when "ret !=
0". Now both fail in the ret != 0 case. It's the "Type A personality"
in me that does that.
John
> xmlDoc *xmldoc;
> xmlXPathContext *xpathctx;
> xmlXPathObject *xpathobj;
> const xmlChar *xpathstr = (xmlChar *)"/pool";
> - const char *name;
>
> CU_DEBUG("Pool XML : %s", xml);
> +
> + /* FIXME: Add support for parsing network pools */
> + if (type == CIM_RES_TYPE_NET)
> + return 0;
> +
> len = strlen(xml) + 1;
>
> if ((xmldoc = xmlParseMemory(xml, len)) == NULL)
> @@ -257,22 +266,14 @@ int get_pool_from_xml(const char *xml, struct virt_pool *pool,
int type)
> if ((xpathobj = xmlXPathEvalExpression(xpathstr, xpathctx)) == NULL)
> goto err3;
>
> - /* FIXME: Add support for parsing network pools */
> - if (type == CIM_RES_TYPE_NET) {
> - ret = 0;
> - goto err1;
> - }
> -
> memset(pool, 0, sizeof(*pool));
>
> - pool->type = CIM_RES_TYPE_DISK;
> - name = parse_disk_pool(xpathobj->nodesetval,
> - &(pool)->pool_info.disk);
> - if (name == NULL)
> + pool->type = CIM_RES_TYPE_DISK;
> + pool->id = parse_disk_pool(xpathobj->nodesetval,
> + &(pool)->pool_info.disk);
> + if (pool->id != NULL)
> ret = 0;
>
> - pool->id = strdup(name);
> -
> xmlXPathFreeObject(xpathobj);
> err3:
> xmlXPathFreeContext(xpathctx);
> diff --git a/src/Virt_SettingsDefineCapabilities.c
b/src/Virt_SettingsDefineCapabilities.c
> index fe16e3f..756e46b 100644
> --- a/src/Virt_SettingsDefineCapabilities.c
> +++ b/src/Virt_SettingsDefineCapabilities.c
> @@ -1376,7 +1376,7 @@ static CMPIStatus _new_volume_template(const CMPIObjectPath
*ref,
> }
>
> ret = get_disk_pool(poolptr, &pool);
> - if (ret == 1) {
> + if (ret != 0) {
> virt_set_status(_BROKER, &s,
> CMPI_RC_ERR_FAILED,
> virStoragePoolGetConnect(poolptr),
>
src/Virt_DevicePool.c line 324 =! change seems to be missing.