3.0.30: Race in reply.c: unlink_internals()

[Sergey Kleyman]

I've found a possible race in the reply.c: unlink_internals() in the
following lines:

                status = can_delete(conn,directory,dirtype,can_defer);
                if (!NT_STATUS_IS_OK(status)) {
                        return status;
                }

                if (SMB_VFS_UNLINK(conn,directory) == 0) {


Since can_delete() is basically open file, check access and then close
file so after close() another Samba process may delete the file and
create a new one with the same name and then SMB_VFS_UNLINK will delete
the new file and not the one we've checked in can_delete(). It seems to
be a breach in security.

I have an idea as to how to fix it. Instead of close file and then
unlink Samba should set DELETE ON CLOSE flag in open in can_delete and
then unlink is unnecessary because the file will be deleted on close in
can_delete.

Thank you in advance for your feedback.

[Jeremy Allison]

On Thu, Jul 24, 2008 at 05:27:28PM +0300, Sergey Kleyman wrote:

> I've found a possible race in the reply.c: unlink_internals() in the
> following lines:
>
> status = can_delete(conn,directory,dirtype,can_defer);
> if (!NT_STATUS_IS_OK(status)) {
> return status;
> }
>
> if (SMB_VFS_UNLINK(conn,directory) == 0) {
>
>
> Since can_delete() is basically open file, check access and then close
> file so after close() another Samba process may delete the file and
> create a new one with the same name and then SMB_VFS_UNLINK will delete
> the new file and not the one we've checked in can_delete(). It seems to
> be a breach in security.

No, it's not a security problem as all this is done as the
connected user.

This is already done the way you suggest (set delete on close)
in Samba 3.2.x.

Thanks for reviewing the code,

Jeremy