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

Ehsan Mohandesi
2023-05-15T22:00:01Z

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.