All of lore.kernel.org
 help / color / mirror / Atom feed
* [pseudo][PATCH 1/2] xattr: Fixed corrupting UID&GID when running setfacl -m on a directory
@ 2020-06-26  9:40 johannes.beisswenger
  2020-06-26 14:05 ` [OE-core] " Seebs
  0 siblings, 1 reply; 6+ messages in thread
From: johannes.beisswenger @ 2020-06-26  9:40 UTC (permalink / raw)
  To: openembedded-core; +Cc: Johannes Beisswenger

The file mode was accidentally overwritten with only the permission
bits, causing the server to falsely assume that the database was
corrupted (because the msg_header.mode did not contain S_IFDIR
anymore) even though it was the client doing the corruption.
In practice that had the effect of leaking the UID of the user, into
the pseudo environment.

This fixes Bug 13959 -- https://bugzilla.yoctoproject.org/show_bug.cgi?id=13959

Signed-off-by: Johannes Beisswenger <johannes.beisswenger@cetitec.com>
---
 ports/linux/xattr/pseudo_wrappers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ports/linux/xattr/pseudo_wrappers.c b/ports/linux/xattr/pseudo_wrappers.c
index 590af30..3e122d9 100644
--- a/ports/linux/xattr/pseudo_wrappers.c
+++ b/ports/linux/xattr/pseudo_wrappers.c
@@ -197,7 +197,7 @@ static int shared_setxattr(const char *path, int fd, const char *name, const voi
 			mode |= get_special_bits(path, fd);
 			pseudo_debug(PDBGF_XATTR, "posix_acl_access translated to mode %04o. Remaining attribute(s): %d.\n",
 				mode, extra);
-			buf.st_mode = mode;
+
 			/* we want to actually issue a corresponding chmod,
 			 * as well, or else the file ends up 0600 on the
 			 * host. Using the slightly-less-efficient wrap_chmod
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [OE-core] [pseudo][PATCH 1/2] xattr: Fixed corrupting UID&GID when running setfacl -m on a directory
  2020-06-26  9:40 [pseudo][PATCH 1/2] xattr: Fixed corrupting UID&GID when running setfacl -m on a directory johannes.beisswenger
@ 2020-06-26 14:05 ` Seebs
  2020-06-26 14:56   ` Johannes Beisswenger
  0 siblings, 1 reply; 6+ messages in thread
From: Seebs @ 2020-06-26 14:05 UTC (permalink / raw)
  To: Johannes Beisswenger; +Cc: openembedded-core

On Fri, 26 Jun 2020 11:40:31 +0200
"Johannes Beisswenger" <johannes.beisswenger@cetitec.com> wrote:

> The file mode was accidentally overwritten with only the permission
> bits, causing the server to falsely assume that the database was
> corrupted (because the msg_header.mode did not contain S_IFDIR
> anymore) even though it was the client doing the corruption.
> In practice that had the effect of leaking the UID of the user, into
> the pseudo environment.

Good catch, but we should still be masking in the permissions bits, we
just shouldn't be overwriting the non-permissions bits, I think?

-s

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [OE-core] [pseudo][PATCH 1/2] xattr: Fixed corrupting UID&GID when running setfacl -m on a directory
  2020-06-26 14:05 ` [OE-core] " Seebs
@ 2020-06-26 14:56   ` Johannes Beisswenger
  2020-06-26 15:36     ` Seebs
  0 siblings, 1 reply; 6+ messages in thread
From: Johannes Beisswenger @ 2020-06-26 14:56 UTC (permalink / raw)
  To: Seebs; +Cc: openembedded-core

Thanks for the fast reply!

> Good catch, but we should still be masking in the permissions bits, we
> just shouldn't be overwriting the non-permissions bits, I think?

The OP_CREATE/REPLACE/SET_XATTR operations in pseudo_op() don't use 
msg_header.mode at all, it is only used for DB sanity checks, which in 
turn only use the non-permission bits.
So it certainly does not hurt to add them back in, and thus might be the 
proper thing to do.
Not knowing the code base I simply was being conservative and left the 
original mode unaltered.
I can send an updated patch on Monday.

On a different note, the shared_setxattr() code for the darwin port is 
very similar and also overwrites buf.st_mode and thus is also affected 
by this buf. In addition to that it is also missing the changes 
introduced by this commit 
http://git.yoctoproject.org/cgit/cgit.cgi/pseudo/commit/?id=cc6b2008646318c01153d39fef191da3e35a5fc8, 
since I don't have access to a darwin system to test any changes I left 
the port alone, but someone should look into this.

