RFC patchreceive-pack.c: only accept push-cert if push_cert_nonce was advertised

If the server did not advertise the capability to have signed pushes it should not accept signed pushes as stated in Documentation/technical/protocol-capabilities.txt:

    Client will then send a space separated list of capabilities it wants
    to be in effect. The client MUST NOT ask for capabilities the server
    did not say it supports.

    Server MUST diagnose and abort if capabilities it does not understand
    was sent.  Server MUST NOT ignore capabilities that client requested
    and server advertised.  As a consequence of these rules, server MUST
    NOT advertise capabilities it does not understand.

After rereading the second paragraph I think they should also be reworded to

    Server MUST diagnose and abort if capabilities it did not advertise
    was sent.

Suppose there would be hypothetical flaw in the capability of signed pushes (or any capability for the current reasoning) which may harm the server. This would require a bugfix release if it was severe and would put us on time pressure to get it done.

A change like the one proposed would allow us to tell the community to simply configure the server to not advertise the feature and if not advertised the flaw could not be abused.

I am not saying there is a problem now, but I am rather saying patches similar to this one would buy us time in case of problems arising.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    As I discovered the idea while composing the
    atomic push series and the changes line of this
    patch is closeby, this applies on top of
    origin/sb/atomic-push (v12 as sent on Jan. 7th)
    
    This patch is RFC, thinking about security best practice.
    It's not enough to document the intended behavior in
    Documentation/technical/protocol-capabilities.txt, but
    rather enforce it in the code as well.
    
    Any thoughts on that welcome!
    
    Thanks,
    Stefan

 builtin/receive-pack.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Stefan Beller <sbeller@google.com> writes:

Except for s/was sent/was requested/, I think that rule makes sense very much.

This implementation is somewhat questionable.

The server knows how to parse "push-cert" line, knows that what follows after that line up to "push-cert-end" line are shaped very differently from protocol commands outside the push-cert block. In other words, it knows how to parse the request meant for the capable server; it just wants to refuse to serve that request.

The patched code will make it fail by hoping that queue_command() that only handles "40-hex 40-hex ref" will reject the line that begins with "push-cert". Instead of relying on such a hidden dependency, wouldn't it be cleaner to actually parse the push-cert block and then at the end notice and explictly say "Your requests were syntactically correct, but I am not going to honor your request to use the push-cert extension, because I never told you that I'd offer you that capability", instead of rejecting the request with "I was expecting old/new/ref but you sent a line with 'push-cert' on it; what are you talking about?"

Junio C Hamano <gitster@pobox.com> writes:

Note that I said "somewhat" questionable, not "horribly broken", because another part of me wants to say that this implementation may be better than "parse and reject" from security point of view. The pusher cannot tell if the push failed because the server is old to know about the extension, or the server is new but is refusing, by observing the failure.

But the other side of the same coin is that it makes it harder to diagnose the failure.

When the protocol exchange gets to this state, in practice, we know we are talking with somebody who has push privilege into the repository, so revealing a bit more about the server by taking "parse and reject" route may be a reasonable price to give diagnosis that is more useful to help the users in this case, though.

On Fri, Jan 9, 2015 at 2:39 PM, Junio C Hamano <gitster@pobox.com> wrote:

When reading the documentation at first I understood it the way: "If I did not advertise the feature, I am expected to pretend it doesn't even exist". That lead me to the implementation as proposed.

Your proposal to acknowledge the correctness of the message leads to more questions. How would we proceed? We would not accept the featured capability, but we could still do a normal push operation. This would not be desired by a authentic user, so aborting the whole push process would be ok.

I expect such behavior only from malicious clients which actively want to abuse a feature which wasn't advertised, so I wouldn't mind being slightly dishonest to the client and pretend we don't know that capability. But relying on the hidden dependency is bad indeed. So maybe just answer more rudely "I did not advertise one of the capability you requested, go away! (closes connection)".

The push-cert case is special compared to other capabilities as it is quite intrusive to the protocol compared to say "quiet", so more diagnosis may be helpful.

Yeah but what is one repository compared to the whole server? (Just painting the devil on the wall now:) Say you could abuse one capability to gain access to the server or create a huge memory allocation such that the server becomes unresponsive for others.

I know my argument is weak for the "parse and reject route", so I will see if I can adjust the patch to give diagnosis but still reject the un-advertised capability.

Stefan Beller <sbeller@google.com> writes:

How would it fail if we pretend that "push-cert" line had to be old/new/ref line? Failing the same way, but with a better diagnosis, would be sufficient.

Do not assume malice; it is not 2005 anymore. You have to remember that we are mature enough that there are many reimplementations of Git, all of which (us included ;-) start with a buggy version.

Huh? If an auth good enough for one repository allows things to another repository, then I consider that to that other repository the pusher also has push privilege. So what is the problem?

