openssh unix dev - Feb 2012 - Potential memory leak in sshd [detected by melton]

Hi all,

After the memory leaks (bug 1967
<https://bugzilla.mindrot.org/show_bug.cgi?id=1967>) I reported in
bugzilla are fixed,

I also applied melton(http://lcs.ios.ac.cn/~xuzb/melton.html)

to detect the potential bugs in sshd (openssh-5.9p1).


The url below is the index of bug reports that are checked as real
bugs manually.

http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/index.html


Shall we fix these bugs? Or just let them go since they are not so serious?

Hope for your replies!


-- 
Zhenbo Xu

By the way, I submitted this report in bugzilla a few days ago, but there
is no response.
Should I report bugs in this mailing list rather than in bugzilla?

2012/2/4 Zhenbo Xu <zhenbo1987 at gmail.com>
> Hi all,
>
> After the memory leaks (bug 1967
<https://bugzilla.mindrot.org/show_bug.cgi?id=1967>) I reported in
bugzilla are fixed,
>
> I also applied melton(http://lcs.ios.ac.cn/~xuzb/melton.html)
>
> to detect the potential bugs in sshd (openssh-5.9p1).
>
>
> The url below is the index of bug reports that are checked as real bugs
manually.
>
>
http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/index.html
>
>
> Shall we fix these bugs? Or just let them go since they are not so serious?
>
> Hope for your replies!
>
>
> --
> Zhenbo Xu
>


-- 
Zhenbo Xu

On 04/02/12 02:55, Zhenbo Xu wrote:
> I also applied melton(http://lcs.ios.ac.cn/~xuzb/melton.html)
>
> to detect the potential bugs in sshd (openssh-5.9p1).
>
>
> The url below is the index of bug reports that are checked as real
> bugs manually.
>
>
http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/index.html
>
>
> Shall we fix these bugs? Or just let them go since they are not so serious?
> Hope for your replies!
The second leak

Logic error 	Memory leak 	session.i 	13193 	13 	View Report
<http://lcs.ios.ac.cn/%7Exuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/report-zSMfqI.html#EndPath>


http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/report-zSMfqI.html#EndPath

is a false positive.

The only way to exit the infinite loop is the return of line 13267.
And line 13266 calls session_close(), which frees s->auth_data in line
14994.
(granted, it's tricky to follow...)

The analysis seem to have stopped in line 13193, after step 14.

As a suggestion, I recommend you to make the messages "Execution
continues on line 12345"
links to line 12345.
Also, going to the opening brace from the closing one would be useful
when dealing with big blocks.

On 04/02/12 02:55, Zhenbo Xu wrote:
> The url below is the index of bug reports that are checked as real
> bugs manually.
>
>
http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/index.html
>
>
> Shall we fix these bugs? Or just let them go since they are not so serious?
>
> Hope for your replies!
The third error
Logic error    Memory leak    monitor.i    13658    3
seems like a good catch.
There should be a call to buffer_free(&logmsg); before the return -1 of
monitor_read_log.

Although it only happens if the client closed the socket, in which case
the next poll of
line 13702 should fail and monitor_read_log never called again.
So it probably only leaks once.

Attaching fix.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: monitor_read_log-buffer_free.patch
Type: text/x-patch
Size: 377 bytes
Desc: not available
URL:
<http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20120205/d139984d/attachment.bin>

On 05/02/12 21:51, ?ngel Gonz?lez wrote:
> The second leak
> 
> Logic error 	Memory leak 	session.i 	13193 	13 	View Report
>
<http://lcs.ios.ac.cn/%7Exuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/report-zSMfqI.html#EndPath>
> 
> 
>
http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/report-zSMfqI.html#EndPath
> 
> is a false positive.
> 
> The only way to exit the infinite loop is the return of line 13267.
> And line 13266 calls session_close(), which frees s->auth_data in line
> 14994.

The fifth one is a duplicate of this (same path, but the variable
allocated on next line).
12th one is the same issue of freeing at session_close not detected. But
this time with s->auth_display

On 04/02/12 02:55, Zhenbo Xu wrote:
> The url below is the index of bug reports that are checked as real
> bugs manually.
>
>
http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/index.html
>
> Shall we fix these bugs? Or just let them go since they are not so serious?
>
> Hope for your replies!
The seventh report is not complete, but seem a genuine leak:
Logic error 	Memory leak 	auth2-chall.i 	9868 	5 	View Report
<http://lcs.ios.ac.cn/%7Exuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/report-0o5DSr.html#EndPath>


http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/report-0o5DSr.html#EndPath

Melton complains that authctxt->kbdintctxt is never freed.
The return value passes authctxt to auth2_challenge_start(), and
auth2_challenge_start() may call
auth2_challenge_stop(), which frees it (line 9878) [melton doesn't
realise this].
In the path where it doesn't free it, it sets authctxt->postponed and
returns 0 to userauth_kbdint(),
itself called from input_userauth_request() at auth2.c line 283.
There then calls userauth_finish() at line 285. Which return in line 336.
And input_userauth_request() finishes with authctxt getting out of
scope, without authctxt->kbdintctxt
having been freed.

On 04/02/12 02:55, Zhenbo Xu wrote:
> The url below is the index of bug reports that are checked as real
> bugs manually.
>
>
http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/index.html
>
>
> Shall we fix these bugs? Or just let them go since they are not so serious?
>
> Hope for your replies!
The 8th report is another false positive
Logic error 	Memory leak 	auth2.i 	11503 	6 	View
<http://lcs.ios.ac.cn/%7Exuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/report-ABKOwr.html#EndPath>


http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/report-ABKOwr.html#EndPath

fakepw() returns a static variable.

On 04/02/12 02:55, Zhenbo Xu wrote:
> The url below is the index of bug reports that are checked as real
> bugs manually.
>
>
http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/index.html
>
>
> Shall we fix these bugs? Or just let them go since they are not so serious?
>
> Hope for your replies!
The 10th report is another false positive:
Logic error 	Memory leak 	auth-options.i 	10587 	28 	View Report
<http://lcs.ios.ac.cn/%7Exuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/report-mVEeJj.html#EndPath>


http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/report-mVEeJj.html#EndPath

Melton complains that in line 10587 the memory of data wasn't released,
but there's a call to buffer_free(&data);
in line 10585.

2012/2/6 ?ngel Gonz?lez <keisial at gmail.com>
>  On 04/02/12 02:55, Zhenbo Xu wrote:
>
> The url below is the index of bug reports that are checked as real
> bugs manually.
>
http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/index.html
>
>
> Shall we fix these bugs? Or just let them go since they are not so serious?
>
> Hope for your replies!
>
>  The 8th report is another false positive
>   Logic error Memory leak auth2.i 11503 6
View<http://lcs.ios.ac.cn/%7Exuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/report-ABKOwr.html#EndPath>
>
http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/report-ABKOwr.html#EndPath
>
> fakepw() returns a static variable.
>
> Shall we free authctxt->pw before this assignment authctxt->pw
fakepw();,
 Since authctxt->pw gets a heap space at 11496. (authctxt->pw (use_privsep
? mm_getpwnamallow(user) :
getpwnamallow(user)<http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/sshd/linked_files/linked-oH0nvi.html#Path7_2>
);)



-- 
Zhenbo Xu