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

Try to fix #35 #36

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Try to fix #35 #36

wants to merge 8 commits into from

Conversation

probonopd
Copy link
Member

Closes #35
Thanks @Bqleine

@probonopd
Copy link
Member Author

Build artifacts for testing.

@probonopd
Copy link
Member Author

Actually... won't

https://github.com/AppImage/type2-runtime/pull/32/files

fix this once it finally gets reviewed and merged?

+ prog = findBinaryInFusermountDir("fusermount" + i);
+ if (access(prog, X_OK) == 0)
+ char binaryName[13];
+ sprintf(binaryName, "fusermount%d", i);
Copy link
Member

Choose a reason for hiding this comment

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

Use snprintf which takes the size into account:

Suggested change
+ sprintf(binaryName, "fusermount%d", i);
+ sprintf(binaryName, sizeof(binaryName) "fusermount%d", i);

You should further check the size of the return value:

if (snprintf(...) >= 13)
# alternativelly, since you expect a single digit
if (snprintf(...) != 11)

Copy link
Member Author

@probonopd probonopd Jul 3, 2024

Choose a reason for hiding this comment

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

fusermount4... fusermount99 is what I had in mind

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what you mean with the further check - in which case would this even be needed?

Copy link
Member

Choose a reason for hiding this comment

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

If i overflows for whatever reason (might be future changes), snprintf will not render additional digits but it'd be unexpected to run into the limit anyway. It's good practice to check the return value. See, e.g., https://stackoverflow.com/questions/54084570/how-to-detect-snprintf-errors.

Copy link
Member

@TheAssassin TheAssassin left a comment

Choose a reason for hiding this comment

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

Had a first glance. Please revisit, I'll have another look afterwards.

patches/libfuse/mount.c.diff Outdated Show resolved Hide resolved
@Bqleine
Copy link

Bqleine commented Jul 3, 2024

The more I look into this, the more it seems like a bad idea.

Firstly, @TheAssassin noticed the security issues in your patch and told you, but you merged the patch without more discussion. #11 (comment)

Then in the patch you wrote that « For security reasons, we do not search the binary on the $PATH; », but now we are doing it in #32?

I don't understand everything but we should seriously reconsider the security implications of doing such a patch. And ask libfuse's developers' opinion, as they either have a good reason of not doing it or could merge it directly upstream and give it a proper review for security issues.

Because putting buffer overflows in someone else's code is not ok.

@TheAssassin
Copy link
Member

I wish I had more time to work on AppImage and all of my other projects because I know I could handle most of this better, however that's likely not going to change soon. This is a tiny project team wise but has a large impact on users and developers, which is perfectly illustrated by https://xkcd.com/2347/ (change Nebraska to Germany).

@Bqleine Bqleine mentioned this pull request Jul 4, 2024
Copy link

github-actions bot commented Aug 8, 2024

Build for testing:
artifacts
Use at your own risk.

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.

Buffer overflow in libfuse patch
3 participants