Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot

[Sean Edmond <seanedmond@linux.microsoft.com>]

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;
>
>