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.

Sign up for GitHub

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

Fix #1175, Use fstat and fchmod for TOCTOU Bug #1181

Merged
merged 1 commit into from
Oct 3, 2022
Merged

Fix #1175, Use fstat and fchmod for TOCTOU Bug #1181

merged 1 commit into from
Oct 3, 2022

Conversation

ArielSAdamsNASA
Copy link
Contributor

Describe the contribution
Fixes #1175

Testing performed
GitHub Actions

Expected behavior changes
Resolve TOCTOU bug by using fstat and fchmod instead of stat and chmod.

Contributor Info - All information REQUIRED for consideration of pull request
Ariel Adams, ASRC Federal

@ArielSAdamsNASA ArielSAdamsNASA force-pushed the fix-1175-toctou-bug-chmod branch 3 times, most recently from ccc2a0c to 41a8277 Compare October 13, 2021 14:16
@ArielSAdamsNASA
Copy link
Contributor Author

@skliper There is an error ‘fstat’ makes integer from pointer without a cast [-Wint-conversion] fstat(Filename, &dststat);. Should I cast Filename or use fd = open(local_path, O_WRONLY, 0); instead of fp = fopen(Filename, "w")?

skliper
skliper requested changes Oct 18, 2021
View reviewed changes
Copy link
Contributor

@skliper skliper left a comment •
edited
Loading

Choose a reason for hiding this comment

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

fstat and fchmod take the file descriptor, not the file name as an input, as in fstat(fd, &dststat). See https://linux.die.net/man/2/fstat. Also, the second fstat doesn't seem to do anything useful, so it could probably be removed.

@ArielSAdamsNASA ArielSAdamsNASA marked this pull request as draft October 19, 2021 19:10
@ArielSAdamsNASA ArielSAdamsNASA force-pushed the fix-1175-toctou-bug-chmod branch 4 times, most recently from b1b6c9c to 49c48c5 Compare October 19, 2021 19:27
@ArielSAdamsNASA ArielSAdamsNASA marked this pull request as ready for review October 19, 2021 19:27
@skliper
Copy link
Contributor

skliper commented Oct 19, 2021 •
edited
Loading

So this will probably need more discussion since the assert library needs to work cross-platform (not just POSIX). It may be that we will need to leave this warning as-is and just disposition. Really its intended for single, privileged user use so TOCTOU isn't really any more of a security concern than any of the other capabilities (like loading and running code). Also the ut_assert library is test code only, not production code.

jphickey added a commit to jphickey/osal that referenced this pull request Aug 10, 2022
@jphickey
Fix nasa#1181, global variable cleanup
e8f73aa 
Consolidate all ES global variables under a single CFE_ES_Global.

Removes the separate CFE_ES_TaskData as well as some random pointers
that were stored at global scope.

All references adjusted accordingly (search and replace).
jphickey added a commit to jphickey/osal that referenced this pull request Aug 10, 2022
@jphickey
Merge pull request nasa#1254 from jphickey/fix-1181-global-cleanup
3397016 
Fix nasa#1181, global variable cleanup
@ArielSAdamsNASA ArielSAdamsNASA mentioned this pull request Sep 8, 2022
Merged
2 tasks
@chillfig chillfig added the CCB:Approved Indicates code review and approval by community CCB label Sep 29, 2022
@dzbaker dzbaker mentioned this pull request Oct 3, 2022
Merged
2 tasks
dzbaker added a commit to nasa/cFS that referenced this pull request Oct 3, 2022
@dzbaker
IC: Caelum-rc4+dev23
d1dbf85 
*Combines:*

cfe v7.0.0-rc4+dev183
psp v1.6.0-rc4+dev55
osal v6.0.0-rc4+dev116
sample_app v1.3.0-rc4+dev27
sch_lab v2.5.0-rc4:dev35
tblCRCTool v1.3.0-rc4+dev18
ci_lab v2.5.0-rc4+dev30
sample_lib v1.3.0-rc4+dev20
cFS-GroundSystem v3.0.0-rc4+dev25

**Includes:**

*cFS*
- #580, Addresses invalid CodeQL language option

*cFE*
- nasa/cFE#2145, Fixes issue #2144- Propagate CMAKE_EXPORT_COMPILE_COMMANDS variable
- nasa/cFE#2148, Remove CodeQL Paths Ignore
- nasa/cFE#2151, Duplicated Logic in CFE_SB_BroadcastBufferToRoute
- nasa/cFE#2156, Remove 'return;' from last line of void functions.
- nasa/cFE#2154, Remove unnecessary parentheses around return values.

*osal*
- nasa/osal#1181, Use fstat and fchmod for TOCTOU Bug
- nasa/osal#1294, Remove 'return;' from last line of void functions.
- nasa/osal#1292, Remove unnecessary parentheses around return values.

*sample_app*
- nasa/sample_app#177, Misaligned comments
- nasa/sample_app#179, Remove unnecessary parentheses around return values.
- nasa/sample_app#181, Remove 'return;' from last line of void functions.

*sch_lab*
- nasa/sch_lab#120, Remove unnecessary parentheses around return values.

*tblCRCTool*
- nasa/tblCRCTool#69, Remove unnecessary parentheses around return values.

