Hi I have reviewed the changes and it looks good. I'll see if I can get some time to perform any relevant tests too.
// Ola On Sat, 15 May 2021 at 23:34, Sylvain Beucler <[email protected]> wrote: > Hi Ola, > > You can check the LTS version at: > https://www.beuc.net/tmp/debian-lts/curl/ > > I followed the method from Ubuntu and SUSE and backported the URL API > for LTS and ELTS, plus the new test case for the CVE. > > I'm currently diffing the test suite results, cf. my notes at: > https://wiki.debian.org/LTS/TestSuites/curl > > Cheers! > Sylvain > > On 15/05/2021 23:22, Ola Lundqvist wrote: > > Hi Sylvain > > > > Great! Let me know if you want help with review, testing or something > else. > > > > // Ola > > > > On Sat, 15 May 2021 at 23:18, Sylvain Beucler <[email protected] > > <mailto:[email protected]>> wrote: > > > > Hi, > > > > I claimed it yesterday and my work is mostly done. > > > > Cheers! > > Sylvain > > > > On 15/05/2021 23:11, Ola Lundqvist wrote: > > > Hi Utkarsh > > > > > > I have looked into your patch and I think it looks good. I do not > > fully > > > understand why all the changes in url.c were done but I think it > > looks > > > fine anyway. > > > The risk of regression should be small. > > > > > > Do you want me to do the update, or do you want to do it yourself? > > > Or do you think we should ignore it? > > > > > > Best regards > > > > > > // Ola > > > > > > On Thu, 8 Apr 2021 at 22:33, Ola Lundqvist <[email protected] > > <mailto:[email protected]> > > > <mailto:[email protected] <mailto:[email protected]>>> wrote: > > > > > > Hi Utkarsh, all > > > > > > I have done some more investigation on this matter. I have > > checked > > > the statement from upstream that we can re-use some existing > > strip > > > code to remove the strings this way. > > > The thing is that I cannot find any code that do URL > stripping so > > > that is not really a viable option. This leaves only the two > > options > > > you have already stated. > > > > > > Either we ignore, or we port the entire URL API. > > > > > > I think the risk of regression is rather small if we port it, > > > because this is only used in this place. Assuming there is no > > name > > > clash introduced. > > > > > > So what do you all think? Ignore or fix? > > > There are good arguments for both. > > > > > > Ignore is ok because this only happens with a specific > > command line > > > option, and even if used the risk of problem is quite small. > > > > > > On the other hand curl is a very common tool which means that > it > > > could be worth fixing even small issues. > > > > > > I think both are ok. > > > > > > Best regards > > > > > > // Ola > > > > > > On Wed, 7 Apr 2021 at 23:07, Ola Lundqvist <[email protected] > > <mailto:[email protected]> > > > <mailto:[email protected] <mailto:[email protected]>>> wrote: > > > > > > Hi Utkarsh, all > > > > > > After reading the description of this CVE again I realize > > that I > > > misunderstood the description last time. > > > > > > The problem is that the "referrer" header is not stripped. > > > > > > This changes my conclusion to some extent. > > > > > > I see no problem with fixing this issue from a regression > > point > > > of view (apart from what has already been expressed). > > > The amount of services that rely on the referrer field > > should be > > > small, if any. > > > > > > I still think we can ignore it though with the > > same reasoing as > > > I expressed in the last email. The problem should be > minor. > > > There are other worse problem by providing sensitive data > > in the > > > URL. > > > And again if the attacker can make a redirect, the > > attacker can > > > most likely get the URL anyway so then nothing has leaked. > > > > > > Cheers > > > > > > // Ola > > > > > > > > > // Ola > > > > > > On Wed, 7 Apr 2021 at 13:19, Ola Lundqvist > > <[email protected] <mailto:[email protected]> > > > <mailto:[email protected] <mailto:[email protected]>>> wrote: > > > > > > Hi Utkarsh, all > > > > > > Is this even a vulnerability? > > > The problem is that authentication information is not > > > stripped if the browser is redirected to another > place. > > > > > > If you trust a site enough to provide authentication > > data, I > > > guess you also trust that if that site happens to be > > > relocated you should also trust the new place. > > > I mean if the attacker has the power to redirect I > expect > > > that it has the power to read the authentication data > > > anyway. There could be cases when this is not the > > case, but > > > in general it should not be possible for the attacker > to > > > redirect without also having more power. > > > > > > We could of course consider to apply this fix, but it > > > certainly will cause a regression since my > expectation is > > > that authentication information is forwarded. > > > > > > I think it should be ignored. If we fix it, it should > be > > > with a configuration option, but I think that is a > > > little too intrusive for (E)LTS. > > > > > > Or have I missed something? > > > > > > Best regards > > > > > > // Ola > > > > > > On Mon, 5 Apr 2021 at 02:20, Utkarsh Gupta > > > <[email protected] <mailto:[email protected]> > > <mailto:[email protected] <mailto:[email protected]>>> wrote: > > > > > > Hello, > > > > > > [CCing the Security team in case they have some > > ideas or > > > suggestions > > > for CVE-2021-22876/curl] > > > > > > Whilst triaging and looking thoroughly for this > CVE, > > > affecting curl, I > > > noticed that the upstream patch uses elements > > like CURLU, > > > CURLUPART_{URL,FRAGMENT,USER,PASSWORD}. This > > comes from > > > the URL API > > > which seems to be missing in both, stretch and > > jessie. > > > > > > There seem to be two plausible options at this > point: > > > > > > 1. Given that this CVE has been assigned low > > severity by > > > upstream, we > > > could perhaps mark this as no-dsa or ignored, > with an > > > appropriate > > > comment; or > > > > > > 2. Backport the entire URL API (patch for that is > > > attached; is > > > intrusive) and then apply the fix for > CVE-2021-22876 > > > (patch attached) > > > on top of that. Whilst this option makes sense, > but > > > backporting the > > > entire URL API could have an unforeseen effect (or > > > chances of > > > potential regressions) and in any case, looks > > somewhat > > > intrusive. > > > > > > So for now, I've added curl to dla-needed and > > ela-needed > > > but if we > > > decide to mark this as no-dsa or ignored, we could > > > simply drop this > > > from there as this is the only CVE that needs > > working on. > > > > > > Let me know what y'all think. > -- --- Inguza Technology AB --- MSc in Information Technology ---- | [email protected] [email protected] | | http://inguza.com/ Mobile: +46 (0)70-332 1551 | ---------------------------------------------------------------
