[ovs-dev] [PATCH v3 1/3] Windows: Add internal switch port per OVS bridge

[Alin Serdean]

+Ben +Guru

Thanks a lot for the review!

To be short the subject of interest is the sanitization of the input during queries.

You need to have root access or know how the remote management was created and have access to it, thus you could do worse things.

WQL is based on DMTF(https://www.dmtf.org/) which lacks UPDATE/INSERT/DELETE/DROP commands.

Disregarding the above, the only character which can play a role is " ' ".

My question is the following:
is "etc'port" a valid port name?

If it is I will escape that character, if it isn't it should be trimmed further up.

Rest of the comments inlined.

> >+void
> >+get_hres_error(HRESULT hres)
> 
> Sai: This function looks very similar to ovs_format_message(int error).
> Can we reuse/modify the other one?
> FormatMessage needs the FORMAT_MESSAGE_IGNORE_INSERTS for the
> following
> reason:
> 
> 
> // Important! will fail otherwise, since we're not
>    // (and CANNOT) pass insertion parameters
>    |FORMAT_MESSAGE_IGNORE_INSERTS,
[Alin Gabriel Serdean: ] I could try to split. HRESULT can be a different type.
> 
> 
> 
> >+HRESULT
> >+get_uint16_t_value(IWbemClassObject* pcls_obj, wchar_t* field_name,
> >+                   uint16_t* value)
> >+{
> 
> Sai: You should move the following lines to a different function and just
> cast the result.
> --------
[Alin Gabriel Serdean: ] <<<<<
> >+    VARIANT vt_prop;
> >+    VariantInit(&vt_prop);
> >+    HRESULT hres = pcls_obj->lpVtbl->Get(pcls_obj, field_name, 0,
> >&vt_prop,
> >+                                         0, 0);
> >+
> >+    if (FAILED(hres)) {
> 
> Is this missing: VariantClear(&vt_prop); ?
> 
> >+        return hres;
> >+    }
> >+
[Alin Gabriel Serdean: ] >>>>>
> >+    *value = V_UI2(&vt_prop);
[Alin Gabriel Serdean: ] The problem is variant is a struct and uses macros to get the type(i.e. V_UI2). I can make the snippet common and just use the macros per type.
> >+    VariantClear(&vt_prop);
> >+
> >+        if(!check_return_value(psvc->lpVtbl->GetObject(psvc, job_path,
> >0, NULL,
> 
> Sai: Can you split the arguments? Goes over the 78 char limit.
[Alin Gabriel Serdean: ] From the coding style (https://github.com/openvswitch/ovs/blob/master/CodingStyle.md) 79 is the limit and I think the above fits.
> 
> 
> >+    /* Get the port with the element name equal to the name input */
> >+    wchar_t internal_port_query[2048] = L"SELECT * from "
> >+        L"Msvm_EthernetPortAllocationSettingData  WHERE ElementName =
> >\"" ;
> >+
> 
> 
> Sai: We should sanitize the input to avoid sql injections or errors.
[Alin Gabriel Serdean: ] Please see the WQL comment
> 
> >+    /* Check if the element already exists on the switch */
> >+    wchar_t internal_port_query[2048] = L"SELECT * FROM "
> >+    L"Msvm_InternalEthernetPort WHERE ElementName = \"";
> >+
> 
> Sai: Same as above. Try to sanitize the input to get rid of ³.
[Alin Gabriel Serdean: ] Please see the WQL comment
>