Re: Useless error message?

Jonathan Nieder [off-list ref] writes:

The problem is this: as far as I can tell, the git protocols are
designed around the success case.  Sometimes if there is an error or
other interesting event, the servers are kind enough to notify the
user “on the side”.  But in the end, all too often, they do not bother
to inform the client _program_ that a fatal error occured.

The true story is a bit different.

To avoid information leak to git-daemon clients, we deliberately choose
not to give detailed error messages, so that you cannot tell if an error
means a user "u" does not exist or "u" does but ~u/repo.git repository
does not exist.

So the trick is to make it expected more often.  See the side-band-64k
capability in Documentation/technical/protocol-capabilities.txt: the
goal is to have fatal error messages for as many failure modes as
possible.

For authenticated users (read: services that typically are behind auth) it
would be a good thing, but "as many as possible" you shouldn't be followed
blindly.

Junio C Hamano wrote:

The true story is a bit different.

To avoid information leak to git-daemon clients, we deliberately choose
not to give detailed error messages, so that you cannot tell if an error
means a user "u" does not exist or "u" does but ~u/repo.git repository
does not exist.

Thanks for the clarification.  As I see it, these are two different
classes of problem:

1. The git daemon is very quiet, usually for good reason, as you
   mentioned [1] [2].

2. The git daemon and protocol helpers do not always send the datum “a
   controlled fatal error occured” by writing some message (any
   message) to side band 3.

Fixing the daemon’s share in both might require setting up a side band
very early.  If an RFC patch appears setting up the side band (or an
explanation for why that’s not possible), I would be happy to start
work building from there.

That has been the big obstacle for me experimenting with it, more than
the information disclosure.  But this is easy to say.  The doing is
more important.

Thanks again, and sorry for the noise.
Jonathan

[1] I do suspect that in the case of failing enter_repo() or missing
git-daemon-export-ok, saying “cannot read the specified repo” would be
fine.  Most of the time, there is not much value in disclosing a more
detailed reason, anyway.

[2] Example fix for a problem in this class:
http://thread.gmane.org/gmane.comp.version-control.git/139029

On 04/22/2010 11:42 AM, Jonathan Nieder wrote:

Junio C Hamano wrote:

quoted

The true story is a bit different.

To avoid information leak to git-daemon clients, we deliberately choose
not to give detailed error messages, so that you cannot tell if an error
means a user "u" does not exist or "u" does but ~u/repo.git repository
does not exist.

Thanks for the clarification.  As I see it, these are two different
classes of problem:

1. The git daemon is very quiet, usually for good reason, as you
    mentioned [1] [2].

2. The git daemon and protocol helpers do not always send the datum “a
    controlled fatal error occured” by writing some message (any
    message) to side band 3.

[1] I do suspect that in the case of failing enter_repo() or missing
git-daemon-export-ok, saying “cannot read the specified repo” would be
fine.  Most of the time, there is not much value in disclosing a more
detailed reason, anyway.

That would make it possible for random attackers to determine whether
a specific user exists on the system, which is very bad indeed.

[2] Example fix for a problem in this class:
http://thread.gmane.org/gmane.comp.version-control.git/139029

That's a different problem. We only end up in {send,receive}-pack if
the remote user asked for an existing repository, which means he or
she is either a very determined guesser or, more likely, already
knows that the user exists and where he or she keeps git repos. A
possible issue, to be sure, but definitely a far narrower window
than just guessing a username.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.

Andreas Ericsson wrote:

On 04/22/2010 11:42 AM, Jonathan Nieder wrote:

quoted

[1] I do suspect that in the case of failing enter_repo() or missing
git-daemon-export-ok, saying “cannot read the specified repo” would be
fine.  Most of the time, there is not much value in disclosing a more
detailed reason, anyway.

That would make it possible for random attackers to determine whether
a specific user exists on the system, which is very bad indeed.

I guess I am missing something.  How would

(*) $ git clone git://git.example.com/~u/foo
    remote: Cannot read the specified repo

tell me whether that user existed on the system?  If the daemon gives
the same message for ENOENT, missing git-daemon-export-ok, EPERM, and
so on so I cannot distinguish the cases, then I just don’t see the
problem.

If the daemon failed for some other reason, like a flaky network, I
would see

    $ git clone git://git.example.com/~u/foo
    fatal: The remote end hung up unexpectedly

So the extra information could still be helpful, without unwanted
information disclosure.  In the case (*) I learn definitively that the
address I specified does not represent a repo I have access to, rather
than this being some random, transient unexplained problem.

Thanks for the comment.
Jonathan

On 04/22/2010 12:15 PM, Jonathan Nieder wrote:

Andreas Ericsson wrote:

quoted

On 04/22/2010 11:42 AM, Jonathan Nieder wrote:

quoted

quoted

[1] I do suspect that in the case of failing enter_repo() or missing
git-daemon-export-ok, saying “cannot read the specified repo” would be
fine.  Most of the time, there is not much value in disclosing a more
detailed reason, anyway.

