posix_lchown: possible overflow of uid, gid

[boya]

BPO	6873
Nosy	@loewis, @gpshead, @vstinner
Files	patch_6873.diff: This fix deals with uid/gid overflow patch_6873.diff: Correct the fix to use l argument parser

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/gpshead'
closed_at = <Date 2011-03-15.16:25:08.004>
created_at = <Date 2009-09-09.21:01:22.053>
labels = []
title = 'posix_lchown: possible overflow of uid, gid'
updated_at = <Date 2011-03-15.16:25:08.002>
user = 'https://bugs.python.org/boya'

bugs.python.org fields:

activity = <Date 2011-03-15.16:25:08.002>
actor = 'gregory.p.smith'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2011-03-15.16:25:08.004>
closer = 'gregory.p.smith'
components = []
creation = <Date 2009-09-09.21:01:22.053>
creator = 'boya'
dependencies = []
files = ['14873', '14900']
hgrepos = []
issue_num = 6873
keywords = ['patch']
message_count = 13.0
messages = ['92465', '92473', '92476', '92496', '92504', '92505', '92512', '92544', '92696', '92705', '96836', '96848', '130991']
nosy_count = 4.0
nosy_names = ['loewis', 'gregory.p.smith', 'vstinner', 'boya']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue6873'
versions = []

[boya]

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 bpo-5705.

Patch attached. Any comment is appreciated!

Boya

[vstinner]

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().

[loewis]

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.

[boya]

Martin,

The reason why I think there is a possible overflow is that according to
bpo-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.

[loewis]

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?

[vstinner]

@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).

[boya]

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

[loewis]

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.

[boya]

Martin,

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

Boya

[loewis]

Yes, it looks correct now. I still wish it could be tested on a system
where the problem actually occurs.

[gpshead]

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.

[boya]

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

[gpshead]

a test would still be a good thing but this should be fixed regardless.