Re: [PATCH] Authentication support for pserver

On Dec 15, 2007 10:08 AM, Ævar Arnfjörð Bjarmason <avar@cpan.org> wrote:

I have been mulling a bit about this change. Seems correct from a code flow POV. But anon pserver support was added with a lot of trepidation. Authenticated, write-enabled pserver support fills me with dread ;-)

A few things that come to mind

 - git/config is very likely to be readable if the site is served via
other means, like dumb http protocol, or git+ssh. So even if the
password scrambling is mickey-mouse. it might make sense to force the
password data to live elsewhere.

 - umasks/file permissions will probably be handled by the underlying
git tools and core.sharedrepository, since we no longer update refs
"manually". So after a bit of thinking, this should not be an issue.

 - I still worry about running cvsserver with an acct that has write
privileges. With anon, all we need write access is the sqlite db, and
that's not even based on user input. And it can be relocated elsewhere
via git/config.

So, wondering about input validation and related matters re-read most of cvsserver. The only suspicious bit I find is a caller to transmitfile @ lines 1117, where the 2nd parameter doesn't come from tempfile() - if a nasty filename sneaked in, we could (perhaps) be in for a surprise. We do that for diff header prettyness - we could use --label / -L instead. Will try and prep a patch for that tomorrow if noone beats me to it (bedtime in nz!).

Hmmm. Does "worried ACK" count as an ACK? Can we add big fat warnings along the lines of "run this in a locked down environment, pretty please"?

cheers,

m