That would make it possible for random attackers to determine whether
a specific user exists on the system, which is very bad indeed.

I guess I am missing something.  How would

(*) $ git clone git://git.example.com/~u/foo
     remote: Cannot read the specified repo

tell me whether that user existed on the system?  If the daemon gives
the same message for ENOENT, missing git-daemon-export-ok, EPERM, and
so on so I cannot distinguish the cases, then I just don’t see the
problem.

If the daemon failed for some other reason, like a flaky network, I
would see

     $ git clone git://git.example.com/~u/foo
     fatal: The remote end hung up unexpectedly

So the extra information could still be helpful, without unwanted
information disclosure.  In the case (*) I learn definitively that the
address I specified does not represent a repo I have access to, rather
than this being some random, transient unexplained problem.

So that would be the new error message for everything that fails, then?

One big reason why I'm not bothered with running the git-daemon on a
public server is that it's very simple. If something goes wrong, it
dies without fiddling about.

How would it benefit you if it said "fatal: Something went wrong, but
I didn't crash" instead of just hanging up? If you have the wrong
repo address, you'd still have to check up with whoever gave it to
you to get it right. If it *does* crash, you'd still have to get
hold of the server admin to tell him that it has crashed.

A minor patch to git-fetch, updating the error message with a few
possible reasons would be far better. I don't care about it myself,
but I'm sure such a patch would be a lot easier to get into git.git
than something that adds a lot of complexity to the git daemon.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.

Andreas Ericsson wrote:

On 04/22/2010 12:15 PM, Jonathan Nieder wrote:

quoted

(*) $ git clone git://git.example.com/~u/foo
     remote: Cannot read the specified repo

[...]

So that would be the new error message for everything that fails, then?

No.  Of course, the opposite is the point.  I just mean there should
be an error message for all conditions that are lumped together with
ENOENT to avoid information disclosure.  I don’t care much how the
message is phrased.

If you have the wrong
repo address, you'd still have to check up with whoever gave it to
you to get it right. If it *does* crash, you'd still have to get
hold of the server admin to tell him that it has crashed.

Many things can go wrong other than a missing repo.  For example,
there might be objects missing, or high load, or memory corruption.

In the “wrong address” case, I know who to go to to fix it: ask the
person who gave me the address.

In the “corrupt repository” case, I should go to the repository
owner.

In the “server hung up without any good reason” case, I should try to
narrow down the problem, perhaps with the help of the server admin or
my network administrator.  Is it really so unusual to want to
distinguish this from other cases?

Without code this is all theoretical anyway.  And that’s the real
problem: to the uninitiated, it is not easy to write code to try this
out because the side band is not set up in time.

Sigh,
Jonathan

On Thu, Apr 22, 2010 at 04:42:16AM -0500, Jonathan Nieder wrote:

Thanks for the clarification.  As I see it, these are two different
classes of problem:

1. The git daemon is very quiet, usually for good reason, as you
   mentioned [1] [2].

2. The git daemon and protocol helpers do not always send the datum “a
   controlled fatal error occured” by writing some message (any
   message) to side band 3.

Fixing the daemon’s share in both might require setting up a side band
very early.  If an RFC patch appears setting up the side band (or an
explanation for why that’s not possible), I would be happy to start
work building from there.

There are few subcases of daemon-level errors:

1A) Invalid request

No feedback is needed. These are protocol violations and well-behaved
clients don't send these.

1B) Request for invalid repository

These should have one error. That error can be sent using ERR response
(already supported).

1C) Request for disabled service

These too can be reported via ERR. One has to be careful not to create
information leak using these.

1D) Catastrophic network error

One can't do anything about these.

1E) Relay error

Really shouldn't happen. Due to service state being unknown at time
of things going wrong, one can't do much about these (what if
relay error occurs in middle of packet? pad packet with zeroes?)


So, pretty much the only daemon-level errors with feedback required
would be one for invalid repository and disabled service. How about:

"foo/example: unreadable or anonymous fetching not allowed."
"foo/example: unreadable or anonymous pushing not allowed."
"foo/example: unreadable or anonymous snapshotting not allowed."
"fooserv: requested service unknown."

And all of these can be sent over ERR. I don't see need for using
sidebands.

That has been the big obstacle for me experimenting with it, more than
the information disclosure.  But this is easy to say.  The doing is
more important.

-Ilari

This is a follow-up to v1.6.1-rc1~101 (connect.c: add a way for
git-daemon to pass an error back to client, 2008-11-01).  Although
not all clients support that protocol extension yet, it should be safe
to start using it anyway, as explained below.

This patch teaches ‘git daemon’ to let the client know when the
request to access the repository was denied.  Instead of just hanging
up, now the server lets the client know that access was denied, with a
carefully worded error that echoes the request:

  fatal: remote error: /foo/example: unreadable or fetching not allowed.
  fatal: remote error: /foo/example: unreadable or pushing not allowed.
  fatal: remote error: /foo/example: unreadable or snapshotting not allowed.

