Hi,
we have been contacted by our partner about the following permission
discrepancy
1. Create a shared memory segment with permissions 600 with user A using
shmget(key, 1024, 0600 | IPC_CREAT)
2. ipcs -m should return an output as follows:
------ Shared Memory Segments --------
key shmid owner perms bytes nattch status
0x58b74326 759562241 A 600 1024 0
3. Try to read the metadata with shmctl(0, SHM_STAT,...) as user B.
4. shmctl will return -EACCES
The supper set information provided by shmctl can be retrieved by
reading /proc/sysvipc/shm which does not require read permissions
because it is 444.
It seems that the discrepancy is there since ae7817745eef ("[PATCH] ipc:
add generic struct ipc_ids seq_file iteration") when the proc interface
has been introduced. The changelog is really modest on information or
intention but I suspect this just got overlooked during review. SHM_STAT
has always been about read permission and it is explicitly documented
that way.
I am not a security expert to judge whether this leak can have some
interesting consequences but I am really interested whether this is
something we want to keep that way. Do we want to filter and dump only
shmids the caller has access to? This would break the delegation AFAICS.
Do we want to make the file root only? That would probably break an
existing userspace as well.
Or should we simply allow SHM_STAT for processes without a read permission
because the same information can be read by other means already?
Any other ideas?
--
Michal Hocko
SUSE Labs
Hello Michal,
On 19 December 2017 at 10:48, Michal Hocko <[email protected]> wrote:
> Hi,
> we have been contacted by our partner about the following permission
> discrepancy
>
> 1. Create a shared memory segment with permissions 600 with user A using
> shmget(key, 1024, 0600 | IPC_CREAT)
> 2. ipcs -m should return an output as follows:
>
> ------ Shared Memory Segments --------
> key shmid owner perms bytes nattch status
> 0x58b74326 759562241 A 600 1024 0
>
> 3. Try to read the metadata with shmctl(0, SHM_STAT,...) as user B.
> 4. shmctl will return -EACCES
>
> The supper set information provided by shmctl can be retrieved by
> reading /proc/sysvipc/shm which does not require read permissions
> because it is 444.
>
> It seems that the discrepancy is there since ae7817745eef ("[PATCH] ipc:
> add generic struct ipc_ids seq_file iteration") when the proc interface
> has been introduced. The changelog is really modest on information or
> intention but I suspect this just got overlooked during review. SHM_STAT
> has always been about read permission and it is explicitly documented
> that way.
Yes, this was always a weirdness on Linux. Back before we got
/proc/sysvipc, it meant that ipcs(1) on Linux did not did not display
all IPC objects (unlike most other implementations, where ipcs(1)
showed everyone's objects, regardless of permissions). I remember
having an email conversation with Andries Brouwer about this, around
15 years ago. Eventually, an October 2012 series of util-linux patches
by Sami Kerola switched ipcs(1) to use /proc/sysvipc so that ipcs(1)
does now show all System V IPC objects.
> I am not a security expert to judge whether this leak can have some
> interesting consequences but I am really interested whether this is
> something we want to keep that way. Do we want to filter and dump only
> shmids the caller has access to?
Do you mean change /proc/sysvipc/* output? I don't think that should
be changed. Modern ipcs(1) relies on it to do The Right Thing.
> This would break the delegation AFAICS.
> Do we want to make the file root only? That would probably break an
> existing userspace as well.
>
> Or should we simply allow SHM_STAT for processes without a read permission
> because the same information can be read by other means already?
>
> Any other ideas?
The situation is certainly odd. The only risk that I see is that
modifying *_STAT behavior could lead to behavior changes in (strange?)
programs that expect SHM_STAT / MSG_STAT / SEM_STAT to return only
information about objects for which they have read permission. But, is
there a pressing reason to make the change? (Okay, I guess iterating
using *_STAT is nicer than parsing /proc/sysvipc/*.)
Cheers,
Michael
> --
> Michal Hocko
> SUSE Labs
> --
> To unsubscribe from this list: send the line "unsubscribe linux-api" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
Hi Michal,
On 12/19/2017 10:48 AM, Michal Hocko wrote:
> Hi,
> we have been contacted by our partner about the following permission
> discrepancy
> 1. Create a shared memory segment with permissions 600 with user A using
> shmget(key, 1024, 0600 | IPC_CREAT)
> 2. ipcs -m should return an output as follows:
>
> ------ Shared Memory Segments --------
> key shmid owner perms bytes nattch status
> 0x58b74326 759562241 A 600 1024 0
>
> 3. Try to read the metadata with shmctl(0, SHM_STAT,...) as user B.
> 4. shmctl will return -EACCES
>
> The supper set information provided by shmctl can be retrieved by
> reading /proc/sysvipc/shm which does not require read permissions
> because it is 444.
>
> It seems that the discrepancy is there since ae7817745eef ("[PATCH] ipc:
> add generic struct ipc_ids seq_file iteration") when the proc interface
> has been introduced. The changelog is really modest on information or
> intention but I suspect this just got overlooked during review. SHM_STAT
> has always been about read permission and it is explicitly documented
> that way.
Are you sure that this patch changed the behavior?
The proc interface is much older.
--
Manfred
Hi Manfred,
On 20 December 2017 at 09:32, Dr. Manfred Spraul
<[email protected]> wrote:
> Hi Michal,
>
> On 12/19/2017 10:48 AM, Michal Hocko wrote:
>>
>> Hi,
>> we have been contacted by our partner about the following permission
>> discrepancy
>> 1. Create a shared memory segment with permissions 600 with user A using
>> shmget(key, 1024, 0600 | IPC_CREAT)
>> 2. ipcs -m should return an output as follows:
>>
>> ------ Shared Memory Segments --------
>> key shmid owner perms bytes nattch status
>> 0x58b74326 759562241 A 600 1024 0
>>
>> 3. Try to read the metadata with shmctl(0, SHM_STAT,...) as user B.
>> 4. shmctl will return -EACCES
>>
>> The supper set information provided by shmctl can be retrieved by
>> reading /proc/sysvipc/shm which does not require read permissions
>> because it is 444.
>>
>> It seems that the discrepancy is there since ae7817745eef ("[PATCH] ipc:
>> add generic struct ipc_ids seq_file iteration") when the proc interface
>> has been introduced. The changelog is really modest on information or
>> intention but I suspect this just got overlooked during review. SHM_STAT
>> has always been about read permission and it is explicitly documented
>> that way.
>
> Are you sure that this patch changed the behavior?
> The proc interface is much older.
Yes, I think that's correct. The /proc/sysvipc interface appeared in
2.3.x, and AFAIK the behavior was already different from *_STAT back
then.
Cheers,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
On Wed 20-12-17 09:44:47, Michael Kerrisk wrote:
> Hi Manfred,
>
> On 20 December 2017 at 09:32, Dr. Manfred Spraul
> <[email protected]> wrote:
> > Hi Michal,
> >
> > On 12/19/2017 10:48 AM, Michal Hocko wrote:
> >>
> >> Hi,
> >> we have been contacted by our partner about the following permission
> >> discrepancy
> >> 1. Create a shared memory segment with permissions 600 with user A using
> >> shmget(key, 1024, 0600 | IPC_CREAT)
> >> 2. ipcs -m should return an output as follows:
> >>
> >> ------ Shared Memory Segments --------
> >> key shmid owner perms bytes nattch status
> >> 0x58b74326 759562241 A 600 1024 0
> >>
> >> 3. Try to read the metadata with shmctl(0, SHM_STAT,...) as user B.
> >> 4. shmctl will return -EACCES
> >>
> >> The supper set information provided by shmctl can be retrieved by
> >> reading /proc/sysvipc/shm which does not require read permissions
> >> because it is 444.
> >>
> >> It seems that the discrepancy is there since ae7817745eef ("[PATCH] ipc:
> >> add generic struct ipc_ids seq_file iteration") when the proc interface
> >> has been introduced. The changelog is really modest on information or
> >> intention but I suspect this just got overlooked during review. SHM_STAT
> >> has always been about read permission and it is explicitly documented
> >> that way.
> >
> > Are you sure that this patch changed the behavior?
> > The proc interface is much older.
>
> Yes, I think that's correct. The /proc/sysvipc interface appeared in
> 2.3.x, and AFAIK the behavior was already different from *_STAT back
> then.
I have probably misread the patch. It surely adds sysvipc_proc_fops,
maybe there was a different implementation previously. I haven't
checked.
--
Michal Hocko
SUSE Labs
On Tue 19-12-17 17:45:40, Michael Kerrisk wrote:
> Hello Michal,
>
> On 19 December 2017 at 10:48, Michal Hocko <[email protected]> wrote:
> > Hi,
> > we have been contacted by our partner about the following permission
> > discrepancy
> >
> > 1. Create a shared memory segment with permissions 600 with user A using
> > shmget(key, 1024, 0600 | IPC_CREAT)
> > 2. ipcs -m should return an output as follows:
> >
> > ------ Shared Memory Segments --------
> > key shmid owner perms bytes nattch status
> > 0x58b74326 759562241 A 600 1024 0
> >
> > 3. Try to read the metadata with shmctl(0, SHM_STAT,...) as user B.
> > 4. shmctl will return -EACCES
> >
> > The supper set information provided by shmctl can be retrieved by
> > reading /proc/sysvipc/shm which does not require read permissions
> > because it is 444.
> >
> > It seems that the discrepancy is there since ae7817745eef ("[PATCH] ipc:
> > add generic struct ipc_ids seq_file iteration") when the proc interface
> > has been introduced. The changelog is really modest on information or
> > intention but I suspect this just got overlooked during review. SHM_STAT
> > has always been about read permission and it is explicitly documented
> > that way.
>
> Yes, this was always a weirdness on Linux. Back before we got
> /proc/sysvipc, it meant that ipcs(1) on Linux did not did not display
> all IPC objects (unlike most other implementations, where ipcs(1)
> showed everyone's objects, regardless of permissions). I remember
> having an email conversation with Andries Brouwer about this, around
> 15 years ago. Eventually, an October 2012 series of util-linux patches
> by Sami Kerola switched ipcs(1) to use /proc/sysvipc so that ipcs(1)
> does now show all System V IPC objects.
Thanks for the clarification.
> > I am not a security expert to judge whether this leak can have some
> > interesting consequences but I am really interested whether this is
> > something we want to keep that way. Do we want to filter and dump only
> > shmids the caller has access to?
>
> Do you mean change /proc/sysvipc/* output? I don't think that should
> be changed. Modern ipcs(1) relies on it to do The Right Thing.
OK, I somehow suspected somebody will rely on this.
> > This would break the delegation AFAICS.
> > Do we want to make the file root only? That would probably break an
> > existing userspace as well.
> >
> > Or should we simply allow SHM_STAT for processes without a read permission
> > because the same information can be read by other means already?
> >
> > Any other ideas?
>
> The situation is certainly odd. The only risk that I see is that
> modifying *_STAT behavior could lead to behavior changes in (strange?)
> programs that expect SHM_STAT / MSG_STAT / SEM_STAT to return only
> information about objects for which they have read permission.
Hmm, do you mean those would iterate shmid space to find their own? That
would be certainly odd.
> But, is
> there a pressing reason to make the change? (Okay, I guess iterating
> using *_STAT is nicer than parsing /proc/sysvipc/*.)
The reporter of this issue claims that "Reading /proc/sysvipc/shm is way
slower than executing the system call." I haven't checked that but I can
imagine that /proc/sysvipc/shm can take quite some time when there are
_many_ segments registered. So they would like to use the syscall but
the interacting parties do not have compatible permissions.
--
Michal Hocko
SUSE Labs
Hello Michal,
On 20 December 2017 at 10:20, Michal Hocko <[email protected]> wrote:
> On Tue 19-12-17 17:45:40, Michael Kerrisk wrote:
>> But, is
>> there a pressing reason to make the change? (Okay, I guess iterating
>> using *_STAT is nicer than parsing /proc/sysvipc/*.)
>
> The reporter of this issue claims that "Reading /proc/sysvipc/shm is way
> slower than executing the system call." I haven't checked that but I can
> imagine that /proc/sysvipc/shm can take quite some time when there are
> _many_ segments registered.
Yes, that makes sense.
> So they would like to use the syscall but
> the interacting parties do not have compatible permissions.
So, I don't think there is any security issue, since the same info is
available in /proc/sysvipc/*. The only question would be whether
change in the *_STAT behavior might surprise some applications into
behaving differently. I presume the chances of that are low, but if it
was a concert, one could add new shmctl/msgctl/semctl *_STAT_ALL (or
some such) operations that have the desired behavior.
Cheers,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
On Wed 20-12-17 17:17:46, Michael Kerrisk wrote:
> Hello Michal,
>
> On 20 December 2017 at 10:20, Michal Hocko <[email protected]> wrote:
> > On Tue 19-12-17 17:45:40, Michael Kerrisk wrote:
> >> But, is
> >> there a pressing reason to make the change? (Okay, I guess iterating
> >> using *_STAT is nicer than parsing /proc/sysvipc/*.)
> >
> > The reporter of this issue claims that "Reading /proc/sysvipc/shm is way
> > slower than executing the system call." I haven't checked that but I can
> > imagine that /proc/sysvipc/shm can take quite some time when there are
> > _many_ segments registered.
>
> Yes, that makes sense.
>
> > So they would like to use the syscall but
> > the interacting parties do not have compatible permissions.
>
> So, I don't think there is any security issue, since the same info is
> available in /proc/sysvipc/*.
Well, I am not sure this is a valid argument (maybe I just misread your
statement). Our security model _might_ be broken because of the sysipc
proc interface existance already. I am not saying it is broken because
I cannot see an attack vector based solely on the metadata information
knowledge. An attacker still cannot see/modify the real data. But maybe
there are some bugs lurking there and knowing the metadata might help to
exploit them. I dunno.
You are certainly right that modifying/adding STAT flag to comply with
the proc interface permission model will not make the system any more
vulnerable, though.
> The only question would be whether
> change in the *_STAT behavior might surprise some applications into
> behaving differently. I presume the chances of that are low, but if it
> was a concert, one could add new shmctl/msgctl/semctl *_STAT_ALL (or
> some such) operations that have the desired behavior.
I would lean towards _STAT_ALL because this is Linux specific behavior
(I have looked at what BSD does here and they are checking permissions
for STAT as well). It would also be simpler to revert if we ever find
that this is a leak with security consequences.
What do other people think? I can prepare a patch.
--
Michal Hocko
SUSE Labs
Hi Michal,
On 21 December 2017 at 09:02, Michal Hocko <[email protected]> wrote:
> On Wed 20-12-17 17:17:46, Michael Kerrisk wrote:
>> Hello Michal,
>>
>> On 20 December 2017 at 10:20, Michal Hocko <[email protected]> wrote:
>> > On Tue 19-12-17 17:45:40, Michael Kerrisk wrote:
>> >> But, is
>> >> there a pressing reason to make the change? (Okay, I guess iterating
>> >> using *_STAT is nicer than parsing /proc/sysvipc/*.)
>> >
>> > The reporter of this issue claims that "Reading /proc/sysvipc/shm is way
>> > slower than executing the system call." I haven't checked that but I can
>> > imagine that /proc/sysvipc/shm can take quite some time when there are
>> > _many_ segments registered.
>>
>> Yes, that makes sense.
>>
>> > So they would like to use the syscall but
>> > the interacting parties do not have compatible permissions.
>>
>> So, I don't think there is any security issue, since the same info is
>> available in /proc/sysvipc/*.
>
> Well, I am not sure this is a valid argument (maybe I just misread your
> statement).
(Or perhaps I was not clear enough; see below)
> Our security model _might_ be broken because of the sysipc
> proc interface existance already. I am not saying it is broken because
> I cannot see an attack vector based solely on the metadata information
> knowledge. An attacker still cannot see/modify the real data. But maybe
> there are some bugs lurking there and knowing the metadata might help to
> exploit them. I dunno.
>
> You are certainly right that modifying/adding STAT flag to comply with
> the proc interface permission model will not make the system any more
> vulnerable, though.
Yep, that was my point. Modifying _STAT behavior won't decrease security.
That said, /proc/sysvipc/* has been around for a long time now, and
nothing bad seems to have happened so far, AFAIK.
>> The only question would be whether
>> change in the *_STAT behavior might surprise some applications into
>> behaving differently. I presume the chances of that are low, but if it
>> was a concert, one could add new shmctl/msgctl/semctl *_STAT_ALL (or
>> some such) operations that have the desired behavior.
>
> I would lean towards _STAT_ALL because this is Linux specific behavior
> (I have looked at what BSD does here and they are checking permissions
> for STAT as well). It would also be simpler to revert if we ever find
> that this is a leak with security consequences.
Oh -- I was unaware of this BSD behavior. At least on the various UNIX
systems that I ever used SYSVIPC (including one or two ancient
commercial BSD derivatives), ipcs(1) showed all IPC objects. (On
FeeBSD, at least, it looks like ipcs(1) doesn't use the *_STAT
interfaces.)
Cheers,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
On Thu, 21 Dec 2017, Michael Kerrisk (man-pages) wrote:
>Hi Michal,
>
>On 21 December 2017 at 09:02, Michal Hocko <[email protected]> wrote:
>> On Wed 20-12-17 17:17:46, Michael Kerrisk wrote:
>>> Hello Michal,
>>>
>>> On 20 December 2017 at 10:20, Michal Hocko <[email protected]> wrote:
>>> > On Tue 19-12-17 17:45:40, Michael Kerrisk wrote:
>>> >> But, is
>>> >> there a pressing reason to make the change? (Okay, I guess iterating
>>> >> using *_STAT is nicer than parsing /proc/sysvipc/*.)
>>> >
>>> > The reporter of this issue claims that "Reading /proc/sysvipc/shm is way
>>> > slower than executing the system call." I haven't checked that but I can
>>> > imagine that /proc/sysvipc/shm can take quite some time when there are
>>> > _many_ segments registered.
>>>
>>> Yes, that makes sense.
>>>
>>> > So they would like to use the syscall but
>>> > the interacting parties do not have compatible permissions.
>>>
>>> So, I don't think there is any security issue, since the same info is
>>> available in /proc/sysvipc/*.
>>
>> Well, I am not sure this is a valid argument (maybe I just misread your
>> statement).
>
>(Or perhaps I was not clear enough; see below)
>
>> Our security model _might_ be broken because of the sysipc
>> proc interface existance already. I am not saying it is broken because
>> I cannot see an attack vector based solely on the metadata information
>> knowledge. An attacker still cannot see/modify the real data. But maybe
>> there are some bugs lurking there and knowing the metadata might help to
>> exploit them. I dunno.
>>
>> You are certainly right that modifying/adding STAT flag to comply with
>> the proc interface permission model will not make the system any more
>> vulnerable, though.
>
>Yep, that was my point. Modifying _STAT behavior won't decrease security.
>
>That said, /proc/sysvipc/* has been around for a long time now, and
>nothing bad seems to have happened so far, AFAIK.
>
>>> The only question would be whether
>>> change in the *_STAT behavior might surprise some applications into
>>> behaving differently. I presume the chances of that are low, but if it
>>> was a concert, one could add new shmctl/msgctl/semctl *_STAT_ALL (or
>>> some such) operations that have the desired behavior.
>>
>> I would lean towards _STAT_ALL because this is Linux specific behavior
>> (I have looked at what BSD does here and they are checking permissions
>> for STAT as well). It would also be simpler to revert if we ever find
>> that this is a leak with security consequences.
So I took a crack at this, and my only doubt is whether or not the lsm
security hooks should be considered or not. Specifically, should the
SHM_STAT_ALL case consider security_shm_shmctl()?
While the relevant persmission checks that allow for the discripancies
between 0444 procfs and a 0600 via creating the ipc object are done in
ipcperms() returning -1, is there a scenario where some lsm policy could
change the /proc/sysvipc/ interface? If not, I think we can avoid it, but
I'm not a security person.
Thanks,
Davidlohr
>
>Oh -- I was unaware of this BSD behavior. At least on the various UNIX
>systems that I ever used SYSVIPC (including one or two ancient
>commercial BSD derivatives), ipcs(1) showed all IPC objects. (On
>FeeBSD, at least, it looks like ipcs(1) doesn't use the *_STAT
>interfaces.)
>
>Cheers,
>
>Michael
>
>
>
>--
>Michael Kerrisk
>Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
>Linux/UNIX System Programming Training: http://man7.org/training/