On Mon, Sep 19, 2016 at 06:02:45AM -0700, scan-admin@coverity.com wrote:
Hi,
Please find the latest report on new defect(s) introduced to Osmocom found with Coverity Scan.
5 new defect(s) introduced to Osmocom found with Coverity Scan. 6 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan Showing 5 of 5 defect(s)
** CID 148210: Error handling issues (NEGATIVE_RETURNS) /source-iuh/openbsc/openbsc/src/gprs/gprs_llc.c: 273 in rx_llc_xid()
*** CID 148210: Error handling issues (NEGATIVE_RETURNS) /source-iuh/openbsc/openbsc/src/gprs/gprs_llc.c: 273 in rx_llc_xid() 267 268 response_len = 269 gprs_llc_process_xid_ind(gph->data, gph->data_len, 270 response, sizeof(response), 271 lle); 272 xid = msgb_put(resp, response_len);
CID 148210: Error handling issues (NEGATIVE_RETURNS) "response_len" is passed to a parameter that cannot be negative.
@dexter ^ check that response_len > 0
273 memcpy(xid, response, response_len); 274 275 gprs_llc_tx_xid(lle, resp, 0); 276 } else { 277 LOGP(DLLC, LOGL_NOTICE, 278 "Received XID confirmation from phone.\n");
** CID 148209: Control flow issues (MISSING_BREAK) /source-iuh/openbsc/openbsc/src/libmsc/db.c: 414 in check_db_revision()
*** CID 148209: Control flow issues (MISSING_BREAK) /source-iuh/openbsc/openbsc/src/libmsc/db.c: 414 in check_db_revision() 408 409 /* Incremental migration waterfall */ 410 switch (db_rev) { 411 case 2: 412 if (update_db_revision_2()) 413 goto error;
CID 148209: Control flow issues (MISSING_BREAK) The above case falls through to this one.
add: /* fall through */ ?
414 case 3: 415 if (update_db_revision_3()) 416 goto error; 417 418 /* The end of waterfall */ 419 break;
** CID 148208: Control flow issues (DEADCODE) /source-iuh/openbsc/openbsc/src/osmo-bsc/osmo_bsc_ctrl.c: 408 in set_net_timezone()
*** CID 148208: Control flow issues (DEADCODE)
CID 148208: Control flow issues (DEADCODE) Execution cannot reach the expression "0L" inside this statement: "tz->hr = (hourstr ? atol(ho...".
The website has more detail:
"
static int set_net_timezone(struct ctrl_cmd *cmd, void *data) 386{ 387 char *saveptr, *hourstr, *minstr, *dststr, *tmp = 0; 388 int override; 389 struct gsm_network *net = (struct gsm_network*)cmd->node; 390 391 tmp = talloc_strdup(cmd, cmd->value); 392 if (!tmp) 393 goto oom; 394 395 hourstr = strtok_r(tmp, ",", &saveptr); 396 minstr = strtok_r(NULL, ",", &saveptr); 397 dststr = strtok_r(NULL, ",", &saveptr); 398 399 override = 0; 400
cond_notnull: Condition hourstr != NULL, taking true branch. Now the value of hourstr is not NULL.
401 if (hourstr != NULL) 402 override = strcasecmp(hourstr, "off") != 0; 403 404 struct gsm_tz *tz = &net->tz; 405 tz->override = override; 406 407 if (override) {
notnull: At condition hourstr, the value of hourstr cannot be NULL. dead_error_condition: The condition hourstr must be true. CID 148208 (#1 of 1): Logically dead code (DEADCODE)dead_error_line: Execution cannot reach the expression 0L inside this statement: tz->hr = (hourstr ? atol(ho....
408 tz->hr = hourstr ? atol(hourstr) : 0; 409 tz->mn = minstr ? atol(minstr) : 0; 410 tz->dst = dststr ? atol(dststr) : 0; 411 } 412 413 talloc_free(tmp); 414 tmp = NULL; 415 416 return get_net_timezone(cmd, data); 417 418oom: 419 cmd->reply = "OOM"; 420 return CTRL_CMD_ERROR; 421}
"
So override == 0 by default. override will only be set if hourstr != NULL. So inside 'if (override)', hourstr will always be != NULL, and the first '?' is superfluous. This exists since the function was added in 2013.
*** CID 148207: Control flow issues (MISSING_BREAK) /source-iuh/openbsc/openbsc/src/libbsc/abis_rsl.c: 415 in channel_mode_from_lchan() 409 default: 410 LOGP(DRSL, LOGL_ERROR, 411 "unsupported lchan->csd_mode %u\n", 412 lchan->csd_mode); 413 return -EINVAL; 414 }
CID 148207: Control flow issues (MISSING_BREAK) The above case falls through to this one.
This missing 'break' is an error introduced in e4227988665cae6e44fc0f77c2463aae759f15ca "RSL: Add basic support for CSD transparent mode"
415 default: 416 LOGP(DRSL, LOGL_ERROR, 417 "unsupported lchan->tch_mode %u\n", 418 lchan->tch_mode); 419 return -EINVAL; 420 }
** CID 148206: Security best practices violations (DC.WEAK_CRYPTO) /source-iuh/openbsc/openbsc/src/gprs/gprs_gmm.c: 560 in gsm48_tx_gmm_auth_ciph_req()
*** CID 148206: Security best practices violations (DC.WEAK_CRYPTO) /source-iuh/openbsc/openbsc/src/gprs/gprs_gmm.c: 560 in gsm48_tx_gmm_auth_ciph_req() 554 /* 10.5.5.7: */ 555 acreq->force_stby = force_standby; 556 /* 3GPP TS 24.008 10.5.5.19: */ 557 if (RAND_bytes(&rbyte, 1) != 1) { 558 LOGP(DMM, LOGL_NOTICE, "RAND_bytes failed for A&C ref, falling " 559 "back to rand()\n");
CID 148206: Security best practices violations (DC.WEAK_CRYPTO) "rand" should not be used for security related applications, as linear congruential algorithms are too easy to break.
well spotted, coverity :) Do they actually analyse whether that it is security related code? rand() is only the fallback for RAND_bytes().
~Neels
560 acreq->ac_ref_nr = rand(); 561 } else 562 acreq->ac_ref_nr = rbyte; 563 mm->ac_ref_nr_used = acreq->ac_ref_nr; 564 565 /* Only if authentication is requested we need to set RAND + CKSN */
To view the defects in Coverity Scan visit, https://u2389337.ct.sendgrid.net/wf/click?upn=08onrYu34A-2BWcWUl-2F-2BfV0V05...
To manage Coverity Scan email notifications for "nhofmeyr@sysmocom.de", click https://u2389337.ct.sendgrid.net/wf/click?upn=08onrYu34A-2BWcWUl-2F-2BfV0V05...
-
Neels Hofmeyr