Re: [PATCH] gitweb: Harden parse_commit and parse_commits

Please do not remove git@vger.kernel.org (git mailing list) from Cc,
i.e. please use "Reply to all" instead of just "Reply to author".

On Wed, 8 Feb 2012, rajesh boyapati wrote:

2012/2/8 Jakub Narebski [off-list ref]

 
[...]

quoted

Does the following patch help, and does it fix the issue?

(Nb. you can try to simply change filename, and apply it with fuzz
against index.cgi file).
-- >8 -- ----- ----- ----- ----- ----- -- >8 --
From: Jakub Narebski <redacted>
Subject: [PATCH] gitweb: Harden parse_commit and parse_commits

[...]

When I applied the above patch and also the patch from your previous
e-mail, I am getting this error

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

[2012-02-08 14:09:58,396] ERROR
com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
'HEAD'
[2012-02-08 14:10:06,732] ERROR
com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
'HEAD'
[2012-02-08 14:10:11,404] ERROR
com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
'HEAD'
[2012-02-08 14:10:15,270] ERROR
com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: Not a valid
object name HEAD
<<<<<<<<<<<<<<
With these patches, the previous errors at line numbers are gone.

Thanks for information.


This final issue will be a bit harder to fix.  This error message

  fatal: bad revision 'HEAD'

comes from git (I think from "git rev-list" command), and not from gitweb.
It is printed on STDERR of git command.  What has to be done to fix it is
to capture stderr of a process, or silence it.

Unfortunately it is not that easy.  We use list form of open, which avoids
using a shell interpreter to run command, and is safer wrt. shell escaping.

The only place where gitweb cares about redirecting standard error from git
command is git_object().  It is a bit hacky, and might be not entirely safe.
To fix this issue we would have to do the same in parse_commit*() as in
git_object(), or provide some kind of wrapper like IPC::Run provides
for redirecting stderr of called command.

Note that this issue was not considered very important, because this message
doesn't goes into web server logs when running gitweb via mod_cgi with
Apache... and probably also with other web servers.  Gerrit (or rather
whatever it uses for serving CGI scripts) might be exception here.

-- 
Jakub Narebski
Poland