| Summary: | unchecked returned value from pam_get_item() | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Portable OpenSSH | Reporter: | Loganaden Velvindron <loganaden> | ||||||||||||||||||||
| Component: | PAM support | Assignee: | Assigned to nobody <unassigned-bugs> | ||||||||||||||||||||
| Status: | NEW --- | ||||||||||||||||||||||
| Severity: | minor | CC: | djm, dtucker | ||||||||||||||||||||
| Priority: | P5 | ||||||||||||||||||||||
| Version: | -current | ||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
Created attachment 2357 [details]
sftp.c.diff
Created attachment 2358 [details]
session.c.diff
Created attachment 2359 [details]
gss-serv.c.diff
Created attachment 2360 [details]
ssh-keyscan.c.diff
Created attachment 2361 [details]
ssh-pkcs11-client.c.diff
Created attachment 2362 [details]
sshconnect.c.diff
Created attachment 2363 [details]
sshconnect2.c.diff
Similar issues in other files. Comment on attachment 2357 [details]
sftp.c.diff
IMO it's better return return a NULL array on failure. I'll make a patch.
Created attachment 2381 [details]
sftp.c leak fix
(In reply to Damien Miller from comment #10) > Created attachment 2381 [details] > sftp.c leak fix Looks better ! Comment on attachment 2356 [details]
auth-pam.c.diff
auth-pam.c change applied, thanks. still yet to look at the others.
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. (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. Retarget incomplete bugs / feature requests to 6.6 release Retarget incomplete bugs / feature requests to 6.6 release Retarget to 6.7 release, since 6.6 was mostly bugfixing. Remove from 6.6 tracking bug Retarget incomplete bugs to 6.8 release. These bugs are no longer targeted at the imminent 6.7 release OpenSSH 6.8 is approaching release and closed for major work. Retarget these bugs for the next release. Retarget to 6.9 detarget for now, will deal with after 6.9 release |
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.