[PATCH] http-push: trim trailing newline from remote symref

[PATCH] http-push: trim trailing newline from remote symref

[Jeff King]

When we fetch a symbolic ref file from the remote, we get
the whole string "ref: refs/heads/master\n", recognize it by
skipping past the "ref: ", and store the rest. We should
chomp the trailing newline.

This bug was introduced in ae021d8 (use skip_prefix to avoid
magic numbers, 2014-06-18), which did not notice that the
length computation fed to xmemdupz was quietly tweaked by 1
to account for this.

We can solve it by explicitly trimming the newline, which is
more obvious. Note that we use strbuf_rtrim here, which will
actually cut off any trailing whitespace, not just a single
newline. This is a good thing, though, as it makes our
parsing more liberal (and spaces are not valid in refnames
anyway).

Signed-off-by: Jeff King <peff@peff.net>
---
This is a regression in v2.1.0.

It was causing t5540 to fail, but I realized I have been building with
NO_EXPAT for a while, so I didn't notice. Frankly, I'm kind of surprised
and disturbed that nobody noticed it before now. More evidence that we
can kill off dumb http-push? I would have thought somebody else would
have noticed the test failure, though.

I am embarrassed to have introduced the bug during a refactoring patch.
But in my defense, the original code was quite subtle and horrible, and
I think the end result at least is much obvious (and is a good point in
favor of skip_prefix's existence!). The original came from eecc836
(Another memory overrun in http-push.c, 2007-03-01). Looking at that
patch, I can't understand how the code before it ever worked in the
first place.

 http-push.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/http-push.c b/http-push.c
index 26dfa67..184d24a 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1578,6 +1578,9 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1)
 	if (buffer.len == 0)
 		return;
 