The failure could be due to one of a few causes.  The message does not
distinguish them:

 - chdir() failure
 - the protocol was disabled
 - not a git repository
 - not marked for export

Non-admin clients have no reason to care --- all of these situations
represent the same “not a public repository” condition.  Server
admins, on the other hand, would care a great deal to know that the
daemon does not reveal information about the machine’s configuration
(such as what directories exist) aside from what requests it is
configured to honor.

The corresponding safety for protocol helpers used over ssh has been
in place since v0.99.9k^2~54 (Server-side support for user-relative
paths, 2005-11-17).

The error message used does _not_ match the output from backends
(which is sent to stderr rather than to the client, anyway) when
enter_repo() fails.  That is fine since ‘git daemon’ already checks
that the repository exists by calling enter_repo() itself.

Pre-1.6.1 git versions will treat the error request as a breach of
protocol.  The result is an acceptable if somewhat funny message.

  fatal: protocol error: expected sha/ref, got 'ERR /foo/example:
  unreadable or fetching not allowed.'

Thanks to Dscho, Ilari, and Andreas for help.  Thanks especially to
Ilari for explaining how this should work.

Cc: Johannes Schindelin <redacted>
Cc: Ilari Liusvaara <redacted>
Cc: Andreas Ericsson <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
Ilari Liusvaara wrote:

There are few subcases of daemon-level errors:

[...]

So, pretty much the only daemon-level errors with feedback required
would be one for invalid repository and disabled service. How about:

"foo/example: unreadable or anonymous fetching not allowed."
"foo/example: unreadable or anonymous pushing not allowed."
"foo/example: unreadable or anonymous snapshotting not allowed."
"fooserv: requested service unknown."

And all of these can be sent over ERR. I don't see need for using
sidebands.

I omitted the qualifier “anonymous” in case we want to reuse these
messages for authenticated situations.  I haven’t thought about it
deeply, though.

I would also like to write tests.  To begin with, it might be easiest
to test in inetd mode to avoid allocating a port for it.

Anyway, that shouldn’t hold up giving you a chance to nak the patch. :)

Thanks for the pointers.  ERR packets do seem like the right way to
do this.

Jonathan

 daemon.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/daemon.c b/daemon.c
index a90ab10..732a339 100644
--- a/daemon.c
+++ b/daemon.c

@@ -222,6 +222,7 @@ static char *path_ok(char *directory)
 typedef int (*daemon_service_fn)(void);
 struct daemon_service {
 	const char *name;
+	const char *description;
 	const char *config_name;
 	daemon_service_fn fn;
 	int enabled;

@@ -231,6 +232,12 @@ struct daemon_service {
 static struct daemon_service *service_looking_at;
 static int service_enabled;
 
+static void report_inaccessible(char *dir, struct daemon_service *service)
+{
+	packet_write(1, "ERR %s: unreadable or %s not allowed.\n",
+	             dir, service->description);
+}
+
 static int git_daemon_config(const char *var, const char *value, void *cb)
 {
 	if (!prefixcmp(var, "daemon.") &&

@@ -252,12 +259,15 @@ static int run_service(char *dir, struct daemon_service *service)
 
 	if (!enabled && !service->overridable) {
 		logerror("'%s': service not enabled.", service->name);
+		report_inaccessible(dir, service);
 		errno = EACCES;
 		return -1;
 	}
 
-	if (!(path = path_ok(dir)))
+	if (!(path = path_ok(dir))) {
+		report_inaccessible(dir, service);
 		return -1;
+	}
 
 	/*
 	 * Security on the cheap.

@@ -272,6 +282,7 @@ static int run_service(char *dir, struct daemon_service *service)
 
 	if (!export_all_trees && access("git-daemon-export-ok", F_OK)) {
 		logerror("'%s': repository not exported.", path);
+		report_inaccessible(dir, service);
 		errno = EACCES;
 		return -1;
 	}

@@ -286,6 +297,7 @@ static int run_service(char *dir, struct daemon_service *service)
 	if (!enabled) {
 		logerror("'%s': service not enabled for '%s'",
 			 service->name, path);
+		report_inaccessible(dir, service);
 		errno = EACCES;
 		return -1;
 	}

@@ -362,9 +374,9 @@ static int receive_pack(void)
 }
 
 static struct daemon_service daemon_service[] = {
-	{ "upload-archive", "uploadarch", upload_archive, 0, 1 },
-	{ "upload-pack", "uploadpack", upload_pack, 1, 1 },
-	{ "receive-pack", "receivepack", receive_pack, 0, 1 },
+	{ "upload-archive", "snapshotting", "uploadarch", upload_archive, 0, 1 },
+	{ "upload-pack", "fetching", "uploadpack", upload_pack, 1, 1 },
+	{ "receive-pack", "pushing", "receivepack", receive_pack, 0, 1 },
 };
 
 static void enable_service(const char *name, int ena)

-- 
1.7.1.rc2.8.ga54f9