Fwd: New Defects reported by Coverity Scan for Das U-Boot
Here's the latest defect report:
---------- Forwarded message --------- From: scan-admin@coverity.com Date: Mon, May 8, 2023, 2:29 PM Subject: New Defects reported by Coverity Scan for Das U-Boot To: tom.rini@gmail.com
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"
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,
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
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;
On 5/8/2023 3:20 PM, Tom Rini wrote:
Here's the latest defect report:
---------- Forwarded message --------- From:scan-admin@coverity.com Date: Mon, May 8, 2023, 2:29 PM Subject: New Defects reported by Coverity Scan for Das U-Boot To:tom.rini@gmail.com
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 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;
The problem here is that although the lower bound of the variable remaining_option_len is checked, the upper bound is not checked. Coverity is complaining that the function's argument len which is read from a packet content is assigned to remaining_option_len and therefore has made it a tainted scalar.
I will compare the value of len with ETH_MAX_MTU constant and make sure it is less than that as shown below.
if(len > ETH_MAX_MTU) return-EMSGSIZE;
** 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++;
This is a false positive. The function rand() is not used for encryption here. It is used to just make a random delay to avoid collisions on the network. It has nothing to do with encryption.
On 2023-05-08 1:20 p.m., Tom Rini wrote:
Here's the latest defect report:
---------- Forwarded message --------- From: scan-admin@coverity.com Date: Mon, May 8, 2023, 2:29 PM Subject: New Defects reported by Coverity Scan for Das U-Boot To: tom.rini@gmail.com
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;
participants (3)
-
Ehsan Mohandesi -
Sean Edmond -
Tom Rini