But again, our first version could just be "pretend we do not know anything about push-cert", with discussions on alternative considered in its log message. I do not think it is a blocker to lack the "more helpful diagnosis" feature.

If the server is configured to not advertise push certificates, a push certificate that gets pushed nevertheless will not be fully recorded because push_cert_nonce is NULL.

The recording of GIT_PUSH_CERT_NONCE_STATUS should be dependent on the status being there instead of push_cert_nonce being non NULL.

Without this patch an unsolicited nonce never makes to the stage when being exported with GIT_PUSH_CERT_NONCE_STATUS, because in the unsolicited case push_cert_nonce is always NULL.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/receive-pack.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Stefan Beller <sbeller@google.com> writes:

Sorry, but I do not quite see what you are trying to get at.

When we did not advertise that this feature is supported, we know the incoming nonce is meaningless junk because we didn't supply the correct answer the pusher can give us; we do not even have the correct answer ourselves.

Besides, while reviewing the other patch, didn't we agree that we should reject such a push? Then there is nothing to log anyway, no?

    ... reads the patch and code beyond the context while scratching
    head ...

I notice that the above three lines do not correspond what you did in this patch. The certificate is exported via the blob interface fully with or without this change.

What is not given to the hook is the push-cert-nonce-status. While it is sufficient to tell the hook that we are not getting a good nonce (i.e. the hook does not see GIT_PUSH_CERT_NONCE_STATUS=G), we are not showing that nonce-status is "unsolicited", even though we internally compute and decide that; is that what you are trying to fix?

Still puzzled...

On 09.01.2015 17:52, Junio C Hamano wrote:

Yes we did. This is unrelated to the previous patch. I stuffed it into this thread as I found it was touching the same feature.

I had problems with wording the commit message because I have no expertise with the feature. I am sorry for wasting your time there.

yes that's what I was trying to hint at. The hook would just see it is unsolicited instead of not having the state available.

Stefan Beller <stefanbeller@gmail.com> writes:

Heh, remember, the time spent discussing Git on this list is not a wasted time.

OK.  That makes sort of sense.  So if we:

 1) did not apply either patch (i.e. we accept unsolicited
    certificate, and the fact that we did not exchange "give me this
    nonce" "here is your nonce" is conveyed to the hooks by the lack
    of GIT_PUSH_CERT_NONCE environment and possible presense of
    unsolicited nonce in the GIT_PUSH_CERT blob); and then

 2) applied this patch first (i.e. we still allow unsolicited
    certificate, and the same fact is now conveyed to the hooks also
    by GIT_PUSH_CERT_NONCE_STATUS=UNSOLICITED if they sent a nonce,
    or GIT_PUSH_CERT_NONCE_STATUS=MISSING); and then finally

 3) applied the other patch to reject unsolicited certificate.

then we can stop at any of these three steps and the behaviour of three resulting systems make sense and the pre-receive hook can reject unsolicited certificates if it wants to, even at step #1.

The second step makes it easier for the hook to make that decision because it would make $GIT_PUSH_CERT_NONCE_STATUS the only thing it needs to inspect, instead of checking it and also having to check $GIT_PUSH_CERT_NONCE, which is a simplification [*1*].

And then in the third step, the hook does not even have to worry, which makes what #2 does more or less pointless.

This patch is still a good thing to do from the "correctness" point of view; the current code may accept certificates without nonce only because in an earlier iteration of the protocol design, the nonce was optional and the code the earlier patch fixes is a remnant of that. As we do not advertise push-cert without nonce in the final and current protocol, there is no reason to be loose anymore.

[Footnote]

*1* A hypothetical naive hook implementation:

	case "$GIT_PUSH_CERT_NONCE_STATUS" in
        OK | SLOP)
        	: good and happy
                exit 0;;
	*)
        	: bad and unhappy
                exit 1;;
	esac

would diagnose an unsolicited certificate with or without nonce as bad. Though it cannot tell between unsolicited and missing cases, it would not be so serious a defect in practice.

On Mon, Jan 12, 2015 at 11:07 AM, Junio C Hamano <gitster@pobox.com> wrote:

I do not quite understand the intention of your mail. Are you saying I should make a patch series to solve the problems as outlined above?

Stefan Beller <sbeller@google.com> writes:

Not really. All I was saying was that a hypothetical patch series that progressed in the order above would "make sort-of sense".

I was hoping that readers would ask themselves this question: if we know that our endgame will be #3, then does it still make much sense to have the state that only patch #2 is applied?

I think #3 makes #2 unnecessary, as we always ask for nonce when advertising push-cert capability in the released versions of Git, so we might get an unsolicited push-cert (which #3 will reject), but we will never see an unsolicited nonce in a push-cert, as long as we told the pusher that it is OK to send a push-cert to us.