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

[Ehsan Mohandesi <emohandesi@linux.microsoft.com>]

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.