From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id B9087C77B73 for ; Thu, 18 May 2023 21:04:53 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 9DB3C8574D; Thu, 18 May 2023 23:04:51 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linux.microsoft.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=linux.microsoft.com header.i=@linux.microsoft.com header.b="o6A6YLap"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id E8D368574D; Thu, 18 May 2023 23:04:49 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by phobos.denx.de (Postfix) with ESMTP id CB7538467F for ; Thu, 18 May 2023 23:04:46 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linux.microsoft.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=seanedmond@linux.microsoft.com Received: from [10.16.12.88] (unknown [131.107.8.88]) by linux.microsoft.com (Postfix) with ESMTPSA id 85A7020FB1B4; Thu, 18 May 2023 14:04:45 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 85A7020FB1B4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1684443885; bh=w6ImB1E2vPQWwjfyEoUq9wOyeylPWpM7dN81DeNQqtI=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=o6A6YLap9DZKplHuSA3DSHyUUMxBGTN/K7Lk8+LfcJ0AyyIs0kIHTlvAibCBMJd5y k0VurBxeWjfAIucxI58X6rHJWh7MEI2SYALqkaHAv9xBQxvblVy9+e/fZSDx9IQFnQ S8Xf5002bs9wSyV4om4M9vQhDdBr8GTFUE6+y5zI= Message-ID: <8ed48347-1e66-0f4f-3c7c-28e7b56bd3e4@linux.microsoft.com> Date: Thu, 18 May 2023 14:04:37 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot To: Tom Rini , u-boot@lists.denx.de Cc: Sean Edmond References: <20230508202020.GI2398826@bill-the-cat> From: Sean Edmond In-Reply-To: <20230508202020.GI2398826@bill-the-cat> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.8 at phobos.denx.de X-Virus-Status: Clean On 2023-05-08 1:20 p.m., Tom Rini wrote: > Here's the latest defect report: > > ---------- Forwarded message --------- > From: > Date: Mon, May 8, 2023, 2:29 PM > Subject: New Defects reported by Coverity Scan for Das U-Boot > To: > > > Hi, > > Please find the latest report on new defect(s) introduced to Das U-Boot > found with Coverity Scan. > > 5 new defect(s) introduced to Das U-Boot found with Coverity Scan. > 1 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 453851: Memory - corruptions (OVERLAPPING_COPY) > /cmd/net.c: 279 in netboot_update_env() > > > ________________________________________________________________________________________________________ > *** CID 453851: Memory - corruptions (OVERLAPPING_COPY) > /cmd/net.c: 279 in netboot_update_env() > 273 > 274 if (IS_ENABLED(CONFIG_IPV6)) { > 275 if (!ip6_is_unspecified_addr(&net_ip6) || > 276 net_prefix_length != 0) { > 277 sprintf(tmp, "%pI6c", &net_ip6); > 278 if (net_prefix_length != 0) >>>> CID 453851: Memory - corruptions (OVERLAPPING_COPY) >>>> In the call to function "sprintf", the arguments "tmp" and "tmp" Just submitted a patch to fix 453851. > may point to the same object. > 279 sprintf(tmp, "%s/%d", tmp, > net_prefix_length); > 280 > 281 env_set("ip6addr", tmp); > 282 } > 283 > 284 if (!ip6_is_unspecified_addr(&net_server_ip6)) { > > ** CID 450971: Insecure data handling (TAINTED_SCALAR) > /net/ndisc.c: 391 in process_ra() > > > ________________________________________________________________________________________________________ > *** CID 450971: Insecure data handling (TAINTED_SCALAR) > /net/ndisc.c: 391 in process_ra() > 385 /* Ignore the packet if router lifetime is 0. */ > 386 if (!icmp->icmp6_rt_lifetime) > 387 return -EOPNOTSUPP; > 388 > 389 /* Processing the options */ > 390 option = msg->opt; >>>> CID 450971: Insecure data handling (TAINTED_SCALAR) >>>> Using tainted variable "remaining_option_len" as a loop boundary. > 391 while (remaining_option_len > 0) { > 392 /* The 2nd byte of the option is its length. */ > 393 option_len = option[1]; > 394 /* All included options should have a positive > length. */ > 395 if (option_len == 0) > 396 return -EINVAL; > > ** CID 450969: Security best practices violations (DC.WEAK_CRYPTO) > /net/ndisc.c: 209 in ip6_send_rs() > > > ________________________________________________________________________________________________________ > *** CID 450969: Security best practices violations (DC.WEAK_CRYPTO) > /net/ndisc.c: 209 in ip6_send_rs() > 203 icmp_len, PROT_ICMPV6, pcsum); > 204 msg->icmph.icmp6_cksum = csum; > 205 pkt += icmp_len; > 206 > 207 /* Wait up to 1 second if it is the first try to get the RA > */ > 208 if (retry_count == 0) >>>> CID 450969: Security best practices violations (DC.WEAK_CRYPTO) >>>> "rand" should not be used for security-related applications, > because linear congruential algorithms are too easy to break. > 209 udelay(((unsigned int)rand() % 1000000) * > MAX_SOLICITATION_DELAY); > 210 > 211 /* send it! */ > 212 net_send_packet(net_tx_packet, (pkt - net_tx_packet)); > 213 > 214 retry_count++; > > ** CID 436282: (DC.WEAK_CRYPTO) > /net/dhcpv6.c: 621 in dhcp6_state_machine() > /net/dhcpv6.c: 627 in dhcp6_state_machine() > /net/dhcpv6.c: 628 in dhcp6_state_machine() > /net/dhcpv6.c: 662 in dhcp6_state_machine() > /net/dhcpv6.c: 613 in dhcp6_state_machine() > > > ________________________________________________________________________________________________________ > *** CID 436282: (DC.WEAK_CRYPTO) > /net/dhcpv6.c: 621 in dhcp6_state_machine() > 615 /* handle state machine entry conditions */ > 616 if (sm_params.curr_state != sm_params.next_state) { > 617 sm_params.retry_cnt = 0; > 618 > 619 if (sm_params.next_state == DHCP6_SOLICIT) { > 620 /* delay a random ammount (special for > SOLICIT) */ >>>> CID 436282: (DC.WEAK_CRYPTO) >>>> "rand" should not be used for security-related applications, > because linear congruential algorithms are too easy to break. > 621 udelay((rand() % SOL_MAX_DELAY_MS) * 1000); > 622 /* init timestamp variables after SOLICIT > delay */ > 623 sm_params.dhcp6_start_ms = get_timer(0); > 624 sm_params.dhcp6_retry_start_ms = > sm_params.dhcp6_start_ms; > 625 sm_params.dhcp6_retry_ms = > sm_params.dhcp6_start_ms; > 626 /* init transaction and ia_id */ > /net/dhcpv6.c: 627 in dhcp6_state_machine() > 621 udelay((rand() % SOL_MAX_DELAY_MS) * 1000); > 622 /* init timestamp variables after SOLICIT > delay */ > 623 sm_params.dhcp6_start_ms = get_timer(0); > 624 sm_params.dhcp6_retry_start_ms = > sm_params.dhcp6_start_ms; > 625 sm_params.dhcp6_retry_ms = > sm_params.dhcp6_start_ms; > 626 /* init transaction and ia_id */ >>>> CID 436282: (DC.WEAK_CRYPTO) >>>> "rand" should not be used for security-related applications, > because linear congruential algorithms are too easy to break. > 627 sm_params.trans_id = rand() & 0xFFFFFF; > 628 sm_params.ia_id = rand(); > 629 /* initialize retransmission parameters */ > 630 sm_params.irt_ms = SOL_TIMEOUT_MS; > 631 sm_params.mrt_ms = updated_sol_max_rt_ms; > 632 /* RFCs default MRC is be 0 (try infinitely) > /net/dhcpv6.c: 628 in dhcp6_state_machine() > 622 /* init timestamp variables after SOLICIT > delay */ > 623 sm_params.dhcp6_start_ms = get_timer(0); > 624 sm_params.dhcp6_retry_start_ms = > sm_params.dhcp6_start_ms; > 625 sm_params.dhcp6_retry_ms = > sm_params.dhcp6_start_ms; > 626 /* init transaction and ia_id */ > 627 sm_params.trans_id = rand() & 0xFFFFFF; >>>> CID 436282: (DC.WEAK_CRYPTO) >>>> "rand" should not be used for security-related applications, > because linear congruential algorithms are too easy to break. > 628 sm_params.ia_id = rand(); > 629 /* initialize retransmission parameters */ > 630 sm_params.irt_ms = SOL_TIMEOUT_MS; > 631 sm_params.mrt_ms = updated_sol_max_rt_ms; > 632 /* RFCs default MRC is be 0 (try infinitely) > 633 * give up after CONFIG_NET_RETRY_COUNT > number of tries (same as DHCPv4) > /net/dhcpv6.c: 662 in dhcp6_state_machine() > 656 (sm_params.mrd_ms != 0 && > 657 ((sm_params.dhcp6_retry_ms - > sm_params.dhcp6_retry_start_ms) >= sm_params.mrd_ms))) { > 658 sm_params.next_state = DHCP6_FAIL; > 659 } > 660 > 661 /* calculate retransmission timeout (RT) */ >>>> CID 436282: (DC.WEAK_CRYPTO) >>>> "rand" should not be used for security-related applications, > because linear congruential algorithms are too easy to break. > 662 rand_minus_plus_100 = ((rand() % 200) - 100); > 663 if (sm_params.retry_cnt == 0) { > 664 sm_params.rt_ms = sm_params.irt_ms + > 665 ((sm_params.irt_ms * > rand_minus_plus_100) / 1000); > 666 } else { > 667 sm_params.rt_ms = (2 * sm_params.rt_prev_ms) + > /net/dhcpv6.c: 613 in dhcp6_state_machine() > 607 * Proceed anyway to proceed DONE/FAIL actions > 608 */ > 609 debug("Unexpected DHCP6 state : %d\n", > sm_params.curr_state); > 610 break; > 611 } > 612 /* re-seed the RNG */ >>>> CID 436282: (DC.WEAK_CRYPTO) >>>> "rand" should not be used for security-related applications, We can ignore 436282. For DHCP6, we seed using srand_mac() to ensure that rand() will produce enough variation for each device on the network.  The numbers from rand() are used to introduce variation in the delay between re-transmissions to avoid excessive server congestion if all clients are started at the same time (such as a power loss).  These values are not used for crypto. > because linear congruential algorithms are too easy to break. > 613 srand(get_ticks() + rand()); > 614 > 615 /* handle state machine entry conditions */ > 616 if (sm_params.curr_state != sm_params.next_state) { > 617 sm_params.retry_cnt = 0; > 618 > > ** CID 436278: (TAINTED_SCALAR) > /net/dhcpv6.c: 321 in dhcp6_parse_options() > > > ________________________________________________________________________________________________________ > *** CID 436278: (TAINTED_SCALAR) > /net/dhcpv6.c: 376 in dhcp6_parse_options() > 370 if (sm_params.curr_state == > DHCP6_SOLICIT) > 371 sm_params.mrt_ms = > updated_sol_max_rt_ms; > 372 } > 373 break; > 374 case DHCP6_OPTION_OPT_BOOTFILE_URL: > 375 debug("DHCP6_OPTION_OPT_BOOTFILE_URL > FOUND\n"); >>>> CID 436278: (TAINTED_SCALAR) >>>> Passing tainted expression "option_len + 1" to "copy_filename", > which uses it as a loop boundary. > 376 copy_filename(net_boot_file_name, > option_ptr, option_len + 1); > 377 debug("net_boot_file_name: %s\n", > net_boot_file_name); > 378 > 379 /* copy server_ip6 (required for PXE) */ > 380 s = strchr(net_boot_file_name, '['); > 381 e = strchr(net_boot_file_name, ']'); > /net/dhcpv6.c: 321 in dhcp6_parse_options() > 315 while (option_hdr < (struct dhcp6_option_hdr *)(rx_pkt + > len)) { > 316 option_ptr = ((uchar *)option_hdr) + sizeof(struct > dhcp6_hdr); > 317 option_len = ntohs(option_hdr->option_len); > 318 > 319 switch (ntohs(option_hdr->option_id)) { > 320 case DHCP6_OPTION_CLIENTID: >>>> CID 436278: (TAINTED_SCALAR) >>>> Passing tainted expression "option_len" to "memcmp", which uses it Just submitted a patch to fix 436278. > as an offset. [Note: The source code implementation of the function has > been overridden by a builtin model.] > 321 if (memcmp(option_ptr, sm_params.duid, > option_len) > 322 != 0) { > 323 debug("CLIENT ID DOESN'T MATCH\n"); > 324 } else { > 325 debug("CLIENT ID FOUND and > MATCHES\n"); > 326 sm_params.rx_status.client_id_match > = true; > >