classification
Title: posix_lchown: possible overflow of uid, gid
Type: Stage:
Components: Versions:
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: boya, gregory.p.smith, loewis, vstinner
Priority: normal Keywords: patch

Created on 2009-09-09 21:01 by boya, last changed 2011-03-15 16:25 by gregory.p.smith. This issue is now closed.

Files
File name Uploaded Description Edit
patch_6873.diff boya, 2009-09-10 17:39 This fix deals with uid/gid overflow
patch_6873.diff boya, 2009-09-16 16:16 Correct the fix to use l argument parser
Messages (13)
msg92465 - (view) Author: Boya Sun (boya) Date: 2009-09-09 21:01
posix_lchown(PyObject *self, PyObject *args)
{
        ...
 	int uid, gid;
        ...
 	if (!PyArg_ParseTuple(args, "etii:lchown",
 	                      Py_FileSystemDefaultEncoding, &path,
 	                      &uid, &gid))
        ...
}

uid and gid could cause over flow. A similar bug is issue 5705.

Patch attached.  Any comment is appreciated!

Boya
msg92473 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2009-09-10 08:02
posix modules contains a lot of function parsing uid_t / gid_t types. I would be
nice to factorize the code: create a function to get an uid_t, and another to
get a gid_t. I don't know the name of such callback, but it's used with:
PyArg_ParseTuple(args, "...O&...", ..., &uid, get_uid, ...)).

Such callbacks will be useful for: posix_chown(), posix_fchown(),
posix_lchown(), posix_setuid(), posix_seteuid(), posix_setreuid(),
posix_setegid(), posix_setregid(), posix_setgid().

And maybe also in: posix_setgroups().

In Python trunk, posix_set*id() function do check for uid_t/gid_d overflow, but
not the posix_*chown() functions. The patch only fixes posix_lchown().
msg92476 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-09-10 09:03
The patch is incorrect. Why do you think there is an overflow? There is 
none in the call to ParseTuple: the i argument parser expects a signed 
int*; passing a long* will break on systems where 
sizeof(int)!=sizeof(long) (such as typical 64-bit Unix).

In addition, the *actual* overflow in the current code (casting to uid_t) 
is not handled in the patch.
msg92496 - (view) Author: Boya Sun (boya) Date: 2009-09-10 17:39
Martin,

The reason why I think there is a possible overflow is that according to
issue 5705, uid/gid overflows are fixed in the following functions:
posix_setegid, posix_setreuid(), posix_setregid(), posix_setgid(). So I
think a similar fix should also be applied to the function posix_lchown.
Or did I misunderstand anything?

And you're right. The previous patch is incorrect. I now submitted
another patch that deals with the *actual* overflow of gid and uid. 

---
Victor,

I agree that all posix_*chown() functions should also be fixed for the
same overflow problem, and it's a good idea to create callback functions
as you described. But if nobody does that, I can at least created more
patches to fix other posix_*chown() functions.
msg92504 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-09-11 07:54
I think the new patch is still incorrect. You now pass long variables into 
the i argument parser. Also, I would expect that compilers prefer to see 
an explicit cast from long to uid_t, in case it's a truncating cast.

Can you try your patch on a system where all this is an actual problem?
msg92505 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2009-09-11 08:05
@loewis: I don't think that the explicit cast is required. posix_setuid() has no
explicit cast. But I would also prefer an explicit cast (just for the readability).
msg92512 - (view) Author: Boya Sun (boya) Date: 2009-09-11 16:03
Martin,

I am sorry that I do not have a system where this code actually
triggered a problem, since this bug is discovered by a *static* analysis
tool that is recently developed by our research group, which finds code
segments that are similar to a previously fixed bugs as potential bugs.
 You are saying that if I pass a long to the i argument parser it will
cause a problem. But if I passed a int, it will be same as before and
overflow will not be detected at all.  

---
Victor,

Do you also agree that it will cause a problem if I pass a long to the i
argument parser? If so, I think maybe the overflow problem cannot be
solved by the patch I submitted.

Boya
msg92544 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-09-12 13:25
>  You are saying that if I pass a long to the i argument parser it will
> cause a problem. But if I passed a int, it will be same as before and
> overflow will not be detected at all.  

Correct. So you should use the l argument parser.
msg92696 - (view) Author: Boya Sun (boya) Date: 2009-09-16 16:16
Martin,

Corrected the patch accordingly. Can you verify whether the fix is
correct or not now? 

Boya
msg92705 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-09-16 17:59
Yes, it looks correct now. I still wish it could be tested on a system 
where the problem actually occurs.
msg96836 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2009-12-23 10:45
I applied the same fix that was applied to chown in trunk r77007 for 
lchown and fchown.  Could you test it on a platform where it previously 
failed?

The existing code might still have issues if there are platforms where 
uid_t and gid_t are unsigned but not the same size as a long as at the 
moment it merely casts and does not test to see that the values are the 
same as the patch you have supplied here.
msg96848 - (view) Author: Boya Sun (boya) Date: 2009-12-24 04:36
Gregory,

I discovered this bug by static analysis, so I do not have a system 
that this bug is actually triggered. But I am happy to see the fix 
applied since this makes code safer. It would be great if anyone could 
write a test case that cause uid and gid to overflow, then use the test 
case as a regression test on the fix.

Boya
msg130991 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2011-03-15 16:25
a test would still be a good thing but this should be fixed regardless.
History
Date User Action Args
2011-03-15 16:25:08gregory.p.smithsetstatus: open -> closed

messages: + msg130991
resolution: accepted
nosy: loewis, gregory.p.smith, vstinner, boya
2009-12-24 04:36:59boyasetmessages: + msg96848
2009-12-23 10:45:51gregory.p.smithsetmessages: + msg96836
2009-11-02 06:47:43gregory.p.smithsetpriority: normal
2009-11-02 06:47:35gregory.p.smithsetassignee: gregory.p.smith

nosy: + gregory.p.smith
2009-09-16 17:59:49loewissetmessages: + msg92705
2009-09-16 16:16:39boyasetfiles: + patch_6873.diff

messages: + msg92696
2009-09-12 13:25:29loewissetmessages: + msg92544
2009-09-11 16:03:03boyasetmessages: + msg92512
2009-09-11 08:05:54vstinnersetmessages: + msg92505
2009-09-11 07:54:16loewissetmessages: + msg92504
2009-09-10 17:40:02boyasetfiles: - patch.diff
2009-09-10 17:39:56boyasetfiles: + patch_6873.diff

messages: + msg92496
2009-09-10 09:03:24loewissetnosy: + loewis
messages: + msg92476
2009-09-10 08:02:20vstinnersetnosy: + vstinner
messages: + msg92473
2009-09-09 21:01:22boyacreate