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

Fix: Time-of-check time-of-use filesystem race condition #4473

Merged
merged 5 commits into from
Oct 30, 2024

Conversation

brustolin
Copy link
Contributor

Fixes https://github.com/getsentry/sentry-cocoa/security/code-scanning/6

To fix the TOCTOU race condition, we should avoid using stat and open separately. Instead, we can use the open function with the O_RDONLY flag to open the file directly and then use fstat to get the file status. This ensures that the file we are operating on is the same file we checked, as both operations are performed on the file descriptor.

Suggested fixes powered by Copilot Autofix. Review carefully before merging.

…ace condition

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@brustolin brustolin changed the title Fix code scanning alert no. 6: Time-of-check time-of-use filesystem race condition Fix: Time-of-check time-of-use filesystem race condition Oct 24, 2024
Copy link

github-actions bot commented Oct 24, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1244.00 ms 1265.75 ms 21.75 ms
Size 21.90 KiB 709.17 KiB 687.26 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
e80771f 1208.41 ms 1233.94 ms 25.53 ms
1ce939e 1216.79 ms 1242.38 ms 25.59 ms
86b89b9 1237.28 ms 1256.27 ms 18.99 ms
fb53d97 1226.08 ms 1245.12 ms 19.04 ms
5de0a56 1214.49 ms 1235.56 ms 21.07 ms
3a6495e 1227.61 ms 1239.22 ms 11.60 ms
3297d6e 1203.79 ms 1218.76 ms 14.97 ms
ca91a5c 1234.53 ms 1249.86 ms 15.33 ms
4d5eb78 1197.86 ms 1215.73 ms 17.88 ms
feba2be 1246.67 ms 1254.64 ms 7.97 ms

App size

Revision Plain With Sentry Diff
e80771f 21.58 KiB 697.69 KiB 676.10 KiB
1ce939e 22.85 KiB 412.98 KiB 390.13 KiB
86b89b9 21.58 KiB 638.25 KiB 616.66 KiB
fb53d97 20.76 KiB 425.81 KiB 405.04 KiB
5de0a56 20.76 KiB 432.87 KiB 412.11 KiB
3a6495e 21.58 KiB 422.66 KiB 401.08 KiB
3297d6e 21.58 KiB 418.45 KiB 396.86 KiB
ca91a5c 22.84 KiB 403.19 KiB 380.34 KiB
4d5eb78 21.58 KiB 418.74 KiB 397.16 KiB
feba2be 20.76 KiB 414.45 KiB 393.69 KiB

Previous results on branch: fix-TOCTOU-filesystem-race-condition

Startup times

Revision Plain With Sentry Diff
214bed9 1211.96 ms 1235.48 ms 23.52 ms

App size

Revision Plain With Sentry Diff
214bed9 21.90 KiB 709.00 KiB 687.10 KiB

@brustolin brustolin marked this pull request as ready for review October 28, 2024 07:49
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I think we should still ask somebody from the native team to have a look.

Copy link

codecov bot commented Oct 29, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.797%. Comparing base (2095ae0) to head (df9c421).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
...SentryCrash/Recording/Tools/SentryCrashFileUtils.c 60.000% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4473       +/-   ##
=============================================
- Coverage   91.803%   91.797%   -0.007%     
=============================================
  Files          611       611               
  Lines        68300     68294        -6     
  Branches     24529     24511       -18     
=============================================
- Hits         62702     62692       -10     
- Misses        5507      5509        +2     
- Partials        91        93        +2     
Files with missing lines Coverage Δ
...SentryCrash/Recording/Tools/SentryCrashFileUtils.c 85.882% <60.000%> (ø)

... and 10 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2095ae0...df9c421. Read the comment docs.

Copy link
Member

@JoshuaMoelans JoshuaMoelans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@brustolin brustolin merged commit a90f228 into main Oct 30, 2024
62 of 64 checks passed
@brustolin brustolin deleted the fix-TOCTOU-filesystem-race-condition branch October 30, 2024 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants