Bug 3167 - Bugs found by static tests
Summary: Bugs found by static tests
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: sshd (show other bugs)
Version: 8.2p1
Hardware: All All
: P5 normal
Assignee: Assigned to nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-20 09:24 AEST by tester7632
Modified: 2021-03-04 09:54 AEDT (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description tester7632 2020-05-20 09:24:47 AEST
Running the current version of openssh-portable through SonarQube revealed a number of bugs.

While there are a lot of false positives or simply unimportant bugs, I found several areas that seem like legitimate coding errors.


https://sonarcloud.io/project/issues?id=openssh-portable_allfeatures&open=AXIu_cwVij8m0Z5KWuok&resolved=false&types=BUG

https://sonarcloud.io/project/issues?id=openssh-portable_allfeatures&open=AXIu_cwVij8m0Z5KWuop&resolved=false&types=BUG

https://sonarcloud.io/project/issues?id=openssh-portable_allfeatures&open=AXIu_cwxij8m0Z5KWutU&resolved=false&types=BUG

https://sonarcloud.io/project/issues?id=openssh-portable_allfeatures&open=AXIu_c8Tij8m0Z5KWvbw&resolved=false&types=BUG

https://sonarcloud.io/project/issues?id=openssh-portable_allfeatures&open=AXIu_c2tij8m0Z5KWvEw&resolved=false&types=BUG

https://sonarcloud.io/project/issues?id=openssh-portable_allfeatures&open=AXIu_c21ij8m0Z5KWvE9&resolved=false&types=BUG


The complete report can be found here: 
https://sonarcloud.io/project/issues?id=openssh-portable_allfeatures&open=AXIu_c21ij8m0Z5KWvE9&resolved=false&types=BUG


At a first glance I found also issues in the test cases:

https://sonarcloud.io/project/issues?id=openssh-portable-tests&open=AXIu8yLf_34BaxGawOJ2&resolved=false&tags=pitfall&types=BUG

https://sonarcloud.io/project/issues?id=openssh-portable-tests&open=AXIu8yLf_34BaxGawOKT&resolved=false&severities=CRITICAL&types=BUG



I have not gone through all of the reported bugs yet.
The use after free bugs probably need some closer look.
Comment 1 Damien Miller 2020-05-29 14:26:53 AEST
(In reply to tester7632 from comment #0)
> https://sonarcloud.io/project/issues?id=openssh-
> portable_allfeatures&open=AXIu_cwVij8m0Z5KWuok&resolved=false&types=B
> UG

obvious false positive - the static analyser doesn't seem to consider that an expression may have side-effects.

> 
> https://sonarcloud.io/project/issues?id=openssh-
> portable_allfeatures&open=AXIu_cwVij8m0Z5KWuop&resolved=false&types=B
> UG

likewise

> https://sonarcloud.io/project/issues?id=openssh-
> portable_allfeatures&open=AXIu_cwxij8m0Z5KWutU&resolved=false&types=B
> UG

static analyser seems to not understand that variable 'p' is updated here.

> https://sonarcloud.io/project/issues?id=openssh-
> portable_allfeatures&open=AXIu_c8Tij8m0Z5KWvbw&resolved=false&types=B
> UG

also false positive. Analyser misses that resolved_len is set around https://sonarcloud.io/code?id=openssh-portable_allfeatures&selected=openssh-portable_allfeatures%3Asftp-realpath.c&line=99

> https://sonarcloud.io/project/issues?id=openssh-
> portable_allfeatures&open=AXIu_c2tij8m0Z5KWvEw&resolved=false&types=B
> UG

Another false positive. The path the analyser took demonstrates that the posited overflow can't occur.

> https://sonarcloud.io/project/issues?id=openssh-
> portable_allfeatures&open=AXIu_c21ij8m0Z5KWvE9&resolved=false&types=B
> UG

analyser misses the sshbuf_free() call on the very line before the putative memleak occurs :(

> 
> The complete report can be found here: 
> https://sonarcloud.io/project/issues?id=openssh-
> portable_allfeatures&open=AXIu_c21ij8m0Z5KWvE9&resolved=false&types=B
> UG
> 
> 
> At a first glance I found also issues in the test cases:
> 
> https://sonarcloud.io/project/issues?id=openssh-portable-
> tests&open=AXIu8yLf_34BaxGawOJ2&resolved=false&tags=pitfall&types=BUG
> 
> https://sonarcloud.io/project/issues?id=openssh-portable-
> tests&open=AXIu8yLf_34BaxGawOKT&resolved=false&severities=CRITICAL&ty
> pes=BUG

I think the first one of these at least was fixed in the upstream version of netcat. I'll sync the OpenSSH copy against that
Comment 2 Damien Miller 2020-05-29 15:04:51 AEST
> https://sonarcloud.io/project/issues?id=openssh-portable-
> tests&open=AXIu8yLf_34BaxGawOKT&resolved=false&severities=CRITICAL&ty
> pes=BUG


This one is another false positive. The analyser misses that the unix_listener() call is only reachable when family==AF_UNIX and there there is a check that ensures "host" is not NULL for that case:

https://sonarcloud.io/code?id=openssh-portable-tests&selected=openssh-portable-tests%3Aregress%2Fnetcat.c&line=296

I've synced regress/netcat.c with upstream to fix the duplicate ==-1 check in the other report.
Comment 3 Damien Miller 2021-03-04 09:54:21 AEDT
close bugs that were resolved in OpenSSH 8.5 release cycle