New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

racecondition() #2

Closed
RootUp opened this Issue Jul 12, 2018 · 2 comments

Comments

Projects
None yet
2 participants
@RootUp

RootUp commented Jul 12, 2018

File: /mstdlib/blob/master/base/fs/m_fs_perms_unx.c#L277

i.e
if (access(norm_path, access_mode) == -1) {

I believe this indicates a security flaw, If an attacker can change anything along the path between the call access() and the files actually used, attacker may exploit the race condition.

Request team to have a look and validate.

@user-none

This comment has been minimized.

Show comment
Hide comment
@user-none

user-none Jul 12, 2018

Member

You are correct that it is possible to make changes to a path between calling M_fs_perms_can_access and doing something with the path. However, this function is conforming to ISO/IEC 9945-1:1990 and the same security considerations apply when using "access" directly. I'll add a note to the documentation making this more explicit.

As long as it’s not used to make an access control decision it’s okay to use the function. Meaning if the file system call is made regardless of the outcome of access it should be safe to use this function.

M_fs_file_open_sys on Windows is a place where M_fs_perms_can_access is used. It’s used to determine if the file exists before opening to know if the modified time should be updated due to “file system tunneling” not updating the access time as expected. There is no way to know from CreateFile if the file was created and opened or just opened. In this situation the CreateFile call is not dependent on the access call. Worst case the file time is not updated or is updated when it didn’t need to be. Since CreateFile is called regardless of the result of access and access is only used to determine if the file time should be updated this is not an insecure use.

We will review other places it this is used to determine if they are safe uses.

Member

user-none commented Jul 12, 2018

You are correct that it is possible to make changes to a path between calling M_fs_perms_can_access and doing something with the path. However, this function is conforming to ISO/IEC 9945-1:1990 and the same security considerations apply when using "access" directly. I'll add a note to the documentation making this more explicit.

As long as it’s not used to make an access control decision it’s okay to use the function. Meaning if the file system call is made regardless of the outcome of access it should be safe to use this function.

M_fs_file_open_sys on Windows is a place where M_fs_perms_can_access is used. It’s used to determine if the file exists before opening to know if the modified time should be updated due to “file system tunneling” not updating the access time as expected. There is no way to know from CreateFile if the file was created and opened or just opened. In this situation the CreateFile call is not dependent on the access call. Worst case the file time is not updated or is updated when it didn’t need to be. Since CreateFile is called regardless of the result of access and access is only used to determine if the file time should be updated this is not an insecure use.

We will review other places it this is used to determine if they are safe uses.

@user-none

This comment has been minimized.

Show comment
Hide comment
@user-none

user-none Jul 12, 2018

Member

A review of the file portion of the fs code has been completed. This is not a comprehensive review but addresses the concerns of this ticket.

  • Additional documentation about use of M_fs_perms_can_access was added in 4c000aa. Added documentation notes that this function is POSIX.1 compliant and warns about potential security issues related to its use. Since there are legitimate and secure uses (which mstdlib uses) for the function we are not going to remove it. It’s up to whoever’s using the function to use it securely. The best we can do is tell people to be careful with it.
  • Various places M_fs_perms_can_access is used has comments about why the use is legitimate and not a security issue.
  • New info and set perms functions were added for currently open files. These functions are being used when possible to prevent issues where the path changes between open and getting information about the file. 46e6074, 962db8f, and f82091a
  • Fixed an issue where M_fs_perms_can_access tried to delete a file during copy but this could lead to a security flaw related to providing a third party access to a file they may not have had access to. db124b8.
Member

user-none commented Jul 12, 2018

A review of the file portion of the fs code has been completed. This is not a comprehensive review but addresses the concerns of this ticket.

  • Additional documentation about use of M_fs_perms_can_access was added in 4c000aa. Added documentation notes that this function is POSIX.1 compliant and warns about potential security issues related to its use. Since there are legitimate and secure uses (which mstdlib uses) for the function we are not going to remove it. It’s up to whoever’s using the function to use it securely. The best we can do is tell people to be careful with it.
  • Various places M_fs_perms_can_access is used has comments about why the use is legitimate and not a security issue.
  • New info and set perms functions were added for currently open files. These functions are being used when possible to prevent issues where the path changes between open and getting information about the file. 46e6074, 962db8f, and f82091a
  • Fixed an issue where M_fs_perms_can_access tried to delete a file during copy but this could lead to a security flaw related to providing a third party access to a file they may not have had access to. db124b8.

@user-none user-none closed this Jul 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment