The Wayback Machine - http://web.archive.org/web/20240324153623/https://github.com/github/codeql/issues/6834
Skip to content
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

Jump to bottom

LGTM.com - false positive for cpp/toctou-race-condition #6834

Open

eldering opened this issue Oct 8, 2021 · 2 comments
Open

LGTM.com - false positive for cpp/toctou-race-condition #6834

eldering opened this issue Oct 8, 2021 · 2 comments
Labels
C++ false-positive

Comments

@eldering
Copy link

eldering commented Oct 8, 2021

Description of the false positive

Even though the code first does a stat and then after that an open on the same file, it still afterwards checks that the open call succeeded before operating on the file descriptor. Thus, there is no toctou race condition. I understand that this can be considered an edge case, but still reporting it here for consideration.

URL to the alert on the project page on LGTM.com

https://lgtm.com/projects/g/DOMjudge/domjudge/snapshot/66789bbc091123b7a00dc368032b9dbb5f3e0e96/files/judge/evict.c?sort=name&dir=ASC&mode=heatmap#x5362a542144696a7:1
@hvitved hvitved added the C++ label Oct 8, 2021
@geoffw0
Copy link
Contributor

geoffw0 commented Oct 8, 2021

Hi, thanks for raising this.

We agree that checking the return value of open covers the case that the file no longer exists, however we believe there is still a potential TOCTOU issue involving the path to a file that's replaced with a directory. In this case the S_ISDIR check could be fooled and a directory unexpectedly passed to posix_fadvise. We're suspicious this wouldn't be harmful but that's beyond the scope of this query.

We'll be keeping an eye on this query, and adding some new test cases in the near future.

I suggest that you double check you're happy with the code in evict_directory, and then hide the result in the LGTM interface. Let us know if you have any further questions or concerns.

@geoffw0 geoffw0 mentioned this issue Oct 8, 2021

Merged
eldering added a commit to eldering/domjudge that referenced this issue Oct 9, 2021
@eldering
Use fstat() with a file descriptor to prevent TOCTOU race conditions.
309fa44 
As pointed out in github/codeql#6834,
even though there is no issue with the open() call since we
check the filedescriptor afterwards, there is a (not problematic)
race condition in the old code, where a file could be replaced by
a directory or vice versa. Neither of these pose a security risk
though.

This should fix the LGTM alert.

@eldering eldering mentioned this issue Oct 9, 2021

Merged
@eldering
Copy link
Author

eldering commented Oct 9, 2021

Thanks, that's a correct point that I overlooked. Should be fixed with my PR DOMjudge/domjudge#1249.

eldering added a commit to eldering/domjudge that referenced this issue Oct 9, 2021
@eldering
Use fstat() with a file descriptor to prevent TOCTOU race conditions.
0b9ce7d 
As pointed out in github/codeql#6834,
even though there is no issue with the open() call since we
check the filedescriptor afterwards, there is a (not problematic)
race condition in the old code, where a file could be replaced by
a directory or vice versa. Neither of these pose a security risk
though.

This should fix the LGTM alert.
eldering added a commit to DOMjudge/domjudge that referenced this issue Oct 9, 2021
@eldering
Use fstat() with a file descriptor to prevent TOCTOU race conditions.
f3a2d96 
As pointed out in github/codeql#6834,
even though there is no issue with the open() call since we
check the filedescriptor afterwards, there is a (not problematic)
race condition in the old code, where a file could be replaced by
a directory or vice versa. Neither of these pose a security risk
though.

This should fix the LGTM alert.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C++ false-positive
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@hvitved @eldering @geoffw0