*ci_lab*
- nasa/ci_lab#116, Remove unnecessary parentheses around return values.
- nasa/ci_lab#118, Remove 'return;' from last line of void functions.

*sample_lib*
- nasa/sample_lib#84, Remove unnecessary parentheses around return values.

*cFS-GroundSystem*
- nasa/cFS-GroundSystem#222, Remove 'return;' from last line of void functions.

Co-authored-by: Avi Weiss <[email protected]>
Co-authored-by: Andrew Liounis <[email protected]>
Co-authored by: Ariel Adams <[email protected]>
@dzbaker dzbaker mentioned this pull request Oct 3, 2022
Merged
2 tasks
@dzbaker dzbaker merged commit 063221a into nasa:main Oct 3, 2022
dzbaker added a commit to nasa/cFS that referenced this pull request Oct 3, 2022
@dzbaker
IC: Caelum-rc4+dev23
ef44a74 
*Combines:*

cfe v7.0.0-rc4+dev183
psp v1.6.0-rc4+dev55
osal v6.0.0-rc4+dev116
sample_app v1.3.0-rc4+dev27
sch_lab v2.5.0-rc4:dev35
tblCRCTool v1.3.0-rc4+dev18
ci_lab v2.5.0-rc4+dev30
sample_lib v1.3.0-rc4+dev20
cFS-GroundSystem v3.0.0-rc4+dev25

**Includes:**

*cFS*
- #580, Addresses invalid CodeQL language option

*cFE*
- nasa/cFE#2145, Fixes issue #2144- Propagate CMAKE_EXPORT_COMPILE_COMMANDS variable
- nasa/cFE#2148, Remove CodeQL Paths Ignore
- nasa/cFE#2151, Duplicated Logic in CFE_SB_BroadcastBufferToRoute
- nasa/cFE#2156, Remove 'return;' from last line of void functions.
- nasa/cFE#2154, Remove unnecessary parentheses around return values.

*osal*
- nasa/osal#1181, Use fstat and fchmod for TOCTOU Bug
- nasa/osal#1294, Remove 'return;' from last line of void functions.
- nasa/osal#1292, Remove unnecessary parentheses around return values.

*sample_app*
- nasa/sample_app#177, Misaligned comments
- nasa/sample_app#179, Remove unnecessary parentheses around return values.
- nasa/sample_app#181, Remove 'return;' from last line of void functions.

*sch_lab*
- nasa/sch_lab#120, Remove unnecessary parentheses around return values.

*tblCRCTool*
- nasa/tblCRCTool#69, Remove unnecessary parentheses around return values.

*ci_lab*
- nasa/ci_lab#116, Remove unnecessary parentheses around return values.
- nasa/ci_lab#118, Remove 'return;' from last line of void functions.

*sample_lib*
- nasa/sample_lib#84, Remove unnecessary parentheses around return values.

*cFS-GroundSystem*
- nasa/cFS-GroundSystem#222, Remove 'return;' from last line of void functions.

Co-authored-by: Avi Weiss <[email protected]>
Co-authored-by: Andrew Liounis <[email protected]>
Co-authored by: Ariel Adams <[email protected]>
@skliper
Copy link
Contributor

skliper commented Oct 4, 2022

@dzbaker @dmknutsen ef44a74 is failing. I don't think this one was ready to merge... causing failures in all other repo unit tests.

@dmknutsen
Copy link
Contributor

Thanks for the heads up! We will revert back.

@jphickey
Copy link
Contributor

jphickey commented Oct 4, 2022

Just came here because I was seeing some strange build failures - confirm this needs to be reverted - it is definitely breaking stuff.

Problem is that it uses POSIX APIs (fileno, fstat) in code that is supposed to be plain C99.

@jphickey
Copy link
Contributor

jphickey commented Oct 4, 2022

NOTE: I'd actually propose reverting the whole thing - that is, not just the use of fstat/fchmod but also the use of stat/chmod that came before it.

It looks like the genesis of all this was a codeQL warning back here: #780

HOWEVER - context is important, these are coverage tests and not running on a shared server. C99 does not have any concept of "file permissions" - that's a POSIX thing. This code is supposed to be portable and should adhere to strict C99, nothing more. That means we can't change file permissions - on a UNIX-type system this will be controlled by the user's "umask" setting. So permission is already controlled - codeQL was generating a false positive warning here IMO in saying that permissions were not set. They were, just not directly in the source code.

@skliper
Copy link
Contributor

skliper commented Oct 4, 2022

I agree 100% w/ @jphickey. We've snowballed in complexity trying to squash warnings that aren't really applicable.

@dmknutsen dmknutsen modified the milestone: Draco Feb 6, 2023
@chillfig chillfig added this to the Draco milestone Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skliper skliper skliper approved these changes

Assignees
No one assigned
Labels
CCB:Approved Indicates code review and approval by community CCB
Projects
None yet
Milestone
Draco
Development

Successfully merging this pull request may close these issues.

TOCTOU Bug for chmod
7 participants
@ArielSAdamsNASA @skliper @dmknutsen @jphickey @dzbaker @chillfig @arielswalker