2010/6/30 Dag-Erling Smørgrav <des_at_des.no>: > Tom Evans <tevans.uk_at_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. Your analysis is correct, but I'm not > 100% sure about the patch. Can you verify that your change does not > introduce the possibility of an infinite loop in edge cases? I 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ørgrav - des_at_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
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:05 UTC