--
Johannes Beisswenger

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [OE-core] [pseudo][PATCH 1/2] xattr: Fixed corrupting UID&GID when running setfacl -m on a directory
  2020-06-26 14:56   ` Johannes Beisswenger
@ 2020-06-26 15:36     ` Seebs
  2020-06-26 17:33       ` Richard Purdie
  0 siblings, 1 reply; 6+ messages in thread
From: Seebs @ 2020-06-26 15:36 UTC (permalink / raw)
  To: Johannes Beisswenger; +Cc: openembedded-core

On Fri, 26 Jun 2020 16:56:48 +0200
"Johannes Beisswenger" <johannes.beisswenger@cetitec.com> wrote:

> The OP_CREATE/REPLACE/SET_XATTR operations in pseudo_op() don't use 
> msg_header.mode at all, it is only used for DB sanity checks, which
> in turn only use the non-permission bits.

Right, the relevant part is the immediately following
wrap_chmod/wrap_fchmod. I had forgotten about this chunk of code.

And... that leads us to the realization of how it is possible that this
ever didn't bite us before.

The historical reason this exists is that at some point, GNU tar
decided that it was a crime against nature, humanity, and God to use a
stable existing API when a new API existed which could do the same
thing at hundreds of times the computational cost, and abandoned all
use of chmod() in circumstances when extended attributes are available.
But in practice, this only happens for regular filesystem modes that
do not require an ACL, and allow us to replace "here's your 12 bits
of values" with "here's a small binary blob you can decode expensively".

But usually that's all that's present, so we just hit the path where
we call chmod/fchmod and return early.

For us to hit this, we have to have gotten a system.posix_acl_access
message which contains access list values other than those, which of
course never happens, because who would actually use ACLs for their
intended purpose, instead of as a ridiculously expensive way to
implement chmod?

And yeah, I think you're right -- this assignment is just plain
unneeded.

(I think the Darwin port is long-dead because Apple's security stuff
makes it a pain to work with. Basically, "can we get this working on
Darwin" was a high priority and common request until about three days
after I got it running, and I don't think anyone's asked about it
since.)

-s

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [OE-core] [pseudo][PATCH 1/2] xattr: Fixed corrupting UID&GID when running setfacl -m on a directory
  2020-06-26 15:36     ` Seebs
@ 2020-06-26 17:33       ` Richard Purdie
  2020-06-26 18:58         ` Seebs
  0 siblings, 1 reply; 6+ messages in thread
From: Richard Purdie @ 2020-06-26 17:33 UTC (permalink / raw)
  To: Seebs, Johannes Beisswenger; +Cc: openembedded-core

On Fri, 2020-06-26 at 10:36 -0500, Seebs wrote:
> On Fri, 26 Jun 2020 16:56:48 +0200
> "Johannes Beisswenger" <johannes.beisswenger@cetitec.com> wrote:
> 
> > The OP_CREATE/REPLACE/SET_XATTR operations in pseudo_op() don't
> > use 
> > msg_header.mode at all, it is only used for DB sanity checks, which
> > in turn only use the non-permission bits.
> 
> Right, the relevant part is the immediately following
> wrap_chmod/wrap_fchmod. I had forgotten about this chunk of code.
> 
> And... that leads us to the realization of how it is possible that
> this
> ever didn't bite us before.
> 
> The historical reason this exists is that at some point, GNU tar
> decided that it was a crime against nature, humanity, and God to use
> a
> stable existing API when a new API existed which could do the same
> thing at hundreds of times the computational cost, and abandoned all
> use of chmod() in circumstances when extended attributes are
> available.
> But in practice, this only happens for regular filesystem modes that
> do not require an ACL, and allow us to replace "here's your 12 bits
> of values" with "here's a small binary blob you can decode
> expensively".
> 
> But usually that's all that's present, so we just hit the path where
> we call chmod/fchmod and return early.
> 
> For us to hit this, we have to have gotten a system.posix_acl_access
> message which contains access list values other than those, which of
> course never happens, because who would actually use ACLs for their
> intended purpose, instead of as a ridiculously expensive way to
> implement chmod?
> 
> And yeah, I think you're right -- this assignment is just plain
> unneeded.
> 
> (I think the Darwin port is long-dead because Apple's security stuff
> makes it a pain to work with. Basically, "can we get this working on
> Darwin" was a high priority and common request until about three days
> after I got it running, and I don't think anyone's asked about it
> since.)

I need to do something about the patches queued up for pseudo. I'm
going to put them all on an "oe-core" branch in the pseudo repository
so we at least have a common place to use them from and get the recipe
cleaned up until such times as we can get proper review/merge to
master.

That shouldn't cause any problems and lets us improve things a bit.

There is one patch I'm not going to put there as looking at it, I
suspect it needs reworking and really isn't upstreamable, I think the
rest are appropriate given OE is one of pseudo's main users and can
hence influence its development.

Hope that is ok with you seebs!

Cheers,

Richard




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [OE-core] [pseudo][PATCH 1/2] xattr: Fixed corrupting UID&GID when running setfacl -m on a directory
  2020-06-26 17:33       ` Richard Purdie
@ 2020-06-26 18:58         ` Seebs
  0 siblings, 0 replies; 6+ messages in thread
From: Seebs @ 2020-06-26 18:58 UTC (permalink / raw)
  To: Richard Purdie; +Cc: openembedded-core

On Fri, 26 Jun 2020 18:33:39 +0100
"Richard Purdie" <richard.purdie@linuxfoundation.org> wrote:

> There is one patch I'm not going to put there as looking at it, I
> suspect it needs reworking and really isn't upstreamable, I think the
> rest are appropriate given OE is one of pseudo's main users and can
> hence influence its development.
> 
> Hope that is ok with you seebs!

Seems reasonable to me. SOME DAY I will have free time. :P

-s

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-06-26 18:58 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-26  9:40 [pseudo][PATCH 1/2] xattr: Fixed corrupting UID&GID when running setfacl -m on a directory johannes.beisswenger
2020-06-26 14:05 ` [OE-core] " Seebs
2020-06-26 14:56   ` Johannes Beisswenger
2020-06-26 15:36     ` Seebs
2020-06-26 17:33       ` Richard Purdie
2020-06-26 18:58         ` Seebs

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.