[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: git-annex security update ready for testing and review

Antoine Beaupré wrote:
> It was challenging work, especially for the two main patches.
> The first big patch is "limit url downloads to whitelisted schemes":
> http://source.git-annex.branchable.com/?p=source.git;a=commitdiff;h=28720c795ff57a55b48e56d15f9b6bcb977f48d9
> The main challenge with this patch is that `getUrlInfo` is quite different
> in 5.x: it's called `exists` and has a somewhat different structure. I
> think I've been able to port it correctly, but reviewers will want to
> pay special attention to that function in Utility/Url.hs.

It looks ok.

> More concerning are chunks affecting code which does a
> `verifyKeyContent`. There is no equivalent in 5.x and I am worried this
> was used to also check for bad URLs as a side-effect. More critically,
> I'm concerned that the lack of such infrastructure makes it impossible
> to fix CVE-2018-10859 in the 5.x branch without backporting a
> significant part of that work. It seems to me the patchset I have now
> does not actually have provisions to restrict restriction to certain
> content.

I agree, verifyKeyContent is an essential part of the security fix.
As described in

  CVE-2018-10859 was fixed by making git-annex refuse to download encrypted
  content from special remotes, unless it knows the hash of the expected
  content. When the attacker provides some other gpg encrypted content, it
  will fail the hash check and be discarded.

So I think that backporting 2fb3722ce993f01adb3ea9d5f8855c3e6a99c249 is a
prerequisite of your backport, though whether you'd want to expose the 
annex.verify setting is up to you.

> I am worried about Get.hs, where I had to hardcode a
> RetrievalAllKeysSecure policy in case no remote is specified, since I
> couldn't figure out how to find the remote at that level. I similarly
> shot from the hip, in the dark, with Sync.hs, unfortunately.

I can confirm you left it insecure.  Take the Command/Get.hs and
Command/Sync.hs hunks from 2fb3722ce993f01adb3ea9d5f8855c3e6a99c249
which move the getViaTmp into a scope where you have a Remote to

see shy jo

Attachment: signature.asc
Description: PGP signature

Reply to: