[ZBX-10828] Server/proxy crashes when performing Simple checks with invalid key parameters hidden in macro Created: 2016 May 19 Updated: 2017 May 30 Resolved: 2016 Aug 05 |
|
Status: | Closed |
Project: | ZABBIX BUGS AND ISSUES |
Component/s: | Proxy (P), Server (S) |
Affects Version/s: | 2.2.14rc1, 3.0.4rc1, 3.2.0alpha1 |
Fix Version/s: | 2.2.14rc1, 3.0.4rc1, 3.2.0alpha1 |
Type: | Incident report | Priority: | Critical |
Reporter: | Glebs Ivanovskis (Inactive) | Assignee: | Unassigned |
Resolution: | Fixed | Votes: | 0 |
Labels: | crash, macro, simplechecks | ||
Remaining Estimate: | Not Specified | ||
Time Spent: | Not Specified | ||
Original Estimate: | Not Specified |
Issue Links: |
|
Description |
Simple check of the item "net.udp.service[{$BOOM}]" or similar where {$BOOM} has a value ",\" causes server/proxy to crash because return value of parse_item_key() is not checked in get_value_simple(). Discovered it myself "in the wild", but credits should go to Coverity, because it reported it long before me as CID 118898. Just for catching duplicates: 9431:20160519:120038.085 === Backtrace: === 9431:20160519:120038.086 11: src/zabbix_server/zabbix_server: poller #2 [got 0 values in 0.000199 sec, getting values](print_fatal_info+0xae) [0x4663ae] 9431:20160519:120038.086 10: src/zabbix_server/zabbix_server: poller #2 [got 0 values in 0.000199 sec, getting values]() [0x466687] 9431:20160519:120038.086 9: /lib/x86_64-linux-gnu/libc.so.6(+0x36d40) [0x7f948c44fd40] 9431:20160519:120038.086 8: src/zabbix_server/zabbix_server: poller #2 [got 0 values in 0.000199 sec, getting values](get_value_simple+0x6a) [0x41f29a] 9431:20160519:120038.086 7: src/zabbix_server/zabbix_server: poller #2 [got 0 values in 0.000199 sec, getting values]() [0x41d683] 9431:20160519:120038.086 6: src/zabbix_server/zabbix_server: poller #2 [got 0 values in 0.000199 sec, getting values](poller_thread+0xd8) [0x41d798] 9431:20160519:120038.086 5: src/zabbix_server/zabbix_server: poller #2 [got 0 values in 0.000199 sec, getting values](zbx_thread_start+0x45) [0x466fa5] 9431:20160519:120038.086 4: src/zabbix_server/zabbix_server: poller #2 [got 0 values in 0.000199 sec, getting values](MAIN_ZABBIX_ENTRY+0x605) [0x4182b5] 9431:20160519:120038.086 3: src/zabbix_server/zabbix_server: poller #2 [got 0 values in 0.000199 sec, getting values](daemon_start+0x1ad) [0x465cad] 9431:20160519:120038.086 2: src/zabbix_server/zabbix_server: poller #2 [got 0 values in 0.000199 sec, getting values](main+0x38a) [0x41327a] 9431:20160519:120038.086 1: /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf5) [0x7f948c43aec5] 9431:20160519:120038.086 0: src/zabbix_server/zabbix_server: poller #2 [got 0 values in 0.000199 sec, getting values]() [0x4134a5] |
Comments |
Comment by Viktors Tjarve [ 2016 Jun 06 ] |
Fixed in development branch svn://svn.zabbix.com/branches/dev/ZBX-10828 |
Comment by Andris Zeila [ 2016 Jun 07 ] |
(1) It would be better to add check for invalid key parameters earlier – in substitute_key_macros() function. This way the invalid macros in item key parameters would be detected in other places too. However the changes are more complex. The following must be done:
glebs.ivanovskis A hint. Something very similar was done in r60256 but was then reverted. Hope this helps. viktors.tjarve RESOLVED in r60609. wiper CLOSED |
Comment by Andris Zeila [ 2016 Jun 10 ] |
(2) sysinfo.c:quote_key_param() if (0 == forced && '\\' == (*param)[--sz_src]) return FAIL; If we check for not forced quoting here, then we will not check quoted parameters like "{$MACRO}". And we can't decrement sz_src here, as it is used later for coyping data. So it should be: if ('\\' == (*param)[sz_src - 1]) return FAIL; If we define macro: {$A} = A,B$ {$B} = A,B\ then substitute key macros will:
viktors.tjarve RESOLVED in r60633. wiper CLOSED |
Comment by Andris Zeila [ 2016 Jun 10 ] |
(3) expression.c:replace_key_param() if (NULL == strchr(data, '{')) return FAIL; If key parameter does not have macro we must simply return SUCCEED without replacing data (as there is nothing to replace). Otherwise substitute_key_macros() will fail with key[ABC]. viktors.tjarve RESOLVED in r60633. wiper CLOSED |
Comment by Andris Zeila [ 2016 Jun 13 ] |
(4) dbupgrade.c:DBpatch_2010101() - the error messages could be improved.
Also I wondering why the following check was done in the original code:
if (1 != (nparam = num_param(param)))
After all quote_key_param(,0) will quote parameter only if it contains symbols that have to be quoted. But probably it's better to leave it. viktors.tjarve RESOLVED in r60633. wiper added more specific error messages. viktors.tjarve CLOSED |
Comment by Andris Zeila [ 2016 Jun 13 ] |
(5) dbupgrade.c:replace_key_param() should fail only if it failed to quote key parameter. It should return SUCCEED if there was nothing to quote. viktors.tjarve RESOLVED in r60633. wiper It still returned FAIL for the parameters that were not going to be updated: if (1 != level || 4 != num) /* the fourth parameter on first level should be updated */ return FAIL; This practically would cause the patch to fail for all items. viktors.tjarve CLOSED |
Comment by Andris Zeila [ 2016 Jun 13 ] |
(6) It would be more logical to free the allocated parameter if the quoting failed in both replace_key_param() functions (expression.c:replace_key_param() and dbupgrade.c:replace_key_param()) This way str.c:replace_key_param() could check only for NULL != param like it was before. viktors.tjarve RESOLVED in r60633. wiper CLOSED |
Comment by Andris Zeila [ 2016 Jun 15 ] |
Successfully tested. Please review changes in (4) and (5). viktors.tjarve Reviewed and looks good. |
Comment by Viktors Tjarve [ 2016 Jun 16 ] |
Released in:
|
Comment by Andris Zeila [ 2016 Jun 17 ] |
(7) Coverity found one more place in database upgrade patches where quote_key_param() is used (it was added in v3.0): *** CID 149447: Error handling issues (CHECKED_RETURN) /src/libs/zbxdbupgrade/dbupgrade_2050.c: 58 in DBpatch_2050001() 52 while (NULL != (row = DBfetch(result))) 53 { 54 char *param, *oid_esc; 55 size_t oid_offset = 0; 56 57 param = zbx_strdup(NULL, row[1]); >>> CID 149447: Error handling issues (CHECKED_RETURN) >>> Calling "quote_key_param" without checking return value (as is done elsewhere 4 out of 5 times). 58 quote_key_param(¶m, 0); 59 60 zbx_snprintf_alloc(&oid, &oid_alloc, &oid_offset, "discovery[{#SNMPVALUE},%s]", param); 61 62 /* 255 - ITEM_SNMP_OID_LEN */ 63 if (255 < oid_offset && 255 < zbx_strlen_utf8(oid)) viktors.tjarve RESOLVED in r60678. wiper The key quoting failure must be treated similartly to other conversion failures - log a warning and continue with the next item. See r60680 viktors.tjarve CLOSED |
Comment by Viktors Tjarve [ 2016 Jun 17 ] |
Released in:
|
Comment by Glebs Ivanovskis (Inactive) [ 2016 Oct 31 ] |
See (8) in |