linux-kernel - Re: [PATCH 1/2] kernel/sys.c: return the current gid when error occurs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 08 Aug 2013 09:30:11 +0800
From:	Chen Gang <gang.chen@...anux.com>
To:	Andy Lutomirski <luto@...capital.net>
CC:	Oleg Nesterov <oleg@...hat.com>,
	Michael Kerrisk <mtk.manpages@...il.com>,
	Kees Cook <keescook@...omium.org>,
	Al Viro <viro@...iv.linux.org.uk>, holt@....com,
	Andrew Morton <akpm@...ux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] kernel/sys.c: return the current gid when error occurs

On 08/08/2013 12:58 AM, Andy Lutomirski wrote:
> On Wed, Aug 7, 2013 at 9:21 AM, Oleg Nesterov <oleg@...hat.com> wrote:
>> On 08/06, Andy Lutomirski wrote:
>>>
>>> I assume that what the man page means is that the return value is
>>> whatever fsgid was prior to the call.  On error, fsgid isn't changed, so
>>> the return value is still "current".
>>
>> Probably... Still
>>
>>         On success, the previous value of fsuid is returned.
>>         On error, the current value of fsuid is returned.
>>
>> looks confusing. sys_setfsuid() always returns the old value.
>>
>>> (FWIW, this behavior is awful and is probably the cause of a security
>>> bug or three, since success and failure are indistinguishable.
>>
>> At least this all looks strange.
>>
>> I dunno if we can change this old behaviour. I won't be surprized
>> if someone already uses setfsuid(-1) as getfsuid().
> 

Oh, really it is.

Hmm... as a pair function, we need add getfsuid() too, if we do not add
it, it will make negative effect with setfsuid().

Since it is a system call, we have to keep compitable.

So in my opinion, better add getfsuid2()/setfsuid2() instead of current
setfsuid()

  for getfsuid2() can just reference to getuid() definition.
  for setfsuid2() can just reference to setuid() definition.
    (which can return error code when failure occurs).

If possible, I will/should sent the related patches for it.

> I suspect that changing this without introducing security or other
> bugs is impossible.  If someone wanted to add new_setfsuid that
> returned an error when it failed, that would be a different story.
> (I'm not going to do that myself.)
> 
>>
>> And perhaps the man page should be changed. Add Michael.
> 
> Agreed.  The text is a bit odd.
> 

I agree that the text is odd, since it is an system call API, we have
to change it to match our current kernel implementation which is:

  "for system call, whether succeed or not, it always return the previous value, the caller can not know whether succeed or not directly"
  "if the caller need know about success or not, the demo like below"
  "	setfsuid(new);"
  "	if (setfsuid(-1) != new)"
  "		/* report failure */" 

And better to give an additional comment:

  "currently, new system call getfsuid2()/setfsuid2() are supplied, setfsuid() is obseleted"

>>
>> Oleg.
>>
> 
> 
> 

Thanks.
-- 
Chen Gang
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/