[Bug 33207] Results of my suexec.c code audit

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=33207

William A. Rowe Jr. <***@apache.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Resolution|--- |LATER
Status|NEW |RESOLVED
Keywords| |MassUpdate

--- Comment #1 from William A. Rowe Jr. <***@apache.org> ---
Please help us to refine our list of open and current defects; this is a mass
update of old and inactive Bugzilla reports which reflect user error, already
resolved defects, and still-existing defects in httpd.

As repeatedly announced, the Apache HTTP Server Project has discontinued all
development and patch review of the 2.2.x series of releases. The final release
2.2.34 was published in July 2017, and no further evaluation of bug reports or
security risks will be considered or published for 2.2.x releases. All reports
older than 2.4.x have been updated to status RESOLVED/LATER; no further action
is expected unless the report still applies to a current version of httpd.

If your report represented a question or confusion about how to use an httpd
feature, an unexpected server behavior, problems building or installing httpd,
or working with an external component (a third party module, browser etc.) we
ask you to start by bringing your question to the User Support and Discussion
mailing list, see [https://httpd.apache.org/lists.html#http-users] for details.
Include a link to this Bugzilla report for completeness with your question.

If your report was clearly a defect in httpd or a feature request, we ask that
you retest using a modern httpd release (2.4.33 or later) released in the past
year. If it can be reproduced, please reopen this bug and change the Version
field above to the httpd version you have reconfirmed with.

Your help in identifying defects or enhancements still applicable to the
current httpd server software release is greatly appreciated.

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=33207

Roland Illig <***@gmx.de> changed:

What |Removed |Added
----------------------------------------------------------------------------
Version|2.0.52 |2.4-HEAD
Resolution|LATER |---
Status|RESOLVED |REOPENED

--- Comment #2 from Roland Illig <***@gmx.de> ---
Yep, this bug report is still relevant.

In the last 13 years, only the null check for strdup has been added. The other
6 issues mentioned in this bug report are still open.

The appended patch probably won't apply cleanly after waiting 13 years, but the
idea should still be very clear.

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=33207

Joe Orton <***@redhat.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|REOPENED |NEEDINFO

--- Comment #3 from Joe Orton <***@redhat.com> ---
Where is the dependency on sizeof(int) exactly, and what's the logging problem
after execve failure?

I am not sure that setting environ[x] to an unwritable string literal is
actually safe (environ is declared as char **), I changed it to a simpler
strdup in r1846253.

I don't see a variable "prog" in the current code.

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=33207

--- Comment #4 from Roland Illig <***@gmx.de> ---
Sorry, my bad, I didn't look close enough.

The issue with atoi is still there. That function should never be used in any
code. Not even when you know that the string only consists of digits, since
there is still the possibility of overflow. Undefined behavior. ;)

The sprintf call may still overflow the buffer.

The "unable to log" refers to the "exec failed" at the very bottom of the file.
At that point, the log files have been closed (see closelog and fclose further
above), and since the process has changed ownership by then, it will not be
able to write to the suexec log file. Luckily this is only in very specific
circumstances (file not executable even though it has the executable bit set),
so it would probably not happen too often. Still the code should be solid here.

All other items from the original report have been fixed in the meantime.

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=33207

--- Comment #5 from Joe Orton <***@redhat.com> ---
sprintf was removed entirely in the commit above. By inspection err_output()
should re-open the log file after it was closed, though I haven't tested that.

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=33207

Roland Illig <***@gmx.de> changed:

What |Removed |Added
----------------------------------------------------------------------------
Resolution|--- |FIXED
Status|NEEDINFO |RESOLVED

--- Comment #6 from Roland Illig <***@gmx.de> ---
(In reply to Joe Orton from comment #5)

Post by b***@apache.org
sprintf was removed entirely in the commit above.

Thank you for fixing this. I my comment #4 I must have overlooked that the
sprintf is gone. I'm not sure why I overlooked it since
https://svn.apache.org/viewvc/ shows all changes immediately, so I should have
seen your fix already when looking at the HEAD revision. Maybe my browser
showed an outdated version.

Thanks again.

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=33207

--- Comment #7 from Roland Illig <***@gmx.de> ---
I you want to test whether the logging fails, it should be possible have an ELF
binary for another architecture in cgi-bin. This scenario should trigger
exactly that code path.

Background and intentions

The reason I'm so picky with the suexec code is that for me, back in 2005, it
was the primary example of how to program very carefully. It's a setuid binary
after all, therefore it should demonstrate best practices and document many
pitfalls. And it does a great job at this, serving well for education.
Therefore, and because it is such a prominent example of security critical
code, I wanted it to be perfect.

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=33207

--- Comment #8 from Joe Orton <***@redhat.com> ---
Very good intentions!

Good idea about making execve fail, made me think of a bogus shebang line which
fails execve as well. I tested it with suexec as built in Fedora (which has
syslog) and it logged it fine, FYI -

# journalctl --no-hostname -r -u httpd | head -3
-- Logs begin at Wed 2017-11-08 10:55:43 GMT, end at Tue 2018-11-13 16:22:05
GMT. --
Nov 13 16:19:22 suexec[707]: (2)No such file or directory: exec failed (env.sh)
Nov 13 16:19:22 suexec[707]: uid: (1000/jorton) gid: (1000/jorton) cmd: env.sh

I will fix the atoi as well.