+	/* Cut off trailing newline. */
+	strbuf_rtrim(&buffer);
+
 	/* If it's a symref, set the refname; otherwise try for a sha1 */
 	if (skip_prefix(buffer.buf, "ref: ", &name)) {
 		*symref = xmemdupz(name, buffer.len - (name - buffer.buf));
-- 
2.2.1.425.g441bb3c

Re: [PATCH] http-push: trim trailing newline from remote symref

[Kyle J. McKay]

On Jan 12, 2015, at 18:28, Jeff King wrote:

> When we fetch a symbolic ref file from the remote, we get
> the whole string "ref: refs/heads/master\n", recognize it by
> skipping past the "ref: ", and store the rest. We should
> chomp the trailing newline.
> [..]
> This is a regression in v2.1.0.
>
> It was causing t5540 to fail, but I realized I have been building with
> NO_EXPAT for a while, so I didn't notice. Frankly, I'm kind of  
> surprised
> and disturbed that nobody noticed it before now. More evidence that we
> can kill off dumb http-push? I would have thought somebody else would
> have noticed the test failure, though.

I have this line in my 2.1.4 test output log:

t5540-http-push-webdav.sh .......................... ok

And again in my 2.2.2 test output log:

t5540-http-push-webdav.sh .......................... ok

Running the 2.2.2 version with -v I get:

t5540-http-push-webdav.sh ..
ok 1 - setup remote repository
ok 2 - create password-protected repository
ok 3 - setup askpass helper
ok 4 - clone remote repository
ok 5 - push to remote repository with packed refs
ok 6 - push already up-to-date
ok 7 - push to remote repository with unpacked refs
ok 8 - http-push fetches unpacked objects
ok 9 - http-push fetches packed objects
ok 10 - create and delete remote branch
ok 11 - MKCOL sends directory names with trailing slashes
ok 12 - PUT and MOVE sends object to URLs with SHA-1 hash suffix
ok 13 - non-fast-forward push fails
ok 14 - non-fast-forward push show ref status
ok 15 - non-fast-forward push shows help message
not ok 16 - force with lease aka cas # TODO known breakage
ok 17 - push to password-protected repository (user in URL)
not ok 18 - user was prompted only once for password # TODO known  
breakage
not ok 19 - push to password-protected repository (no user in URL) #  
TODO known breakage
# still have 3 known breakage(s)
# passed all remaining 16 test(s)
1..19
ok

I also get the same results using v2.3.0-rc0.

I do not build with NO_EXPAT.  This is running the tests on OS X  
without this patch applied.  Is something else required to get a  
failure?

-Kyle

Re: [PATCH] http-push: trim trailing newline from remote symref

[Jeff King]

On Tue, Jan 13, 2015 at 08:26:31AM -0800, Kyle J. McKay wrote:

> I have this line in my 2.1.4 test output log:
> 
> t5540-http-push-webdav.sh .......................... ok
> [...]
> I do not build with NO_EXPAT.  This is running the tests on OS X without
> this patch applied.  Is something else required to get a failure?

Hmm. I think it is probably this:

  http://curl.haxx.se/docs/adv_20150108B.html

where curl has started complaining about URLs with newlines in them. So
ae021d8 did introduce a bug, but older versions of curl did not really
care. The combination of ae021d8 with a new version of curl triggers the
problem.

And that also explains why it worked prior to eecc8367f4; curl was more
forgiving. Also, interestingly, if you "git log -S'- 6' http-push.c",
you can see the exact same bug reappear and go away in 2006/2007. The
implicit "chop one character" behavior is there in the original
3dfaf7bc, adding http-push support. Then it disappears as a side effect
of bfbd0bb6, and then comes back again in eecc8367.

Anyway. I think my patch is still the right thing. But that does explain
why we didn't notice the test failure.

-Peff

Re: [PATCH] http-push: trim trailing newline from remote symref

[Jeff King]

On Mon, Jan 12, 2015 at 09:28:58PM -0500, Jeff King wrote:

> When we fetch a symbolic ref file from the remote, we get
> the whole string "ref: refs/heads/master\n", recognize it by
> skipping past the "ref: ", and store the rest. We should
> chomp the trailing newline.
> 
> This bug was introduced in ae021d8 (use skip_prefix to avoid
> magic numbers, 2014-06-18), which did not notice that the
> length computation fed to xmemdupz was quietly tweaked by 1
> to account for this.
> 
> We can solve it by explicitly trimming the newline, which is
> more obvious. Note that we use strbuf_rtrim here, which will
> actually cut off any trailing whitespace, not just a single
> newline. This is a good thing, though, as it makes our
> parsing more liberal (and spaces are not valid in refnames
> anyway).

While looking into Kyle's earlier response, I found that there is a
semi-duplicate of this function for the http-walker side:
http_fetch_ref. It already uses strbuf_rtrim, so I feel doubly good
about moving to its use here.

Almost certainly this duplicated functionality could be factored out. I
have very little interest in spending time cleaning up the http-push
code, though.

-Peff

Re: [PATCH] http-push: trim trailing newline from remote symref

[Kyle J. McKay]

On Jan 13, 2015, at 11:58, Jeff King wrote:
> On Tue, Jan 13, 2015 at 08:26:31AM -0800, Kyle J. McKay wrote:
>
>> I have this line in my 2.1.4 test output log:
>>
>> t5540-http-push-webdav.sh .......................... ok
>> [...]
>> I do not build with NO_EXPAT.  This is running the tests on OS X  
>> without
>> this patch applied.  Is something else required to get a failure?
>
> Hmm. I think it is probably this:
>
>  http://curl.haxx.se/docs/adv_20150108B.html
>
> where curl has started complaining about URLs with newlines in them.  
> So
> ae021d8 did introduce a bug, but older versions of curl did not really
> care. The combination of ae021d8 with a new version of curl triggers  
> the
> problem.

I'm running curl 7.38, and in this context "older" is anything before  
7.40, so that would explain it.  curl 7.38 was released 2014-09-10, so  
it's only 4 months old at this point.  7.40 was only released 5 days  
ago on 2015-01-08 which is probably why there have not been a whole  
lot of reports about this so far.

> And that also explains why it worked prior to eecc8367f4; curl was  
> more
> forgiving. Also, interestingly, if you "git log -S'- 6' http-push.c",
> you can see the exact same bug reappear and go away in 2006/2007. The
> implicit "chop one character" behavior is there in the original
> 3dfaf7bc, adding http-push support. Then it disappears as a side  
> effect
> of bfbd0bb6, and then comes back again in eecc8367.

After updating to curl 7.40 I get:

t5540-http-push-webdav.sh (Wstat: 256 Tests: 19 Failed: 1)
   Failed test:  10
   Non-zero exit status: 1

> Anyway. I think my patch is still the right thing. But that does  
> explain
> why we didn't notice the test failure.

And then after applying your patch I'm back to:

t5540-http-push-webdav.sh .. ok

So definitely a needed patch.

Tested-by: Kyle J. McKay <mackyle@gmail.com>

-Kyle

Re: [PATCH] http-push: trim trailing newline from remote symref

[Junio C Hamano]

"Kyle J. McKay" <mackyle@gmail.com> writes:

> On Jan 13, 2015, at 11:58, Jeff King wrote:
> ...
> I'm running curl 7.38, and in this context "older" is anything before
> 7.40, so that would explain it.  curl 7.38 was released 2014-09-10, so
> it's only 4 months old at this point.  7.40 was only released 5 days
> ago on 2015-01-08 which is probably why there have not been a whole
> lot of reports about this so far.
>
> After updating to curl 7.40 I get:
>
> t5540-http-push-webdav.sh (Wstat: 256 Tests: 19 Failed: 1)
>   Failed test:  10
>   Non-zero exit status: 1
>
>> Anyway. I think my patch is still the right thing. But that does
>> explain
>> why we didn't notice the test failure.
>
> And then after applying your patch I'm back to:
>
> t5540-http-push-webdav.sh .. ok

I see a Ubuntu box I have nearby has this:

curl (7.35.0-1ubuntu2.3) trusty-security; urgency=medium

  * SECURITY UPDATE: URL request injection
    - debian/patches/CVE-2014-8150.patch: drop bad chars from URL in
      lib/url.c, added test to tests/data/Makefile.am, tests/data/test1529,
      tests/libtest/Makefile.inc, tests/libtest/lib1529.c.
    - CVE-2014-8150

 -- Marc Deslauriers <marc.deslauriers@ubuntu.com>  Wed, 14 Jan 2015 08:49:32 -0500

That explains why I started seeing the same on a box with 7.35.x
which looks older than 7.40.