Bug 2163 - unchecked returned value from pam_get_item()
Summary: unchecked returned value from pam_get_item()
Status: NEW
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: PAM support (show other bugs)
Version: -current
Hardware: All All
: P5 minor
Assignee: Assigned to nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-21 04:28 AEDT by Loganaden Velvindron
Modified: 2015-06-05 14:42 AEST (History)
2 users (show)

See Also:


Attachments
auth-pam.c.diff (614 bytes, patch)
2013-10-21 04:28 AEDT, Loganaden Velvindron
no flags Details | Diff
sftp.c.diff (590 bytes, patch)
2013-10-22 23:09 AEDT, Loganaden Velvindron
no flags Details | Diff
session.c.diff (481 bytes, patch)
2013-10-22 23:10 AEDT, Loganaden Velvindron
djm: ok+
Details | Diff
gss-serv.c.diff (1.21 KB, patch)
2013-10-22 23:11 AEDT, Loganaden Velvindron
no flags Details | Diff
ssh-keyscan.c.diff (526 bytes, patch)
2013-10-22 23:12 AEDT, Loganaden Velvindron
dtucker: ok+
Details | Diff
ssh-pkcs11-client.c.diff (607 bytes, patch)
2013-10-22 23:13 AEDT, Loganaden Velvindron
dtucker: ok+
Details | Diff
sshconnect.c.diff (669 bytes, patch)
2013-10-22 23:14 AEDT, Loganaden Velvindron
dtucker: ok+
Details | Diff
sshconnect2.c.diff (722 bytes, patch)
2013-10-22 23:17 AEDT, Loganaden Velvindron
no flags Details | Diff
sftp.c leak fix (2.55 KB, patch)
2013-12-05 10:55 AEDT, Damien Miller
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Loganaden Velvindron 2013-10-21 04:28:54 AEDT
Created attachment 2356 [details]
auth-pam.c.diff

in auth-pam.c,

A call is made to pam_get_item() but the returned value isn't checked.

In other places, the returned value to pam_get_item() has been checked.
Comment 1 Loganaden Velvindron 2013-10-22 23:09:23 AEDT
Created attachment 2357 [details]
sftp.c.diff
Comment 2 Loganaden Velvindron 2013-10-22 23:10:11 AEDT
Created attachment 2358 [details]
session.c.diff
Comment 3 Loganaden Velvindron 2013-10-22 23:11:14 AEDT
Created attachment 2359 [details]
gss-serv.c.diff
Comment 4 Loganaden Velvindron 2013-10-22 23:12:04 AEDT
Created attachment 2360 [details]
ssh-keyscan.c.diff
Comment 5 Loganaden Velvindron 2013-10-22 23:13:18 AEDT
Created attachment 2361 [details]
ssh-pkcs11-client.c.diff
Comment 6 Loganaden Velvindron 2013-10-22 23:14:50 AEDT
Created attachment 2362 [details]
sshconnect.c.diff
Comment 7 Loganaden Velvindron 2013-10-22 23:17:02 AEDT
Created attachment 2363 [details]
sshconnect2.c.diff
Comment 8 Loganaden Velvindron 2013-10-22 23:18:23 AEDT
Similar issues in other files.
Comment 9 Damien Miller 2013-12-05 10:53:37 AEDT
Comment on attachment 2357 [details]
sftp.c.diff

IMO it's better return return a NULL array on failure. I'll make a patch.
Comment 10 Damien Miller 2013-12-05 10:55:07 AEDT
Created attachment 2381 [details]
sftp.c leak fix
Comment 11 Loganaden Velvindron 2013-12-05 16:09:43 AEDT
(In reply to Damien Miller from comment #10)
> Created attachment 2381 [details]
> sftp.c leak fix

Looks better !
Comment 12 Darren Tucker 2013-12-19 11:35:20 AEDT
Comment on attachment 2356 [details]
auth-pam.c.diff

auth-pam.c change applied, thanks.  still yet to look at the others.
Comment 13 Darren Tucker 2013-12-19 11:47:19 AEDT
Comment on attachment 2358 [details]
session.c.diff

>-	int n_bytes;
>+	int n_bytes = 0;

What's the intent here, silencing a compiler warning?   n_bytes always gets initialized before use, in the case of protocol 1 in session_pty_req():

        /* for SSH1 the tty modes length is not given */
        if (!compat20)
                n_bytes = packet_remaining();
        tty_parse_modes(s->ttyfd, &n_bytes);

and in the protocol 2 case at the start of tty_parse_modes:


        if (compat20) {
                *n_bytes_ptr = packet_get_int();
                if (*n_bytes_ptr == 0)
                        return;

I can imagine a compiler not figuring this out, though.
Comment 14 Loganaden Velvindron 2013-12-19 16:36:07 AEDT
(In reply to Darren Tucker from comment #13)
> Comment on attachment 2358 [details]
> session.c.diff
> 
> >-	int n_bytes;
> >+	int n_bytes = 0;
> 
> What's the intent here, silencing a compiler warning?   n_bytes
> always gets initialized before use, in the case of protocol 1 in
> session_pty_req():
> 
>         /* for SSH1 the tty modes length is not given */
>         if (!compat20)
>                 n_bytes = packet_remaining();
>         tty_parse_modes(s->ttyfd, &n_bytes);
> 
> and in the protocol 2 case at the start of tty_parse_modes:
> 
> 
>         if (compat20) {
>                 *n_bytes_ptr = packet_get_int();
>                 if (*n_bytes_ptr == 0)
>                         return;
> 
> I can imagine a compiler not figuring this out, though.

Yep :-) The compiler didn't figure it out. I should have looked in other parts of the code.
Comment 15 Damien Miller 2014-02-06 10:18:14 AEDT
Retarget incomplete bugs / feature requests to 6.6 release
Comment 16 Damien Miller 2014-02-06 10:20:25 AEDT
Retarget incomplete bugs / feature requests to 6.6 release
Comment 17 Damien Miller 2014-04-12 14:49:58 AEST
Retarget to 6.7 release, since 6.6 was mostly bugfixing.
Comment 18 Damien Miller 2014-04-12 14:54:05 AEST
Remove from 6.6 tracking bug
Comment 19 Damien Miller 2014-08-30 04:38:17 AEST
Retarget incomplete bugs to 6.8 release.
Comment 20 Damien Miller 2014-08-30 04:40:12 AEST
These bugs are no longer targeted at the imminent 6.7 release
Comment 21 Damien Miller 2015-03-03 07:59:25 AEDT
OpenSSH 6.8 is approaching release and closed for major work. Retarget these bugs for the next release.
Comment 22 Damien Miller 2015-03-03 08:01:20 AEDT
Retarget to 6.9
Comment 23 Damien Miller 2015-06-05 14:42:44 AEST
detarget for now, will deal with after 6.9 release