| Message ID | 20241115005404.3747302-1-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | Restore the ability to clone repositories owned by another user | expand |
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > For a long time, we've told users that the only safe way to operate on > an untrusted repository is to clone or fetch from it. We've even > mentioned this policy in a variety of places in our documentation. > > However, f4aa8c8bb1 ("fetch/clone: detect dubious ownership of local > repositories", 2024-04-10), this changed in an attempt to make things > more secure. That broke a lot of user use cases, which have been > reported to the list. > > Because our security model hasn't changed and it's still safe to clone > or fetch from an untrusted repository, let's revert a portion of that > change to allow us to clone and fetch from repositories owned by a > different user. If a malicious repository were a problem for > upload-pack, that would probably also be exploitable on major forges, > and if it were a problem on the client side, then we'd also have a > problem with untrusted HTTPS remotes, so we're not really adding any > security risk here. Nice. Better late than never. > This matter was discussed extensively in the thread starting at > https://lore.kernel.org/git/ZqUc8DJ1uKcHYlcy@imp.flyn.org/. > Note that I haven't signed off on this patch because it's based on one > from Junio and I haven't gotten his sign-off yet. It's fine to add mine > once he's added his. You can forge my sign-off on the old patch ;-) The proposed commit log of the patch makes me wonder what should happen when neither of the two bits is set. Not strict, but we do not allow ourselves to enter a random repo owned by a stranger. It turns out that "strict" has nothing to do with this lifting of excess ownership check, but the dwimming done by suffixing .git, etc. to the given pathnames, so there is nothing strange going on. Thanks.
On 2024-11-15 at 01:14:48, Junio C Hamano wrote: > "brian m. carlson" <sandals@crustytoothpaste.net> writes: > > > For a long time, we've told users that the only safe way to operate on > > an untrusted repository is to clone or fetch from it. We've even > > mentioned this policy in a variety of places in our documentation. > > > > However, f4aa8c8bb1 ("fetch/clone: detect dubious ownership of local > > repositories", 2024-04-10), this changed in an attempt to make things > > more secure. That broke a lot of user use cases, which have been > > reported to the list. > > > > Because our security model hasn't changed and it's still safe to clone > > or fetch from an untrusted repository, let's revert a portion of that > > change to allow us to clone and fetch from repositories owned by a > > different user. If a malicious repository were a problem for > > upload-pack, that would probably also be exploitable on major forges, > > and if it were a problem on the client side, then we'd also have a > > problem with untrusted HTTPS remotes, so we're not really adding any > > security risk here. > > Nice. Better late than never. Yeah, I had intended to get to this sooner, but I got busy with other things, and nobody got to it before me. I had some time so I thought I'd send this out now so we can minimize the number of affected versions. I really appreciate you writing up the original patch for this; it was very helpful and a great start. > > Note that I haven't signed off on this patch because it's based on one > > from Junio and I haven't gotten his sign-off yet. It's fine to add mine > > once he's added his. > > You can forge my sign-off on the old patch ;-) Great. I suspect you'll probably pick this up as-is, but I've added both our sign-offs in case we need a v2. Let me know if you'd prefer me to send out the unmodified v2, and I can do that. > The proposed commit log of the patch makes me wonder what should > happen when neither of the two bits is set. Not strict, but we do > not allow ourselves to enter a random repo owned by a stranger. It > turns out that "strict" has nothing to do with this lifting of > excess ownership check, but the dwimming done by suffixing .git, > etc. to the given pathnames, so there is nothing strange going on. Exactly.
For a long time, we've told users that the only safe way to operate on an untrusted repository is to clone or fetch from it. We've even mentioned this policy in a variety of places in our documentation. However, f4aa8c8bb1 ("fetch/clone: detect dubious ownership of local repositories", 2024-04-10), this changed in an attempt to make things more secure. That broke a lot of user use cases, which have been reported to the list. Because our security model hasn't changed and it's still safe to clone or fetch from an untrusted repository, let's revert a portion of that change to allow us to clone and fetch from repositories owned by a different user. If a malicious repository were a problem for upload-pack, that would probably also be exploitable on major forges, and if it were a problem on the client side, then we'd also have a problem with untrusted HTTPS remotes, so we're not really adding any security risk here. This matter was discussed extensively in the thread starting at https://lore.kernel.org/git/ZqUc8DJ1uKcHYlcy@imp.flyn.org/. Note that I haven't signed off on this patch because it's based on one from Junio and I haven't gotten his sign-off yet. It's fine to add mine once he's added his. brian m. carlson (1): Allow cloning from repositories owned by another user Documentation/git-clone.txt | 9 +++++++++ builtin/upload-pack.c | 5 ++++- daemon.c | 6 ++++-- path.c | 10 ++++++---- path.h | 17 ++++++++++++++++- t/t0411-clone-from-partial.sh | 3 --- t/t5605-clone-local.sh | 10 ++++++++++ 7 files changed, 49 insertions(+), 11 deletions(-)