Date: Thu, 1 Jul 2010 14:47:10 +0100 From: Tom Evans <tevans.uk@googlemail.com> To: =?UTF-8?Q?Dag=2DErling_Sm=C3=B8rgrav?= <des@des.no> Cc: freebsd-current <freebsd-current@freebsd.org> Subject: Re: Ports doesnt respect fetch environment settings Message-ID: <AANLkTikrYfbaKMgruYxOlX5FJGo2NZiaVUkX3nfhuAZu@mail.gmail.com> In-Reply-To: <86pqz8mkcg.fsf@ds4.des.no> References: <AANLkTinn5bPXDf-tRIlpGCp3iFgtYi20mzRSbqkBcj6b@mail.gmail.com> <20100621101046.GA76036@droso.net> <AANLkTilhgzVDFX9MGQTS_DzJt_5QtlaNF--nQss-5mzj@mail.gmail.com> <20100621130743.4df77343@ernst.jennejohn.org> <AANLkTikrXvx_3Y7nkceaKSYqLQemGrIBURt1-5pwl6qr@mail.gmail.com> <AANLkTinxzijyXsAQIdf75cw-GeRH_Wd1E8zIYmU-Xlfd@mail.gmail.com> <86pqz8mkcg.fsf@ds4.des.no>
next in thread | previous in thread | raw e-mail | index | archive | help
--0015174c3774ff07fa048a53b4fb Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 2010/6/30 Dag-Erling Sm=C3=B8rgrav <des@des.no>: > Tom Evans <tevans.uk@googlemail.com> writes: >> Sorry to bump this again. I've diluted this issue down to the core >> points and raised as a pr - can someone take a look, see if my >> solution is correct and commit if appropriate? > > Sorry, fell through the cracks. =C2=A0Your analysis is correct, but I'm n= ot > 100% sure about the patch. =C2=A0Can you verify that your change does not > introduce the possibility of an infinite loop in edge cases? =C2=A0I don'= t > *think* it does, but like I said, I'm not 100% sure and I don't have > time to reacquaint myself with the code right now, especially not that > particularly nasty part of it :) > > DES > -- > Dag-Erling Sm=C3=B8rgrav - des@des.no > Like you said, it's quite a large chunk of code to understand. I think you might be right, there could be a situation where a misbehaving proxy server could make it loop. The process is like this: http_auth_challenges_t proxy_challenges struct initialized (line 1497). The flag 'valid' on the struct is initialized to 0 (line 656) We enter the loop for the first time. We dont add proxy auth the first time through the loop, as 'valid' flag not set (line 1586) We receive the reply and switch on the response code (line 1676) If we receive HTTP_NEED_PROXY_AUTH, and the 'valid' flag is not set (implying we didn't send creds), we simply continue to execute the loop (lines 1703 - 1716). This is where the patch I supplied increments the maximum loop counter. The loop now processes the received headers, and when it receives the 'Proxy-Authenticate' header, it calls http_parse_authenticate with proxy_challenges (line 1795) This then populates the proxy_challenges struct, setting the valid flag. Having processed the headers, the loop then checks that receiving a HTTP_NEED_PROXY_AUTH response has resulted in us now having valid flag set in proxy_challenges, otherwise we goto ouch (out of the loop) (line 1806). The loop ends, and we go through again. I thought for a moment while tracing it through that if a misbehaving proxy server sent a 407 response, but did not add a Proxy-Authenticate header, then we could increase the loop counter but without adding any proxy auth, which would mean an infinite loop. However, there is already that sanity check at the end of the loop to check that if we received a proxy authentication error, and still dont have credentials to supply, then we quickly jump out of the loop. I guess that is a little strange, that we could supply proxy auth credentials, but because the server didnt ask for them correctly, we didnt give them - although that would be quite broken behaviour on the part of the proxy server. I think this does show that the patch could be made a little better. We only want to go through the loop one more time if we have credentials to send, and we have credentials to send if the rv of http_parse_authenticate is good. I also think the same bug would affect fetching from servers requiring authentication, so I've made the same fix there as well. New patch attached Cheers Tom --0015174c3774ff07fa048a53b4fb Content-Type: text/plain; charset=US-ASCII; name="lib::libfetch::http.c.diff.txt" Content-Disposition: attachment; filename="lib::libfetch::http.c.diff.txt" Content-Transfer-Encoding: base64 X-Attachment-Id: f_gb3nfay00 SW5kZXg6IGh0dHAuYwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09ClJDUyBmaWxlOiAvaG9tZS9uY3ZzL3NyYy9saWIvbGli ZmV0Y2gvaHR0cC5jLHYKcmV0cmlldmluZyByZXZpc2lvbiAxLjc4LjIuNQpkaWZmIC11IC1yMS43 OC4yLjUgaHR0cC5jCi0tLSBodHRwLmMJMjcgSmFuIDIwMTAgMTQ6NTQ6NDggLTAwMDAJMS43OC4y LjUKKysrIGh0dHAuYwkxIEp1bCAyMDEwIDEzOjQ1OjA2IC0wMDAwCkBAIC0xNzg2LDEyICsxNzg2 LDE0IEBACiAJCQljYXNlIGhkcl93d3dfYXV0aGVudGljYXRlOgogCQkJCWlmIChjb25uLT5lcnIg IT0gSFRUUF9ORUVEX0FVVEgpCiAJCQkJCWJyZWFrOwotCQkJCWh0dHBfcGFyc2VfYXV0aGVudGlj YXRlKHAsICZzZXJ2ZXJfY2hhbGxlbmdlcyk7CisJCQkJaWYgKGh0dHBfcGFyc2VfYXV0aGVudGlj YXRlKHAsICZzZXJ2ZXJfY2hhbGxlbmdlcykpCisJCQkJCSsrbjsKIAkJCQlicmVhazsKIAkJCWNh c2UgaGRyX3Byb3h5X2F1dGhlbnRpY2F0ZToKIAkJCQlpZiAoY29ubi0+ZXJyICE9IEhUVFBfTkVF RF9QUk9YWV9BVVRIKQogCQkJCQlicmVhazsKLQkJCQlodHRwX3BhcnNlX2F1dGhlbnRpY2F0ZShw LCAmcHJveHlfY2hhbGxlbmdlcyk7CisJCQkJaWYgKGh0dHBfcGFyc2VfYXV0aGVudGljYXRlKHAs ICZwcm94eV9jaGFsbGVuZ2VzKSA9PSAwKTsKKwkJCQkJKytuOwogCQkJCWJyZWFrOwogCQkJY2Fz ZSBoZHJfZW5kOgogCQkJCS8qIGZhbGwgdGhyb3VnaCAqLwo= --0015174c3774ff07fa048a53b4fb--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTikrYfbaKMgruYxOlX5FJGo2NZiaVUkX3nfhuAZu>