onvif-util -a crashes with SEGV when receiving wrong information

[steho]

Hello all, I am trying to run onvif-util -a and get a SIGSEGV. After some debugging I noticed that a QNAP NAS in my network answers to the discovery broadcast with the following XML:

<?xml version="1.0" ?>
<soap:Envelope xmlns:soap="http://www.w3.org/2003/05/soap-envelope" xmlns:wsa="http://schemas.xmlsoap.org/ws/2004/08/addressing" xmlns:wsd="http://schemas.xmlsoap.org/ws/2005/04/discovery">
    <soap:Header>
        <wsa:Action>http://schemas.xmlsoap.org/ws/2005/04/discovery/ProbeMatches</wsa:Action>
        <wsa:MessageID>urn:uuid:b992e1c0-8049-41fe-89ec-6b0196345caf</wsa:MessageID>
        <wsa:RelatesTo>urn:uuid:a7430adf-d054-063b-9f2b-3fed22330285</wsa:RelatesTo>
        <wsa:To>http://schemas.xmlsoap.org/ws/2004/08/addressing/role/anonymous</wsa:To>
        <wsd:AppSequence InstanceId="1030752232" MessageNumber="1"/>
    </soap:Header>
    <soap:Body>
        <wsd:ProbeMatches/>
    </soap:Body>
</soap:Envelope>

This causes onvif_data->xaddrs to be zero and then causes the crash in extractHost(onvif_data->xaddrs, host);

[sr99622]

Thank you so much for your efforts to find this bug. Your feedback is very much appreciated as these kinds of problems can be difficult to isolate. After looking through the code, it seems the best approach is to catch this error prior to invoking the extractHost function. There are further downstream processes that will fail as well if this empty string is allowed to propagate. Basically, the xaddrs field is the key used to communicate with the camera, and if it is invalid, there is no point continuing.

There is a new release coming soon that will include this fix, but in the meantime, you can update the showAll function in onvif.cpp as follows, even though I suspect you may have figured this out already.

static void showAll()
{
    std::cout << "Looking for cameras on the network..." << std::endl;
    struct OnvifSession *onvif_session = (struct OnvifSession*)calloc(sizeof(struct OnvifSession), 1);
        struct OnvifData *onvif_data = (struct OnvifData*)malloc(sizeof(struct OnvifData));
    initializeSession(onvif_session);
    int n = broadcast(onvif_session);
    std::cout << "Found " << n << " cameras" << std::endl;
    for (int i = 0; i < n; i++) {
        prepareOnvifData(i, onvif_session, onvif_data);
        char host[128];
        if (std::string(onvif_data->xaddrs).rfind("//") != std::string::npos) {
            extractHost(onvif_data->xaddrs, host);
            getHostname(onvif_data);
            printf("%s %s(%s)\n",host,
                onvif_data->host_name,
                onvif_data->camera_name);
        }
        else {
            std::cout << "found invalid xaddrs in device repsonse" << std::endl;
        }
    }
    closeSession(onvif_session);
    free(onvif_session);
    free(onvif_data);
}

[steho]

Thanks for your quick response.

Indeed I already fixed it locally, but decided to change prepareOnvifData() to return a status. I think the root cause of the problem is that in getCameraName() the return value of getXmlValue() is discarded.

If you like you can have a look at my fork. https://github.com/steho/libonvif

[sr99622]

Yes, I think having prepareOnvifData() return a status is a very good idea, it will relieve higher level programming burden to check for the error. I hesitate to make the check in getCameraName() however, as many cameras do not follow onvif protocol properly and will often fail to discover. The code tries to be as permissive as possible so that it can catch as many different types of camera as it can, and was developed largely by trial and error with many different types of camera. It is very difficult to anticipate what the cameras will do, they fail in ways that defy imagination.

If I remember correctly, the camera can limp along without the scopes field, and many cameras do not include any useful information in the name or hardware fields used in getCameraName(), so it might be eliminating some types of cameras by disqualifying at that level. Maybe it might be better to perform the same check in extractXAddrs() ?

[steho]

Yes, I think having prepareOnvifData() return a status is a very good idea, it will relieve higher level programming burden to check for the error. I hesitate to make the check in getCameraName() however, as many cameras do not follow onvif protocol properly and will often fail to discover.

I admit that my solution is too restrictive at this point. But I insist that doing the check there is right, but not letting its failure discard the peer completely. Your code already provided a useful default value.

Maybe it might be better to perform the same check in extractXAddrs() ?

Yes, defintively. This function provides a side effect as result, unfortunately not only the one that it names but also setting onvif_data->device_service, without either letting the caller know if it fails or providing a meaningful default. So when it fails the following extractOnvifService() has no chance to do anything useful nor does getTimeOffset().

My suggestion would be:

• Change extractXAddrs() to return an error code if not successful
• Move the strcpy(onvif_data->device_service, onvif_data->xaddrs) somewhere else because it does not belong there (see SRP)
• Change extractOnvifService() to build onvif_data->device_service from onvif_data->xaddrs
• Change prepareOnvifData() so that all errors are reported to the caller

I changed my fix, if you like it feel free to use it.

[karma0]

I'm experiencing the same issue with a proprietary NVR solution that's on the network. Are there any updates to this?

[sr99622]

It looks like these changes were added to the source code. If you are compiling from source, I would think this issue would be resolved. If you are installing from apt, it will be the old version without the fix. The apt install is no longer supported, it was just too much work to maintain.