openssh bugs - Jun 2016 - [Bug 2581] New: Coverity patches from Fedora

https://bugzilla.mindrot.org/show_bug.cgi?id=2581

            Bug ID: 2581
           Summary: Coverity patches from Fedora
           Product: Portable OpenSSH
           Version: 7.2p1
          Hardware: Other
                OS: Linux
            Status: NEW
          Keywords: patch
          Severity: enhancement
          Priority: P5
         Component: sshd
          Assignee: unassigned-bugs at mindrot.org
          Reporter: jjelen at redhat.com

Created attachment 2822
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2822&action=edit
Proposed patch for points 1 - 5

I dug up some old patches that are hanging around fedora openssh
package, that could be useful also for upstream. They were reported
probably long time ago by Coverity and for some reason didn't make it
to upstream yet.

1. "debug3: mm_answer_keyallowed: key %p is allowed" is always NULL,
because it is freed before calling the log function.

  There is a flaw in logic of this debug log, when the key pointer is
always null (it is ensured that it is freed before) the call.

2. mm_pty_allocate contains Dead code (if the second dup fails, it
can't be different than -1) and comparison of the return dup() return
value should be >=0 instead of >0 -- 0 is also valid FD.

3. process_server_config_line: sAuthorizedPrincipalsFile option handles
intptr, without any possible label (therefore always null), which is
dead code (probably copy-paste error from sHostKeyFile)

4. box variable for sandbox context is not freed in the child process
after calling ssh_sandbox_child(), which makes it memory leak.

5. server_accept_loop() allocates fdset variable, but does not free it
in the end.

These are the most obvious problems and it would be nice to have them
addressed in the next release.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2581

--- Comment #1 from Jakub Jelen <jjelen at redhat.com> ---
Created attachment 2823
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2823&action=edit
Proposed patch for points 6 - 10

Furthermore there are few more checks that are probably very low
priority, but it is up to the upstream consideration if they will get
applied:

6. Compare >= 0 instead of direct comparison with -1: "!= -1" when
working with file descriptors.

This solution sanitizes also the negative integers, if it would happen
they would get into the arguments somehow. This is used in many places
in the codebase.

7. PAM authentication in pthread_join is using naively waitpid
expecting it can not fail.

Defensive solution would be call the waitpid until we get valid result,
fail on error and retry on EINTR.

Similar case is in scp and sftp, where the return value is also
ignored. Casting to (void) might silent complains a bit.

8. servconf reading the non active subsystems into arg variable, which
is unused. Casting to void might silent warnings.

9. Most of the paths and arguments in sftp-server are already converted
to (const char *), but there are few left in sftp, which would deserve
being const too.

10. ssh-agent is not checking return value of setegid() and setgid()
functions. Ugly solution is again typing to the (void), better would be
real check for the return value.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2581

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
                 CC|                            |djm at mindrot.org

--- Comment #2 from Damien Miller <djm at mindrot.org> ---
I've committed parts of these. The bits I skipped were:

1. The changed tests for fd==-1 -> fd>=0.
2. A couple of one-off memory leak fixes; these require more analysis
and we have a heap of them that we need to do in one pass
3. Return value casts to void

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2581

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
Close all resolved bugs after 7.3p1 release

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.